diff mbox series

tools/libs/ctrl: add and export xc_memory_op

Message ID 5c72f793978997970888254a9050e97b34cbb3c7.1652966447.git.tamas.lengyel@intel.com (mailing list archive)
State New
Headers show
Series tools/libs/ctrl: add and export xc_memory_op | expand

Commit Message

Tamas K Lengyel May 19, 2022, 1:27 p.m. UTC
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(-)

Comments

Juergen Gross May 19, 2022, 1:32 p.m. UTC | #1
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?


Juergen
Tamas K Lengyel May 19, 2022, 1:59 p.m. UTC | #2
> -----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.
2) the patch itself is cleaner because there is no churn in all the files that previously called do_memory_op.

Tamas
Juergen Gross May 19, 2022, 2:31 p.m. UTC | #3
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
Tamas K Lengyel May 19, 2022, 3:03 p.m. UTC | #4
> -----Original Message-----
> From: Juergen Gross <jgross@suse.com>
> Sent: Thursday, May 19, 2022 10:31 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: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.

I'm fine with going the pure rename route  if that's what's preferred.

Tamas
diff mbox series

Patch

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)
-{
-    DECLARE_HYPERCALL_BOUNCE(arg, len, XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
-    long ret = -1;
-
-    if ( xc_hypercall_bounce_pre(xch, arg) )
-    {
-        PERROR("Could not bounce memory for XENMEM hypercall");
-        goto out1;
-    }
-
-#if defined(__linux__) || defined(__sun__)
-    /*
-     * Some sub-ops return values which don't fit in "int". On platforms
-     * without a specific hypercall return value field in the privcmd
-     * interface structure, issue the request as a single-element multicall,
-     * to be able to capture the full return value.
-     */
-    if ( sizeof(long) > sizeof(int) )
-    {
-        multicall_entry_t multicall = {
-            .op = __HYPERVISOR_memory_op,
-            .args[0] = cmd,
-            .args[1] = HYPERCALL_BUFFER_AS_ARG(arg),
-        }, *call = &multicall;
-        DECLARE_HYPERCALL_BOUNCE(call, sizeof(*call),
-                                 XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
-
-        if ( xc_hypercall_bounce_pre(xch, call) )
-        {
-            PERROR("Could not bounce buffer for memory_op hypercall");
-            goto out1;
-        }
-
-        ret = do_multicall_op(xch, HYPERCALL_BUFFER(call), 1);
-
-        xc_hypercall_bounce_post(xch, call);
-
-        if ( !ret )
-        {
-            ret = multicall.result;
-            if ( multicall.result > ~0xfffUL )
-            {
-                errno = -ret;
-                ret = -1;
-            }
-        }
-    }
-    else
-#endif
-        ret = xencall2L(xch->xcall, __HYPERVISOR_memory_op,
-                        cmd, HYPERCALL_BUFFER_AS_ARG(arg));
-
-    xc_hypercall_bounce_post(xch, arg);
- out1:
-    return ret;
-}
-
 int xc_maximum_ram_page(xc_interface *xch, unsigned long *max_mfn)
 {
     long rc = do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
@@ -489,6 +431,11 @@  int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl)
     return do_sysctl(xch, sysctl);
 }
 
+long xc_memory_op(xc_interface *xch, unsigned int cmd, void *arg, size_t len)
+{
+    return do_memory_op(xch, cmd, arg, len);
+}
+
 int xc_version(xc_interface *xch, int cmd, void *arg)
 {
     DECLARE_HYPERCALL_BOUNCE(arg, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT); /* Size unknown until cmd decoded */
diff --git a/tools/libs/ctrl/xc_private.h b/tools/libs/ctrl/xc_private.h
index ebdf78c2bf..cf6ad932b0 100644
--- a/tools/libs/ctrl/xc_private.h
+++ b/tools/libs/ctrl/xc_private.h
@@ -367,7 +367,63 @@  static inline int do_multicall_op(xc_interface *xch,
     return ret;
 }
 
-long do_memory_op(xc_interface *xch, int cmd, void *arg, size_t len);
+static inline long do_memory_op(xc_interface *xch, unsigned int cmd, void *arg, size_t len)
+{
+    DECLARE_HYPERCALL_BOUNCE(arg, len, XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+    long ret = -1;
+
+    if ( xc_hypercall_bounce_pre(xch, arg) )
+    {
+        PERROR("Could not bounce memory for XENMEM hypercall");
+        goto out1;
+    }
+
+#if defined(__linux__) || defined(__sun__)
+    /*
+     * Some sub-ops return values which don't fit in "int". On platforms
+     * without a specific hypercall return value field in the privcmd
+     * interface structure, issue the request as a single-element multicall,
+     * to be able to capture the full return value.
+     */
+    if ( sizeof(long) > sizeof(int) )
+    {
+        multicall_entry_t multicall = {
+            .op = __HYPERVISOR_memory_op,
+            .args[0] = cmd,
+            .args[1] = HYPERCALL_BUFFER_AS_ARG(arg),
+        }, *call = &multicall;
+        DECLARE_HYPERCALL_BOUNCE(call, sizeof(*call),
+                                 XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+
+        if ( xc_hypercall_bounce_pre(xch, call) )
+        {
+            PERROR("Could not bounce buffer for memory_op hypercall");
+            goto out1;
+        }
+
+        ret = do_multicall_op(xch, HYPERCALL_BUFFER(call), 1);
+
+        xc_hypercall_bounce_post(xch, call);
+
+        if ( !ret )
+        {
+            ret = multicall.result;
+            if ( multicall.result > ~0xfffUL )
+            {
+                errno = -ret;
+                ret = -1;
+            }
+        }
+    }
+    else
+#endif
+        ret = xencall2L(xch->xcall, __HYPERVISOR_memory_op,
+                        cmd, HYPERCALL_BUFFER_AS_ARG(arg));
+
+    xc_hypercall_bounce_post(xch, arg);
+ out1:
+    return ret;
+}
 
 void *xc_map_foreign_ranges(xc_interface *xch, uint32_t dom,
                             size_t size, int prot, size_t chunksize,