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

[Wei Chen <wei.chen@xxxxxxxxxx>]

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 using 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.
>
>> +                   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 *...)
>

>> +{
>> +    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