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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > +    paddr_t reserved_heap_start = ~0, reserved_heap_end = 0,
> 
> INVALID_PADDR or ~0ULL

Ack.

> 
> >      /*
> >       * 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;
> > +
> >      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 ?
> > +            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) ) )
> > +        panic("Not enough space for xenheap\n");
> 
> 
> I would skip the do/while loop completely if reserved_heap. We don't
> need it anyway

I agree with this.

> and we can automatically calculate xenheap_pages in a single line.

Here I am a little bit confused. Sorry to ask but could you please explain
a little bit more about why we can calculate the xenheap_pages in a single
line? Below is the code snippet in my mind, is this correct?

if (reserved_heap)
    e = reserved_heap_end;
else
{
    do
    {
        e = consider_modules(ram_start, ram_end,
                             pfn_to_paddr(xenheap_pages),
                             32<<20, 0);
        if ( e )
            break;

        xenheap_pages >>= 1;
    } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) );
}

> 
> >      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;
> 
> Please use INVALID_PADDR or ~0ULL

Ack.

Kind regards,
Henry

> 
> 
> >
> >      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
> > +     * 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
> >