Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Julien Grall <julien.grall@arm.com>]

On 09/06/2017 07:48, Manish Jaggi wrote:
>
> On 6/8/2017 7:28 PM, Julien Grall wrote:
>> Hi,
> Hello Julien,

Hello,

>>> +    list_for_each_entry(its_data, &host_its_list, entry)
>>> +    {
>>
>> Pointless {
>>
>>> +        size += sizeof(struct acpi_madt_generic_translator);
>>> +    }
> Just for readability of code.

You have indentation for that. So I don't think it helps.

>>
>> Same here + add a newline.
>>
> Sure.
>>> +    return size;
>>> +}
>>> +
>>> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
>>> +{
>>> +    struct acpi_madt_generic_translator *gic_its;
>>> +    const struct host_its *its_data;
>>> +    u32 table_len = offset, size;
>>> +
>>> +    /* Update GIC ITS information in hardware domain's MADT */
>>> +    list_for_each_entry(its_data, &host_its_list, entry)
>>> +    {
>>> +        size = sizeof(struct acpi_madt_generic_translator);
>>> +        gic_its = (struct acpi_madt_generic_translator *)(base_ptr +
>>> table_len);
>>
>> This line is likely too long.
>>
> I will check it.
>>> +        gic_its->header.type = ACPI_MADT_TYPE_GENERIC_TRANSLATOR;
>>> +        gic_its->header.length = size;
>>> +        gic_its->base_address = its_data->addr;
>>
>> On the previous patch you had:
>>
>> gic_its->translation_id = its_data->translation_id;
>>
>> I asked to explain why you need to have the same ID as the host. And
>> now you dropped it. This does not match the spec (Table 5-67 in ACPI
>> 6.1):
>>
>> "GIC ITS ID. In a system with multiple GIC ITS units, this value must
>> be unique to each one."
>>
>> But here, the ITS ID will not be unique. So why did you dropped it?
>>
> The reason I dropped it from its_data as I was not setting it. So it
> doesn't belong there.

Where would it belong then?

This function is used to generate ACPI tables for the hardware domain.


> Will the below code be ok?

If you noticed, I didn't say this code is wrong. Instead I asked why you 
use the same ID. Meaning, is there anything in the DSDT requiring this 
value?

> + int tras_id = 0;

unsigned.

> + list_for_each_entry(its_data, &host_its_list, entry)
> + {
> +    gic_its->translation_id = ++trans_id;

You start the translation ID at 1. Why?

>
>>> +        table_len +=  size;
>>> +    }
>>> +    return table_len;
>>> +}
>>> +
>>>  /*
>>>   * Create the respective guest DT nodes from a list of host ITSes.
>>>   * This copies the reg property, so the guest sees the ITS at the same
>>> address
>>> @@ -992,6 +1045,26 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
>>> domain *d,
>>>      return res;
>>>  }
>>>
>>> +int gicv3_its_acpi_init(struct acpi_subtable_header *header, const
>>> unsigned long end)
>>
>> ACPI is an option and is not able by default. Please make sure that
>> this code build without ACPI. Likely this means surrounding with
>> #ifdef CONFIG_ACPI.
> I will get compiled but not called. Do you still want to put ifdef, i
> can add that.

All ACPIs functions are protected by ifdef. So this one should be as well.

>>
>>> +{
>>> +    struct acpi_madt_generic_translator *its_entry;
>>> +    struct host_its *its_data;
>>> +
>>> +    its_data = xzalloc(struct host_its);
>>> +    if (!its_data)
>>
>> Coding style.
>>
> Sure.
>>> +        return -1;
>>> +
>>> +    its_entry = (struct acpi_madt_generic_translator *)header;
>>> +    its_data->addr  = its_entry->base_address;
>>> +    its_data->size = ACPI_GICV3_ITS_MEM_SIZE;
>>> +
>>> +    spin_lock_init(&its_data->cmd_lock);
>>> +
>>> +    printk("GICv3: Found ITS @0x%lx\n", its_data->addr);
>>> +
>>> +    list_add_tail(&its_data->entry, &host_its_list);
>>
>> As said on v1, likely you could re-use factorize a part of
>> gicv3_its_dt_init to avoid implementing twice the initialization.
>>
> For this I have a different opinion.

Why didn't you state it on the previous version? I usually interpret a 
non-answer as an acknowledgment.

> gicv3_its_dt_init has a loop  dt_for_each_child_node(node, its) while
> gicv3_its_acpi_init is a callback.
> Moreover,  apart from xzalloc and list_add_tail most of the code is
> different. so IMHO keeping them separate is better.

You still set addr and size as in the DT counterpart. Also, this is a 
call to forget to initialize a field if we decided to extend the 
structure host_its. So I still don't see any reason to open-code it and 
take the risk to introduce bug in the future...

>> Also newline.
>>
>>> +    return 0;
>>> +}
>>
>> Newline here.
> Sure.
>>
>>>  /* 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)
>>>  {
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index c927306..f0f6d12 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -1333,9 +1333,8 @@ static int gicv3_iomem_deny_access(const struct
>>> domain *d)
>>>          return iomem_deny_access(d, mfn, mfn + nr);
>>>      }
>>>
>>> -    return 0;
>>> +    return gicv3_its_deny_access(d);
>>
>> Copying my answer from v1 for convenience:
>>
>>     if ( vbase != INVALID_PADDR )
>>     {
>>         mfn = vbase >> PAGE_SHIFT;
>>         nr = DIV_ROUND_UP(csize, PAGE_SIZE);
>>         return iomem_deny_access(d, mfn, mfn + nr);
>>     }
>>
>> When GICv3 is able to support GICv2, vbase will be valid and the code
>> will bail out after denying access to the GICV. So the ITS regions
>> will not be denied.
>>
> Ok, got your point. Would the below code be ok?

LGTM.

> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c927306..a3d1eff 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1308,6 +1308,13 @@ static int gicv3_iomem_deny_access(const struct
> domain *d)
>      if ( rc )
>          return rc;
>
> +    if ( gicv3_its_host_has_its() )
> +    {
> +        rc = gicv3_its_deny_access(d);
> +        if ( rc )
> +            return rc;
> +    }
> +
>      for ( i = 0; i < gicv3.rdist_count; i++ )
>      {
>          mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
>>>  }
>>> -
>>
>> Again, why did you drop this newline?
> I will fix it
>>
>>>  #ifdef CONFIG_ACPI
>>>  static void __init
>>>  gic_acpi_add_rdist_region(paddr_t base, paddr_t size, bool
>>> single_rdist)
>>> @@ -1374,6 +1373,7 @@ static int gicv3_make_hwdom_madt(const struct
>>> domain *d, u32 offset)
>>>      for ( i = 0; i < d->max_vcpus; i++ )
>>>      {
>>>          gicc = (struct acpi_madt_generic_interrupt *)(base_ptr +
>>> table_len);
>>> +
>>
>> As said on v1, spurious change.
> No. I wanted to add it intentionally. for a better code readability.

A general and simple rule is to separate code clean-up with new 
functionality. This is not a new functionality and therefore should not 
be there.

>>
>>>          ACPI_MEMCPY(gicc, host_gicc, size);
>>>          gicc->cpu_interface_number = i;
>>>          gicc->uid = i;
>>> @@ -1399,7 +1399,7 @@ static int gicv3_make_hwdom_madt(const struct
>>> domain *d, u32 offset)
>>>          gicr->length = d->arch.vgic.rdist_regions[i].size;
>>>          table_len += size;
>>>      }
>>> -
>>
>> Again why did you drop the newline?
> I will fix it
>>
>>> +    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
>>>      return table_len;
>>>  }
>>>
>>> @@ -1567,6 +1567,9 @@ static void __init gicv3_acpi_init(void)
>>>
>>>      gicv3.rdist_stride = 0;
>>>
>>> +    acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>>> +                          gicv3_its_acpi_init, 0);
>>
>> As said on v1, acpi_table_parse_madt may return an error. Why this is
>> not checked?
>>
> I missed that, I will add it,
>>> +
>>>      /*
>>>       * In ACPI, 0 is considered as the invalid address. However the
>>> rest
>>>       * of the initialization rely on the invalid address to be
>>> @@ -1585,6 +1588,7 @@ static void __init gicv3_acpi_init(void)
>>>      else
>>>          vsize = GUEST_GICC_SIZE;
>>>
>>> +
>>
>> As said on v1, this is a spurious newline.
> ok
>>
>>>  }
>>>  #else
>>>  static void __init gicv3_acpi_init(void) { }
>>> diff --git a/xen/include/asm-arm/gic_v3_its.h
>>> b/xen/include/asm-arm/gic_v3_its.h
>>> index d2a3e53..b72aec2 100644
>>> --- a/xen/include/asm-arm/gic_v3_its.h
>>> +++ b/xen/include/asm-arm/gic_v3_its.h
>>> @@ -105,6 +105,7 @@
>>>
>>>  #include <xen/device_tree.h>
>>>  #include <xen/rbtree.h>
>>> +#include <xen/acpi.h>
>>>
>>>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>>>  #define HOST_ITS_USES_PTA               (1U << 1)
>>> @@ -134,6 +135,7 @@ 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);
>>> +int  gicv3_its_acpi_init(struct acpi_subtable_header *header, const
>>> unsigned long end);
>>
>> This will likely need an #ifdef CONFIG_ACPI. And also a stub would be
>> required if ITS is disabled.
>>
> Sorry didnt got your point. on ifdef.

Look how gicv3_its_dt_init has been introduced. There is a stub for when 
ITS is not built-in. Furthermore, the function is ACPI specific are 
therefore should be protected with #ifdef CONFIG_ACPI.

> I can add a check gicv3_its_host_has_its() before calling
> gicv3_its_acpi_init, would that be ok?

gicv3_its_host_has_its will always return false as it is rely on the 
list of ITS to be populated. However, this will be populated by 
gicv3_its_acpi_init...

Regards,

-- 
Julien Grall

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