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 10:00, Henry Wang wrote:
> 
> Hi Michal,
> 
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@xxxxxxx>
>>>>>
>>>> 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.
> 
> Then let's wait for more input from maintainers.
> 
>>
>>>>>
>>>>> -    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.
> 
> Actually my understanding of "Not enough space for xenheap" is xenheap
> is too large so we need to reserve more space, which is slightly different 
> than
> your opinion. But I am not the native speaker so it is highly likely that I am
> making mistakes...My understanding is exactly the same as yours :), meaning 
> heap is too large.
But your check is against heap being to small (less than 32M).
So basically if the following check fails:
"( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )"
it means that the heap region defined by a user is too small (not too large),
because according to requirements it should be at least 32M.

> 
> How about changing the panic message to "Not enough memory for xenheap"?
> This would remove the ambiguity here IMHO.
> 
>>
>>>>> +     * 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.
> 
> Oh good point, thank you for pointing this out. I will change the comments
> here to "If there are non-empty reserved heap regions". I am not sure if 
> adding
> an empty region check before setting the "reserved_heap" would be a good
> idea, because adding such check would add another for loop to find a non-empty
> reserved heap bank. What do you think?
> 
> Kind regards,
> Henry
> 
>>
>> ~Michal