Mailing List Archive

[PATCH] tools/libs/ctrl: add and export xc_memory_op
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)
-{
- 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,
--
2.34.1
Re: [PATCH] tools/libs/ctrl: add and export xc_memory_op [ In reply to ]
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
RE: [PATCH] tools/libs/ctrl: add and export xc_memory_op [ In reply to ]
> -----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
Re: [PATCH] tools/libs/ctrl: add and export xc_memory_op [ In reply to ]
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
RE: [PATCH] tools/libs/ctrl: add and export xc_memory_op [ In reply to ]
> -----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