Re: [PATCH] tools/libs/ctrl: add and export xc_memory_op

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3072 bytes --]

On 19.05.22 15:59, Lengyel, Tamas wrote:
> 
> 
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
>> Juergen Gross
>> Sent: Thursday, May 19, 2022 9:33 AM
>> To: Lengyel, Tamas <tamas.lengyel@intel.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Liu <wl@xen.org>; Anthony PERARD <anthony.perard@citrix.com>;
>> Cooper, Andrew <andrew.cooper3@citrix.com>
>> Subject: Re: [PATCH] tools/libs/ctrl: add and export xc_memory_op
>>
>> On 19.05.22 15:27, Tamas K Lengyel wrote:
>>> Add and export xc_memory_op so that do_memory_op can be used by tools
>>> linking with libxc. This is effectively in the same spirit as the
>>> existing xc_domctl and xc_sysctl functions, which are already exported.
>>>
>>> In this patch we move do_memory_op into xc_private.h as a static
>>> inline function and convert its 'cmd' input from int to unsigned int
>>> to accurately reflect what the hypervisor expects. No other changes are made
>> to the function.
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
>>> ---
>>>    tools/include/xenctrl.h      |  1 +
>>>    tools/libs/ctrl/xc_private.c | 63 +++---------------------------------
>>>    tools/libs/ctrl/xc_private.h | 58 ++++++++++++++++++++++++++++++++-
>>>    3 files changed, 63 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index
>>> 95bd5eca67..484e354412 100644
>>> --- a/tools/include/xenctrl.h
>>> +++ b/tools/include/xenctrl.h
>>> @@ -1597,6 +1597,7 @@ int xc_vmtrace_set_option(xc_interface *xch,
>>> uint32_t domid,
>>>
>>>    int xc_domctl(xc_interface *xch, struct xen_domctl *domctl);
>>>    int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
>>> +long xc_memory_op(xc_interface *xch, unsigned int cmd, void *arg,
>>> +size_t len);
>>>
>>>    int xc_version(xc_interface *xch, int cmd, void *arg);
>>>
>>> diff --git a/tools/libs/ctrl/xc_private.c
>>> b/tools/libs/ctrl/xc_private.c index c0422662f0..6a247d2b1f 100644
>>> --- a/tools/libs/ctrl/xc_private.c
>>> +++ b/tools/libs/ctrl/xc_private.c
>>> @@ -326,64 +326,6 @@ int xc_flush_mmu_updates(xc_interface *xch, struct
>> xc_mmu *mmu)
>>>        return flush_mmu_updates(xch, mmu);
>>>    }
>>>
>>> -long do_memory_op(xc_interface *xch, int cmd, void *arg, size_t len)
>>
>> Why don't you just rename this function and modify the users to use the new
>> name?
> 
> For two reasons:
> 1) having the do_memory_op as a static inline is consistent with how do_domctl and do_sysctl are implemented, so logically that's what I would expect to see for the memory_op hypercall as well.

It is much more complicated than the do_domctl and do_sysctl inlines.

Additionally it is being used by libxenguest, so making it an inline would
expose lots of libxenctrl internals to libxenguest.

> 2) the patch itself is cleaner because there is no churn in all the files that previously called do_memory_op.

OTOH all callers are in Xen, so its no deal to change those.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]