Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 18 Oct 2023 09:32:47 +0100
David Woodhouse <dwmw2@infradead.org> wrote:

> On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
> > On Mon, 16 Oct 2023 16:19:08 +0100
> > David Woodhouse <dwmw2@infradead.org> wrote:
> >   
> > > From: David Woodhouse <dwmw@amazon.co.uk>
> > >   
> > 
> > is this index a user (guest) visible?  
> 
> Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
> in the guest. In the common case, it literally encodes the Linux
> major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.

that makes 'index' an implicit ABI and a subject to versioning
when the way it's assigned changes (i.e. one has to use versioned
machine types to keep older versions working the they used to).

From what I remember it's discouraged to make QEMU invent
various IDs that are part of ABI (guest or mgmt side).
Instead it's preferred for mgmt side/user to provide that explicitly.

Basically you are trading off manageability/simplicity at QEMU
level with CLI usability for human user.
I don't care much as long as it is hidden within xen code base,
but maybe libvirt does.

> Previously we had to explicitly set it for each disk in Qemu:
> 
>   -drive file=disk1.img,id=drv1 -device xen-disk,drive=drv1,vdev=xvda
>   -drive file=disk2.img,id=drv2 -device xen-disk,drive=drv2,vdev=xvdb
> 
> Now we can just do
> 
>   -drive file=disk1.img,if=xen -drive file-disk2.img,if=xen
>
> (We could go further and make if=xen the default for Xen guests too,
> but that doesn't work right now because xen-block will barf on the
> default provided CD-ROM when it's empty. It doesn't handle empty
> devices. And if I work around that, then `-hda disk1.img` would work on
> the command line... but would make it appear as /dev/xvda instead of
> /dev/hda, and I don't know how I feel about that.)
>
> [root@localhost ~]# xenstore-ls  -f device/vbd
> device/vbd = ""
> device/vbd/51712 = ""
> device/vbd/51712/backend = "/local/domain/0/backend/qdisk/1/51712"
> device/vbd/51712/backend-id = "0"
> device/vbd/51712/device-type = "disk"
> device/vbd/51712/event-channel = "8"
> device/vbd/51712/feature-persistent = "1"
> device/vbd/51712/protocol = "x86_64-abi"
> device/vbd/51712/ring-ref = "8"
> device/vbd/51712/state = "4"
> device/vbd/51712/virtual-device = "51712"
> 
> >   
> > > 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.
> > > 
> > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > > ---
> > >  blockdev.c           | 15 ++++++++++++---
> > >  hw/block/xen-block.c | 25 +++++++++++++++++++++++++
> > >  2 files changed, 37 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 325b7a3bef..9dec4c9c74 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", &error_abort);
> > >          qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> > >                       &error_abort);
> > > +    } else if (type == IF_XEN) {
> > > +        QemuOpts *devopts;
> > > +        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> > > +                                   &error_abort);
> > > +        qemu_opt_set(devopts, "driver",
> > > +                     (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
> > > +                     &error_abort);
> > > +        qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> > > +                     &error_abort);
> > >      }
> > >  
> > >      filename = qemu_opt_get(legacy_opts, "file");
> > > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> > > index 9262338535..20fa783cbe 100644
> > > --- a/hw/block/xen-block.c
> > > +++ b/hw/block/xen-block.c
> > > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error **errp)
> > >      XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
> > >      XenBlockVdev *vdev = &blockdev->props.vdev;
> > >  
> > > +    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> > > +        char name[11];
> > > +        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(name, sizeof(name), "%lu", idx);
> > > +            if (!xen_backend_exists("qdisk", name)) {
> > > +                vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
> > > +                vdev->partition = 0;
> > > +                vdev->disk = disk;
> > > +                vdev->number = idx;
> > > +                return g_strdup(name);
> > > +            }
> > > +            disk++;
> > > +        }
> > > +        error_setg(errp, "cannot find device vdev for block device");
> > > +        return NULL;
> > > +    }
> > >      return g_strdup_printf("%lu", vdev->number);
> > >  }
> > >    
> > 
> >   
>