From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH V5 10/12] xen/vm_event: Relocate memop checks Date: Fri, 13 Feb 2015 21:23:56 +0000 Message-ID: <54DE6B6C.70401@citrix.com> References: <1423845203-18941-1-git-send-email-tamas.lengyel@zentific.com> <1423845203-18941-11-git-send-email-tamas.lengyel@zentific.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1423845203-18941-11-git-send-email-tamas.lengyel@zentific.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel , xen-devel@lists.xen.org Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, tim@xen.org, steve@zentific.com, jbeulich@suse.com, eddie.dong@intel.com, andres@lagarcavilla.org, jun.nakajima@intel.com, rshriram@cs.ubc.ca, keir@xen.org, dgdegra@tycho.nsa.gov, yanghy@cn.fujitsu.com, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 13/02/15 16:33, Tamas K Lengyel wrote: > The memop handler function for paging/sharing responsible for calling XSM > doesn't really have anything to do with vm_event, thus in this patch we > relocate it into mem_paging_memop and mem_sharing_memop. This has already > been the approach in mem_access_memop, so in this patch we just make it > consistent. > > Signed-off-by: Tamas K Lengyel > --- > xen/arch/x86/mm/mem_paging.c | 36 ++++++++++++++----- > xen/arch/x86/mm/mem_sharing.c | 76 ++++++++++++++++++++++++++------------- > xen/arch/x86/x86_64/compat/mm.c | 28 +++------------ > xen/arch/x86/x86_64/mm.c | 26 +++----------- > xen/common/vm_event.c | 43 ---------------------- > xen/include/asm-x86/mem_paging.h | 7 +++- > xen/include/asm-x86/mem_sharing.h | 4 +-- > xen/include/xen/vm_event.h | 1 - > 8 files changed, 97 insertions(+), 124 deletions(-) > > diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c > index e63d8c1..4aee6b7 100644 > --- a/xen/arch/x86/mm/mem_paging.c > +++ b/xen/arch/x86/mm/mem_paging.c > @@ -21,28 +21,45 @@ > */ > > > +#include > #include > -#include > +#include Order of includes. > > - > -int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo) > +int mem_paging_memop(unsigned long cmd, I don't believe cmd is a useful parameter to pass. You know that its value is XENMEM_paging_op by virtue of being in this function. > + XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg) > { > - int rc = -ENODEV; > + int rc; > + xen_mem_paging_op_t mpo; > + struct domain *d; > + > + rc = -EFAULT; > + if ( copy_from_guest(&mpo, arg, 1) ) > + return rc; > + > + rc = rcu_lock_live_remote_domain_by_id(mpo.domain, &d); > + if ( rc ) > + return rc; > + > + rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_paging_op); > + if ( rc ) > + return rc; > + > + rc = -ENODEV; > if ( unlikely(!d->vm_event->paging.ring_page) ) > return rc; > > - switch( mpo->op ) > + switch( mpo.op ) > { > case XENMEM_paging_op_nominate: > - rc = p2m_mem_paging_nominate(d, mpo->gfn); > + rc = p2m_mem_paging_nominate(d, mpo.gfn); > break; > > case XENMEM_paging_op_evict: > - rc = p2m_mem_paging_evict(d, mpo->gfn); > + rc = p2m_mem_paging_evict(d, mpo.gfn); > break; > > case XENMEM_paging_op_prep: > - rc = p2m_mem_paging_prep(d, mpo->gfn, mpo->buffer); > + rc = p2m_mem_paging_prep(d, mpo.gfn, mpo.buffer); > break; > > default: > @@ -50,6 +67,9 @@ int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo) > break; > } > > + if ( !rc && __copy_to_guest(arg, &mpo, 1) ) > + rc = -EFAULT; Do any of the paging ops need to be copied back? Nothing appears to write into mpo in this function. (The original code looks to be overly pessimistic). > + > return rc; > } > > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c > index 4959407..612ed89 100644 > --- a/xen/arch/x86/mm/mem_sharing.c > +++ b/xen/arch/x86/mm/mem_sharing.c > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1293,30 +1294,52 @@ int relinquish_shared_pages(struct domain *d) > return rc; > } > > -int mem_sharing_memop(struct domain *d, xen_mem_sharing_op_t *mec) > +int mem_sharing_memop(unsigned long cmd, > + XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg) > { > - int rc = 0; > + int rc; > + xen_mem_sharing_op_t mso; > + struct domain *d; > + > + rc = -EFAULT; > + if ( copy_from_guest(&mso, arg, 1) ) > + return rc; > + > + if ( mso.op == XENMEM_sharing_op_audit ) > + return mem_sharing_audit(); > + > + rc = rcu_lock_live_remote_domain_by_id(mso.domain, &d); > + if ( rc ) > + return rc; > > /* Only HAP is supported */ > if ( !hap_enabled(d) || !d->arch.hvm_domain.mem_sharing_enabled ) > return -ENODEV; > > - switch(mec->op) > + rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_sharing_op); > + if ( rc ) > + return rc; > + > + rc = -ENODEV; > + if ( unlikely(!d->vm_event->share.ring_page) ) > + return rc; > + > + switch(mso.op) Style ( spaces ) > @@ -1465,6 +1488,9 @@ int mem_sharing_memop(struct domain *d, xen_mem_sharing_op_t *mec) > break; > } > > + if ( !rc && __copy_to_guest(arg, &mso, 1) ) > + return -EFAULT; Only certain subops need to copy information back. It is common to have a function-level bool_t copyback which relevant subops sets. ~Andrew > + > return rc; > }