diff mbox series

[v2,05/11] tools/foreignmem: Support querying the size of a resource

Message ID 20200922182444.12350-6-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series Multiple fixes to XENMEM_acquire_resource | expand

Commit Message

Andrew Cooper Sept. 22, 2020, 6:24 p.m. UTC 
With the Xen side of this interface fixed to return real sizes, userspace
needs to be able to make the query.

Introduce xenforeignmemory_resource_size() for the purpose, bumping the
library minor version and providing compatibility for the non-Linux builds.

Its not possible to reuse the IOCTL_PRIVCMD_MMAP_RESOURCE infrastructure,
because it depends on having already mmap()'d a suitably sized region before
it will make an XENMEM_acquire_resource hypercall to Xen.

Instead, open a xencall handle and make an XENMEM_acquire_resource hypercall
directly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Wei Liu <wl@xen.org>
Reviewed-by: Paul Durrant <paul@xen.org>
---
CC: Ian Jackson <iwj@xenproject.org>
CC: Michał Leszczyński <michal.leszczynski@cert.pl>
CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
CC: Tamas K Lengyel <tamas@tklengyel.com>

v2:
 * Fix build in some environments.  Extend the rubric for making foreignmem
   depend on call
 * Rebase over Juergens library changes.
 * Spelling fixes.
---
 tools/libs/foreignmemory/Makefile                  |  2 +-
 tools/libs/foreignmemory/core.c                    | 14 +++++++++
 .../libs/foreignmemory/include/xenforeignmemory.h  | 15 +++++++++
 tools/libs/foreignmemory/libxenforeignmemory.map   |  4 +++
 tools/libs/foreignmemory/linux.c                   | 36 ++++++++++++++++++++++
 tools/libs/foreignmemory/private.h                 | 14 +++++++++
 tools/libs/uselibs.mk                              |  2 +-
 7 files changed, 85 insertions(+), 2 deletions(-)

Comments

Andrew Cooper Jan. 8, 2021, 5:52 p.m. UTC | #1 
On 22/09/2020 19:24, Andrew Cooper wrote:
> diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
> index fe73d5ab72..eec089e232 100644
> --- a/tools/libs/foreignmemory/linux.c
> +++ b/tools/libs/foreignmemory/linux.c
> @@ -339,6 +342,39 @@ int osdep_xenforeignmemory_map_resource(
>      return 0;
>  }
>  
> +int osdep_xenforeignmemory_resource_size(
> +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> +    unsigned int id, unsigned long *nr_frames)
> +{
> +    int rc;
> +    struct xen_mem_acquire_resource *xmar =
> +        xencall_alloc_buffer(fmem->xcall, sizeof(*xmar));
> +
> +    if ( !xmar )
> +    {
> +        PERROR("Could not bounce memory for acquire_resource hypercall");
> +        return -1;
> +    }
> +
> +    *xmar = (struct xen_mem_acquire_resource){
> +        .domid = domid,
> +        .type = type,
> +        .id = id,
> +    };
> +
> +    rc = xencall2(fmem->xcall, __HYPERVISOR_memory_op,
> +                  XENMEM_acquire_resource, (uintptr_t)xmar);
> +    if ( rc )
> +        goto out;
> +
> +    *nr_frames = xmar->nr_frames;
> +
> + out:
> +    xencall_free_buffer(fmem->xcall, xmar);
> +
> +    return rc;
> +}

Having talked this through with Roger, it's broken.

In the meantime, foreignmem has gained acquire_resource on FreeBSD.
Nothing in this osdep function is linux-specific, so it oughtn't to be
osdep.

However, its also not permitted to make hypercalls like this in
restricted mode, and that isn't something we should be breaking. 
Amongst other things, it will prevent us from supporting >128 cpus, as
Qemu needs updating to use this interface in due course.

The only solution (which keeps restricted mode working) is to fix
Linux's ioctl() to be able to understand size requests.  This also
avoids foreignmem needing to open a xencall handle which was fugly in
the first place.

This patch (well - a subset of it) will be needed to remain, to make a
usable interface in userspace.

