Re: [PULL 06/15] hw/xen: automatically assign device index to block devices
On Thu, 2023-11-09 at 14:33 +, Peter Maydell wrote:
> On Tue, 7 Nov 2023 at 09:24, 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.
>
> Hi; Coverity points out an issue in this code (CID 1523906):
Thanks! I think this one is a false positive, although I'm happy to
explore possible cleanups which make it clearer both to the human
reader and to Coverity.
> > +/*
> > + * 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) {
>
> Here we check whether existing_frontends is NULL, implying it
> might be NULL (and the && in the condition means we might not
> take this error-exit path even if it is NULL)...
True, but nr_existing will be zero in that case, and we'll never go
into the loop where existing_frontends[] is dereferenced.
I suppose we could add something like this. Would it help Coverity to
realise that it's a false positive?
/*
* If the directory didn't exist (the ENOENT case) then nr_existing
* will still be zero, and the loop below won't dereference the
* existing_frontends pointer which is NULL.
*/
assert(existing_frontends || !nr_existing);
> > + error_setg_errno(errp, errno, "cannot read %s", fe_path);
> > + return false;
> > + }
> > +
> > + memset(used_devs, 0, sizeof(used_devs));
> > + for (i = 0; i < nr_existing; i++) {
> > + if (qemu_strtoui(existing_frontends[i], NULL, 10, _nr)) {
>
> ...but here we deref existing_frontends, implying it can't be NULL.
>
If you got to this line of code, it can't.
> > + 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);
*Here* it can be NULL. But free(NULL) is fine.
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 06/15] hw/xen: automatically assign device index to block devices
On Tue, 7 Nov 2023 at 09:24, 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.
Hi; Coverity points out an issue in this code (CID 1523906):
> +/*
> + * 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) {
Here we check whether existing_frontends is NULL, implying it
might be NULL (and the && in the condition means we might not
take this error-exit path even if it is NULL)...
> +error_setg_errno(errp, errno, "cannot read %s", fe_path);
> +return false;
> +}
> +
> +memset(used_devs, 0, sizeof(used_devs));
> +for (i = 0; i < nr_existing; i++) {
> +if (qemu_strtoui(existing_frontends[i], NULL, 10, _nr)) {
...but here we deref existing_frontends, implying it can't be NULL.
> +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);
thanks
-- PMM
[PULL 06/15] hw/xen: automatically assign device index to block devices
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
Reviewed-by: Paul Durrant
---
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);
+
