On Fri, Feb 13, 2015 at 10:23 PM, Andrew Cooper <andrew.coop...@citrix.com> wrote: > 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 <tamas.leng...@zentific.com> >> --- >> 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 <xen/guest_access.h> >> #include <asm/p2m.h> >> -#include <xen/vm_event.h> >> +#include <xsm/xsm.h> > > 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).
I'm not sure if any of these actually copy back - I just tried to keep as much of the existing flow intact as possible while sanitizing the vm_event side of things. That is not to say mem_sharing and mem_paging couldn't use more scrutiny and optimizations, it's just a bit out of the scope of what this series is about. > >> + >> 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 <xen/grant_table.h> >> #include <xen/sched.h> >> #include <xen/rcupdate.h> >> +#include <xen/guest_access.h> >> #include <xen/vm_event.h> >> #include <asm/page.h> >> #include <asm/string.h> >> @@ -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 ) Ack. > >> @@ -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. If it's not a critical fix I would rather just keep it as it was in this series. It could be part of another cleanup series for mem_paging and mem_sharing. Thanks, Tamas > > ~Andrew > >> + >> return rc; >> } > > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel