Re: [PATCH v2 19/19] xen/xenbus: eliminate xenbus_grant_ring()

From: Oleksandr
Date: Mon May 02 2022 - 10:12:32 EST



On 02.05.22 16:30, Juergen Gross wrote:

Hello Juergen


On 29.04.22 17:10, Oleksandr wrote:

On 28.04.22 11:27, Juergen Gross wrote:


Hello Juergen


There is no external user of xenbus_grant_ring() left, so merge it into
the only caller xenbus_setup_ring().

Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
V2:
- make error message more precise (Andrew Cooper)
---
  drivers/xen/xenbus/xenbus_client.c | 65 +++++++++---------------------
  include/xen/xenbus.h               |  2 -
  2 files changed, 19 insertions(+), 48 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 1a2e0d94ccd1..d6fdd2d209d3 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -363,50 +363,6 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
          __xenbus_switch_state(dev, XenbusStateClosing, 1);
  }
-/**
- * xenbus_grant_ring
- * @dev: xenbus device
- * @vaddr: starting virtual address of the ring
- * @nr_pages: number of pages to be granted
- * @grefs: grant reference array to be filled in
- *
- * Grant access to the given @vaddr to the peer of the given device.
- * Then fill in @grefs with grant references.  Return 0 on success, or
- * -errno on error.  On error, the device will switch to
- * XenbusStateClosing, and the error will be saved in the store.
- */
-int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
-              unsigned int nr_pages, grant_ref_t *grefs)
-{
-    int err;
-    unsigned int i;
-    grant_ref_t gref_head;
-
-    err = gnttab_alloc_grant_references(nr_pages, &gref_head);
-    if (err) {
-        xenbus_dev_fatal(dev, err, "granting access to ring page");
-        return err;
-    }
-
-    for (i = 0; i < nr_pages; i++) {
-        unsigned long gfn;
-
-        if (is_vmalloc_addr(vaddr))
-            gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
-        else
-            gfn = virt_to_gfn(vaddr);
-
-        grefs[i] = gnttab_claim_grant_reference(&gref_head);
-        gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
-                        gfn, 0);
-
-        vaddr = vaddr + XEN_PAGE_SIZE;
-    }
-
-    return 0;
-}
-EXPORT_SYMBOL_GPL(xenbus_grant_ring);
-
  /*
   * xenbus_setup_ring
   * @dev: xenbus device
@@ -424,6 +380,7 @@ int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
                unsigned int nr_pages, grant_ref_t *grefs)
  {
      unsigned long ring_size = nr_pages * XEN_PAGE_SIZE;
+    grant_ref_t gref_head;
      unsigned int i;
      int ret;
@@ -433,9 +390,25 @@ int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
          goto err;
      }
-    ret = xenbus_grant_ring(dev, *vaddr, nr_pages, grefs);
-    if (ret)
+    ret = gnttab_alloc_grant_references(nr_pages, &gref_head);
+    if (ret) {
+        xenbus_dev_fatal(dev, ret, "granting access to %u ring pages",
+                 nr_pages);
          goto err;
+    }
+
+    for (i = 0; i < nr_pages; i++) {
+        unsigned long gfn;
+
+        if (is_vmalloc_addr(*vaddr))
+            gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr[i]));
+        else
+            gfn = virt_to_gfn(vaddr[i]);
+
+        grefs[i] = gnttab_claim_grant_reference(&gref_head);

gnttab_claim_grant_reference() can return error if no free grant reference remains.

This can happen only in case gnttab_alloc_grant_references() didn't
allocate enough grants but told us it succeeded doing so.

I understand this patch only moves the code, but probably it would be better to add a missing check here (and likely rollback already processed grants if any?).

I don't think this is needed, as this would be a clear bug in the code.

I would put WARN_ON_ONCE if ref is an error value then like xen-netfront does. Either way, with or without it you can add my:

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>






Juergen

--
Regards,

Oleksandr Tyshchenko