diff mbox series

[v3,1/8] xen_backend: add grant table helpers

Message ID 1525461967-32174-2-git-send-email-paul.durrant@citrix.com
State New
Headers show
Series xen_disk: legacy code removal and cleanup | expand

Commit Message

Paul Durrant May 4, 2018, 7:26 p.m. UTC 
This patch adds grant table helper functions to the xen_backend code to
localize error reporting and use of xen_domid.

The patch also defers the call to xengnttab_open() until just before the
initialise method in XenDevOps is invoked. This method is responsible for
mapping the shared ring. No prior method requires access to the grant table.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>

v2:
 - New in v2
---
 hw/xen/xen_backend.c         | 123 ++++++++++++++++++++++++++++++++++++++-----
 include/hw/xen/xen_backend.h |  33 ++++++++++++
 2 files changed, 144 insertions(+), 12 deletions(-)

Comments

Anthony PERARD May 16, 2018, 1:50 p.m. UTC | #1 
On Fri, May 04, 2018 at 08:26:00PM +0100, Paul Durrant wrote:
> This patch adds grant table helper functions to the xen_backend code to
> localize error reporting and use of xen_domid.
> 
> The patch also defers the call to xengnttab_open() until just before the
> initialise method in XenDevOps is invoked. This method is responsible for
> mapping the shared ring. No prior method requires access to the grant table.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> 
> v2:
>  - New in v2
> ---
>  hw/xen/xen_backend.c         | 123 ++++++++++++++++++++++++++++++++++++++-----
>  include/hw/xen/xen_backend.h |  33 ++++++++++++
>  2 files changed, 144 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index 7445b50..50412d6 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -106,6 +106,103 @@ int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
>      return 0;
>  }
>  
> +void xen_be_set_max_grant_refs(struct XenDevice *xendev,
> +                               unsigned int nr_refs)

Is it fine to ignore error from set_max_grants and continue ? xen_disk.c
seems to fail the initialisation if set_max_grants call fails. On the
other end, xen-usb.c just keep going.

> +{
> +    assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
> +
> +    if (xengnttab_set_max_grants(xendev->gnttabdev, nr_refs)) {
> +        xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n",
> +                      strerror(errno));
> +    }
> +}
> +

> +int xen_be_copy_grant_refs(struct XenDevice *xendev,
> +                           bool to_domain,
> +                           XenGrantCopySegment segs[],
> +                           unsigned int nr_segs)
> +{
> +    xengnttab_grant_copy_segment_t *xengnttab_segs;
> +    unsigned int i;
> +    int rc;
> +
> +    assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
> +
> +    xengnttab_segs = g_new0(xengnttab_grant_copy_segment_t, nr_segs);
> +
> +    for (i = 0; i < nr_segs; i++) {
> +        XenGrantCopySegment *seg = &segs[i];
> +        xengnttab_grant_copy_segment_t *xengnttab_seg = &xengnttab_segs[i];
> +
> +        if (to_domain) {
> +            xengnttab_seg->flags = GNTCOPY_dest_gref;
> +            xengnttab_seg->dest.foreign.domid = xen_domid;
> +            xengnttab_seg->dest.foreign.ref = seg->dest.foreign.ref;
> +            xengnttab_seg->dest.foreign.offset = seg->dest.foreign.offset;
> +            xengnttab_seg->source.virt = seg->source.virt;
> +        } else {
> +            xengnttab_seg->flags = GNTCOPY_source_gref;
> +            xengnttab_seg->source.foreign.domid = xen_domid;
> +            xengnttab_seg->source.foreign.ref = seg->source.foreign.ref;
> +            xengnttab_seg->source.foreign.offset =
> +                seg->source.foreign.offset;
> +            xengnttab_seg->dest.virt = seg->dest.virt;
> +        }

That's not going to work because xengnttab_grant_copy_segment_t doesn't
exist on Xen 4.7.

> +
> +        xengnttab_seg->len = seg->len;
> +    }
> +
> +    rc = xengnttab_grant_copy(xendev->gnttabdev, nr_segs, xengnttab_segs);

Thanks,
Paul Durrant May 16, 2018, 2 p.m. UTC | #2 
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 16 May 2018 14:50
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; qemu-block@nongnu.org; qemu-
> devel@nongnu.org; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v3 1/8] xen_backend: add grant table helpers
> 
> On Fri, May 04, 2018 at 08:26:00PM +0100, Paul Durrant wrote:
> > This patch adds grant table helper functions to the xen_backend code to
> > localize error reporting and use of xen_domid.
> >
> > The patch also defers the call to xengnttab_open() until just before the
> > initialise method in XenDevOps is invoked. This method is responsible for
> > mapping the shared ring. No prior method requires access to the grant
> table.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> >
> > v2:
> >  - New in v2
> > ---
> >  hw/xen/xen_backend.c         | 123
> ++++++++++++++++++++++++++++++++++++++-----
> >  include/hw/xen/xen_backend.h |  33 ++++++++++++
> >  2 files changed, 144 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> > index 7445b50..50412d6 100644
> > --- a/hw/xen/xen_backend.c
> > +++ b/hw/xen/xen_backend.c
> > @@ -106,6 +106,103 @@ int xen_be_set_state(struct XenDevice *xendev,
> enum xenbus_state state)
> >      return 0;
> >  }
> >
> > +void xen_be_set_max_grant_refs(struct XenDevice *xendev,
> > +                               unsigned int nr_refs)
> 
> Is it fine to ignore error from set_max_grants and continue ? xen_disk.c
> seems to fail the initialisation if set_max_grants call fails. On the
> other end, xen-usb.c just keep going.
> 

I guess the upshot will be that a subsequent grant map would fail, so I think it should be sufficient to deal with the failure there. As you say it's use is inconsistent, and just plain missing in some cases.

> > +{
> > +    assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
> > +
> > +    if (xengnttab_set_max_grants(xendev->gnttabdev, nr_refs)) {
> > +        xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n",
> > +                      strerror(errno));
> > +    }
> > +}
> > +
> 
> > +int xen_be_copy_grant_refs(struct XenDevice *xendev,
> > +                           bool to_domain,
> > +                           XenGrantCopySegment segs[],
> > +                           unsigned int nr_segs)
> > +{
> > +    xengnttab_grant_copy_segment_t *xengnttab_segs;
> > +    unsigned int i;
> > +    int rc;
> > +
> > +    assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
> > +
> > +    xengnttab_segs = g_new0(xengnttab_grant_copy_segment_t,
> nr_segs);
> > +
> > +    for (i = 0; i < nr_segs; i++) {
> > +        XenGrantCopySegment *seg = &segs[i];
> > +        xengnttab_grant_copy_segment_t *xengnttab_seg =
> &xengnttab_segs[i];
> > +
> > +        if (to_domain) {
> > +            xengnttab_seg->flags = GNTCOPY_dest_gref;
> > +            xengnttab_seg->dest.foreign.domid = xen_domid;
> > +            xengnttab_seg->dest.foreign.ref = seg->dest.foreign.ref;
> > +            xengnttab_seg->dest.foreign.offset = seg->dest.foreign.offset;
> > +            xengnttab_seg->source.virt = seg->source.virt;
> > +        } else {
> > +            xengnttab_seg->flags = GNTCOPY_source_gref;
> > +            xengnttab_seg->source.foreign.domid = xen_domid;
> > +            xengnttab_seg->source.foreign.ref = seg->source.foreign.ref;
> > +            xengnttab_seg->source.foreign.offset =
> > +                seg->source.foreign.offset;
> > +            xengnttab_seg->dest.virt = seg->dest.virt;
> > +        }
> 
> That's not going to work because xengnttab_grant_copy_segment_t doesn't
> exist on Xen 4.7.

Ah, I'd missed the ifdef around that in xen_disk. I'll add it here.

Cheers,

  Paul

> 
> > +
> > +        xengnttab_seg->len = seg->len;
> > +    }
> > +
> > +    rc = xengnttab_grant_copy(xendev->gnttabdev, nr_segs,
> xengnttab_segs);
> 
> Thanks,
> 
> --
> Anthony PERARD
diff mbox series

Patch

diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
index 7445b50..50412d6 100644
--- a/hw/xen/xen_backend.c
+++ b/hw/xen/xen_backend.c
@@ -106,6 +106,103 @@  int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state)
     return 0;
 }
 
