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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxx>
> > > + * 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel