Re: [RFC 02/11] acpi: arm: API to query estimated size of hardware domain's IORT

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

Hi,

On 19/01/18 06:10, Manish Jaggi wrote:
> 
> 
> On 01/17/2018 12:22 AM, Julien Grall wrote:
>>>   IORT for hardware domain is generated using the requesterId and 
>>> deviceId map.
>>>
>>>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.com>
>>> ---
>>>   xen/arch/arm/domain_build.c     |  12 ++++-
>>>   xen/drivers/acpi/arm/Makefile   |   1 +
>>>   xen/drivers/acpi/arm/gen-iort.c | 101 
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   xen/include/acpi/gen-iort.h     |   6 +++
>>>   4 files changed, 119 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index c74f4dd69d..f5d5e3d271 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -14,6 +14,7 @@
>>>   #include <xen/acpi.h>
>>>   #include <xen/warning.h>
>>>   #include <acpi/actables.h>
>>> +#include <acpi/gen-iort.h>
>>>   #include <asm/device.h>
>>>   #include <asm/setup.h>
>>>   #include <asm/platform.h>
>>> @@ -1799,7 +1800,7 @@ static int acpi_create_fadt(struct domain *d, 
>>> struct membank tbl_add[])
>>>     static int estimate_acpi_efi_size(struct domain *d, struct 
>>> kernel_info *kinfo)
>>>   {
>>> -    size_t efi_size, acpi_size, madt_size;
>>> +    size_t efi_size, acpi_size, madt_size, iort_size;
>>
>> Rather than introduce a variable for 10 instructions, you can rename 
>> madt_size so it can be re-used. I would be ok for this to be in the 
>> same patch (providing a proper commit message).
> Why would you want to replace iort_size with madt_size ?
> What is the harm if adding a variable makes the code more verbose.
> I am not able to appreciate your point here.

I didn't ask to replace iort_size with madt_size. But rename madt_size 
to table_size or some other name that could be reused for both.

This is very similar to when you store the error return of a function. 
You are not going to name ret_foo, ret_bar, ret_fish... You are just 
going to use one variable and re-use it.

Anyway, I am not going to fight with that and just send a patch to clean 
that up once it has been merged.

[...]

>>>   diff --git a/xen/drivers/acpi/arm/Makefile 
>>> b/xen/drivers/acpi/arm/Makefile
>>> index 046fad5e3d..13f1a9159f 100644
>>> --- a/xen/drivers/acpi/arm/Makefile
>>> +++ b/xen/drivers/acpi/arm/Makefile
>>> @@ -1 +1,2 @@
>>>   obj-y = ridmap.o
>>> +obj-y += gen-iort.o
>>> diff --git a/xen/drivers/acpi/arm/gen-iort.c 
>>> b/xen/drivers/acpi/arm/gen-iort.c
>>> new file mode 100644
>>> index 0000000000..3fc32959c6
>>> --- /dev/null
>>> +++ b/xen/drivers/acpi/arm/gen-iort.c
>>> @@ -0,0 +1,101 @@
>>> +/*
>>> + * xen/drivers/acpi/arm/gen-iort.c
>>> + *
>>> + * Code to generate IORT for hardware domain using the requesterId
>>> + * and deviceId map.
>>> + *
>>> + * Manish Jaggi <manish.jaggi@linaro.com>
>>> + * Copyright (c) 2018 Linaro.
>>> + *
>>> + * 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.
>>
>> The license is wrong (see patch #1).
> Please see my comment in patch #1.
> This license is used from an existing file in xen.
> So there are a lot of wrong licenses in xen code.

Well yes. But does it mean you have to add more wrong code? ;)

Cheers,

-- 
Julien Grall

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