[PATCH 0/5] allow xc_domain_maximum_gpfn() to observe full GFN value

[PATCH 0/5] allow xc_domain_maximum_gpfn() to observe full GFN value

[Jan Beulich]

The present remaining osstest failures are due to truncation of the GFN
resulting from the hypercall return value being passed back through the
ioctl() return value (on Linux and Solaris), which is "int", plus the
same for some further internal interfaces (osdep_hypercall(),
xencall<N>()). Some of the memory-op sub-ops, like the one involved
here, may pass back values which don't fit into "int".

Different effects can be observed with a 32- and 64-bit tool stack,
each causing one test to fail. The changes here will only deal with
the truncation resulting when sizeof(int) < sizeof(long), i.e. only on
64-bit. For the 32-bit tool stack case to work in such a situation,
yet uglier hackery would be needed. But even if the full value got
passed back, we'd then hit:

#ifdef __i386__
    /* Very large domains (> 1TB) will exhaust virtual address space. */
    if ( nr_pfns > 0x0fffffff )
    {
        errno = E2BIG;
        PERROR("Cannot save this big a guest");
        return -1;
    }
#endif

in xg_sr_save_x86_hvm.c:x86_hvm_setup() (and there's a similar check
on the restore path).

I wonder in how far a guest property can legitimately cause an osstest
push to be prevented by causing a failure like this one. And of course
I'm also puzzled by the ovmf change having managed to make it through
its push gate.

Note that I can't tell at this point whether there aren't further
issues, as I've not actually tried the ovmf case. I could easily see
there being oom issues there then, once to full value gets used for
setting up the p2m monitoring during migration. Or processing might
then take overly long.

1: x86/HVM: wire up multicalls
2: libxencall: osdep_hypercall() should return long
3: libxencall: introduce variant of xencall2() returning long
4: libxc: use multicall for memory-op on Linux (and Solaris)
5: libxc: make xc_domain_maximum_gpfn() endianness-agnostic

Jan

[PATCH 1/5] x86/HVM: wire up multicalls

[Jan Beulich]

To be able to use them from, in particular, the tool stack, they need to
be supported for all guest types. Note that xc_resource_op() already
does, so would not work without this on PVH Dom0.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -26,6 +26,7 @@
 #include <asm/hvm/emulate.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/viridian.h>
+#include <asm/multicall.h>
 
 #include <public/hvm/hvm_op.h>
 #include <public/hvm/params.h>
