Re: [PATCH v4 06/17] hw/xen: automatically assign device index to block devices

2023-11-06 Thread Paul Durrant

On 06/11/2023 14:34, David Woodhouse wrote:

From: David Woodhouse 

There's no need to force the user to assign a vdev. We can automatically
assign one, starting at xvda and searching until we find the first disk
name that's unused.

This means we can now allow '-drive if=xen,file=xxx' to work without an
explicit separate -driver argument, just like if=virtio.

Rip out the legacy handling from the xenpv machine, which was scribbling
over any disks configured by the toolstack, and didn't work with anything
but raw images.

Signed-off-by: David Woodhouse 
Acked-by: Kevin Wolf 
---
  blockdev.c  |  15 +++-
  hw/block/xen-block.c| 118 ++--
  hw/xen/xen_devconfig.c  |  28 ---
  hw/xenpv/xen_machine_pv.c   |   9 ---
  include/hw/xen/xen-legacy-backend.h |   1 -
  5 files changed, 125 insertions(+), 46 deletions(-)



Reviewed-by: Paul Durrant 




[PATCH v4 06/17] hw/xen: automatically assign device index to block devices

2023-11-06 Thread David Woodhouse
From: David Woodhouse 

There's no need to force the user to assign a vdev. We can automatically
assign one, starting at xvda and searching until we find the first disk
name that's unused.

This means we can now allow '-drive if=xen,file=xxx' to work without an
explicit separate -driver argument, just like if=virtio.

Rip out the legacy handling from the xenpv machine, which was scribbling
over any disks configured by the toolstack, and didn't work with anything
but raw images.

Signed-off-by: David Woodhouse 
Acked-by: Kevin Wolf 
---
 blockdev.c  |  15 +++-
 hw/block/xen-block.c| 118 ++--
 hw/xen/xen_devconfig.c  |  28 ---
 hw/xenpv/xen_machine_pv.c   |   9 ---
 include/hw/xen/xen-legacy-backend.h |   1 -
 5 files changed, 125 insertions(+), 46 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1517dc6210..e9b7e38dc4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -255,13 +255,13 @@ void drive_check_orphaned(void)
  * Ignore default drives, because we create certain default
  * drives unconditionally, then leave them unclaimed.  Not the
  * users fault.
- * Ignore IF_VIRTIO, because it gets desugared into -device,
- * so we can leave failing to -device.
+ * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
+ * -device, so we can leave failing to -device.
  * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
  * available for device_add is a feature.
  */
 if (dinfo->is_default || dinfo->type == IF_VIRTIO
-|| dinfo->type == IF_NONE) {
+|| dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
 continue;
 }
 if (!blk_get_attached_dev(blk)) {
@@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type,
 qemu_opt_set(devopts, "driver", "virtio-blk", _abort);
 qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
  _abort);
+} else if (type == IF_XEN) {
+QemuOpts *devopts;
+devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
+   _abort);
+qemu_opt_set(devopts, "driver",
+ (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
+ _abort);
+qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
+ _abort);
 }
 
 filename = qemu_opt_get(legacy_opts, "file");
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index bfa53960c3..6d64ede94f 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -27,13 +27,119 @@
 #include "sysemu/block-backend.h"
 #include "sysemu/iothread.h"
 #include "dataplane/xen-block.h"
+#include "hw/xen/interface/io/xs_wire.h"
 #include "trace.h"
 
+#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
+
+
+static int vdev_to_diskno(unsigned int vdev_nr)
+{
+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) & 0xf;
+
+default:
+return -1;
+}
+}
+
+#define MAX_AUTO_VDEV 4096
+
+/*
+ * Find a free device name in the xvda → xvdfan range and set it in
+ * blockdev->props.vdev. Our definition of "free" is that there must
+ * be no other disk or partition with the same disk number.
+ *
+ * You are technically permitted to have all of hda, hda1, sda, sda1,
+ * xvda and xvda1 as *separate* PV block devices with separate backing
+ * stores. That doesn't make it a good idea. This code will skip xvda
+ * if *any* of those "conflicting" devices already exists.
+ *
+ * The limit of xvdfan (disk 4095) is fairly arbitrary just to avoid a
+ * stupidly sized bitmap, but Linux as of v6.6 doesn't support anything
+ * higher than that anyway.
+ */
+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 = >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,
+   _existing);
+if (!existing_frontends && errno != ENOENT) {
+error_setg_errno(errp, errno, "cannot read %s", fe_path);
+return false;
+}
+
+