RE: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, January 9, 2023 6:58 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> when host address not provided
> 
> 
> 
> On 09/01/2023 07:49, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 
> > Happy new year~~~~
> 
> Happy new year too!
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: Sunday, January 8, 2023 8:53 PM
> >> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> >> <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> >> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@xxxxxxxx>
> >> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> >> when host address not provided
> >>
> >> Hi,
> >>
> >> On 15/11/2022 02:52, Penny Zheng wrote:
> >>> @@ -922,33 +927,82 @@ static mfn_t __init
> >> acquire_shared_memory_bank(struct domain *d,
> >>>        d->max_pages += nr_pfns;
> >>>
> >>>        smfn = maddr_to_mfn(pbase);
> >>> -    res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> >>> -    if ( res )
> >>> +    page = mfn_to_page(smfn);
> >>> +    /*
> >>> +     * If page is allocated from heap as static shared memory, then we
> just
> >>> +     * assign it to the owner domain
> >>> +     */
> >>> +    if ( page->count_info == (PGC_state_inuse | PGC_static) )
> >> I am a bit confused how this can help differentiating
> >> becaPGC_state_inuse is 0. So effectively, you are checking that count_info
> is equal to PGC_static.
> >>
> >
> > When host address is provided, the host address range defined in
> > xen,static-mem will be stored as a "struct membank" with type of
> > "MEMBANK_STATIC_DOMAIN" in "bootinfo.reserved_mem"
> > Then it will be initialized as static memory through "init_staticmem_pages"
> > So here its page->count_info is PGC_state_free |PGC_static.
> > For pages allocated from heap, its page state is different, being
> PGC_state_inuse.
> > So actually, we are checking page state to tell the
> difference                                                    .
> 
> Ok. This is definitely not obvious from the code. But I think this is a very
> fragile assumption.
> 
> Instead, it would be better if we allocate the memory in
> acquire_shared_memory_bank() when the host address is not provided.
> 

Right now, acquire_shared_memory_bank is called only when domain is owner 
domain.
It is applicable when host address is provided, we could still do guest physmap 
when
borrower accessed before owner, as address is provided.
However when host address is not provided, we must allocate memory at first 
domain.
So allocating the memory shall be called outside acquire_shared_memory_bank

> >
> >> But as I wrote in a previous patch, I don't think you should convert
> >> {xen,dom}heap pages to a static pages.
> >>
> >
> > I agree that taking reference could also prevent giving these pages back to
> heap.
> > But may I ask what is your concern on converting {xen,dom}heap pages to
> a static pages?
> 
> A few reasons:
>   1) I consider them as two distinct allocators. So far they have the same
> behavior, but in the future this may change.
>   2) If the page is freed you really don't want the domain to be able to 
> re-use
> the page for a different purpose.
> 
> 
> I realize that 2) is already a problem today with static pages. So I think the
> best is to ensure that pages allocated for shared memory never reach the
> any of the allocators.
> 
> 
> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Julien Grall