|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen/arm: gicv2: Export GICv2m register frames to domain0 by device tree
Sorry, I forget to change to the plain text mode. On 25 April 2016 at 17:45, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote: > Hello Wei, > > could you please send email replies in plain text (rather than html)? > > Thanks, > > Stefano > > On Mon, 25 Apr 2016, Wei Chen wrote: >> Hi Julien, >> >> On 23 April 2016 at 02:29, Julien Grall <julien.grall@xxxxxxx> wrote: >> Hi Wei, >> >> On 21/04/16 09:24, Wei Chen wrote: >> This patch adds v2m extension support in GIC-v2 driver. The >> GICv2 driver >> detects the MSI frames from device tree and creates >> corresponding device >> tree nodes in Domain0's DTB. It also provides one hw_ops >> callback to map >> v2m MMIO regions to domain0 and route v2m SPIs to domain0. >> >> With this GICv2m extension support, the domain0 kernel can do >> GICv2m >> frame setup and initialization. >> >> This patch is based on the GICv2m patch of Suravee Suthikulpanit: >> [PATCH 2/2] xen/arm: gicv2: Adding support for GICv2m in Dom0 >> >> http://lists.xen.org/archives/html/xen-devel/2015-04/msg02613.html >> >> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxxxxx> >> --- >> xen/arch/arm/domain_build.c | 5 + >> xen/arch/arm/gic-v2.c | 285 >> ++++++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/gic.c | 9 ++ >> xen/include/asm-arm/gic.h | 3 + >> 4 files changed, 302 insertions(+) >> >> diff --git a/xen/arch/arm/domain_build.c >> b/xen/arch/arm/domain_build.c >> index aba714c..b5d82fc 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -2176,6 +2176,11 @@ int construct_dom0(struct domain *d) >> if ( rc < 0 ) >> return rc; >> >> + /* Map extra GIC MMIO, irqs and other hw stuffs to domain0. >> */ >> >> >> NIT: s/domain0/dom0/ >> >> + rc = gic_map_hwdom_extra_mappings(d); >> + if ( rc < 0 ) >> + return rc; >> + >> rc = platform_specific_mapping(d); >> if ( rc < 0 ) >> return rc; >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >> index 450755f..4a7d65a 100644 >> --- a/xen/arch/arm/gic-v2.c >> +++ b/xen/arch/arm/gic-v2.c >> @@ -21,6 +21,7 @@ >> #include <xen/lib.h> >> #include <xen/init.h> >> #include <xen/mm.h> >> +#include <xen/vmap.h> >> >> >> Why do you need to include this header? >> >> This header is used for "iounmap" API. >> >> #include <xen/irq.h> >> #include <xen/iocap.h> >> #include <xen/sched.h> >> @@ -66,6 +67,41 @@ >> #define GICH_V2_VMCR_PRIORITY_MASK 0x1f >> #define GICH_V2_VMCR_PRIORITY_SHIFT 27 >> >> +/* GICv2m extension register definitions. */ >> +/* >> +* MSI_TYPER: >> +* [31:26] Reserved >> +* [25:16] lowest SPI assigned to MSI >> +* [15:10] Reserved >> +* [9:0] Number of SPIs assigned to MSI >> +*/ >> +#define V2M_MSI_TYPER 0x008 >> +#define V2M_MSI_TYPER_BASE_SHIFT 16 >> +#define V2M_MSI_TYPER_BASE_MASK 0x3FF >> +#define V2M_MSI_TYPER_NUM_MASK 0x3FF >> +#define V2M_MSI_SETSPI_NS 0x040 >> +#define V2M_MIN_SPI 32 >> +#define V2M_MAX_SPI 1019 >> +#define V2M_MSI_IIDR 0xFCC >> + >> +#define V2M_MSI_TYPER_BASE_SPI(x) \ >> + (((x) >> V2M_MSI_TYPER_BASE_SHIFT) & >> V2M_MSI_TYPER_BASE_MASK) >> + >> +#define V2M_MSI_TYPER_NUM_SPI(x) ((x) & >> V2M_MSI_TYPER_NUM_MASK) >> + >> +struct v2m_data { >> + struct list_head entry; >> + /* Pointer to the DT node representing the v2m frame */ >> + const struct dt_device_node *dt_node; >> + paddr_t addr; /* Register frame base */ >> + paddr_t size; /* Register frame size */ >> + u32 spi_start; /* The SPI number that MSIs start */ >> + u32 nr_spis; /* The number of SPIs for MSIs */ >> +}; >> + >> +/* v2m extension register frame information list */ >> +static LIST_HEAD(gicv2m_info); >> + >> /* Global state */ >> static struct { >> void __iomem * map_dbase; /* IO mapped Address of >> distributor registers */ >> @@ -551,6 +587,173 @@ static void gicv2_irq_set_affinity(struct >> irq_desc *desc, const cpumask_t *cpu_m >> spin_unlock(&gicv2.lock); >> } >> >> +static int gicv2_map_hwdown_extra_mappings(struct domain *d) >> +{ >> + const struct v2m_data *v2m_data; >> + >> + /* >> + * For the moment, we'll assign all v2m frames to a single >> domain >> + * (i.e Domain0). >> >> >> NIT: s/Domain0/hardware domain/ >> >> Also, I would drop the i.e and say "... frames to the hardware domain." >> >> + */ >> + list_for_each_entry( v2m_data, &gicv2m_info, entry ) >> + { >> + int ret, spi; >> >> >> Please use "u32" for the variable spi. >> >> + >> + printk("GICv2: Mapping v2m frame to domain%d: addr=0x%lx >> size=0x%lx spi_base=%u num_spis=%u\n", >> >> >> NIT: You can shorten domain%d to d%d >> >> + d->domain_id, v2m_data->addr, v2m_data->size, >> + v2m_data->spi_start, v2m_data->nr_spis); >> + >> + ret = map_mmio_regions(d, paddr_to_pfn(v2m_data->addr), >> + DIV_ROUND_UP(v2m_data->size, >> PAGE_SIZE), >> + paddr_to_pfn(v2m_data->addr)); >> + if ( ret ) >> + { >> + printk(XENLOG_ERR >> + "GICv2: Map v2m frame to domain%d failed.\n", >> >> >> Ditto. >> >> Also the indentation looks wrong. >> >> It's very strange. This indentation looks well in the patch file, and all >> indentations are space chars. >> I'll try to send a revised patch. >> >> >> + d->domain_id); >> + return ret; >> + } >> + >> + /* >> + * Map all SPIs that are allocated to MSIs in a single >> frame to the >> >> >> s/in a single frame/for the frame/ >> >> + * domain. >> + */ >> + for ( spi = v2m_data->spi_start; >> + spi < (v2m_data->spi_start + v2m_data->nr_spis); >> spi++ ) >> + { >> + /* >> + * MSIs are always edge-triggered. Configure the >> associated SPIs >> + * to be edge-rising. >> + */ >> + ret = irq_set_spi_type(spi, IRQ_TYPE_EDGE_RISING); >> + if ( ret ) >> + { >> + printk(XENLOG_ERR >> + "GICv2: Failed to set v2m MSI SPI[%d] >> type.\n", spi); >> + return ret; >> + } >> + >> + /* Route a SPI that is allocated to MSI to the >> domain. */ >> + ret = route_irq_to_guest(d, spi, spi, "v2m"); >> + if ( ret ) >> + { >> + printk(XENLOG_ERR >> + "GICv2: Failed to route v2m MSI SPI[%d] >> to Dom%d.\n", >> + spi, d->domain_id); >> + return ret; >> + } >> + >> + /* Reserve a SPI that is allocated to MSI for the >> domain. */ >> + if ( !vgic_reserve_virq(d, spi) ) >> + { >> + printk(XENLOG_ERR >> + "GICv2: Failed to reserve v2m MSI >> SPI[%d] for Dom%d.\n", >> + spi, d->domain_id); >> + return -EINVAL; >> + } >> + } >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * Set up gic v2m DT sub-node. >> + * Please refer to the binding document: >> + * >> https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt >> + */ >> +static int gicv2m_make_dt_node(const struct domain *d, >> + const struct dt_device_node *gic, >> + void *fdt) >> >> >> The indentation looks wrong to me. It should be: >> >> foo(const struct ..., >> const struct ... >> void *...) >> >> Ditto. >> +{ >> + u32 len; >> + int res; >> + const void *prop = NULL; >> + const struct dt_device_node *v2m = NULL; >> + const struct v2m_data *v2m_data; >> + >> + /* The sub-nodes require the ranges property */ >> + prop = dt_get_property(gic, "ranges", &len); >> + if ( !prop ) >> + { >> + printk(XENLOG_ERR "Can't find ranges property for the >> gic node\n"); >> + return -FDT_ERR_XEN(ENOENT); >> + } >> + >> + res = fdt_property(fdt, "ranges", prop, len); >> + if ( res ) >> + return res; >> + >> + list_for_each_entry( v2m_data, &gicv2m_info, entry ) >> + { >> + v2m = v2m_data->dt_node; >> + >> + printk("GICv2: Creating v2m DT node for domain%d: >> addr=0x%lx size=0x%lx spi_base=%u num_spis=%u\n", >> >> >> NIT: s/domain%d/d%d/ >> >> + d->domain_id, v2m_data->addr, v2m_data->size, >> + v2m_data->spi_start, v2m_data->nr_spis); >> + >> + res = fdt_begin_node(fdt, v2m->name); >> + if ( res ) >> + return res; >> + >> + res = fdt_property_string(fdt, "compatible", >> "arm,gic-v2m-frame"); >> + if ( res ) >> + return res; >> + >> + res = fdt_property(fdt, "msi-controller", NULL, 0); >> + if ( res ) >> + return res; >> + >> + if ( v2m->phandle ) >> + { >> + res = fdt_property_cell(fdt, "phandle", >> v2m->phandle); >> + if ( res ) >> + return res; >> + } >> + >> + /* Use the same reg regions as v2m node in DTB. */ >> >> >> s/DTB/host DTB/ to make clear we are speaking about the host one and >> not the guest one. >> >> + prop = dt_get_property(v2m, "reg", &len); >> + if ( !prop ) >> + { >> + printk(XENLOG_ERR "GICv2: Can't find v2m reg >> property.\n"); >> + res = -FDT_ERR_XEN(ENOENT); >> + return res; >> + } >> + >> + res = fdt_property(fdt, "reg", prop, len); >> + if ( res ) >> + return res; >> + >> + /* >> + * Create msi-base-spi and msi-num-spis properties in >> Guest DT to >> + * override the hardware setting. From the binding >> document, we can >> + * find that these two properties are optional. But if >> these two >> + * properties are present in the v2m DT node, the guest >> v2m driver >> + * should read the configuration from these two >> properties instead >> + * of reading from hardware register. >> >> >> This paragraph is not clear. I would reword; >> >> "The properties msi-base-spi and msi-num-spis are used to override the >> hardware settings. Therefore it is fine to always write them in the >> guest DT.". >> >> + */ >> + res = fdt_property_u32(fdt, "arm,msi-base-spi", >> v2m_data->spi_start); >> + if ( res ) >> + { >> + printk(XENLOG_ERR >> + "GICv2: Failed to create v2m msi-base-spi in >> Guest DT.\n"); >> + return res; >> + } >> + >> + res = fdt_property_u32(fdt, "arm,msi-num-spis", >> v2m_data->nr_spis); >> + if ( res ) >> + { >> + printk(XENLOG_ERR >> + "GICv2: Failed to create v2m msi-num-spis in >> Guest DT.\n"); >> + return res; >> + } >> + >> + fdt_end_node(fdt); >> + } >> + >> + return res; >> +} >> + >> static int gicv2_make_hwdom_dt_node(const struct domain *d, >> const struct >> dt_device_node *gic, >> void *fdt) >> @@ -587,6 +790,10 @@ static int gicv2_make_hwdom_dt_node(const >> struct domain *d, >> len *= 2; >> >> res = fdt_property(fdt, "reg", regs, len); >> + if ( res ) >> + return res; >> + >> + res = gicv2m_make_dt_node(d, gic, fdt); >> >> return res; >> } >> @@ -632,6 +839,77 @@ static bool_t gicv2_is_aliased(paddr_t >> cbase, paddr_t csize) >> return ((val_low & 0xfff0fff) == 0x0202043B && val_low == >> val_high); >> } >> >> +static void gicv2_extension_dt_init(const struct dt_device_node >> *node) >> +{ >> + int res; >> + u32 spi_start, nr_spis; >> + paddr_t addr, size; >> + const struct dt_device_node *v2m = NULL; >> + >> + /* >> + * Check whether this GIC implements the v2m extension. If >> so, >> + * add v2m register frames to gicv2m_info. >> + */ >> + dt_for_each_child_node(node, v2m) >> + { >> + struct v2m_data *v2m_data; >> + >> + if ( !dt_device_is_compatible(v2m, "arm,gic-v2m-frame") >> ) >> + continue; >> + >> + v2m_data = xzalloc_bytes(sizeof(struct v2m_data)); >> + if ( !v2m_data ) >> + panic("GICv2: Cannot allocate memory for v2m >> frame"); >> + >> + /* >> + * Check whether DT uses msi-base-spi and msi-num-spis >> properties to >> + * override the hardware setting. >> + */ >> + if ( !dt_property_read_u32(v2m, "arm,msi-base-spi", >> &spi_start) || >> + !dt_property_read_u32(v2m, "arm,msi-num-spis", >> &nr_spis) ) >> + { >> + u32 msi_typer; >> + void __iomem *base; >> + >> + /* >> + * DT does not override the hardware setting, so we >> have to read >> + * base_spi and num_spis from hardware registers to >> reserve irqs. >> + */ >> + res = dt_device_get_address(v2m, 0, &addr, &size); >> + if ( res ) >> + panic("GICv2: Cannot find a valid v2m frame >> address"); >> + >> + base = ioremap_nocache(addr, size); >> + if ( !base ) >> + panic("GICv2: Cannot remap v2m register frame"); >> + >> + msi_typer = readl_relaxed(base + V2M_MSI_TYPER); >> + spi_start = V2M_MSI_TYPER_BASE_SPI(msi_typer); >> + nr_spis = V2M_MSI_TYPER_NUM_SPI(msi_typer); >> + >> + iounmap(base); >> + } >> + >> + /* Initialize a new entry to record new frame >> infomation. */ >> + INIT_LIST_HEAD(&v2m_data->entry); >> + v2m_data->addr = addr; >> + v2m_data->size = size; >> + v2m_data->spi_start = spi_start; >> + v2m_data->nr_spis = nr_spis; >> + v2m_data->dt_node = v2m; >> + >> + printk("GICv2m extension register frame:\n" >> + " gic_v2m_addr=%"PRIpaddr"\n" >> + " gic_v2m_size=%"PRIpaddr"\n" >> + " gic_v2m_spi_base=%u\n" >> + " gic_v2m_num_spis=%u\n", >> + v2m_data->addr, v2m_data->size, >> + v2m_data->spi_start, v2m_data->nr_spis); >> + >> + list_add_tail(&v2m_data->entry, &gicv2m_info); >> >> >> I would pull non-DT code in a separate function, so we can re-use the code >> later for the support of GICv2m with ACPI. >> >> + } >> +} >> + >> static paddr_t __initdata hbase, dbase, cbase, csize, vbase; >> >> static void __init gicv2_dt_init(void) >> @@ -683,6 +961,12 @@ static void __init gicv2_dt_init(void) >> if ( csize != vsize ) >> panic("GICv2: Sizes of GICC (%#"PRIpaddr") and GICV >> (%#"PRIpaddr") don't match\n", >> csize, vsize); >> + >> + /* >> + * Check whether this GIC implements the v2m extension. If so, >> + * add v2m register frames to gicv2_extension_info. >> + */ >> + gicv2_extension_dt_init(node); >> } >> >> static int gicv2_iomem_deny_access(const struct domain *d) >> @@ -936,6 +1220,7 @@ const static struct gic_hw_operations gicv2_ops = >> { >> .read_apr = gicv2_read_apr, >> .make_hwdom_dt_node = gicv2_make_hwdom_dt_node, >> .make_hwdom_madt = gicv2_make_hwdom_madt, >> + .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings, >> .iomem_deny_access = gicv2_iomem_deny_access, >> }; >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 2bfe4de..12bb0ab 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -230,6 +230,15 @@ int gic_irq_xlate(const u32 *intspec, unsigned >> int intsize, >> return 0; >> } >> >> +/* Map extra GIC MMIO, irqs and other hw stuffs to the hardware >> domain. */ >> +int gic_map_hwdom_extra_mappings(struct domain *d) >> +{ >> + if ( gic_hw_ops->map_hwdom_extra_mappings ) >> + return gic_hw_ops->map_hwdom_extra_mappings(d); >> + >> + return 0; >> +} >> + >> static void __init gic_dt_preinit(void) >> { >> int rc; >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >> index cd97bb2..836f1ad 100644 >> --- a/xen/include/asm-arm/gic.h >> +++ b/xen/include/asm-arm/gic.h >> @@ -360,6 +360,8 @@ struct gic_hw_operations { >> const struct dt_device_node *gic, void >> *fdt); >> /* Create MADT table for the hardware domain */ >> int (*make_hwdom_madt)(const struct domain *d, u32 offset); >> + /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware >> domain. */ >> + int (*map_hwdom_extra_mappings)(struct domain *d); >> /* Deny access to GIC regions */ >> int (*iomem_deny_access)(const struct domain *d); >> }; >> @@ -369,6 +371,7 @@ int gic_make_hwdom_dt_node(const struct domain *d, >> const struct dt_device_node *gic, >> void *fdt); >> int gic_make_hwdom_madt(const struct domain *d, u32 offset); >> +int gic_map_hwdom_extra_mappings(struct domain *d); >> int gic_iomem_deny_access(const struct domain *d); >> >> #endif /* __ASSEMBLY__ */ >> >> >> Regards, >> >> -- >> Julien Grall >> >> >> >> _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |