Re: [Xen-devel] [PATCH V5 03/12] xen/mem_paging: Convert mem_event_op to mem_paging_op

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 <tamas.lengyel@xxxxxxxxxxxx>
> Acked-by: Tim Deegan <tim@xxxxxxx>
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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.

> -    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.

Once fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel