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

[Henry Wang <Henry.Wang@arm.com>]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> >
> > Oh I think get your point. Let me try to explain myself and thanks for your
> > patience :))
> >
> > The reserved heap region defined in the device tree should be used for
> both
> > Xenheap and domain heap, so if we reserved a too small region (<32M),
> > an error should pop because the reserved region is not enough for
> xenheap,
> > and user should reserve more.
> > [...]
> >
> >> 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.
> >
> > [...]
> > So in that case, printing "Not enough space for xenheap" means the
> reserved
> > region cannot satisfy the minimal requirement of the space of xenheap (at
> least
> > 32M), and this is in consistent with the check.
> 
> Ok, it clearly depends on the way someone understands this sentence.
> Currently this panic can be triggered if the heap size is too large and
> should be read as "heap is too large to fit in because there is not enough
> space
> within RAM considering modules (e - s < size)". Usually (and also in this case)
> space refers to a region to contain another one.
> 
> You are reusing the same message for different meaning, that is "user
> defined too
> small heap and this space (read as size) is not enough".

Yes, thanks for the explanation. I think maybe rewording the message
to "Not enough memory for allocating xenheap" would remove the ambiguity
to some extent? Because the user-defined heap region should cover both
xenheap and domain heap at the same time, the small user-defined heap
means "xenheap is too large to fit in the user-defined heap region", which is
in consistent with your interpretation of the current "xenheap is too large to fit
in because there is not enough space within RAM considering modules"

> 
> Let's leave it to someone else to decide.

I agree.

Kind regards,
Henry

> 
> ~Michal