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

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

On Fri, Feb 13, 2015 at 7:17 PM, Andrew Cooper
<andrew.cooper3@citrix.com> 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 <tamas.lengyel@zentific.com>
>> Acked-by: Tim Deegan <tim@xen.org>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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 <andrew.cooper3@citrix.com>
>