Re: [Xen-devel] [PATCH v2] xen/arm: gicv2: Export GICv2m register frames to domain0 by device tree

[Wei Chen <wei.chen@xxxxxxxxxx>]

Hi Julien,

On 26 April 2016 at 18:49, Julien Grall <julien.grall@xxxxxxx> wrote:
> Hello Wei,
>
> On 25/04/2016 10:39, 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
>
>
> NIT: s/Domain0/dom0/
>
>> v2m MMIO regions to domain0 and route v2m SPIs to domain0.
>
>
> Ditto.
>
>>
>> With this GICv2m extension support, the domain0 kernel can do GICv2m
>
>
> Ditto
>
>> 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>
>
>
> [...]
>
>> +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 the hardware
>> domain. */
>> +    list_for_each_entry( v2m_data, &gicv2m_info, entry )
>> +    {
>> +        int ret;
>> +        u32 spi;
>> +
>> +        printk("GICv2: Mapping v2m frame to d%d: addr=0x%lx size=0x%lx
>> spi_base=%u num_spis=%u\n",
>> +               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 s%d failed.\n",
>
>
> s/s%d/d%d/
>
>> +                   d->domain_id);
>> +            return ret;
>> +        }
>> +
>> +        /*
>> +         * Map all SPIs that are allocated to MSIs for the frame to the
>> +         * 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.
>
>
> How did you find that SPIs should be configured edge-rising?
Before route_irq_to_guest, the SPI must be configured. I found Linux
v2m driver set
the SPI type to edge-rising, so I set edge-rising as v2m SPI default
type here too.
>
>
>> +             */
>> +            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 is still wrong here.
>
> The start of the second and third lines should be aligned to "const..." on
> the first line. I.e
>
> foo(const
> ____const
> ____void
>
> Where _ is the empty space.
>
> [...]
>

Thanks for your detailed example!

>> +static void gicv2_add_v2m_frame_to_list(paddr_t addr, paddr_t size,
>> +                                        u32 spi_start, u32 nr_spis,
>> +                                        const struct dt_device_node *v2m)
>> +{
>> +    struct v2m_data *v2m_data;
>> +
>> +    v2m_data = xzalloc_bytes(sizeof(struct v2m_data));
>> +    if ( !v2m_data )
>> +        panic("GICv2: Cannot allocate memory for v2m frame");
>> +
>> +    /* Initialize a new entry to record new frame infomation. */
>
>
> s/infomation/information/
>
>
>> +    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);
>> +}
>> +
>> +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)
>> +    {
>> +        if ( !dt_device_is_compatible(v2m, "arm,gic-v2m-frame") )
>> +            continue;
>> +
>> +        /*
>> +         * 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.
>> +            */
>
>
> The indentation of the comments if wrong.
>
> It should be:
>
> /*
>  * FOo
>  * Bar
>  */
>
>> +            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);
>
>
> The ACPI code will likely need to read those registers. So this could go in
> the new function you have added.
>
> You could pass pointers to your function to know whether the values are
> overridden by the firmware.
>
I will refine the new function.
>> +
>> +            iounmap(base);
>> +        }
>> +
>> +        /* Add this v2m frame information to list. */
>> +        gicv2_add_v2m_frame_to_list(addr, size, spi_start, nr_spis, v2m);
>> +    }
>> +}
>> +
>
>
> Regards,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel