Re: [PATCH v3 13/28] hw/xen: automatically assign device index to block devices

From: David Woodhouse <dwmw2@infradead.org>
To: paul@xen.org, qemu-devel@nongnu.org
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Hanna Reitz" <hreitz@redhat.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Anthony Perard" <anthony.perard@citrix.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Jason Wang" <jasowang@redhat.com>,
	"Marcelo Tosatti" <mtosatti@redhat.com>,
	qemu-block@nongnu.org, xen-devel@lists.xenproject.org,
	kvm@vger.kernel.org, "Bernhard Beschow" <shentey@gmail.com>,
	"Joel Upham" <jupham125@gmail.com>
Subject: Re: [PATCH v3 13/28] hw/xen: automatically assign device index to block devices
Date: Fri, 27 Oct 2023 13:02:45 +0100	[thread overview]
Message-ID: <31acd48a13b3e198a025da1add7d8f7253f262ba.camel@infradead.org> (raw)
In-Reply-To: <c538ea45-dac7-49f3-ad50-8c3a59755dee@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 7459 bytes --]

On Fri, 2023-10-27 at 11:32 +0100, Durrant, Paul wrote:
> On 27/10/2023 11:25, David Woodhouse wrote:
> > On Fri, 2023-10-27 at 10:01 +0100, Durrant, Paul wrote:
> > > 
> > > This code is allocating a name automatically so I think the onus is on
> > > it not create a needless clash which is likely to have unpredictable
> > > results depending on what the guest is. Just avoid any aliasing in the
> > > first place and things will be fine.
> > 
> > 
> > Yeah, fair enough. In which case I'll probably switch to using
> > xs_directory() and then processing those results to find a free slot,
> > instead of going out to XenStore for every existence check.
> > 
> > This isn't exactly fast path and I'm prepared to tolerate a little bit
> > of O(n²), but only within reason :)
> 
> Yes, doing an xs_directory() and then using the code 
> xen_block_get_vdev() to populate a list of existent disks will be neater.

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 5011fe9430..9b7d7ef7e1 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -30,48 +30,105 @@
 #include "hw/xen/interface/io/xs_wire.h"
 #include "trace.h"
 
-static char *xen_block_get_name(XenDevice *xendev, Error **errp)
+#define XVDA_MAJOR 202
+#define XVDQ_MAJOR (1<<20)
+#define XVDBGQCV_MAJOR ((1<<21) - 1)
+#define HDA_MAJOR 3
+#define HDC_MAJOR 22
+#define SDA_MAJOR 8
+
+/*
+ * This is fairly arbitrary just to avoid a stupidly sized bitmap, but Linux
+ * as of v6.6 only supports up to /dev/xvdfan (disk 4095) anyway.
+ */
+#define MAX_AUTO_VDEV 4096
+
+static int vdev_to_diskno(unsigned int vdev_nr)
 {
-    XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
+    switch (vdev_nr >> 8) {
+    case XVDA_MAJOR:
+    case SDA_MAJOR:
+        return (vdev_nr >> 4) & 0x15;
+
+    case HDA_MAJOR:
+        return (vdev_nr >> 6) & 1;
+
+    case HDC_MAJOR:
+        return ((vdev_nr >> 6) & 1) + 2;
+
+    case XVDQ_MAJOR ... XVDBGQCV_MAJOR:
+        return (vdev_nr >> 8) & 0xfffff;
+
+    default:
+        return -1;
+    }
+}
+
+static bool xen_block_find_free_vdev(XenBlockDevice *blockdev, Error **errp)
+{
+    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(blockdev)));
+    unsigned long used_devs[BITS_TO_LONGS(MAX_AUTO_VDEV)];
     XenBlockVdev *vdev = &blockdev->props.vdev;
+    char fe_path[XENSTORE_ABS_PATH_MAX + 1];
+    char **existing_frontends;
+    unsigned int nr_existing = 0;
+    unsigned int vdev_nr;
+    int i, disk = 0;
+
+    snprintf(fe_path, sizeof(fe_path), "/local/domain/%u/device/vbd",
+             blockdev->xendev.frontend_id);
+
+    existing_frontends = qemu_xen_xs_directory(xenbus->xsh, XBT_NULL, fe_path,
+                                               &nr_existing);
+    if (!existing_frontends && errno != ENOENT) {
+        error_setg_errno(errp, errno, "cannot read %s", fe_path);
+        return false;
+    }
 
