Re: git: e13c6a6fca71 - main - Teach the GICv3 driver about a vgic child

Reply: Andrew Turner : "Re: git: e13c6a6fca71 - main - Teach the GICv3 driver about a vgic child"
In reply to: Andrew Turner : "git: e13c6a6fca71 - main - Teach the GICv3 driver about a vgic child"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Jessica Clarke <jrtc27_at_freebsd.org>
Date: Wed, 21 Sep 2022 14:10:13 UTC
On 21 Sept 2022, at 11:00, Andrew Turner <andrew@FreeBSD.org> wrote:
> 
> The branch main has been updated by andrew:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=e13c6a6fca7177fb9143ce629c2e33b35b6c4219
> 
> commit e13c6a6fca7177fb9143ce629c2e33b35b6c4219
> Author:     Andrew Turner <andrew@FreeBSD.org>
> AuthorDate: 2022-09-14 16:29:29 +0000
> Commit:     Andrew Turner <andrew@FreeBSD.org>
> CommitDate: 2022-09-21 09:59:13 +0000
> 
>    Teach the GICv3 driver about a vgic child
> 
>    This will be used by bhyve to attach a virtual GIC driver.
> 
>    Sponsored by:   Innovate UK
>    Sponsored by:   The FreeBSD Foundation
>    Differential Revision: https://reviews.freebsd.org/D36590
> ---
> sys/arm/arm/gic_common.h      |  2 ++
> sys/arm64/arm64/gic_v3.c      |  7 +++++++
> sys/arm64/arm64/gic_v3_acpi.c | 36 ++++++++++++++++++++++++++++++++++--
> sys/arm64/arm64/gic_v3_fdt.c  | 23 ++++++++++++++++++++++-
> sys/arm64/arm64/gic_v3_var.h  |  1 +
> 5 files changed, 66 insertions(+), 3 deletions(-)
> 
> diff --git a/sys/arm/arm/gic_common.h b/sys/arm/arm/gic_common.h
> index 9e8fb19300ca..42ec44bb7fab 100644
> --- a/sys/arm/arm/gic_common.h
> +++ b/sys/arm/arm/gic_common.h
> @@ -33,6 +33,7 @@
> 
> #define	GIC_IVAR_HW_REV		500
> #define	GIC_IVAR_BUS		501
> +#define	GIC_IVAR_VGIC		502
> 
> /* GIC_IVAR_BUS values */
> #define	GIC_BUS_UNKNOWN		0
> @@ -42,6 +43,7 @@
> 
> __BUS_ACCESSOR(gic, hw_rev, GIC, HW_REV, u_int);
> __BUS_ACCESSOR(gic, bus, GIC, BUS, u_int);
> +__BUS_ACCESSOR(gic, vgic, GIC, VGIC, u_int);
> 
> /* Software Generated Interrupts */
> #define	GIC_FIRST_SGI		 0	/* Irqs 0-15 are SGIs/IPIs. */
> diff --git a/sys/arm64/arm64/gic_v3.c b/sys/arm64/arm64/gic_v3.c
> index 27f41c58fe92..b4678d95fee5 100644
> --- a/sys/arm64/arm64/gic_v3.c
> +++ b/sys/arm64/arm64/gic_v3.c
> @@ -456,6 +456,7 @@ static int
> gic_v3_read_ivar(device_t dev, device_t child, int which, uintptr_t *result)
> {
> 	struct gic_v3_softc *sc;
> +	struct gic_v3_devinfo *di;
> 
> 	sc = device_get_softc(dev);
> 
> @@ -481,6 +482,12 @@ gic_v3_read_ivar(device_t dev, device_t child, int which, uintptr_t *result)
> 		    ("gic_v3_read_ivar: Invalid bus type %u", sc->gic_bus));
> 		*result = sc->gic_bus;
> 		return (0);
> +	case GIC_IVAR_VGIC:
> +		di = device_get_ivars(child);
> +		if (di == NULL)
> +			return (EINVAL);
> +		*result = di->is_vgic;
> +		return (0);
> 	}
> 
> 	return (ENOENT);
> diff --git a/sys/arm64/arm64/gic_v3_acpi.c b/sys/arm64/arm64/gic_v3_acpi.c
> index cbe4d5989ec2..cc24b7824ff1 100644
> --- a/sys/arm64/arm64/gic_v3_acpi.c
> +++ b/sys/arm64/arm64/gic_v3_acpi.c
> @@ -48,6 +48,9 @@ __FBSDID("$FreeBSD$");
> #include "gic_v3_reg.h"
> #include "gic_v3_var.h"
> 
> +#define	GICV3_PRIV_VGIC		0x80000000
> +#define	GICV3_PRIV_FLAGS	0x80000000
> +
> struct gic_v3_acpi_devinfo {
> 	struct gic_v3_devinfo	di_gic_dinfo;
> 	struct resource_list	di_rl;
> @@ -86,6 +89,7 @@ struct madt_table_data {
> 	ACPI_MADT_GENERIC_DISTRIBUTOR *dist;
> 	int count;
> 	bool rdist_use_gicc;
> +	bool have_vgic;
> };
> 
> static void
> @@ -152,6 +156,8 @@ rdist_map(ACPI_SUBTABLE_HEADER *entry, void *arg)
> 		BUS_SET_RESOURCE(madt_data->parent, madt_data->dev,
> 		    SYS_RES_MEMORY, madt_data->count, intr->GicrBaseAddress,
> 		    count);
> +		if (intr->VgicInterrupt == 0)
> +			madt_data->have_vgic = false;
> 
> 	default:
> 		break;
> @@ -164,6 +170,7 @@ gic_v3_acpi_identify(driver_t *driver, device_t parent)
> 	struct madt_table_data madt_data;
> 	ACPI_TABLE_MADT *madt;
> 	vm_paddr_t physaddr;
> +	uintptr_t private;
> 	device_t dev;
> 
> 	physaddr = acpi_find_table(ACPI_SIG_MADT);
> @@ -210,6 +217,7 @@ gic_v3_acpi_identify(driver_t *driver, device_t parent)
> 
> 	madt_data.dev = dev;
> 	madt_data.rdist_use_gicc = false;
> +	madt_data.have_vgic = true;
> 	acpi_walk_subtables(madt + 1, (char *)madt + madt->Header.Length,
> 	    rdist_map, &madt_data);
> 	if (madt_data.count == 0) {
> @@ -222,7 +230,12 @@ gic_v3_acpi_identify(driver_t *driver, device_t parent)
> 		    rdist_map, &madt_data);
> 	}
> 
> -	acpi_set_private(dev, (void *)(uintptr_t)madt_data.dist->Version);
> +	private = madt_data.dist->Version;
> +	/* Flag that the VGIC is in use */
> +	if (madt_data.have_vgic)
> +		private |= GICV3_PRIV_VGIC;
> +
> +	acpi_set_private(dev, (void *)private);
> 
> out:
> 	acpi_unmap_table(madt);
> @@ -232,7 +245,7 @@ static int
> gic_v3_acpi_probe(device_t dev)
> {
> 
> -	switch((uintptr_t)acpi_get_private(dev)) {
> +	switch((uintptr_t)acpi_get_private(dev) & ~GICV3_PRIV_FLAGS) {
> 	case ACPI_MADT_GIC_VERSION_V3:
> 	case ACPI_MADT_GIC_VERSION_V4:
> 		break;
> @@ -390,9 +403,14 @@ gic_v3_add_children(ACPI_SUBTABLE_HEADER *entry, void *arg)
> static void
> gic_v3_acpi_bus_attach(device_t dev)
> {
> +	struct gic_v3_acpi_devinfo *di;
> +	struct gic_v3_softc *sc;
> 	ACPI_TABLE_MADT *madt;
> +	device_t child;
> 	vm_paddr_t physaddr;
> 
> +	sc = device_get_softc(dev);
> +
> 	physaddr = acpi_find_table(ACPI_SIG_MADT);
> 	if (physaddr == 0)
> 		return;
> @@ -405,6 +423,20 @@ gic_v3_acpi_bus_attach(device_t dev)
> 
> 	acpi_walk_subtables(madt + 1, (char *)madt + madt->Header.Length,
> 	    gic_v3_add_children, dev);
> +	/* Add the vgic child if needed */
> +	if (((uintptr_t)acpi_get_private(dev) & GICV3_PRIV_FLAGS) != 0) {
> +		child = device_add_child(dev, "vgic", -1);

Do we have anything to attach to this, or is this just creating
children that will never do anything?

> +		if (child == NULL) {
> +			device_printf(dev, "Could not add vgic child\n");
> +		} else {
> +			di = malloc(sizeof(*di), M_GIC_V3, M_WAITOK | M_ZERO);
> +			resource_list_init(&di->di_rl);
> +			di->di_gic_dinfo.gic_domain = -1;
> +			di->di_gic_dinfo.is_vgic = 1;
> +			sc->gic_nchildren++;
> +			device_set_ivars(child, di);
> +		}
> +	}
> 
> 	acpi_unmap_table(madt);
> 
> diff --git a/sys/arm64/arm64/gic_v3_fdt.c b/sys/arm64/arm64/gic_v3_fdt.c
> index 0478bdc2962f..0ca362dc4aea 100644
> --- a/sys/arm64/arm64/gic_v3_fdt.c
> +++ b/sys/arm64/arm64/gic_v3_fdt.c
> @@ -212,9 +212,10 @@ static int
> gic_v3_fdt_print_child(device_t bus, device_t child)
> {
> 	struct gic_v3_ofw_devinfo *di = device_get_ivars(child);
> -	struct resource_list *rl = &di->di_rl;
> +	struct resource_list *rl;
> 	int retval = 0;
> 
> +	rl = &di->di_rl;
> 	retval += bus_print_child_header(bus, child);
> 	retval += resource_list_print_type(rl, "mem", SYS_RES_MEMORY, "%#jx");
> 	retval += bus_print_child_footer(bus, child);
> @@ -228,6 +229,8 @@ gic_v3_ofw_get_devinfo(device_t bus __unused, device_t child)
> 	struct gic_v3_ofw_devinfo *di;
> 
> 	di = device_get_ivars(child);
> +	if (di->di_gic_dinfo.is_vgic)
> +		return (NULL);
> 	return (&di->di_dinfo);
> }
> 
> @@ -352,5 +355,23 @@ gic_v3_ofw_bus_attach(device_t dev)
> 		}
> 	}
> 
> +	/*
> +	 * If there is a vgic maintanance interupt add a virtual gic

s/interupt/interrupt/

Maybe also capitalise (V)GIC.

> +	 * child so we can use this in the vmm module for bhyve.
> +	 */
> +	if (OF_hasprop(parent, "interrupts")) {
> +		child = device_add_child(dev, "vgic", -1);
> +		if (child == NULL) {
> +			device_printf(dev, "Could not add vgic child\n");
> +		} else {
> +			di = malloc(sizeof(*di), M_GIC_V3, M_WAITOK | M_ZERO);
> +			resource_list_init(&di->di_rl);
> +			di->di_gic_dinfo.gic_domain = -1;
> +			di->di_gic_dinfo.is_vgic = 1;
> +			device_set_ivars(child, di);
> +			sc->gic_nchildren++;

These two lines are unnecessarily swapped compared with the ACPI
version.

> +		}
> +	}
> +
> 	return (bus_generic_attach(dev));
> }
> diff --git a/sys/arm64/arm64/gic_v3_var.h b/sys/arm64/arm64/gic_v3_var.h
> index 7722a48cb456..b66fe6c57bb2 100644
> --- a/sys/arm64/arm64/gic_v3_var.h
> +++ b/sys/arm64/arm64/gic_v3_var.h
> @@ -89,6 +89,7 @@ struct gic_v3_softc {
> struct gic_v3_devinfo {
> 	int gic_domain;
> 	int msi_xref;
> +	int is_vgic;

bool?..

Jess

> };
> 
> #define GIC_INTR_ISRC(sc, irq)	(&sc->gic_irqs[irq].gi_isrc)