Re: [PATCH 2/2] xen/arm: gicv2: Adding support for GICv2m in Dom0

[Ian Campbell <ian.campbell@citrix.com>]

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 <Suravee.Suthikulpanit@amd.com>
> ---
>  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.