~Andrew
Roger Pau Monné Jan. 11, 2021, 10:50 a.m. UTC | #2 
On Fri, Jan 08, 2021 at 05:52:36PM +0000, Andrew Cooper wrote:
> On 22/09/2020 19:24, Andrew Cooper wrote:
> > diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
> > index fe73d5ab72..eec089e232 100644
> > --- a/tools/libs/foreignmemory/linux.c
> > +++ b/tools/libs/foreignmemory/linux.c
> > @@ -339,6 +342,39 @@ int osdep_xenforeignmemory_map_resource(
> >      return 0;
> >  }
> >  
> > +int osdep_xenforeignmemory_resource_size(
> > +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
> > +    unsigned int id, unsigned long *nr_frames)
> > +{
> > +    int rc;
> > +    struct xen_mem_acquire_resource *xmar =
> > +        xencall_alloc_buffer(fmem->xcall, sizeof(*xmar));
> > +
> > +    if ( !xmar )
> > +    {
> > +        PERROR("Could not bounce memory for acquire_resource hypercall");
> > +        return -1;
> > +    }
> > +
> > +    *xmar = (struct xen_mem_acquire_resource){
> > +        .domid = domid,
> > +        .type = type,
> > +        .id = id,
> > +    };
> > +
> > +    rc = xencall2(fmem->xcall, __HYPERVISOR_memory_op,
> > +                  XENMEM_acquire_resource, (uintptr_t)xmar);
> > +    if ( rc )
> > +        goto out;
> > +
> > +    *nr_frames = xmar->nr_frames;
> > +
> > + out:
> > +    xencall_free_buffer(fmem->xcall, xmar);
> > +
> > +    return rc;
> > +}
> 
> Having talked this through with Roger, it's broken.
> 
> In the meantime, foreignmem has gained acquire_resource on FreeBSD.
> Nothing in this osdep function is linux-specific, so it oughtn't to be
> osdep.
> 
> However, its also not permitted to make hypercalls like this in
> restricted mode, and that isn't something we should be breaking. 
> Amongst other things, it will prevent us from supporting >128 cpus, as
> Qemu needs updating to use this interface in due course.
> 
> The only solution (which keeps restricted mode working) is to fix
> Linux's ioctl() to be able to understand size requests.  This also
> avoids foreignmem needing to open a xencall handle which was fugly in
> the first place.

I think the following patch should allow you to fetch the resource
size from Linux privcmd driver by doing an ioctl with addr = 0 and num
= 0. I've just build tested it, but I haven't tried exercising the
code.

Roger.
---8<---
From 5d717c7b9ad3561ed0b17e7c5cf76b7c9fb536db Mon Sep 17 00:00:00 2001
From: Roger Pau Monne <roger.pau@citrix.com>
Date: Mon, 11 Jan 2021 10:38:59 +0100
Subject: [PATCH] xen/privcmd: allow fetching resource sizes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Allow issuing an IOCTL_PRIVCMD_MMAP_RESOURCE ioctl with num = 0 and
addr = 0 in order to fetch the size of a specific resource.

Add a shortcut to the default map resource path, since fetching the
size requires no address to be passed in, and thus no VMA to setup.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
NB: fetching the size of a resource shouldn't trigger an hypercall
preemption, and hence I've dropped the preempt indications.
---
 drivers/xen/privcmd.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index b0c73c58f987..3278f93eb3da 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -717,7 +717,8 @@ static long privcmd_ioctl_restrict(struct file *file, void __user *udata)
 	return 0;
 }
 
-static long privcmd_ioctl_mmap_resource(struct file *file, void __user *udata)
+static long privcmd_ioctl_mmap_resource(struct file *file,
+				struct privcmd_mmap_resource __user *udata)
 {
 	struct privcmd_data *data = file->private_data;
 	struct mm_struct *mm = current->mm;
@@ -734,6 +735,19 @@ static long privcmd_ioctl_mmap_resource(struct file *file, void __user *udata)
 	if (data->domid != DOMID_INVALID && data->domid != kdata.dom)
 		return -EPERM;
 
+	memset(&xdata, 0, sizeof(xdata));
+
+	if (!kdata.addr && !kdata.num) {
+		/* Query the size of the resource. */
+		xdata.domid = kdata.dom;
+		xdata.type = kdata.type;
+		xdata.id = kdata.id;
+		rc = HYPERVISOR_memory_op(XENMEM_acquire_resource, &xdata);
+		if (rc)
+			return rc;
+		return __put_user(xdata.nr_frames, &udata->num);
+	}
+
 	mmap_write_lock(mm);
 
 	vma = find_vma(mm, kdata.addr);
@@ -768,7 +782,6 @@ static long privcmd_ioctl_mmap_resource(struct file *file, void __user *udata)
 	} else
 		vma->vm_private_data = PRIV_VMA_LOCKED;
 
-	memset(&xdata, 0, sizeof(xdata));
 	xdata.domid = kdata.dom;
 	xdata.type = kdata.type;
 	xdata.id = kdata.id;
