From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 2/2] xen/arm: gicv2: Adding support for GICv2m in Dom0 Date: Fri, 8 May 2015 16:37:53 +0100 Message-ID: <1431099473.2660.520.camel@citrix.com> References: <1429764720-2866-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1429764720-2866-3-git-send-email-Suravee.Suthikulpanit@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1429764720-2866-3-git-send-email-Suravee.Suthikulpanit@amd.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Suravee Suthikulpanit Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, julien.grall@linaro.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Wed, 2015-04-22 at 23:52 -0500, Suravee Suthikulpanit wrote: > This patch detect and propagate the gic-v2m-frame devicetree sub-node. "detects and propagates" > This allows Dom0 kernel to setup and intialize GICv2m MSI frame. "initialize" IIRC the GICv2m is described rather briefly in an appendix to some document or other, could you reference it here please. As Julien noted I think this patch is just exposing the GICv2m to dom0 and not doing a more complex thing where Xen owns the GICv2m and provides a (partially-)emulated vGICv2m to guests (including dom0). It's worth briefly mentioning that here. > Signed-off-by: Suravee Suthikulpanit > --- > xen/arch/arm/gic-v2.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 169 insertions(+) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 80acc62..0c3352e 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -600,6 +600,171 @@ static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_m > spin_unlock(&gicv2.lock); > } > > +/* > + * Set up gic v2m DT sub-node. Please could you give a reference to the appropriate binding document here. > + * > + * gic0: interrupt-controller@e1101000 { > + * compatible = "arm,gic-400", "arm,cortex-a15-gic"; > + * interrupt-controller; > + * #interrupt-cells = <3>; > + * #address-cells = <2>; > + * #size-cells = <2>; > + * reg = <0x0 0xe1110000 0 0x1000>, > + * <0x0 0xe112f000 0 0x2000>, > + * <0x0 0xe1140000 0 0x10000>, > + * <0x0 0xe1160000 0 0x10000>; > + * interrupts = <1 9 0xf04>; > + * ranges = <0 0 0 0xe1100000 0 0x100000>; > + * v2m0: v2m@e0080000 { > + * compatible = "arm,gic-v2m-frame"; > + * msi-controller; > + * arm,msi-base-spi = <64>; > + * arm,msi-num-spis = <256>; > + * reg = <0x0 0x00080000 0 0x1000>; > + * }; > + * }; > + */ > +static int gicv2m_make_dt_node(const struct domain *d, > + const struct dt_device_node *node, > + void *fdt) > +{ > + u32 len, base_spi, num_spis; > + u64 addr, size; > + int res, i; > + const void *prop = NULL; > + struct domain *dom; > + unsigned long mm_start, mm_nr; > + const struct dt_device_node *gic = dt_interrupt_controller; > + const struct dt_device_node *v2m = NULL; > + > + /* v2m is optional */ > + v2m = dt_find_compatible_node(NULL, NULL, "arm,gic-v2m-frame"); Is this required to be a child of the interrupt-controller node,or is there some other cross link which should be checked? > + dom = xzalloc_bytes(sizeof(struct domain)); > + if ( !dom ) > + { > + res = -ENOMEM; > + goto err_out0; > + } > + > + memcpy(dom, d, sizeof(struct domain)); This is all veeeerrrry suspicious, what are you trying to do here? > + mm_start = paddr_to_pfn(addr & PAGE_MASK); > + mm_nr = DIV_ROUND_UP(size, PAGE_SIZE); > + res = map_mmio_regions(dom, mm_start, mm_nr, mm_start); Why not just use d, why make a copy of it? In any case I don't think calls to map_mmio_regios, or the irq stuff below belong here, this function should be making the dtb node and populating it with properties, nothing else. So the mapping stuff needs to be handled elsewhere, perhaps via a new gic_hw_operations (or two map_extra_regions + map_extra_irqs perhaps). This would make much of the complicated error handling here (needed to unwind the mappings) go away and also let you use dt_read_number instead of be32_to_* etc. > + if ( res ) > + { > + dprintk(XENLOG_ERR, "v2m: map_mmio_regions failed.\n"); > + goto err_out1; > + } > + > + /* Set up msi-base-spi dt property */ > + prop = dt_get_property(v2m, "arm,msi-base-spi", &len); > + if ( !prop ) > + { > + dprintk(XENLOG_ERR, "v2m: Can't find msi-base-spi.\n"); > + goto err_out2; > + } > + base_spi = be32_to_cpup(prop); > + res = fdt_property(fdt, "arm,msi-base-spi", prop, len); What about extra properties not hardcoded here? I think you'd be better of looping over all properties and blacklist any which shouldn't be passed through. Similar to how write_properties in domain_build.c does it. > + if ( res ) > + goto err_out2; > + > + /* Set up msi-num-spis dt property */ > + prop = dt_get_property(v2m, "arm,msi-num-spis", &len); > + if ( !prop ) > + { > + dprintk(XENLOG_ERR, "v2m: Can't find msi-num-spis.\n"); > + goto err_out2; > + } > + num_spis = be32_to_cpup(prop); > + res = fdt_property(fdt, "arm,msi-num-spis", prop, len); > + if ( res ) > + goto err_out2; > + > + /* > + * Currently, we assign all SPIs for MSI to dom0 > + */ > + for (i = base_spi; i < (base_spi + num_spis); i++) > + { > + res = irq_set_spi_type(i, DT_IRQ_TYPE_EDGE_RISING); > + if ( res ) > + { > + dprintk(XENLOG_ERR, "v2m: Failed to set MSI interrupt type.\n"); > + goto err_out2; > + } Needs a vgic_reserve_virq call too I think (in its new home, not here). Ian.