From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH V5 03/12] xen/mem_paging: Convert mem_event_op to mem_paging_op Date: Fri, 13 Feb 2015 19:30:30 +0100 Message-ID: References: <1423845203-18941-1-git-send-email-tamas.lengyel@zentific.com> <1423845203-18941-4-git-send-email-tamas.lengyel@zentific.com> <54DE3FCE.4070300@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54DE3FCE.4070300@citrix.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: Andrew Cooper Cc: "Tian, Kevin" , "wei.liu2@citrix.com" , Ian Campbell , Stefano Stabellini , Tim Deegan , Steven Maresca , "xen-devel@lists.xen.org" , Jan Beulich , "Dong, Eddie" , Andres Lagar-Cavilla , Jun Nakajima , "rshriram@cs.ubc.ca" , Keir Fraser , Daniel De Graaf , "yanghy@cn.fujitsu.com" , Ian Jackson List-Id: xen-devel@lists.xenproject.org On Fri, Feb 13, 2015 at 7:17 PM, Andrew Cooper wrote: > On 13/02/15 16:33, Tamas K Lengyel wrote: >> The only use-case of the mem_event_op structure had been in mem_paging, >> thus renaming the structure mem_paging_op and relocating its associated >> functions clarifies its actual usage. >> >> Signed-off-by: Tamas K Lengyel >> Acked-by: Tim Deegan >> Acked-by: Ian Campbell >> Acked-by: Jan Beulich >> --- >> v5: Style fixes >> v4: Style fixes >> v3: Style fixes >> --- >> tools/libxc/xc_mem_event.c | 16 ---------------- >> tools/libxc/xc_mem_paging.c | 26 ++++++++++++++++++-------- >> tools/libxc/xc_private.h | 3 --- >> xen/arch/x86/mm/mem_paging.c | 32 +++++++++++++------------------- >> xen/arch/x86/x86_64/compat/mm.c | 10 ++++++---- >> xen/arch/x86/x86_64/mm.c | 8 ++++---- >> xen/common/mem_event.c | 4 ++-- >> xen/include/asm-x86/mem_paging.h | 2 +- >> xen/include/public/memory.h | 9 ++++----- >> 9 files changed, 48 insertions(+), 62 deletions(-) >> >> diff --git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c >> index ee25cdd..487fcee 100644 >> --- a/tools/libxc/xc_mem_event.c >> +++ b/tools/libxc/xc_mem_event.c >> @@ -40,22 +40,6 @@ int xc_mem_event_control(xc_interface *xch, domid_t domain_id, unsigned int op, >> return rc; >> } >> >> -int xc_mem_event_memop(xc_interface *xch, domid_t domain_id, >> - unsigned int op, unsigned int mode, >> - uint32_t gfn, void *buffer) >> -{ >> - xen_mem_event_op_t meo; >> - >> - memset(&meo, 0, sizeof(meo)); >> - >> - meo.op = op; >> - meo.domain = domain_id; >> - meo.gfn = gfn; >> - meo.buffer = (unsigned long) buffer; >> - >> - return do_memory_op(xch, mode, &meo, sizeof(meo)); >> -} >> - >> void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param, >> uint32_t *port, int enable_introspection) >> { >> diff --git a/tools/libxc/xc_mem_paging.c b/tools/libxc/xc_mem_paging.c >> index 5194423..049aff4 100644 >> --- a/tools/libxc/xc_mem_paging.c >> +++ b/tools/libxc/xc_mem_paging.c >> @@ -23,6 +23,20 @@ >> >> #include "xc_private.h" >> >> +static int xc_mem_paging_memop(xc_interface *xch, domid_t domain_id, >> + unsigned int op, uint32_t gfn, void *buffer) > > As said in patch 1, this gfn must be a uint64_t > >> +{ >> + xen_mem_paging_op_t mpo; >> + >> + memset(&mpo, 0, sizeof(mpo)); >> + >> + mpo.op = op; >> + mpo.domain = domain_id; >> + mpo.gfn = gfn; >> + mpo.buffer = (unsigned long) buffer; >> + >> + return do_memory_op(xch, XENMEM_paging_op, &mpo, sizeof(mpo)); >> +} >> >> int xc_mem_paging_enable(xc_interface *xch, domid_t domain_id, >> uint32_t *port) >> @@ -49,25 +63,22 @@ int xc_mem_paging_disable(xc_interface *xch, domid_t domain_id) >> >> int xc_mem_paging_nominate(xc_interface *xch, domid_t domain_id, unsigned long gfn) >> { > > And these 'unsigned long gfn' should be promoted to a uint64_t gfn to > avoid truncation in 32bit toolstacks. > > Whether you wish to fix this in the same patch, or fix it in a separate > "make mem_event interface 64/32bit safe" patch is up to you. This is > straying somewhat form a simple refactoring of mem_event_op to > mem_paging_op. I'll just do it here for the sake of juggling fewer patches. > >> - return xc_mem_event_memop(xch, domain_id, >> + return xc_mem_paging_memop(xch, domain_id, >> XENMEM_paging_op_nominate, >> - XENMEM_paging_op, >> gfn, NULL); >> } >> >> int xc_mem_paging_evict(xc_interface *xch, domid_t domain_id, unsigned long gfn) >> { >> - return xc_mem_event_memop(xch, domain_id, >> + return xc_mem_paging_memop(xch, domain_id, >> XENMEM_paging_op_evict, >> - XENMEM_paging_op, >> gfn, NULL); >> } >> >> int xc_mem_paging_prep(xc_interface *xch, domid_t domain_id, unsigned long gfn) >> { >> - return xc_mem_event_memop(xch, domain_id, >> + return xc_mem_paging_memop(xch, domain_id, >> XENMEM_paging_op_prep, >> - XENMEM_paging_op, >> gfn, NULL); >> } >> >> @@ -87,9 +98,8 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id, >> if ( mlock(buffer, XC_PAGE_SIZE) ) >> return -1; >> >> - rc = xc_mem_event_memop(xch, domain_id, >> + rc = xc_mem_paging_memop(xch, domain_id, >> XENMEM_paging_op_prep, >> - XENMEM_paging_op, >> gfn, buffer); >> >> old_errno = errno; >> diff --git a/xen/include/asm-x86/mem_paging.h b/xen/include/asm-x86/mem_paging.h >> index 6b7a1fe..92ed2fa 100644 >> --- a/xen/include/asm-x86/mem_paging.h >> +++ b/xen/include/asm-x86/mem_paging.h >> @@ -21,7 +21,7 @@ >> */ >> >> >> -int mem_paging_memop(struct domain *d, xen_mem_event_op_t *meo); >> +int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *meo); > > s/meo/mpo/ like the implementation. Ack. This header also seems to have been missing an #ifdef wrapper so I'm going to add that here as well. Tamas > > Once fixed, Reviewed-by: Andrew Cooper >