Re: [Xen-devel] [PATCH v4 12/18] x86/mem_sharing: Enable mem_sharing on first memop

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jan 16, 2020 at 9:18 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 08.01.2020 18:14, Tamas K Lengyel wrote:
> > It is wasteful to require separate hypercalls to enable sharing on both the
> > parent and the client domain during VM forking. To speed things up we enable
> > sharing on the first memop in case it wasn't already enabled.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > ---
> >  xen/arch/x86/mm/mem_sharing.c | 36 +++++++++++++++++++++--------------
> >  1 file changed, 22 insertions(+), 14 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index 3f36cd6bbc..b8a9228ecf 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1412,6 +1412,24 @@ static int range_share(struct domain *d, struct 
> > domain *cd,
> >      return rc;
> >  }
> >
> > +static inline int mem_sharing_control(struct domain *d, bool enable)
> > +{
> > +    if ( enable )
> > +    {
> > +        if ( unlikely(!is_hvm_domain(d)) )
> > +            return -ENOSYS;
>
> -EOPNOTSUPP or some such please. ENOSYS has a very specific meaning,
> which (according to my understanding) doesn't apply here.
>
> > +        if ( unlikely(!hap_enabled(d)) )
> > +            return -ENODEV;
>
> Doesn't this allow dropping the HAP check from
> mem_sharing_enabled(d)?

Yes, looks like it could be dropped from there.

>
> > +        if ( unlikely(is_iommu_enabled(d)) )
> > +            return -EXDEV;
> > +    }
> > +
> > +    d->arch.hvm.mem_sharing.enabled = enable;
> > +    return 0;
> > +}
> > +
> >  int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
> >  {
> >      int rc;
> > @@ -1433,10 +1451,8 @@ int 
> > mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
> >      if ( rc )
> >          goto out;
> >
> > -    /* Only HAP is supported */
> > -    rc = -ENODEV;
> > -    if ( !mem_sharing_enabled(d) )
> > -        goto out;
> > +    if ( !mem_sharing_enabled(d) && (rc = mem_sharing_control(d, true)) )
> > +        return rc;
>
> Perhaps already in patch 6, doesn't this eliminate the need for the
> individual mem_sharing_enabled() checks in the case blocks?

Yes it does. I think I was planning on removing those checks but it
slipped my mind.

>
> > @@ -1703,18 +1719,10 @@ int mem_sharing_domctl(struct domain *d, struct 
> > xen_domctl_mem_sharing_op *mec)
> >  {
> >      int rc;
> >
> > -    /* Only HAP is supported */
> > -    if ( !hap_enabled(d) )
> > -        return -ENODEV;
> > -
> > -    switch ( mec->op )
> > +    switch( mec->op )
>
> Please don't corrupt proper Xen style.

Ack.

Thanks!
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel