Re: [PATCH v5 4/5] ARM: Update Formula to compute MADT size using new callbacks in gic_hw_operations

[Julien Grall <julien.grall@linaro.org>]

Hi,

On 10/10/17 11:33, Manish Jaggi wrote:
> 
> 
> On 10/10/2017 3:44 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 10/10/17 07:16, mjaggi@caviumnetworks.com wrote:
>>> From: Manish Jaggi <mjaggi@cavium.com>
>>>
>>> estimate_acpi_efi_size needs to be updated to provide correct size of
>>> hardware domains MADT, which now adds ITS information as well.
>>>
>>> This patch updates the formula to compute extra MADT size, as per 
>>> GICv2/3
>>> by calling gic_get_hwdom_extra_madt_size
>>
>> Missing full stop.
> oh i missed it.
>>
>>>
>>> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
>>> ---
>>>   xen/arch/arm/domain_build.c |  7 +------
>>>   xen/arch/arm/gic-v2.c       |  6 ++++++
>>>   xen/arch/arm/gic-v3.c       | 19 +++++++++++++++++++
>>>   xen/arch/arm/gic.c          | 12 ++++++++++++
>>>   xen/include/asm-arm/gic.h   |  3 +++
>>>   5 files changed, 41 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index d6f9585..f17fcf1 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -1808,12 +1808,7 @@ static int estimate_acpi_efi_size(struct 
>>> domain *d, struct kernel_info *kinfo)
>>>       acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
>>>       acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
>>>   -    madt_size = sizeof(struct acpi_table_madt)
>>> -                + sizeof(struct acpi_madt_generic_interrupt) * 
>>> d->max_vcpus
>>> -                + sizeof(struct acpi_madt_generic_distributor);
>>> -    if ( d->arch.vgic.version == GIC_V3 )
>>> -        madt_size += sizeof(struct acpi_madt_generic_redistributor)
>>> -                     * d->arch.vgic.nr_regions;
>>> +    madt_size = gic_get_hwdom_madt_size(d);
>>>       acpi_size += ROUNDUP(madt_size, 8);
>>>         addr = acpi_os_get_root_pointer();
>>> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
>>> index cbe71a9..0123ea4 100644
>>> --- a/xen/arch/arm/gic-v2.c
>>> +++ b/xen/arch/arm/gic-v2.c
>>> @@ -1012,6 +1012,11 @@ static int gicv2_iomem_deny_access(const 
>>> struct domain *d)
>>>       return iomem_deny_access(d, mfn, mfn + nr);
>>>   }
>>>   +static unsigned long gicv2_get_hwdom_extra_madt_size(const struct 
>>> domain *d)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>>   #ifdef CONFIG_ACPI
>>>   static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
>>>   {
>>> @@ -1248,6 +1253,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,
>>> +    .get_hwdom_extra_madt_size = gicv2_get_hwdom_extra_madt_size,
>>>       .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
>>>       .iomem_deny_access   = gicv2_iomem_deny_access,
>>>       .do_LPI              = gicv2_do_LPI,
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>> index b3d605d..447998d 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -1406,6 +1406,19 @@ static int gicv3_make_hwdom_madt(const struct 
>>> domain *d, u32 offset)
>>>       return table_len;
>>>   }
>>>   +static unsigned long gicv3_get_hwdom_extra_madt_size(const struct 
>>> domain *d)
>>> +{
>>> +    unsigned long size;
>>> +
>>> +    size  = sizeof(struct acpi_madt_generic_redistributor)
>>> +                    * d->arch.vgic.nr_regions;
>>
>> Here you align the * with struct. But below, you align with sizeof. 
>> Please stay consistent and always align with sizeof.
>>
>>> +
>>> +    size  += vgic_v3_its_count(d)
>>> +                    * sizeof(struct acpi_madt_generic_translator);
>>
>> Same here.
>>
> Could you please help with the specific section on coding style 
> guidelines on xen for indentation when line over 80 chars which I am not 
> following for this case.

The best guideline is the existing code around in the file, in doubt ask.

The code you copied was aligned with sizeof (see in domain_build.c) and 
now it does not have the same alignment.

Furthermore, you used double tab and not one tab (Xen is using 4 spaces).

Cheers

-- 
Julien Grall

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