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>
> >>>
> >> 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...

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