[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.