Re: [Xen-devel] [PATCH 02/28] ARM: GICv3 ITS: parse and store ITS subnodes from hardware DT

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi Julien,

On 06/02/17 12:39, Julien Grall wrote:
> Hi Andre,
> 
> On 30/01/17 18:31, Andre Przywara wrote:
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> new file mode 100644
>> index 0000000..ff0f571
>> --- /dev/null
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -0,0 +1,71 @@
>> +/*
>> + * xen/arch/arm/gic-v3-its.c
>> + *
>> + * ARM GICv3 Interrupt Translation Service (ITS) support
>> + *
>> + * Copyright (C) 2016,2017 - ARM Ltd
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <xen/config.h>
> 
> No need to include xen/config.h it will be done by default.
> 
>> +#include <xen/lib.h>
>> +#include <xen/device_tree.h>
> 
> 
>> +#include <xen/libfdt/libfdt.h>
>> +#include <asm/gic.h>
>> +#include <asm/gic_v3_defs.h>
> 
> The 3 headers above does not look necessary for now. Please try to
> include them when needed.
> 
>> +#include <asm/gic_v3_its.h>
>> +
>> +/* Scan the DT for any ITS nodes and create a list of host ITSes out
>> of it. */
>> +void gicv3_its_dt_init(const struct dt_device_node *node)
>> +{
>> +    const struct dt_device_node *its = NULL;
>> +    struct host_its *its_data;
>> +
>> +    /*
>> +     * Check for ITS MSI subnodes. If any, add the ITS register
>> +     * frames to the ITS list.
>> +     */
>> +    dt_for_each_child_node(node, its)
>> +    {
>> +        paddr_t addr, size;
> 
> NIT: dt_device_get_address is taking uint64_t variables in parameter. So
> I would prefer to use uint64_t here for consistency.
> 
>> +
>> +        if ( !dt_device_is_compatible(its, "arm,gic-v3-its") )
>> +            continue;
>> +
>> +        if ( !dt_device_is_available(its) )
>> +            continue;
> 
> Can an ITS really be disabled? Or is it just for debugging?

This was indeed introduced for debugging, but is useful with multiple
ITSes. Firmware could ship with a DT covering the maximum hardware
configuration, then disabling not existing hardware at boot time.

And in general I consider this good style to support the status property.

>> +
>> +        if ( dt_device_get_address(its, 0, &addr, &size) )
>> +            panic("GICv3: Cannot find a valid ITS frame address");
>> +
>> +        its_data = xzalloc(struct host_its);
>> +        if ( !its_data )
>> +            panic("GICv3: Cannot allocate memory for ITS frame");
>> +
>> +        its_data->addr = addr;
>> +        its_data->size = size;
>> +        its_data->dt_node = its;
>> +
>> +        printk("GICv3: Found ITS @0x%lx\n", addr);
>> +
>> +        list_add_tail(&its_data->entry, &host_its_list);
>> +    }
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index b8be395..838dd11 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -43,9 +43,12 @@
>>  #include <asm/device.h>
>>  #include <asm/gic.h>
>>  #include <asm/gic_v3_defs.h>
>> +#include <asm/gic_v3_its.h>
>>  #include <asm/cpufeature.h>
>>  #include <asm/acpi.h>
>>
>> +LIST_HEAD(host_its_list);
> 
> I would rather limit the number of variable exported. I've looked at how
> host_its_list is used accross this series and I don't think it is
> necessary to export it.

Yes, I wasn't too happy to introduce it in the first place, but
originally needed it even beyond the current usage. So now that this is
gone, I am happy trying to confining it to this file, which indeed
sounds architecturally cleaner.

Cheers,
Andre.

> The 2 users (not including gic-v3-its.c) are in gic-v3.c and vgic-v3.c.
> I will explain how to replace the one in vgic-v3.c on the corresponding
> patch.
> 
> For gic-v3.c, you use host_its_list to check if ITS is available and
> going through the list. For the former, you could have gicv3_its_dt_init
> returning the number ITS available. For the latter, the loop is calling
> a function living in gic-v3-its.c where host_its_list is already available.
> 
> I will mention again when review the associated patches.
> 
>> +
>>  /* Global state */
>>  static struct {
>>      void __iomem *map_dbase;  /* Mapped address of distributor
>> registers */
>> @@ -1224,11 +1227,12 @@ static void __init gicv3_dt_init(void)
>>       */
>>      res = dt_device_get_address(node, 1 + gicv3.rdist_count,
>>                                  &cbase, &csize);
>> -    if ( res )
>> -        return;
>> +    if ( !res )
>> +        dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
>> +                              &vbase, &vsize);
>>
>> -    dt_device_get_address(node, 1 + gicv3.rdist_count + 2,
>> -                          &vbase, &vsize);
>> +    /* Check for ITS child nodes and build the host ITS list
>> accordingly. */
>> +    gicv3_its_dt_init(node);
>>  }
>>
>>  static int gicv3_iomem_deny_access(const struct domain *d)
>> diff --git a/xen/include/asm-arm/gic_v3_its.h
>> b/xen/include/asm-arm/gic_v3_its.h
>> new file mode 100644
>> index 0000000..2f5c51c
>> --- /dev/null
>> +++ b/xen/include/asm-arm/gic_v3_its.h
>> @@ -0,0 +1,57 @@
>> +/*
>> + * ARM GICv3 ITS support
>> + *
>> + * Andre Przywara <andre.przywara@xxxxxxx>
>> + * Copyright (c) 2016 ARM Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef __ASM_ARM_ITS_H__
>> +#define __ASM_ARM_ITS_H__
>> +
>> +#ifndef __ASSEMBLY__
> 
> Do you expect the ITS header to be included in the assembly code? If
> not, then I would drop the #ifndef __ASSEMBLY.
> 
>> +#include <xen/device_tree.h>
>> +
>> +/* data structure for each hardware ITS */
>> +struct host_its {
>> +    struct list_head entry;
>> +    const struct dt_device_node *dt_node;
>> +    paddr_t addr;
>> +    paddr_t size;
>> +};
>> +
>> +extern struct list_head host_its_list;
>> +
>> +#ifdef CONFIG_HAS_ITS
>> +
>> +/* Parse the host DT and pick up all host ITSes. */
>> +void gicv3_its_dt_init(const struct dt_device_node *node);
>> +
>> +#else
>> +
>> +static inline void gicv3_its_dt_init(const struct dt_device_node *node)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_HAS_ITS */
>> +
>> +#endif /* __ASSEMBLY__ */
>> +#endif
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>>
> 
> Cheers,
> 

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