Re: [PATCH v2 3/4] xen/arm: Handle reserved heap pages in boot and heap allocator

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 06/09/2022 12:11, Henry Wang wrote:
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > > > > +        {
> > > > > +            bank_start = bootinfo.reserved_mem.bank[i].start;
> > > > > +            bank_size = bootinfo.reserved_mem.bank[i].size;
> > > > > +            bank_end = bank_start + bank_size;
> > > > > +
> > > > > +            if ( bank_size < size )
> > > > > +                continue;
> > > > > +
> > > > > +            aligned_end = bank_end & ~(align - 1);
> > > > > +            aligned_start = (aligned_end - size) & ~(align - 1);
> > > > I find the logic a bit confusing. AFAIU, aligned_start could be below
> > > > the start of the RAM which is not what I would usually expect.
> > > Yeah I understand your concern. Here I want to make sure even if
> > > the given size is not aligned (although less likely happen in real life
> > > given the size calculation logic in setup_mm) the code still work.
> Sorry I probably explained in the wrong way in previous mail, but since no
> change requested here this is purely for discussion. In the code we
> are sure aligned_end calculation will make sure the end address will
> satisfy the alignment requirement within the range to a aligned (lower)
> address. The aligned_start = (aligned_end - size) & ~(align - 1) will make
> sure the start address is following the same alignment requirement, but
> the only issue would be in this case the start address will below the region
> start, hence the if ( aligned_start > bank_start ) check.
>
> > I don't think I agree on the less likely here. The regions are provided
> > by in the Device-Tree. And there are more chance they are incorrect
> > because the value will be specific to a software/device stack.
> >
> > Related to this discussion, I can't find any alignment requirement in
> > the device-tree binding. I think we at least want to require 64KB
> > aligned (so the same Device-Tree works if we were going to support 64KB
> > page granularity).
> I agree we need to require 64KB alignment, and currently we are following
> this because we are doing 32MB alignment.
Hmmm... I think we are talking about two different things here. What I 
am referring to is the alignment of the start/end of the region provided 
by the admin.
[...]

> > I think the code should be moved in populate_boot_allocator():
> >
> > if ( bootinfo.reserved_heap )
> > {
> >       for ( ...; i < bootinfo.reserved_mem.nr_banks; i++ )
> >          [....]
> >          init_boot_pages_pages()
> > }
> >
> > Note that to handle arm32, you will also need to exclude the xenheap area.
> When I implement the code, I found that the arm32 Xenheap excluding logic
> somehow can be reused.
>
> So I think I tried to reuse as much as current code. Would below
> populate_boot_allocator() seem ok to you?
I would prefer if they are separate because the logic can be simplified 
when using the static heap (the xenheap cannot across a region).
Something like:

for ( i = 0; i < banks->nr_banks; i++ )
{

#ifdef CONFIG_ARM_32
    if ( (bank_start >= mfn_to_maddr(direct_mfn_start) &&
           bank_end < mfn_to_maddr(direct_mfn_start) )
    {
      /* Add the memory *before* and *after* the region */
    }
    else
#endif
       init_boot_pages(s, e);
}

> static void __init populate_boot_allocator(void)
> {
>      unsigned int i;
>      const struct meminfo *banks = bootinfo.static_heap ?
>                                    &bootinfo.reserved_mem : &bootinfo.mem;
>
>      for ( i = 0; i < banks->nr_banks; i++ )
>      {
>          const struct membank *bank = &banks->bank[i];
>          paddr_t bank_end = bank->start + bank->size;
>          paddr_t s, e;
>
>          if ( bootinfo.static_heap && bank->type != MEMBANK_STATIC_HEAP )
>              continue;
>
>          s = bank->start;
>          while ( s < bank_end )
>          {
>              paddr_t n = bank_end;
>
>              if ( bootinfo.static_heap )
>                  e = bank_end;
>              else
>              {
>                  e = next_module(s, &n);
>
>                  if ( e == ~(paddr_t)0 )
>                      e = n = bank_end;
>
>                  /*
>                   * Module in a RAM bank other than the one which we are
>                   * not dealing with here.
>                   */
>                  if ( e > bank_end )
>                      e = bank_end;
>              }
>
> #ifdef CONFIG_ARM_32
>              /* Avoid the xenheap */
>              if ( s < mfn_to_maddr(directmap_mfn_end) &&
>                   mfn_to_maddr(directmap_mfn_start) < e )
>              {
>                  e = mfn_to_maddr(directmap_mfn_start);
>                  n = mfn_to_maddr(directmap_mfn_end);
>              }
> #endif
>
>              if ( bootinfo.static_heap )
>                  init_boot_pages(s, e);
>              else
>                  fw_unreserved_regions(s, e, init_boot_pages, 0);
>
>              s = n;
>          }
>      }
> }
>
> Kind regards,
> Henry
>
> > Cheers,
> >
> > --
> > Julien Grall
--
Julien Grall