RE: [PATCH 03/10] xen/arm: introduce PGC_reserved

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi julien 

> -----Original Message-----
> From: Penny Zheng
> Sent: Thursday, May 20, 2021 2:20 PM
> To: Julien Grall <julien@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: RE: [PATCH 03/10] xen/arm: introduce PGC_reserved
> 
> Hi Julien
> 
> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: Thursday, May 20, 2021 3:46 AM
> > To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > sstabellini@xxxxxxxxxx
> > Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> > <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> > Subject: Re: [PATCH 03/10] xen/arm: introduce PGC_reserved
> >
> >
> >
> > On 19/05/2021 04:16, Penny Zheng wrote:
> > > Hi Julien
> >
> > Hi Penny,
> >
> > >
> > >> -----Original Message-----
> > >> From: Julien Grall <julien@xxxxxxx>
> > >> Sent: Tuesday, May 18, 2021 5:46 PM
> > >> To: Penny Zheng <Penny.Zheng@xxxxxxx>;
> > >> xen-devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx
> > >> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> > >> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> > >> Subject: Re: [PATCH 03/10] xen/arm: introduce PGC_reserved
> > >>
> > >>
> > >>
> > >> On 18/05/2021 06:21, Penny Zheng wrote:
> > >>> In order to differentiate pages of static memory, from those
> > >>> allocated from heap, this patch introduces a new page flag
> > >>> PGC_reserved
> > to tell.
> > >>>
> > >>> New struct reserved in struct page_info is to describe reserved
> > >>> page info, that is, which specific domain this page is reserved
> > >>> to. > Helper page_get_reserved_owner and page_set_reserved_owner
> > >>> are designated to get/set reserved page's owner.
> > >>>
> > >>> Struct domain is enlarged to more than PAGE_SIZE, due to
> > >>> newly-imported struct reserved in struct page_info.
> > >>
> > >> struct domain may embed a pointer to a struct page_info but never
> > >> directly embed the structure. So can you clarify what you mean?
> > >>
> > >>>
> > >>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > >>> ---
> > >>>    xen/include/asm-arm/mm.h | 16 +++++++++++++++-
> > >>>    1 file changed, 15 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> > >> index
> > >>> 0b7de3102e..d8922fd5db 100644
> > >>> --- a/xen/include/asm-arm/mm.h
> > >>> +++ b/xen/include/asm-arm/mm.h
> > >>> @@ -88,7 +88,15 @@ struct page_info
> > >>>             */
> > >>>            u32 tlbflush_timestamp;
> > >>>        };
> > >>> -    u64 pad;
> > >>> +
> > >>> +    /* Page is reserved. */
> > >>> +    struct {
> > >>> +        /*
> > >>> +         * Reserved Owner of this page,
> > >>> +         * if this page is reserved to a specific domain.
> > >>> +         */
> > >>> +        struct domain *domain;
> > >>> +    } reserved;
> > >>
> > >> The space in page_info is quite tight, so I would like to avoid
> > >> introducing new fields unless we can't get away from it.
> > >>
> > >> In this case, it is not clear why we need to differentiate the "Owner"
> > >> vs the "Reserved Owner". It might be clearer if this change is
> > >> folded in the first user of the field.
> > >>
> > >> As an aside, for 32-bit Arm, you need to add a 4-byte padding.
> > >>
> > >
> > > Yeah, I may delete this change. I imported this change as
> > > considering the functionality of rebooting domain on static allocation.
> > >
> > > A little more discussion on rebooting domain on static allocation.
> > > Considering the major user cases for domain on static allocation are
> > > system has a total pre-defined, static behavior all the time. No
> > > domain allocation on runtime, while there still exists domain rebooting.
> >
> > Hmmm... With this series it is still possible to allocate memory at
> > runtime outside of the static allocation (see my comment on the design
> document [1]).
> > So is it meant to be complete?
> >
> 
> I'm guessing that the "allocate memory at runtime outside of the static
> allocation" is referring to XEN heap on static allocation, that is, users pre-
> defining heap in device tree configuration to let the whole system more static
> and predictable.
> 
> And I've replied you in the design there, sorry for the late reply. Save your 
> time,
> and I’ll paste here:
> 
> "Right now, on AArch64, all RAM, except reserved memory, will be finally
> given to buddy allocator as heap,  like you said, guest RAM for normal domain
> will be allocated from there, xmalloc eventually is get memory from there, 
> etc.
> So we want to refine the heap here, not iterating through `bootinfo.mem` to
> set up XEN heap, but like iterating `bootinfo. reserved_heap` to set up XEN
> heap.
> 
> On ARM32, xen heap and domain heap are separately mapped, which is more
> complicated here. That's why I only talking about implementing these features
> on AArch64 as first step."
> 
>  Above implementation will be delivered through another patch Serie. This
> patch Serie Is only focusing on Domain on Static Allocation.
> 

Oh, Second thought on this. 
And I think you are referring to balloon in/out here, hmm, also, like
I replied there:
"For issues on ballooning out or in, it is not supported here.
Domain on Static Allocation and 1:1 direct-map are all based on
dom0-less right now, so no PV, grant table, event channel, etc, considered.

Right now, it only supports device got passthrough into the guest."

> > >
> > > And when rebooting domain on static allocation, all these reserved
> > > pages could not go back to heap when freeing them.  So I am
> > > considering to use one global `struct page_info*[DOMID]` value to store.
> > >
> > > As Jan suggested, when domain get rebooted, struct domain will not
> > > exist
> > anymore.
> > > But I think DOMID info could last.
> > You would need to make sure the domid to be reserved. But I am not
> > entirely convinced this is necessary here.
> >
> > When recreating the domain, you need a way to know its configuration.
> > Mostly likely this will come from the Device-Tree. At which point, you
> > can also find the static region from there.
> >
> 
> True, true. I'll dig more in your suggestion, thx. 😉
> 
> > Cheers,
> >
> > [1] <7ab73cb0-39d5-f8bf-660f-b3d77f3247bd@xxxxxxx>
> >
> > --
> > Julien Grall
> 
> Cheers
> 
> Penny

Cheers

Penny