Andrew Cooper Jan. 11, 2021, 3 p.m. UTC | #3 
On 11/01/2021 10:50, Roger Pau Monné wrote:
> On Fri, Jan 08, 2021 at 05:52:36PM +0000, Andrew Cooper wrote:
>> On 22/09/2020 19:24, Andrew Cooper wrote:
>>> diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
>>> index fe73d5ab72..eec089e232 100644
>>> --- a/tools/libs/foreignmemory/linux.c
>>> +++ b/tools/libs/foreignmemory/linux.c
>>> @@ -339,6 +342,39 @@ int osdep_xenforeignmemory_map_resource(
>>>      return 0;
>>>  }
>>>  
>>> +int osdep_xenforeignmemory_resource_size(
>>> +    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
>>> +    unsigned int id, unsigned long *nr_frames)
>>> +{
>>> +    int rc;
>>> +    struct xen_mem_acquire_resource *xmar =
>>> +        xencall_alloc_buffer(fmem->xcall, sizeof(*xmar));
>>> +
>>> +    if ( !xmar )
>>> +    {
>>> +        PERROR("Could not bounce memory for acquire_resource hypercall");
>>> +        return -1;
>>> +    }
>>> +
>>> +    *xmar = (struct xen_mem_acquire_resource){
>>> +        .domid = domid,
>>> +        .type = type,
>>> +        .id = id,
>>> +    };
>>> +
>>> +    rc = xencall2(fmem->xcall, __HYPERVISOR_memory_op,
>>> +                  XENMEM_acquire_resource, (uintptr_t)xmar);
>>> +    if ( rc )
>>> +        goto out;
>>> +
>>> +    *nr_frames = xmar->nr_frames;
>>> +
>>> + out:
>>> +    xencall_free_buffer(fmem->xcall, xmar);
>>> +
>>> +    return rc;
>>> +}
>> Having talked this through with Roger, it's broken.
>>
>> In the meantime, foreignmem has gained acquire_resource on FreeBSD.
>> Nothing in this osdep function is linux-specific, so it oughtn't to be
>> osdep.
>>
>> However, its also not permitted to make hypercalls like this in
>> restricted mode, and that isn't something we should be breaking. 
>> Amongst other things, it will prevent us from supporting >128 cpus, as
>> Qemu needs updating to use this interface in due course.
>>
>> The only solution (which keeps restricted mode working) is to fix
>> Linux's ioctl() to be able to understand size requests.  This also
>> avoids foreignmem needing to open a xencall handle which was fugly in
>> the first place.
> I think the following patch should allow you to fetch the resource
> size from Linux privcmd driver by doing an ioctl with addr = 0 and num
> = 0. I've just build tested it, but I haven't tried exercising the
> code.
>
> Roger.
> ---8<---
> From 5d717c7b9ad3561ed0b17e7c5cf76b7c9fb536db Mon Sep 17 00:00:00 2001
> From: Roger Pau Monne <roger.pau@citrix.com>
> Date: Mon, 11 Jan 2021 10:38:59 +0100
> Subject: [PATCH] xen/privcmd: allow fetching resource sizes
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Allow issuing an IOCTL_PRIVCMD_MMAP_RESOURCE ioctl with num = 0 and
> addr = 0 in order to fetch the size of a specific resource.
>
> Add a shortcut to the default map resource path, since fetching the
> size requires no address to be passed in, and thus no VMA to setup.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

> ---
> NB: fetching the size of a resource shouldn't trigger an hypercall
> preemption, and hence I've dropped the preempt indications.

Yeah - that's fine.  Querying the size isn't ever going to turn into a
long running operation from the guest's point of view.

I'll submit the matching patch for libxenforeignmem.

~Andrew
diff mbox series

Patch

diff --git a/tools/libs/foreignmemory/Makefile b/tools/libs/foreignmemory/Makefile
index cf444d3c1a..0ed93edac2 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -2,7 +2,7 @@  XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 MAJOR    = 1
-MINOR    = 3
+MINOR    = 4
 
 SRCS-y                 += core.c
 SRCS-$(CONFIG_Linux)   += linux.c
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 63f12e2450..5d95c59c48 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -53,6 +53,10 @@  xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
         if (!fmem->logger) goto err;
     }
 
+    fmem->xcall = xencall_open(fmem->logger, 0);
+    if ( !fmem->xcall )
+        goto err;
+
     rc = osdep_xenforeignmemory_open(fmem);
     if ( rc  < 0 ) goto err;
 
@@ -61,6 +65,7 @@  xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
 err:
     xentoolcore__deregister_active_handle(&fmem->tc_ah);
     osdep_xenforeignmemory_close(fmem);
+    xencall_close(fmem->xcall);
     xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return NULL;
@@ -75,6 +80,7 @@  int xenforeignmemory_close(xenforeignmemory_handle *fmem)
 
     xentoolcore__deregister_active_handle(&fmem->tc_ah);
     rc = osdep_xenforeignmemory_close(fmem);
+    xencall_close(fmem->xcall);
     xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return rc;
@@ -188,6 +194,14 @@  int xenforeignmemory_unmap_resource(
     return rc;
 }
 
