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

[PATCH v2 0/6] 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.

See the individual patches for changes in v2, all of which are to
address review feedback.

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: libxencall: drop bogus mentioning of xencall6()
6: libxc: make xc_domain_maximum_gpfn() endianness-agnostic

Jan

[PATCH v2 1/6] 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>
Begrudingly acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <iwj@xenproject.org>

--- 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 v2 2/6] 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.)

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <iwj@xenproject.org>

--- 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 v2 3/6] 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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Jackson <iwj@xenproject.org>
---
v2: Move dropping of xencall6 from the version script to a separate
    patch.
---
I wasn't sure whether equivalents for the other xencall<N>() should also
be introduced, and hence I went for the minimal solution first. Otoh
there's also xencall0() without any 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
@@ -27,3 +27,8 @@ VERS_1.2 {
 	global:
 		xencall_fd;
 } VERS_1.1;
+
+VERS_1.3 {
+	global:
+		xencall2L;
+} VERS_1.2;

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

[Jan Beulich]

Some sub-functions, XENMEM_maximum_gpfn and XENMEM_maximum_ram_page 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>
Acked-by: Ian Jackson <iwj@xenproject.org>

--- 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 v2 5/6] libxencall: drop bogus mentioning of xencall6()

[Jan Beulich]

There's no xencall6(), so the version script also shouldn't mention it.
If such a function would ever appear, it shouldn't land in version 1.0.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Jackson <iwj@xenproject.org>
---
v2: Split out of earlier patch.

--- 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;

[PATCH v2 6/6] 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. Introduce
a wrapper struct to deal with the issue. (On architectures with
arguments passed in registers, an intermediate variable would have been
created by the compiler already anyway, just one of the wrong type.)

The public interface change is both source and binary compatible for
the architectures we currently support.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Jackson <iwj@xenproject.org>
---
v2: Introduce wrapper struct in public interface.
---
Together with the comment change I was half tempted to also rename the
sub-function identifier to XENMEM_maximum_gfn. But I then decided this
would go too far here.

--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -856,7 +856,8 @@ 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));
+    struct xen_memory_domain dom = { .domid = domid };
+    long rc = do_memory_op(xch, XENMEM_maximum_gpfn, &dom, sizeof(dom));
 
     if ( rc >= 0 )
     {
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1351,7 +1351,6 @@ long do_memory_op(unsigned long cmd, XEN
     long rc;
     struct xen_memory_reservation reservation;
     struct memop_args args;
-    domid_t domid;
     unsigned long start_extent = cmd >> MEMOP_EXTENT_SHIFT;
     int op = cmd & MEMOP_CMD_MASK;
 
@@ -1452,13 +1451,16 @@ long do_memory_op(unsigned long cmd, XEN
     case XENMEM_current_reservation:
     case XENMEM_maximum_reservation:
     case XENMEM_maximum_gpfn:
+    {
+        struct xen_memory_domain domain;
+
         if ( unlikely(start_extent) )
             return -EINVAL;
 
-        if ( copy_from_guest(&domid, arg, 1) )
+        if ( copy_from_guest(&domain, arg, 1) )
             return -EFAULT;
 
-        d = rcu_lock_domain_by_any_id(domid);
+        d = rcu_lock_domain_by_any_id(domain.domid);
         if ( d == NULL )
             return -ESRCH;
 
@@ -1486,6 +1488,7 @@ long do_memory_op(unsigned long cmd, XEN
         rcu_unlock_domain(d);
 
         break;
+    }
 
     case XENMEM_add_to_physmap:
     {
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -148,16 +148,23 @@ DEFINE_XEN_GUEST_HANDLE(xen_memory_excha
  */
 #define XENMEM_maximum_ram_page     2
 
+struct xen_memory_domain {
+    /* [IN] Domain information is being queried for. */
+    domid_t domid;
+};
+
 /*
  * Returns the current or maximum memory reservation, in pages, of the
  * specified domain (may be DOMID_SELF). Returns -ve errcode on failure.
- * arg == addr of domid_t.
+ * arg == addr of struct xen_memory_domain.
  */
 #define XENMEM_current_reservation  3
 #define XENMEM_maximum_reservation  4
 
 /*
- * Returns the maximum GPFN in use by the guest, or -ve errcode on failure.
+ * Returns the maximum GFN in use by the specified domain (may be DOMID_SELF).
+ * Returns -ve errcode on failure.
+ * arg == addr of struct xen_memory_domain.
  */
 #define XENMEM_maximum_gpfn         14
 

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

[Andrew Cooper]

On 22/06/2021 16:18, 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.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Ian Jackson <iwj@xenproject.org>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 5/6] libxencall: drop bogus mentioning of xencall6()

[Andrew Cooper]

On 22/06/2021 16:19, Jan Beulich wrote:
> There's no xencall6(), so the version script also shouldn't mention it.
> If such a function would ever appear, it shouldn't land in version 1.0.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Ian Jackson <iwj@xenproject.org>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This really does need backporting, and it is probably worth stating
explicitly that there is no change in the resulting object, nor
abi-dumper's view of the object.

~Andrew

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

[Andrew Cooper]

On 22/06/2021 16:20, 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. Introduce
> a wrapper struct to deal with the issue. (On architectures with
> arguments passed in registers, an intermediate variable would have been
> created by the compiler already anyway, just one of the wrong type.)
>
> The public interface change is both source and binary compatible for
> the architectures we currently support.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Ian Jackson <iwj@xenproject.org>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> v2: Introduce wrapper struct in public interface.
> ---
> Together with the comment change I was half tempted to also rename the
> sub-function identifier to XENMEM_maximum_gfn. But I then decided this
> would go too far here.

We ought to fix it, but that could be a followup, or could be when this
interface disappears in ABIv2.

~Andrew

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

[Andrew Cooper]

On 22/06/2021 16:19, Jan Beulich wrote:
> Some sub-functions, XENMEM_maximum_gpfn and XENMEM_maximum_ram_page 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).

It took me 3 readings to realise you weren't saying that BSDs used
multicall (which they don't).

Instead, I'd suggest rewording it.

"Some sub-functions, XENMEM_maximum_gpfn and XENMEM_maximum_ram_page in
particular, can return values requiring more than 31 bits to represent.

The BSDs deliberately avoid using the ioctl() return value, and MiniOS
already uses a multicall.  They aren't affected.

Linux and Solaris however do use the ioctl() return value, which must be
avoided in this case to avoid truncation."

Or something similar.

With a suitable improvement along these lines, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>

Re: [PATCH v2 5/6] libxencall: drop bogus mentioning of xencall6()

[Jan Beulich]

On 22.06.2021 20:25, Andrew Cooper wrote:
> On 22/06/2021 16:19, Jan Beulich wrote:
>> There's no xencall6(), so the version script also shouldn't mention it.
>> If such a function would ever appear, it shouldn't land in version 1.0.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> This really does need backporting,

So far I've been unconvinced it really would need to, unless we
expect that a further backport might introduce such a call without
us noticing. Since on the main branch such a change would then
need to add xencall6 in a new version, the question is what the
linker's behavior is when the same symbol would appear in two
distinct version sections of the script.

In any event, this being a tool stack change, the final word on
whether to backport this one is going to be with Ian. My
backporting request here only covers the first 4 patches of the
series (and I'm likely to take the liberty and include them in my
own backport sets, so Ian, if you want this one backported too,
you may want to indicate to me that I should include the one here
right away, should you agree with Andrew).

> and it is probably worth stating
> explicitly that there is no change in the resulting object, nor
> abi-dumper's view of the object.

I've added a sentence along these lines.

Jan