+void xen_be_set_max_grant_refs(struct XenDevice *xendev,
+                               unsigned int nr_refs)
+{
+    assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+    if (xengnttab_set_max_grants(xendev->gnttabdev, nr_refs)) {
+        xen_pv_printf(xendev, 0, "xengnttab_set_max_grants failed: %s\n",
+                      strerror(errno));
+    }
+}
+
+void *xen_be_map_grant_refs(struct XenDevice *xendev, uint32_t *refs,
+                            unsigned int nr_refs, int prot)
+{
+    void *ptr;
+
+    assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+    ptr = xengnttab_map_domain_grant_refs(xendev->gnttabdev, nr_refs,
+                                          xen_domid, refs, prot);
+    if (!ptr) {
+        xen_pv_printf(xendev, 0,
+                      "xengnttab_map_domain_grant_refs failed: %s\n",
+                      strerror(errno));
+    }
+
+    return ptr;
+}
+
+void xen_be_unmap_grant_refs(struct XenDevice *xendev, void *ptr,
+                             unsigned int nr_refs)
+{
+    assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+    if (xengnttab_unmap(xendev->gnttabdev, ptr, nr_refs)) {
+        xen_pv_printf(xendev, 0, "xengnttab_unmap failed: %s\n",
+                      strerror(errno));
+    }
+}
+
+int xen_be_copy_grant_refs(struct XenDevice *xendev,
+                           bool to_domain,
+                           XenGrantCopySegment segs[],
+                           unsigned int nr_segs)
+{
+    xengnttab_grant_copy_segment_t *xengnttab_segs;
+    unsigned int i;
+    int rc;
+
+    assert(xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV);
+
+    xengnttab_segs = g_new0(xengnttab_grant_copy_segment_t, nr_segs);
+
+    for (i = 0; i < nr_segs; i++) {
+        XenGrantCopySegment *seg = &segs[i];
+        xengnttab_grant_copy_segment_t *xengnttab_seg = &xengnttab_segs[i];
+
+        if (to_domain) {
+            xengnttab_seg->flags = GNTCOPY_dest_gref;
+            xengnttab_seg->dest.foreign.domid = xen_domid;
+            xengnttab_seg->dest.foreign.ref = seg->dest.foreign.ref;
+            xengnttab_seg->dest.foreign.offset = seg->dest.foreign.offset;
+            xengnttab_seg->source.virt = seg->source.virt;
+        } else {
+            xengnttab_seg->flags = GNTCOPY_source_gref;
+            xengnttab_seg->source.foreign.domid = xen_domid;
+            xengnttab_seg->source.foreign.ref = seg->source.foreign.ref;
+            xengnttab_seg->source.foreign.offset =
+                seg->source.foreign.offset;
+            xengnttab_seg->dest.virt = seg->dest.virt;
+        }
+
+        xengnttab_seg->len = seg->len;
+    }
+
+    rc = xengnttab_grant_copy(xendev->gnttabdev, nr_segs, xengnttab_segs);
+
+    if (rc) {
+        xen_pv_printf(xendev, 0, "xengnttab_copy failed: %s\n",
+                      strerror(errno));
+    }
+
+    for (i = 0; i < nr_segs; i++) {
+        xengnttab_grant_copy_segment_t *xengnttab_seg =
+            &xengnttab_segs[i];
+
+        if (xengnttab_seg->status != GNTST_okay) {
+            xen_pv_printf(xendev, 0, "segment[%u] status: %d\n", i,
+                          xengnttab_seg->status);
+            rc = -1;
+        }
+    }
+
+    g_free(xengnttab_segs);
+    return rc;
+}
+
 /*
  * get xen backend device, allocate a new one if it doesn't exist.
  */
