Re: [Xen-devel] [PATCH v4 2/5] ARM: ITS: Populate host_its_list from ACPI MADT Table

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 04/10/17 06:29, Manish Jaggi wrote:
> Hello Julien,
> 
> On 10/3/2017 7:17 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 21/09/17 14:17, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
>>> From: Manish Jaggi <mjaggi@xxxxxxxxxx>
>>>
>>> Added gicv3_its_acpi_init to update host_its_list from MADT table.
>>> For ACPI, host_its structure  stores dt_node as NULL.
>>>
>>> Signed-off-by: Manish Jaggi <mjaggi@xxxxxxxxxx>
>>> ---
>>>   xen/arch/arm/gic-v3-its.c        | 24 ++++++++++++++++++++++++
>>>   xen/arch/arm/gic-v3.c            |  2 ++
>>>   xen/include/asm-arm/gic_v3_its.h | 10 ++++++++++
>>>   3 files changed, 36 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>>> index 0610991..0f662cf 100644
>>> --- a/xen/arch/arm/gic-v3-its.c
>>> +++ b/xen/arch/arm/gic-v3-its.c
>>> @@ -18,6 +18,7 @@
>>>    * along with this program; If not, see
>>> <http://www.gnu.org/licenses/>.
>>>    */
>>>   +#include <xen/acpi.h>
>>>   #include <xen/lib.h>
>>>   #include <xen/delay.h>
>>>   #include <xen/libfdt/libfdt.h>
>>> @@ -1018,6 +1019,29 @@ void gicv3_its_dt_init(const struct
>>> dt_device_node *node)
>>>       }
>>>   }
>>>   +#ifdef CONFIG_ACPI
>>> +static int gicv3_its_acpi_probe(struct acpi_subtable_header *header,
>>> +                                const unsigned long end)
>>> +{
>>> +    struct acpi_madt_generic_translator *its;
>>> +
>>> +    its = (struct acpi_madt_generic_translator *)header;
>>> +    if ( BAD_MADT_ENTRY(its, end) )
>>> +        return -EINVAL;
>>> +
>>> +    add_to_host_its_list(its->base_address, GICV3_ITS_SIZE, NULL);
>>
>> After the comment from Andre, I was expecting some rework to avoid
>> store the size of the ITS in host_its. So what's the plan for that?
> GICV3_ITS_SIZE  is now 128K (prev 64k, see below), same as what used in
> linux code, I think andre mentioned that need to add additional 64K.

That was one thing, but I was wondering about why we would need to store
that value as a *variable* in struct host_its when it is actually an
architecture defined constant. But as it was there before and it seems
cleaner to use the DT provided size, it could stay as well. We might fix
that later on.

>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +void gicv3_its_acpi_init(void)
>>> +{
>>> +    /* Parse ITS information */
>>> +    acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>>> +                            gicv3_its_acpi_probe, 0);
>>
>> The indentation still looks wrong here.
> ah.. ok.

So ignoring that "size" thing above and assuming this w/s issue fixed:

Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>

Cheers,
Andre

>>
>>> +}
>>> +#endif
>>> +
>>>   /*
>>>    * Local variables:
>>>    * mode: C
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index f990eae..6f562f4 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -1567,6 +1567,8 @@ static void __init gicv3_acpi_init(void)
>>>         gicv3.rdist_stride = 0;
>>>   +    gicv3_its_acpi_init();
>>> +
>>>       /*
>>>        * In ACPI, 0 is considered as the invalid address. However the
>>> rest
>>>        * of the initialization rely on the invalid address to be
>>> diff --git a/xen/include/asm-arm/gic_v3_its.h
>>> b/xen/include/asm-arm/gic_v3_its.h
>>> index 1fac1c7..e1be33c 100644
>>> --- a/xen/include/asm-arm/gic_v3_its.h
>>> +++ b/xen/include/asm-arm/gic_v3_its.h
>>> @@ -20,6 +20,7 @@
>>>   #ifndef __ASM_ARM_ITS_H__
>>>   #define __ASM_ARM_ITS_H__
>>>   +#define GICV3_ITS_SIZE                  SZ_128K
>>
>> A less random place for this is close to the ITS_DOORBELL_OFFSET
>> definition.
> ok will do :)
>>
>>>   #define GITS_CTLR 0x000
>>>   #define GITS_IIDR                       0x004
>>>   #define GITS_TYPER                      0x008
>>> @@ -135,6 +136,9 @@ extern struct list_head host_its_list;
>>>   /* Parse the host DT and pick up all host ITSes. */
>>>   void gicv3_its_dt_init(const struct dt_device_node *node);
>>>   +#ifdef CONFIG_ACPI
>>> +void gicv3_its_acpi_init(void);
>>> +#endif
>>>   bool gicv3_its_host_has_its(void);
>>>     unsigned int vgic_v3_its_count(const struct domain *d);
>>> @@ -196,6 +200,12 @@ static inline void gicv3_its_dt_init(const
>>> struct dt_device_node *node)
>>>   {
>>>   }
>>>   +#ifdef CONFIG_ACPI
>>> +static inline void gicv3_its_acpi_init(void)
>>> +{
>>> +}
>>> +#endif
>>> +
>>>   static inline bool gicv3_its_host_has_its(void)
>>>   {
>>>       return false;
>>>
>>
>> Cheers,
>>
> 

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