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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx wrote:
> > > From: Manish Jaggi <mjaggi@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel