RE: [PATCH V3 05/10] xen/arm: static memory initialization

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, July 19, 2021 4:20 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [PATCH V3 05/10] xen/arm: static memory initialization
> 
> On 15.07.2021 07:18, Penny Zheng wrote:
> > v3 change:
> > - include addition of CONFIG_STATIC_ALLOCATION in this commit, where
> > it is firstly used and also change the name to CONFIG_STATIC_MEMORY
> > - Fix TAB typo in Kconfig
> 
> Not sure what this relates to, but ...
> 

Before I was wrongly using " set tabstop=4 " for Kconfig file, so...

> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -106,6 +106,9 @@ config TEE
> >
> >  source "arch/arm/tee/Kconfig"
> >
> > +config STATIC_MEMORY
> > +        def_bool y
> 
> ... this is (wrongly) using spaces for indentation.
> 
> I also wonder about the placement: Shouldn't the option live in common code,
> with Arm "select"ing it?
> 

Sure, I will place it in xen/common/Kconfig, and add "depends on ARM".

> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -622,6 +622,28 @@ static void __init init_pdx(void)
> >      }
> >  }
> >
> > +/* Static memory initialization */
> > +static void __init init_staticmem_pages(void) {
> > +    unsigned int bank;
> > +
> > +    /* TODO: Considering NUMA-support scenario. */
> > +    for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ )
> > +    {
> > +        paddr_t bank_start = bootinfo.static_mem.bank[bank].start;
> > +        paddr_t bank_size = bootinfo.static_mem.bank[bank].size;
> > +        paddr_t bank_end = bank_start + bank_size;
> > +
> > +        bank_start = round_pgup(bank_start);
> > +        bank_end = round_pgdown(bank_end);
> > +        if ( bank_end <= bank_start )
> > +            return;
> > +
> > +        free_staticmem_pages(maddr_to_page(bank_start),
> > +                            (bank_end - bank_start) >> PAGE_SHIFT,
> > + false);
> 
> Indentation (one too few spaces). Perhaps also consider to avoid open-coding
> PFN_DOWN() here; in fact it and PFN_UP() could be used in place of
> round_pg{down,up}() above.
> 

Sure. I will replace round_pg{down,up}() with PFN_DOWN()/PFN_UP().

> Jan

Cheers

Penny Zheng