Re: [Xen-devel] [PATCH v12 1/3] xen/mem_sharing: VM forking

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Mar 25, 2020 at 04:42:07PM +0000, Julien Grall wrote:
> Hi,
> 
> On 25/03/2020 16:34, Tamas K Lengyel wrote:
> > > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > > > index 9f51370327..1ed7d13084 100644
> > > > --- a/xen/arch/x86/mm/p2m.c
> > > > +++ b/xen/arch/x86/mm/p2m.c
> > > > @@ -509,6 +509,12 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, unsigned long gfn_l,
> > > > 
> > > >       mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
> > > > 
> > > > +    /* Check if we need to fork the page */
> > > > +    if ( (q & P2M_ALLOC) && p2m_is_hole(*t) &&
> > > > +         !mem_sharing_fork_page(p2m->domain, gfn, q & P2M_UNSHARE) )
> > > > +        mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
> > > > +
> > > > +    /* Check if we need to unshare the page */
> > > >       if ( (q & P2M_UNSHARE) && p2m_is_shared(*t) )
> > > >       {
> > > >           ASSERT(p2m_is_hostp2m(p2m));
> > > > @@ -588,7 +594,8 @@ struct page_info *p2m_get_page_from_gfn(
> > > >               return page;
> > > > 
> > > >           /* Error path: not a suitable GFN at all */
> > > > -        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) )
> > > > +        if ( !p2m_is_ram(*t) && !p2m_is_paging(*t) && !p2m_is_pod(*t) &&
> > > > +             !mem_sharing_is_fork(p2m->domain) )
> > > >               return NULL;
> > > >       }
> > > > 
> > > > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > > > index b4eb476a9c..62aed53a16 100644
> > > > --- a/xen/common/domain.c
> > > > +++ b/xen/common/domain.c
> > > > @@ -1270,6 +1270,9 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
> > > > 
> > > >       v->vcpu_info = new_info;
> > > >       v->vcpu_info_mfn = page_to_mfn(page);
> > > > +#ifdef CONFIG_MEM_SHARING
> > > > +    v->vcpu_info_offset = offset;
> > > 
> > > There's no need to introduce this field, you can just use v->vcpu_info
> > > & ~PAGE_MASK AFAICT.
> > 
> > Just doing what you suggest above results in:
> > 
> > mem_sharing.c:1603:55: error: invalid operands to binary & (have
> > ‘vcpu_info_t * const’ {aka ‘union <anonymous> * const’} and ‘long
> > int’)
> >                                       d_vcpu->vcpu_info & ~PAGE_MASK);
> > 
> > I can of course cast the vcpu_info pointer to (long int), it's just a
> > bit ugly. Thoughts?
> 
> FWIW, I will also need the offset for liveupdate. I have used (unsigned
> long)v->vcpu_info & ~PAGE_MASK so far but this is not really pretty.

I think using:

(vaddr_t)v->vcpu_info & ~PAGE_MASK

is acceptable, but that would require adding a vaddr_t typedef to x86.

Adding a macro to do so would be OK by me, maybe PAGE_OFFSET or some
such?

> So I am all for either a new field or a macro hiding this uglyness.

Macro would be better IMO, as it's redundant to store the offset when
we can obtain it from an existing field.

Thanks, Roger.