@@ -149,18 +246,6 @@  static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
     }
     qemu_set_cloexec(xenevtchn_fd(xendev->evtchndev));
 
-    if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
-        xendev->gnttabdev = xengnttab_open(NULL, 0);
-        if (xendev->gnttabdev == NULL) {
-            xen_pv_printf(NULL, 0, "can't open gnttab device\n");
-            xenevtchn_close(xendev->evtchndev);
-            qdev_unplug(DEVICE(xendev), NULL);
-            return NULL;
-        }
-    } else {
-        xendev->gnttabdev = NULL;
-    }
-
     xen_pv_insert_xendev(xendev);
 
     if (xendev->ops->alloc) {
@@ -322,6 +407,16 @@  static int xen_be_try_initialise(struct XenDevice *xendev)
         }
     }
 
+    if (xendev->ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
+        xendev->gnttabdev = xengnttab_open(NULL, 0);
+        if (xendev->gnttabdev == NULL) {
+            xen_pv_printf(NULL, 0, "can't open gnttab device\n");
+            return -1;
+        }
+    } else {
+        xendev->gnttabdev = NULL;
+    }
+
     if (xendev->ops->initialise) {
         rc = xendev->ops->initialise(xendev);
     }
@@ -369,6 +464,10 @@  static void xen_be_disconnect(struct XenDevice *xendev, enum xenbus_state state)
         xendev->ops->disconnect) {
         xendev->ops->disconnect(xendev);
     }
+    if (xendev->gnttabdev) {
+        xengnttab_close(xendev->gnttabdev);
+        xendev->gnttabdev = NULL;
+    }
     if (xendev->be_state != state) {
         xen_be_set_state(xendev, state);
     }
diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
index 3a27692..29bf1c3 100644
--- a/include/hw/xen/xen_backend.h
+++ b/include/hw/xen/xen_backend.h
@@ -42,6 +42,39 @@  void xen_be_register_common(void);
 int xen_be_register(const char *type, struct XenDevOps *ops);
 int xen_be_set_state(struct XenDevice *xendev, enum xenbus_state state);
 int xen_be_bind_evtchn(struct XenDevice *xendev);
+void xen_be_set_max_grant_refs(struct XenDevice *xendev,
+                               unsigned int nr_refs);
+void *xen_be_map_grant_refs(struct XenDevice *xendev, uint32_t *refs,
+                            unsigned int nr_refs, int prot);
+void xen_be_unmap_grant_refs(struct XenDevice *xendev, void *ptr,
+                             unsigned int nr_refs);
+
+typedef struct XenGrantCopySegment {
+    union {
+        void *virt;
+        struct {
+            uint32_t ref;
+            off_t offset;
+        } foreign;
+    } source, dest;
+    size_t len;
+} XenGrantCopySegment;
+
+int xen_be_copy_grant_refs(struct XenDevice *xendev,
+                           bool to_domain, XenGrantCopySegment segs[],
+                           unsigned int nr_segs);
+
+static inline void *xen_be_map_grant_ref(struct XenDevice *xendev,
+                                         uint32_t ref, int prot)
+{
+    return xen_be_map_grant_refs(xendev, &ref, 1, prot);
+}
+
+static inline void xen_be_unmap_grant_ref(struct XenDevice *xendev,
+                                          void *ptr)
+{
+    return xen_be_unmap_grant_refs(xendev, ptr, 1);
+}
 
 /* actual backend drivers */
 extern struct XenDevOps xen_console_ops;      /* xen_console.c     */