Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

Sorry for the late answer.

On 26/11/2024 13:52, Luca Fancellu wrote:
> > On 26 Nov 2024, at 13:29, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >
> > On 26.11.2024 14:25, Luca Fancellu wrote:
> > > > This reads better, thanks. Follow-on question: Is what is statically
> > > > configured for the heap guaranteed to never overlap with anything passed
> > > > to init_domheap_pages() in those places that you touch?
> > > I think so, the places of the check are mainly memory regions related to boot 
> > > modules,
> > > when we add a boot module we also do a check in order to see if it clashes with 
> > > any
> > > reserved regions already defined, which the static heap is part of.
> > >
> > > Could you explain me why the question?
> > Well, if there was a chance of overlap, then parts of the free region would
> > need to go one way, and the rest the other way.
> oh ok, sure of course, thanks for answering.
>
> > > > > > > --- a/xen/include/xen/bootfdt.h
> > > > > > > +++ b/xen/include/xen/bootfdt.h
> > > > > > > @@ -132,7 +132,6 @@ struct bootinfo {
> > > > > > > #ifdef CONFIG_STATIC_SHM
> > > > > > >    struct shared_meminfo shmem;
> > > > > > > #endif
> > > > > > > -    bool static_heap;
> > > > > > > };
> > > > > > >
> > > > > > > #ifdef CONFIG_ACPI
> > > > > > > @@ -157,6 +156,10 @@ struct bootinfo {
> > > > > > >
> > > > > > > extern struct bootinfo bootinfo;
> > > > > > >
> > > > > > > +#ifdef CONFIG_STATIC_MEMORY
> > > > > > > +extern bool static_heap;
> > > > > > > +#endif
> > > > > > Just to double check Misra-wise: Is there a guarantee that this header will
> > > > > > always be included by page-alloc.c, so that the definition of the symbol
> > > > > > has an earlier declaration already visible? I ask because this header
> > > > > > doesn't look like one where symbols defined in page-alloc.c would normally
> > > > > > be declared. And I sincerely hope that this header isn't one of those that
> > > > > > end up being included virtually everywhere.
> > > > > I’ve read again MISRA rule 8.4 and you are right, I should have included 
> > > > > bootfdt.h in
> > > > > page-alloc.c in order to have the declaration visible before defining 
> > > > > static_heap.
> > > > >
> > > > > I will include the header in page-alloc.c
> > > > Except that, as said, I don't think that header should be included in this file.
> > > > Instead I think the declaration wants to move elsewhere.
> > > Ok sorry, I misunderstood your comment, I thought you were suggesting to have 
> > > the
> > > declaration visible in that file since we are defining there the variable.
> > >
> > > So Julien suggested that file, it was hosted before in 
> > > common/device-tree/device-tree.c,
> > > see the comment here:
> > > https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-6-luca.fancellu@xxxxxxx/#26125054
> > >
> > > Since it seems you disagree with Julien, could you suggest a more suitable 
> > > place?
> > Anything defined in page-alloc.c should by default have its declaration in
> > xen/mm.h, imo. Exceptions would need justification.
> I would be fine to have the declaration in xen/mm.h, I just need to import 
> xen/mm.h in bootfdt.h so that it is visible to
> “using_static_heap”, @Julien would you be ok with that?
I think using_static_heap() should be defined in xen/mm.h as well 
because we expect everyone to use using_static_heap() rather than 
static_heap.
This is to avoid adding #ifdef for every user.

Cheers,

--
Julien Grall