@@ -125,6 +126,7 @@ static const struct {
     hypercall_fn_t *native, *compat;
 } hvm_hypercall_table[] = {
     HVM_CALL(memory_op),
+    COMPAT_CALL(multicall),
 #ifdef CONFIG_GRANT_TABLE
     HVM_CALL(grant_table_op),
 #endif
@@ -334,6 +336,39 @@ int hvm_hypercall(struct cpu_user_regs *
     return curr->hcall_preempted ? HVM_HCALL_preempted : HVM_HCALL_completed;
 }
 
+enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
+{
+    struct vcpu *curr = current;
+    hypercall_fn_t *func = NULL;
+
+    if ( hvm_guest_x86_mode(curr) == 8 )
+    {
+        struct multicall_entry *call = &state->call;
+
+        if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
+            func = array_access_nospec(hvm_hypercall_table, call->op).native;
+        if ( func )
+            call->result = func(call->args[0], call->args[1], call->args[2],
+                                call->args[3], call->args[4], call->args[5]);
+        else
+            call->result = -ENOSYS;
+    }
+    else
+    {
+        struct compat_multicall_entry *call = &state->compat_call;
+
+        if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
+            func = array_access_nospec(hvm_hypercall_table, call->op).compat;
+        if ( func )
+            call->result = func(call->args[0], call->args[1], call->args[2],
+                                call->args[3], call->args[4], call->args[5]);
+        else
+            call->result = -ENOSYS;
+    }
+
+    return !hvm_get_cpl(curr) ? mc_continue : mc_preempt;
+}
+
 /*
  * Local variables:
  * mode: C
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -20,6 +20,7 @@
  */
 
 #include <xen/hypercall.h>
+#include <asm/multicall.h>
 
 #ifdef CONFIG_COMPAT
 #define ARGS(x, n)                              \
@@ -273,13 +274,18 @@ int hypercall_xlat_continuation(unsigned
     return rc;
 }
 
-#ifndef CONFIG_PV
-/* Stub for arch_do_multicall_call */
-enum mc_disposition arch_do_multicall_call(struct mc_state *mc)
+enum mc_disposition arch_do_multicall_call(struct mc_state *state)
 {
+    const struct domain *currd = current->domain;
+
+    if ( is_pv_domain(currd) )
+        return pv_do_multicall_call(state);
+
+    if ( is_hvm_domain(currd) )
+        return hvm_do_multicall_call(state);
+
     return mc_exit;
 }
-#endif
 
 /*
  * Local variables:
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -23,6 +23,7 @@
 #include <xen/hypercall.h>
 #include <xen/nospec.h>
 #include <xen/trace.h>
+#include <asm/multicall.h>
 #include <irq_vectors.h>
 
 #ifdef CONFIG_PV32
@@ -245,7 +246,7 @@ void pv_hypercall(struct cpu_user_regs *
     perfc_incr(hypercalls);
 }
 
-enum mc_disposition arch_do_multicall_call(struct mc_state *state)
+enum mc_disposition pv_do_multicall_call(struct mc_state *state)
 {
     struct vcpu *curr = current;
     unsigned long op;
--- /dev/null
+++ b/xen/include/asm-x86/multicall.h
@@ -0,0 +1,12 @@
+/******************************************************************************
+ * asm-x86/multicall.h
+ */
+
+#ifndef __ASM_X86_MULTICALL_H__
+#define __ASM_X86_MULTICALL_H__
+
+#include <xen/multicall.h>
+
+typeof(arch_do_multicall_call) pv_do_multicall_call, hvm_do_multicall_call;
+
+#endif /* __ASM_X86_MULTICALL_H__ */

[PATCH 2/5] libxencall: osdep_hypercall() should return long

[Jan Beulich]

Some hypercalls, memory-op in particular, can return values requiring
more than 31 bits to represent. Hence the underlying layers need to make
sure they won't truncate such values. (Note that for Solaris the
function also gets renamed, to match the other OSes.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/libs/call/freebsd.c
+++ b/tools/libs/call/freebsd.c
@@ -62,7 +62,7 @@ int osdep_xencall_close(xencall_handle *
     return close(fd);
 }
 
-int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
+long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
 {
     int fd = xcall->fd;
     int ret;
--- a/tools/libs/call/linux.c
+++ b/tools/libs/call/linux.c
@@ -80,7 +80,7 @@ int osdep_xencall_close(xencall_handle *
     return 0;
 }
 
-int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
+long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
 {
     return ioctl(xcall->fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
 }
--- a/tools/libs/call/minios.c
+++ b/tools/libs/call/minios.c
@@ -38,7 +38,7 @@ int osdep_xencall_close(xencall_handle *
     return 0;
 }
 
-int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
+long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
 {
     multicall_entry_t call;
     int i, ret;
--- a/tools/libs/call/netbsd.c
+++ b/tools/libs/call/netbsd.c
@@ -96,7 +96,7 @@ void osdep_free_pages(xencall_handle *xc
     free(ptr);
 }
 
-int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
+long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
 {
     int fd = xcall->fd;
     int error = ioctl(fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
--- a/tools/libs/call/private.h
+++ b/tools/libs/call/private.h
@@ -55,7 +55,7 @@ struct xencall_handle {
 int osdep_xencall_open(xencall_handle *xcall);
 int osdep_xencall_close(xencall_handle *xcall);
 
-int osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall);
+long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall);
 
 void *osdep_alloc_pages(xencall_handle *xcall, size_t nr_pages);
 void osdep_free_pages(xencall_handle *xcall, void *p, size_t nr_pages);
--- a/tools/libs/call/solaris.c
+++ b/tools/libs/call/solaris.c
@@ -80,7 +80,7 @@ void osdep_free_hypercall_buffer(xencall
     free(ptr);
 }
 
-int do_xen_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
+long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall)
 {
     int fd = xcall->fd;
     return ioctl(fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);

[PATCH 3/5] libxencall: introduce variant of xencall2() returning long

[Jan Beulich]

Some hypercalls, memory-op in particular, can return values requiring
more than 31 bits to represent. Hence the underlying layers need to make
sure they won't truncate such values.

While adding the new function to the map file, I noticed the stray
xencall6 there. I'm taking the liberty to remove it at this occasion. If
such a function would ever appear, it shouldn't lane in version 1.0.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wasn't sure whether euqivalents for the other xencall<N>() should also
be introduced, and hence went for the minimal solution first. Otoh there
is also xencall0() which has no users ...

--- a/tools/include/xencall.h
+++ b/tools/include/xencall.h
@@ -113,6 +113,10 @@ int xencall5(xencall_handle *xcall, unsi
              uint64_t arg1, uint64_t arg2, uint64_t arg3,
              uint64_t arg4, uint64_t arg5);
 
+/* Variant(s) of the above, as needed, returning "long" instead of "int". */
+long xencall2L(xencall_handle *xcall, unsigned int op,
+               uint64_t arg1, uint64_t arg2);
+
 /*
  * Allocate and free memory which is suitable for use as a pointer
  * argument to a hypercall.
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -127,6 +127,17 @@ int xencall2(xencall_handle *xcall, unsi
     return osdep_hypercall(xcall, &call);
 }
 
+long xencall2L(xencall_handle *xcall, unsigned int op,
+               uint64_t arg1, uint64_t arg2)
+{
+    privcmd_hypercall_t call = {
+        .op = op,
+        .arg = { arg1, arg2 },
+    };
+
+    return osdep_hypercall(xcall, &call);
+}
+
 int xencall3(xencall_handle *xcall, unsigned int op,
              uint64_t arg1, uint64_t arg2, uint64_t arg3)
 {
--- a/tools/libs/call/libxencall.map
+++ b/tools/libs/call/libxencall.map
@@ -9,7 +9,6 @@ VERS_1.0 {
 		xencall3;
 		xencall4;
 		xencall5;
-		xencall6;
 
 		xencall_alloc_buffer;
 		xencall_free_buffer;
@@ -27,3 +26,8 @@ VERS_1.2 {
 	global:
 		xencall_fd;
 } VERS_1.1;
+
+VERS_1.3 {
+	global:
+		xencall2L;
+} VERS_1.2;

[PATCH 4/5] libxc: use multicall for memory-op on Linux (and Solaris)

[Jan Beulich]

Some sub-functions, XENMEM_maximum_gpfn in particular, can return values
requiring more than 31 bits to represent. Hence we cannot issue the
hypercall directly when the return value of ioctl() is used to propagate
this value (note that this is not the case for the BSDs, and MiniOS
already wraps all hypercalls in a multicall).

Suggested-by: Jürgen Groß <jgross@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/libs/ctrl/xc_private.c
+++ b/tools/libs/ctrl/xc_private.c
@@ -337,8 +337,47 @@ long do_memory_op(xc_interface *xch, int
         goto out1;
     }
 
-    ret = xencall2(xch->xcall, __HYPERVISOR_memory_op,
-                   cmd, HYPERCALL_BUFFER_AS_ARG(arg));
+#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:

[PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic

[Jan Beulich]

libxc generally uses uint32_t to represent domain IDs. This is fine as
long as addresses of such variables aren't taken, to then pass into
hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
intermediate variable to deal with the issue. (On architectures with
arguments passed in registers, such an intermediate variable would have
been created by the compiler already anyway, just one of the wrong
type.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
 
 int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
 {
-    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
+    domid_t xen_domid = domid;
+    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
+                           sizeof(xen_domid));
 
     if ( rc >= 0 )
     {

Re: [PATCH 1/5] x86/HVM: wire up multicalls

[Andrew Cooper]

On 18/06/2021 11:23, Jan Beulich wrote:
> To be able to use them from, in particular, the tool stack, they need to
> be supported for all guest types. Note that xc_resource_op() already
> does, so would not work without this on PVH Dom0.

I'm not a fan of multicalls as a concept - they're mostly a layering
violation adding substantial complexity - and frankly, working around a
Linux kernel/user ABI error is a terrible reason to make this change.

But I won't object if it happens to be the least terrible option going. 
I accept that there are no good options here.

> @@ -334,6 +336,39 @@ int hvm_hypercall(struct cpu_user_regs *
>      return curr->hcall_preempted ? HVM_HCALL_preempted : HVM_HCALL_completed;
>  }
>  
> +enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
> +{
> +    struct vcpu *curr = current;
> +    hypercall_fn_t *func = NULL;
> +
> +    if ( hvm_guest_x86_mode(curr) == 8 )
> +    {
> +        struct multicall_entry *call = &state->call;
> +
> +        if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
> +            func = array_access_nospec(hvm_hypercall_table, call->op).native;
> +        if ( func )
> +            call->result = func(call->args[0], call->args[1], call->args[2],
> +                                call->args[3], call->args[4], call->args[5]);
> +        else
> +            call->result = -ENOSYS;
> +    }
> +    else
> +    {
> +        struct compat_multicall_entry *call = &state->compat_call;
> +
> +        if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
> +            func = array_access_nospec(hvm_hypercall_table, call->op).compat;
> +        if ( func )
> +            call->result = func(call->args[0], call->args[1], call->args[2],
> +                                call->args[3], call->args[4], call->args[5]);
> +        else
> +            call->result = -ENOSYS;
> +    }
> +
> +    return !hvm_get_cpl(curr) ? mc_continue : mc_preempt;

This is ported across from XSA-213, but even for PV guests, it was just
defence in depth IIRC for any cases we hadn't spotted, changing privilege.

There is no pagetable accounting in HVM guests to become confused by a
privilege change, and hvm_get_cpl() isn't totally free.  Any kernel
which puts VCPUOP_initialise in a multicall gets to keep all resulting
pieces.

I think this wants to be just "return mc_continue;"

If so, Begrudingly acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

~Andrew

Re: [PATCH 0/5] allow xc_domain_maximum_gpfn() to observe full GFN value

[Jan Beulich]

As to the title, xc_maximum_ram_page() is similarly affected. Plus,
perhaps less worrying, xc_sharing_{freed,used}_pages().

Jan

Re: [PATCH 1/5] x86/HVM: wire up multicalls

[Jan Beulich]

On 18.06.2021 15:02, Andrew Cooper wrote:
> On 18/06/2021 11:23, Jan Beulich wrote:
>> To be able to use them from, in particular, the tool stack, they need to
>> be supported for all guest types. Note that xc_resource_op() already
>> does, so would not work without this on PVH Dom0.
> 
> I'm not a fan of multicalls as a concept - they're mostly a layering
> violation adding substantial complexity - and frankly, working around a
> Linux kernel/user ABI error is a terrible reason to make this change.

While I agree with the latter, I don't think there's much complexity
here, and there are certainly savings in terms of mode switch between
guest and hypervisor when you can batch up arbitrary calls (and not
just sufficiently similar ones with built-in batching).

>> @@ -334,6 +336,39 @@ int hvm_hypercall(struct cpu_user_regs *
>>      return curr->hcall_preempted ? HVM_HCALL_preempted : HVM_HCALL_completed;
>>  }
>>  
>> +enum mc_disposition hvm_do_multicall_call(struct mc_state *state)
>> +{
>> +    struct vcpu *curr = current;
>> +    hypercall_fn_t *func = NULL;
>> +
>> +    if ( hvm_guest_x86_mode(curr) == 8 )
>> +    {
>> +        struct multicall_entry *call = &state->call;
>> +
>> +        if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
>> +            func = array_access_nospec(hvm_hypercall_table, call->op).native;
>> +        if ( func )
>> +            call->result = func(call->args[0], call->args[1], call->args[2],
>> +                                call->args[3], call->args[4], call->args[5]);
>> +        else
>> +            call->result = -ENOSYS;
>> +    }
>> +    else
>> +    {
>> +        struct compat_multicall_entry *call = &state->compat_call;
>> +
>> +        if ( call->op < ARRAY_SIZE(hvm_hypercall_table) )
>> +            func = array_access_nospec(hvm_hypercall_table, call->op).compat;
>> +        if ( func )
>> +            call->result = func(call->args[0], call->args[1], call->args[2],
>> +                                call->args[3], call->args[4], call->args[5]);
>> +        else
>> +            call->result = -ENOSYS;
>> +    }
>> +
>> +    return !hvm_get_cpl(curr) ? mc_continue : mc_preempt;
> 
> This is ported across from XSA-213, but even for PV guests, it was just
> defence in depth IIRC for any cases we hadn't spotted, changing privilege.
> 
> There is no pagetable accounting in HVM guests to become confused by a
> privilege change, and hvm_get_cpl() isn't totally free.  Any kernel
> which puts VCPUOP_initialise in a multicall gets to keep all resulting
> pieces.
> 
> I think this wants to be just "return mc_continue;"

I had it this way first, but I think the state setting hypercalls
ought to be similarly protected. In fact I did this adjustment at
the last moment before sending, after having looked at Arm code.
If we don't want it here, it ought to go away there as well, and
then also for PV (where then only IRET would need special casing).

> If so, Begrudingly acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, I'll take this for the moment (ignoring the "if so"), but
I'll wait some to see whether the above wants further discussing.

Jan

Re: [PATCH 2/5] libxencall: osdep_hypercall() should return long

[Andrew Cooper]

On 18/06/2021 11:23, Jan Beulich wrote:
> Some hypercalls, memory-op in particular, can return values requiring
> more than 31 bits to represent. Hence the underlying layers need to make
> sure they won't truncate such values. (Note that for Solaris the
> function also gets renamed, to match the other OSes.)

Spot the environment which obviously hasn't been compiled since the
4.5(?) timeframe...

Also, I'm fairly sure the comment in the NetBSD version is false when it
says it is copying the Linux way of doing things - I'm pretty sure it
means copying the FreeBSD way.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I think the commit message needs to state that this doesn't fix
truncation in the Linux or Solaris, nor the truncation in the
xencall{0..5}() public APIs.  It only fixes truncation issues for
FreeBSD, MiniOS and NetBSD.

With a suitable adjustment, and ideally a fix to the NetBSD comment,
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 2/5] libxencall: osdep_hypercall() should return long

[Jan Beulich]

On 18.06.2021 15:26, Andrew Cooper wrote:
> On 18/06/2021 11:23, Jan Beulich wrote:
>> Some hypercalls, memory-op in particular, can return values requiring
>> more than 31 bits to represent. Hence the underlying layers need to make
>> sure they won't truncate such values. (Note that for Solaris the
>> function also gets renamed, to match the other OSes.)
> 
> Spot the environment which obviously hasn't been compiled since the
> 4.5(?) timeframe...
> 
> Also, I'm fairly sure the comment in the NetBSD version is false when it
> says it is copying the Linux way of doing things - I'm pretty sure it
> means copying the FreeBSD way.

It doesn't say "copy", but "mimic", and I think what it means is the
effect for its callers. Therefore I think the comment makes sense in
its current shape. And I don't think I should change it right here
anyway.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I think the commit message needs to state that this doesn't fix
> truncation in the Linux or Solaris, nor the truncation in the
> xencall{0..5}() public APIs.  It only fixes truncation issues for
> FreeBSD, MiniOS and NetBSD.

I've added

"Due to them merely propagating ioctl()'s return value, this change is
 benign on Linux and Solaris. IOW there's an actual effect here only for
 the BSDs and MiniOS, but even then further adjustments are needed at the
 xencall<N>() level."

> With a suitable adjustment, and ideally a fix to the NetBSD comment,
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan

Re: [PATCH 3/5] libxencall: introduce variant of xencall2() returning long

[Andrew Cooper]

On 18/06/2021 11:24, Jan Beulich wrote:
> Some hypercalls, memory-op in particular, can return values requiring
> more than 31 bits to represent. Hence the underlying layers need to make
> sure they won't truncate such values.
>
> While adding the new function to the map file, I noticed the stray
> xencall6 there. I'm taking the liberty to remove it at this occasion. If
> such a function would ever appear, it shouldn't lane in version 1.0.

s/lane/land/ ?

I'm tempted to suggest spitting this out into a separate change anyway. 
I'm not sure of the implications on the ABI.

ABI-dumper appears not to have picked anything up, nor has readelf on
the object itself, so we're probably ok ABI wise.

That said, I would really have expected a compile/link error for a bad
symbol in a map file.

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wasn't sure whether euqivalents for the other xencall<N>() should also
> be introduced, and hence went for the minimal solution first. Otoh there
> is also xencall0() which has no users ...
>
> --- a/tools/include/xencall.h
> +++ b/tools/include/xencall.h
> @@ -113,6 +113,10 @@ int xencall5(xencall_handle *xcall, unsi
>               uint64_t arg1, uint64_t arg2, uint64_t arg3,
>               uint64_t arg4, uint64_t arg5);
>  
> +/* Variant(s) of the above, as needed, returning "long" instead of "int". */
> +long xencall2L(xencall_handle *xcall, unsigned int op,

If we're fixing ABIs, can we see about not truncate op on the way up?

> +               uint64_t arg1, uint64_t arg2);
> +
>  /*
>   * Allocate and free memory which is suitable for use as a pointer
>   * argument to a hypercall.
> --- a/tools/libs/call/core.c
> +++ b/tools/libs/call/core.c
> @@ -127,6 +127,17 @@ int xencall2(xencall_handle *xcall, unsi
>      return osdep_hypercall(xcall, &call);
>  }
>  
> +long xencall2L(xencall_handle *xcall, unsigned int op,
> +               uint64_t arg1, uint64_t arg2)
> +{
> +    privcmd_hypercall_t call = {
> +        .op = op,
> +        .arg = { arg1, arg2 },
> +    };
> +
> +    return osdep_hypercall(xcall, &call);

(If we're not changing op), I take it there are no alias tricks we can
play to reuse the same implementation?

~Andrew

Re: [PATCH 3/5] libxencall: introduce variant of xencall2() returning long

[Jan Beulich]

On 18.06.2021 15:46, Andrew Cooper wrote:
> On 18/06/2021 11:24, Jan Beulich wrote:
>> Some hypercalls, memory-op in particular, can return values requiring
>> more than 31 bits to represent. Hence the underlying layers need to make
>> sure they won't truncate such values.
>>
>> While adding the new function to the map file, I noticed the stray
>> xencall6 there. I'm taking the liberty to remove it at this occasion. If
>> such a function would ever appear, it shouldn't lane in version 1.0.
> 
> s/lane/land/ ?

Yeah, spotted this already.

> I'm tempted to suggest spitting this out into a separate change anyway. 
> I'm not sure of the implications on the ABI.

There are none, as a non-existing symbol can't possibly appear in a
DSO's symbol table. But well, yes, I can certainly make this a
separate change; it merely seemed excessive to me because of the
no-op effect the change has at this point in time.

> ABI-dumper appears not to have picked anything up, nor has readelf on
> the object itself, so we're probably ok ABI wise.
> 
> That said, I would really have expected a compile/link error for a bad
> symbol in a map file.

So would I, but reality tells us otherwise.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wasn't sure whether euqivalents for the other xencall<N>() should also
>> be introduced, and hence went for the minimal solution first. Otoh there
>> is also xencall0() which has no users ...
>>
>> --- a/tools/include/xencall.h
>> +++ b/tools/include/xencall.h
>> @@ -113,6 +113,10 @@ int xencall5(xencall_handle *xcall, unsi
>>               uint64_t arg1, uint64_t arg2, uint64_t arg3,
>>               uint64_t arg4, uint64_t arg5);
>>  
>> +/* Variant(s) of the above, as needed, returning "long" instead of "int". */
>> +long xencall2L(xencall_handle *xcall, unsigned int op,
> 
> If we're fixing ABIs, can we see about not truncate op on the way up?

You mean making it unsigned long, when I don't see us ever
gathering enough hypercalls. Even if it were flags to add in, they
would surely fit in the low 32 bits. I'm afraid there's too much
code out there assuming the hypercall numbers can be passed in the
low half of a 64-bit register.

But of course, if you insist ...

>> --- a/tools/libs/call/core.c
>> +++ b/tools/libs/call/core.c
>> @@ -127,6 +127,17 @@ int xencall2(xencall_handle *xcall, unsi
>>      return osdep_hypercall(xcall, &call);
>>  }
>>  
>> +long xencall2L(xencall_handle *xcall, unsigned int op,
>> +               uint64_t arg1, uint64_t arg2)
>> +{
>> +    privcmd_hypercall_t call = {
>> +        .op = op,
>> +        .arg = { arg1, arg2 },
>> +    };
>> +
>> +    return osdep_hypercall(xcall, &call);
> 
> (If we're not changing op), I take it there are no alias tricks we can
> play to reuse the same implementation?

Re-use would only be possible if we knew that all psABI-s match up
wrt the treatment of a "long" value becoming the return value of
a function returning "int". An ABI might require sign-extension to
register width (leaving aside yet more exotic options). Then, yes,
the "int" returning one(s) could become alias(es) of the "long"
returning ones.

Jan

Re: [PATCH 4/5] libxc: use multicall for memory-op on Linux (and Solaris)

[Andrew Cooper]

On 18/06/2021 11:24, Jan Beulich wrote:
> Some sub-functions, XENMEM_maximum_gpfn in particular, can return values
> requiring more than 31 bits to represent. Hence we cannot issue the
> hypercall directly when the return value of ioctl() is used to propagate
> this value (note that this is not the case for the BSDs, and MiniOS
> already wraps all hypercalls in a multicall).
>
> Suggested-by: Jürgen Groß <jgross@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/tools/libs/ctrl/xc_private.c
> +++ b/tools/libs/ctrl/xc_private.c
> @@ -337,8 +337,47 @@ long do_memory_op(xc_interface *xch, int
>          goto out1;
>      }
>  
> -    ret = xencall2(xch->xcall, __HYPERVISOR_memory_op,
> -                   cmd, HYPERCALL_BUFFER_AS_ARG(arg));
> +#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) )

This is very fragile.  I spent a while coming up with

    __builtin_types_compatible_p(
        typeof(ioctl) *,
        long (*)(int, unsigned long, ...));

(which does work if you change int for long), just to realise that this
won't actually help.  I'm suck on trying to see whether
privcmd_hypercall_t has a result member.

> +    {
> +        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 )

Wouldn't this be clearer as > -4095 ?

~Andrew

> +            {
> +                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:
>
>

Re: [PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic

[Andrew Cooper]

On 18/06/2021 11:25, Jan Beulich wrote:
> libxc generally uses uint32_t to represent domain IDs. This is fine as
> long as addresses of such variables aren't taken, to then pass into
> hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
> intermediate variable to deal with the issue. (On architectures with
> arguments passed in registers, such an intermediate variable would have
> been created by the compiler already anyway, just one of the wrong
> type.)
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
>  
>  int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
>  {
> -    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
> +    domid_t xen_domid = domid;
> +    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
> +                           sizeof(xen_domid));

Why on earth do we pass the domid in by pointer and not value?

~Andrew

Re: [PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic

[Andrew Cooper]

On 18/06/2021 16:06, Andrew Cooper wrote:
> On 18/06/2021 11:25, Jan Beulich wrote:
>> libxc generally uses uint32_t to represent domain IDs. This is fine as
>> long as addresses of such variables aren't taken, to then pass into
>> hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
>> intermediate variable to deal with the issue. (On architectures with
>> arguments passed in registers, such an intermediate variable would have
>> been created by the compiler already anyway, just one of the wrong
>> type.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/tools/libs/ctrl/xc_domain.c
>> +++ b/tools/libs/ctrl/xc_domain.c
>> @@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
>>  
>>  int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
>>  {
>> -    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
>> +    domid_t xen_domid = domid;
>> +    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
>> +                           sizeof(xen_domid));
> Why on earth do we pass the domid in by pointer and not value?

This is horrible.

What we're logically doing is passing a  pointer to struct
xen_memory_$FOO { domid_t domid; }, except its all done by void
pointers, and even the public header files don't provide a suitable
structure.

I think we really do want to retrofit a suitable structure in the public
interface and use that, rather than to continue games like this.

~Andrew

Re: [PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic

[Andrew Cooper]

On 18/06/2021 16:22, Andrew Cooper wrote:
> On 18/06/2021 16:06, Andrew Cooper wrote:
>> On 18/06/2021 11:25, Jan Beulich wrote:
>>> libxc generally uses uint32_t to represent domain IDs. This is fine as
>>> long as addresses of such variables aren't taken, to then pass into
>>> hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
>>> intermediate variable to deal with the issue. (On architectures with
>>> arguments passed in registers, such an intermediate variable would have
>>> been created by the compiler already anyway, just one of the wrong
>>> type.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/tools/libs/ctrl/xc_domain.c
>>> +++ b/tools/libs/ctrl/xc_domain.c
>>> @@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
>>>  
>>>  int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
>>>  {
>>> -    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
>>> +    domid_t xen_domid = domid;
>>> +    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
>>> +                           sizeof(xen_domid));
>> Why on earth do we pass the domid in by pointer and not value?
> This is horrible.
>
> What we're logically doing is passing a  pointer to struct
> xen_memory_$FOO { domid_t domid; }, except its all done by void
> pointers, and even the public header files don't provide a suitable
> structure.
>
> I think we really do want to retrofit a suitable structure in the public
> interface and use that, rather than to continue games like this.

Alternatively, declare this interface broken and reimplement it as a
domctl, which is where the functionality ought logically to live.

~Andrew

Re: [PATCH 4/5] libxc: use multicall for memory-op on Linux (and Solaris)

[Jan Beulich]

On 18.06.2021 17:05, Andrew Cooper wrote:
> On 18/06/2021 11:24, Jan Beulich wrote:
>> Some sub-functions, XENMEM_maximum_gpfn in particular, can return values
>> requiring more than 31 bits to represent. Hence we cannot issue the
>> hypercall directly when the return value of ioctl() is used to propagate
>> this value (note that this is not the case for the BSDs, and MiniOS
>> already wraps all hypercalls in a multicall).
>>
>> Suggested-by: Jürgen Groß <jgross@suse.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/tools/libs/ctrl/xc_private.c
>> +++ b/tools/libs/ctrl/xc_private.c
>> @@ -337,8 +337,47 @@ long do_memory_op(xc_interface *xch, int
>>          goto out1;
>>      }
>>  
>> -    ret = xencall2(xch->xcall, __HYPERVISOR_memory_op,
>> -                   cmd, HYPERCALL_BUFFER_AS_ARG(arg));
>> +#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) )
> 
> This is very fragile.  I spent a while coming up with
> 
>     __builtin_types_compatible_p(
>         typeof(ioctl) *,
>         long (*)(int, unsigned long, ...));
> 
> (which does work if you change int for long), just to realise that this
> won't actually help.

Help with what exactly? I'm not sure I see the severe fragility that
you see. I'd call this a little fragile because new OSes would need
to explicitly be checked for which behavior they ought to get. But
if one failed to do so, all that would happen is that they'd start
out with the same issue we're now trying to address.

>  I'm suck on trying to see whether
> privcmd_hypercall_t has a result member.

I.e. you're imagining __builtin_has_field()?

>> +    {
>> +        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 )
> 
> Wouldn't this be clearer as > -4095 ?

Not to me.

Jan

Re: [PATCH 5/5] libxc: make xc_domain_maximum_gpfn() endianness-agnostic

[Jan Beulich]

On 18.06.2021 17:24, Andrew Cooper wrote:
> On 18/06/2021 16:22, Andrew Cooper wrote:
>> On 18/06/2021 16:06, Andrew Cooper wrote:
>>> On 18/06/2021 11:25, Jan Beulich wrote:
>>>> libxc generally uses uint32_t to represent domain IDs. This is fine as
>>>> long as addresses of such variables aren't taken, to then pass into
>>>> hypercalls: To the hypervisor, a domain ID is a 16-bit value. Use an
>>>> intermediate variable to deal with the issue. (On architectures with
>>>> arguments passed in registers, such an intermediate variable would have
>>>> been created by the compiler already anyway, just one of the wrong
>>>> type.)
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/tools/libs/ctrl/xc_domain.c
>>>> +++ b/tools/libs/ctrl/xc_domain.c
>>>> @@ -856,7 +856,9 @@ int xc_domain_get_tsc_info(xc_interface
>>>>  
>>>>  int xc_domain_maximum_gpfn(xc_interface *xch, uint32_t domid, xen_pfn_t *gpfns)
>>>>  {
>>>> -    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &domid, sizeof(domid));
>>>> +    domid_t xen_domid = domid;
>>>> +    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &xen_domid,
>>>> +                           sizeof(xen_domid));
>>> Why on earth do we pass the domid in by pointer and not value?
>> This is horrible.
>>
>> What we're logically doing is passing a  pointer to struct
>> xen_memory_$FOO { domid_t domid; }, except its all done by void
>> pointers, and even the public header files don't provide a suitable
>> structure.
>>
>> I think we really do want to retrofit a suitable structure in the public
>> interface and use that, rather than to continue games like this.
> 
> Alternatively, declare this interface broken and reimplement it as a
> domctl, which is where the functionality ought logically to live.

Not really, no - this is something a domain can inquire on itself.

Jan

Re: [PATCH 3/5] libxencall: introduce variant of xencall2() returning long

[Ian Jackson]

Jan Beulich writes ("[PATCH 3/5] libxencall: introduce variant of xencall2() returning long"):
> Some hypercalls, memory-op in particular, can return values requiring
> more than 31 bits to represent. Hence the underlying layers need to make
> sure they won't truncate such values.

Thanks for this.

All 5 patches:

Acked-by: Ian Jackson <iwj@xenproject.org>

Nit:

> While adding the new function to the map file, I noticed the stray
> xencall6 there. I'm taking the liberty to remove it at this occasion. If
> such a function would ever appear, it shouldn't lane in version 1.0.
                                                  ^^^^

Typo for "land", I think.

Ian.