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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

Thanks for the clarification and your patience. For the
populate_boot_allocator() change, I attached my change in the end,
and personally I would like to hear your opinion before sending v3
since we now have limited time.

> -----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. I will add a comment in the
function comment to mention we at least want a 64KB alignment so that
future callers will not make mistakes.

> 
> >>
> >>> +
> >>> +            if ( aligned_start > bank_start )
> >>> +                /*
> >>> +                 * Arm32 allocates xenheap from higher address to lower, 
> >>> so if
> >>
> >> This code is also called on arm32. So what are you referring to? Is it
> >> consider_modules()?
> >
> > Yes, I think the current arm32 behavior in consider_modules() is what
> > I am referring to. In fact, I just want to add some comments that explain
> why
> > we need the end = max(end, aligned_end) since technically if there are
> > multiple reserved heap banks and all of them can fit the xenheap region,
> > we can use either of them. But following the current behavior we can only
> use
> > the highest bank to keep the consistency.
> 
> Xenheap is currently allocated the highest possible so there is enough
> low memory available for domain memory. This is in order to allow 32-bit
> DMA device to function.
> 
> I am less certain this makes sense when the heap is reserved. Because an
> admin could decide to define the heap solely above/below 4GB.
> 
> That said, nothing in the document suggests that domain memory would not
> be allocated from the reserved heap. So I would suggest to write the
> following comment:
> 
> "Allocate the xenheap as high as possible to keep low-memory available
> (assuming the admin supplied region below 4GB) for other use (e.g.
> domain memory allocation)."

Sure.

> 
> Also, I think the documentation wants to be updated to clarify whether
> the reserved heap could be used to allocate domain. If it could, then I
> think we need to explain that the region should contain enough memory
> below some 4GB to cater 32-bit DMA.

Ok I will add in v3.

> 
> >>> +    /*
> >>> +     * No reserved heap regions:
> >>>         * For simplicity, add all the free regions in the boot allocator.
> >>>         */
> >>> -    populate_boot_allocator();
> >>> +    else
> >>> +        populate_boot_allocator();
> >>
> >> For arm32, shouldn't we also only add the reserved heap (minus the
> >> xenheap) to the boot allocator? At which point, I would move the change
> >> in populate_boot_allocator().
> >
> > Sorry I am not sure what this comment about...as here the code is for
> arm64.
> 
> Right, I wasn't sure where to comment because you don't touch the call
> to populate_boot_allocator().
> 
> > For the question, yes.
> > For the latter one, do you request some changes? If so, could you please
> kindly
> > elaborate a little bit more? Thanks.
> 
> Yes I am requesting some change because I think the code on arm32 is
> incorrect (the boot allocator will not be populated with the reserved heap).
> 
> 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?

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