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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

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

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.

> >   };
> >
> >   #define PG_shift(idx)   (BITS_PER_LONG - (idx))
> > @@ -108,6 +116,9 @@ struct page_info
> >     /* Page is Xen heap? */
> >   #define _PGC_xen_heap     PG_shift(2)
> >   #define PGC_xen_heap      PG_mask(1, 2)
> > +  /* Page is reserved, referring static memory */
> 
> I would drop the second part of the sentence because the flag could be used
> for other purpose. One example is reserved memory when Live Updating.
> 

Sure, I will drop it.

> > +#define _PGC_reserved     PG_shift(3)
> > +#define PGC_reserved      PG_mask(1, 3)
> >   /* ... */
> >   /* Page is broken? */
> >   #define _PGC_broken       PG_shift(7)
> > @@ -161,6 +172,9 @@ extern unsigned long xenheap_base_pdx;
> >   #define page_get_owner(_p)    (_p)->v.inuse.domain
> >   #define page_set_owner(_p,_d) ((_p)->v.inuse.domain = (_d))
> >
> > +#define page_get_reserved_owner(_p)    (_p)->reserved.domain
> > +#define page_set_reserved_owner(_p,_d) ((_p)->reserved.domain = (_d))
> > +
> >   #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
> >
> >   #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
> >
> 
> Cheers,
> 
> --
> Julien Grall


Cheers,

Penny Zheng