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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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;
-- 
2.29.2