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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Henry,

On 30/08/2022 08:11, Henry Wang wrote:
> 
> 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.
This is what I think would be better but maintainers will have a decisive vote.

> 
>> 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.
This is totally fine. What I mean is that the check you introduced does not 
correspond
to the panic message below. In case of reserved heap, its size is selected by 
the user.
"Not enough space for xenheap" means that there is not enough space to be 
reserved for heap,
meaning its size is too large. But your check is about size being too small.

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

FWICS, reserved_heap will be set to true even if a user describes an empty 
region
for reserved heap. This cannot be consider a properly defined region for a heap.

~Michal