-    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
-        XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
-        char fe_path[XENSTORE_ABS_PATH_MAX + 1];
-        char *value;
-        int disk = 0;
-        unsigned long idx;
-
-        /* Find an unoccupied device name */
-        while (disk < (1 << 20)) {
-            if (disk < (1 << 4)) {
-                idx = (202 << 8) | (disk << 4);
-            } else {
-                idx = (1 << 28) | (disk << 8);
-            }
-            snprintf(fe_path, sizeof(fe_path),
-                     "/local/domain/%u/device/vbd/%lu",
-                     xendev->frontend_id, idx);
-            value = qemu_xen_xs_read(xenbus->xsh, XBT_NULL, fe_path, NULL);
-            if (!value) {
-                if (errno == ENOENT) {
-                    vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
-                    vdev->partition = 0;
-                    vdev->disk = disk;
-                    vdev->number = idx;
-                    goto found;
-                }
-                error_setg(errp, "cannot read %s: %s", fe_path,
-                           strerror(errno));
-                return NULL;
-            }
-            free(value);
-            disk++;
+    memset(used_devs, 0, sizeof(used_devs));
+    for (i = 0; i < nr_existing; i++) {
+        if (qemu_strtoui(existing_frontends[i], NULL, 10, &vdev_nr)) {
+            free(existing_frontends[i]);
+            continue;
         }
+
+        free(existing_frontends[i]);
+
+        disk = vdev_to_diskno(vdev_nr);
+        if (disk < 0 || disk >= MAX_AUTO_VDEV) {
+            continue;
+        }
+
+        set_bit(disk, used_devs);
+    }
+    free(existing_frontends);
+
+    disk = find_first_zero_bit(used_devs, MAX_AUTO_VDEV);
+    if (disk == MAX_AUTO_VDEV) {
         error_setg(errp, "cannot find device vdev for block device");
+        return false;
+    }
+
+    vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
+    vdev->partition = 0;
+    vdev->disk = disk;
+    if (disk < (1 << 4)) {
+        vdev->number = (XVDA_MAJOR << 8) | (disk << 4);
+    } else {
+        vdev->number = (XVDQ_MAJOR << 8) | (disk << 8);
+    }
+    return true;
+}
+
+static char *xen_block_get_name(XenDevice *xendev, Error **errp)
+{
+    XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
+    XenBlockVdev *vdev = &blockdev->props.vdev;
+
+    if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID &&
+        !xen_block_find_free_vdev(blockdev, errp)) {
         return NULL;
     }
- found:
     return g_strdup_printf("%lu", vdev->number);
 }
 
@@ -520,10 +577,10 @@ static void xen_block_set_vdev(Object *obj, Visitor *v, const char *name,
     case XEN_BLOCK_VDEV_TYPE_DP:
     case XEN_BLOCK_VDEV_TYPE_XVD:
         if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
-            vdev->number = (202 << 8) | (vdev->disk << 4) |
+            vdev->number = (XVDA_MAJOR << 8) | (vdev->disk << 4) |
                 vdev->partition;
         } else if (vdev->disk < (1 << 20) && vdev->partition < (1 << 8)) {
-            vdev->number = (1 << 28) | (vdev->disk << 8) |
+            vdev->number = (XVDQ_MAJOR << 8) | (vdev->disk << 8) |
                 vdev->partition;
         } else {
             goto invalid;
@@ -533,10 +590,10 @@ static void xen_block_set_vdev(Object *obj, Visitor *v, const char *name,
     case XEN_BLOCK_VDEV_TYPE_HD:
         if ((vdev->disk == 0 || vdev->disk == 1) &&
             vdev->partition < (1 << 6)) {
-            vdev->number = (3 << 8) | (vdev->disk << 6) | vdev->partition;
+            vdev->number = (HDA_MAJOR << 8) | (vdev->disk << 6) | vdev->partition;
         } else if ((vdev->disk == 2 || vdev->disk == 3) &&
                    vdev->partition < (1 << 6)) {
-            vdev->number = (22 << 8) | ((vdev->disk - 2) << 6) |
+            vdev->number = (HDC_MAJOR << 8) | ((vdev->disk - 2) << 6) |
                 vdev->partition;
         } else {
             goto invalid;
@@ -545,7 +602,7 @@ static void xen_block_set_vdev(Object *obj, Visitor *v, const char *name,
 
     case XEN_BLOCK_VDEV_TYPE_SD:
         if (vdev->disk < (1 << 4) && vdev->partition < (1 << 4)) {
-            vdev->number = (8 << 8) | (vdev->disk << 4) | vdev->partition;
+            vdev->number = (SDA_MAJOR << 8) | (vdev->disk << 4) | vdev->partition;
         } else {
             goto invalid;
         }


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]
