RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Michal,

Sorry about the late reply - I had a couple of days off. Thank you very
much for the review! I will add my reply and answer some of your
questions below.

> -----Original Message-----
> From: Michal Orzel <michal.orzel@xxxxxxx>
> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
> 
> > This commit firstly adds a global variable `reserved_heap`.
> > This newly introduced global variable is set at the device tree
> > parsing time if the reserved heap ranges are defined in the device
> > tree chosen node.
> >
> Did you consider putting reserved_heap into bootinfo structure?

Actually I did, but I saw current bootinfo only contains some structs so
I was not sure if this is the preferred way, but since you are raising this
question, I will follow this method in v2.

> It would help to avoid introducing new global variables that are only used
> in places making use of the bootinfo anyway.

Ack.

> 
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > +            {
> > +                bank_start = bootinfo.reserved_mem.bank[i].start;
> > +                bank_size = bootinfo.reserved_mem.bank[i].size;
> > +                bank_end = bank_start + bank_size;
> > +
> > +                reserved_heap_size += bank_size;
> > +                reserved_heap_start = min(reserved_heap_start, bank_start);
> You do not need reserved_heap_start as you do not use it at any place later
> on.
> In your current implementation you just need reserved_heap_size and
> reserved_heap_end.

Good point, thank you and I will remove in v2.

> 
> >      /*
> >       * If the user has not requested otherwise via the command line
> >       * then locate the xenheap using these constraints:
> > @@ -743,7 +766,8 @@ static void __init setup_mm(void)
> >       * We try to allocate the largest xenheap possible within these
> >       * constraints.
> >       */
> > -    heap_pages = ram_pages;
> > +    heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages;
> I must say that the reverted logic is harder to read. This is a matter of 
> taste
> but
> please consider the following:
> heap_pages = reserved_heap ? reserved_heap_pages : ram_pages;
> The same applies to ...

Sure, I will use the way you suggested.

> 
> > +
> >      if ( opt_xenheap_megabytes )
> >          xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT);
> >      else
> > @@ -755,17 +779,21 @@ static void __init setup_mm(void)
> >
> >      do
> >      {
> > -        e = consider_modules(ram_start, ram_end,
> > +        e = !reserved_heap ?
> ... here.

And here :))

> 
> > +            consider_modules(ram_start, ram_end,
> >                               pfn_to_paddr(xenheap_pages),
> > -                             32<<20, 0);
> > +                             32<<20, 0) :
> > +            reserved_heap_end;
> > +
> >          if ( e )
> >              break;
> >
> >          xenheap_pages >>= 1;
> >      } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-
> PAGE_SHIFT) );
> >
> > -    if ( ! e )
> > -        panic("Not not enough space for xenheap\n");
> > +    if ( ! e ||
> > +         ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )
> I'm not sure about this. You are checking if the size of the reserved heap is
> less than 32MB
> and this has nothing to do with the following panic message.

Hmmm, I am not sure if I understand your question correctly, so here there
are actually 2 issues:
(1) The double not in the panic message.
(2) The size of xenheap.

If you check the comment of the xenheap constraints above, one rule of the
xenheap size is it "must be at least 32M". If I am not mistaken, we need to
follow the same rule with the reserved heap setup, so here we need to check
the size and if <32M then panic.

> 
> > +        panic("Not enough space for xenheap\n");
> >
> >      domheap_pages = heap_pages - xenheap_pages;
> >
> > @@ -810,9 +838,9 @@ static void __init setup_mm(void)
> >  static void __init setup_mm(void)
> >  {
> >      const struct meminfo *banks = &bootinfo.mem;
> > -    paddr_t ram_start = ~0;
> > -    paddr_t ram_end = 0;
> > -    paddr_t ram_size = 0;
> > +    paddr_t ram_start = ~0, bank_start = ~0;
> > +    paddr_t ram_end = 0, bank_end = 0;
> > +    paddr_t ram_size = 0, bank_size = 0;
> >      unsigned int i;
> >
> >      init_pdx();
> > @@ -821,17 +849,36 @@ static void __init setup_mm(void)
> >       * We need some memory to allocate the page-tables used for the
> xenheap
> >       * mappings. But some regions may contain memory already allocated
> >       * for other uses (e.g. modules, reserved-memory...).
> > -     *
> > +     * If reserved heap regions are properly defined, (only) add these
> regions
> How can you say at this stage whether the reserved heap regions are defined
> properly?

Because if the reserved heap regions are not properly defined, in the device
tree parsing phase the global variable "reserved_heap" can never be true.

Did I understand your question correctly? Or maybe we need to change the
wording here in the comment?

Kind regards,
Henry

> 
> > +     * in the boot allocator.
> > +     */
> > +    if ( reserved_heap )
> > +    {
> > +        for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            if ( bootinfo.reserved_mem.bank[i].xen_heap )
> > +            {
> > +                bank_start = bootinfo.reserved_mem.bank[i].start;
> > +                bank_size = bootinfo.reserved_mem.bank[i].size;
> > +                bank_end = bank_start + bank_size;
> > +
> > +                init_boot_pages(bank_start, bank_end);
> > +            }
> > +        }
> > +    }
> > +    /*
> > +     * No reserved heap regions:
> >       * For simplicity, add all the free regions in the boot allocator.
> >       */
> > -    populate_boot_allocator();
> > +    else
> > +        populate_boot_allocator();
> >
> >      total_pages = 0;
> >
> >      for ( i = 0; i < banks->nr_banks; i++ )
> >      {
> >          const struct membank *bank = &banks->bank[i];
> > -        paddr_t bank_end = bank->start + bank->size;
> > +        bank_end = bank->start + bank->size;
> >
> >          ram_size = ram_size + bank->size;
> >          ram_start = min(ram_start, bank->start);
> > --
> > 2.17.1
> >
> >
> 
> ~Michal