+int xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames)
+{
+    return osdep_xenforeignmemory_resource_size(fmem, domid, type,
+                                                id, nr_frames);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/foreignmemory/include/xenforeignmemory.h b/tools/libs/foreignmemory/include/xenforeignmemory.h
index d594be8df0..1ba2f5316b 100644
--- a/tools/libs/foreignmemory/include/xenforeignmemory.h
+++ b/tools/libs/foreignmemory/include/xenforeignmemory.h
@@ -179,6 +179,21 @@  xenforeignmemory_resource_handle *xenforeignmemory_map_resource(
 int xenforeignmemory_unmap_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
 
+/**
+ * Determine the maximum size of a specific resource.
+ *
+ * @parm fmem handle to the open foreignmemory interface
+ * @parm domid the domain id
+ * @parm type the resource type
+ * @parm id the type-specific resource identifier
+ *
+ * Return 0 on success and fills in *nr_frames.  Sets errno and return -1 on
+ * error.
+ */
+int xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames);
+
 #endif
 
 /*
diff --git a/tools/libs/foreignmemory/libxenforeignmemory.map b/tools/libs/foreignmemory/libxenforeignmemory.map
index d5323c87d9..8aca341b99 100644
--- a/tools/libs/foreignmemory/libxenforeignmemory.map
+++ b/tools/libs/foreignmemory/libxenforeignmemory.map
@@ -19,3 +19,7 @@  VERS_1.3 {
 		xenforeignmemory_map_resource;
 		xenforeignmemory_unmap_resource;
 } VERS_1.2;
+VERS_1.4 {
+	global:
+		xenforeignmemory_resource_size;
+} VERS_1.3;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index fe73d5ab72..eec089e232 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -21,12 +21,15 @@ 
 #include <errno.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <stdint.h>
 #include <string.h>
 
 #include <sys/mman.h>
 #include <sys/ioctl.h>
 #include <xen-tools/libs.h>
 
+#include <xen/memory.h>
+
 #include "private.h"
 
 #ifndef O_CLOEXEC
@@ -339,6 +342,39 @@  int osdep_xenforeignmemory_map_resource(
     return 0;
 }
 
+int osdep_xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames)
+{
+    int rc;
+    struct xen_mem_acquire_resource *xmar =
+        xencall_alloc_buffer(fmem->xcall, sizeof(*xmar));
+
+    if ( !xmar )
+    {
+        PERROR("Could not bounce memory for acquire_resource hypercall");
+        return -1;
+    }
+
+    *xmar = (struct xen_mem_acquire_resource){
+        .domid = domid,
+        .type = type,
+        .id = id,
+    };
+
+    rc = xencall2(fmem->xcall, __HYPERVISOR_memory_op,
+                  XENMEM_acquire_resource, (uintptr_t)xmar);
+    if ( rc )
+        goto out;
+
+    *nr_frames = xmar->nr_frames;
+
+ out:
+    xencall_free_buffer(fmem->xcall, xmar);
+
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h
index 8f1bf081ed..1a6b685f45 100644
--- a/tools/libs/foreignmemory/private.h
+++ b/tools/libs/foreignmemory/private.h
@@ -4,6 +4,7 @@ 
 #include <xentoollog.h>
 
 #include <xenforeignmemory.h>
+#include <xencall.h>
 
 #include <xentoolcore_internal.h>
 
@@ -20,6 +21,7 @@ 
 
 struct xenforeignmemory_handle {
     xentoollog_logger *logger, *logger_tofree;
+    xencall_handle *xcall;
     unsigned flags;
     int fd;
     Xentoolcore__Active_Handle tc_ah;
@@ -74,6 +76,15 @@  static inline int osdep_xenforeignmemory_unmap_resource(
 {
     return 0;
 }
+
+static inline int osdep_xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames)
+{
+    errno = EOPNOTSUPP;
+    return -1;
+}
+
 #else
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
                                     domid_t domid);
@@ -81,6 +92,9 @@  int osdep_xenforeignmemory_map_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
 int osdep_xenforeignmemory_unmap_resource(
     xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres);
+int osdep_xenforeignmemory_resource_size(
+    xenforeignmemory_handle *fmem, domid_t domid, unsigned int type,
+    unsigned int id, unsigned long *nr_frames);
 #endif
 
 #define PERROR(_f...) \
diff --git a/tools/libs/uselibs.mk b/tools/libs/uselibs.mk
index a9dc2ce994..cb09e54de5 100644
--- a/tools/libs/uselibs.mk
+++ b/tools/libs/uselibs.mk
@@ -11,7 +11,7 @@  USELIBS_gnttab := toollog toolcore
 LIBS_LIBS += call
 USELIBS_call := toollog toolcore
 LIBS_LIBS += foreignmemory
-USELIBS_foreignmemory := toollog toolcore
+USELIBS_foreignmemory := toollog toolcore call
 LIBS_LIBS += devicemodel
 USELIBS_devicemodel := toollog toolcore call
 LIBS_LIBS += hypfs