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.gr...@arm.com> 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.c...@linaro.org> > --- > 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@lists.xen.org http://lists.xen.org/xen-devel
