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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and
> heap allocator
> 
> On Tue, 30 Aug 2022, Henry Wang wrote:
> > > -----Original Message-----
> > > From: Michal Orzel <michal.orzel@xxxxxxx>
> > > >
> > > > 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"
> 
> I think we should have a separate check specific for the device tree
> input parameters to make sure the region is correct, that way we can
> have a specific error message, such as:
> 
> "xen,static-heap address needs to be 32MB aligned and the size a
> multiple of 32MB."

Sure, will follow this.

Kind regards,
Henry