| Message ID | 4e285f09f6c68deccf009b16c86898a78e349997.1578503483.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
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@intel.com> > --- > 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)? > + 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? > @@ -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. Jan
On Thu, Jan 16, 2020 at 9:18 AM Jan Beulich <jbeulich@suse.com> 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@intel.com> > > --- > > 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
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; + + if ( unlikely(!hap_enabled(d)) ) + return -ENODEV; + + 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; switch ( mso.op ) { @@ -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 ) { case XEN_DOMCTL_MEM_SHARING_CONTROL: - rc = 0; - if ( unlikely(is_iommu_enabled(d) && mec->u.enable) ) - rc = -EXDEV; - else - d->arch.hvm.mem_sharing_enabled = mec->u.enable; + rc = mem_sharing_control(d, mec->u.enable); break; default:
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@intel.com> --- xen/arch/x86/mm/mem_sharing.c | 36 +++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-)