[Qemu-devel] [PATCH] virtio-blk: Allow startup of empty cdroms

[Qemu-devel] [PATCH] virtio-blk: Allow startup of empty cdroms

[Michal Privoznik]

If you have an empty IDE cdrom we will start just fine:

-drive if=none,id=drive-ide0-0-0,readonly=on
-device ide-cd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0

However, that's not the case with virtio disk:

-drive if=none,media=cdrom,id=drive-virtio-disk1,readonly=on
-device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x2,drive=drive-virtio-disk1,id=virtio-disk1

One will get the following error:

qemu-system-x86_64: -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x2,drive=drive-virtio-disk1,id=virtio-disk1: Device needs media, but drive is empty

The error comes from virtio_blk_device_realize() where we check
if virtio block device has a media inserted. This should,
however, be not required for cdroms.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/block/virtio-blk.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 51f867b..2f687d2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -893,6 +893,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBlock *s = VIRTIO_BLK(dev);
     VirtIOBlkConf *conf = &s->conf;
+    DriveInfo *dinfo;
     Error *err = NULL;
     static int virtio_blk_id;
 
@@ -900,7 +901,10 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "drive property not set");
         return;
     }
-    if (!blk_is_inserted(conf->conf.blk)) {
+
+    dinfo = blk_legacy_dinfo(conf->conf.blk);
+    if (!((dinfo && dinfo->media_cd) ||
+          blk_is_inserted(conf->conf.blk))) {
         error_setg(errp, "Device needs media, but drive is empty");
         return;
     }
-- 
2.4.10

Re: [Qemu-devel] [PATCH] virtio-blk: Allow startup of empty cdroms

[P J P]

+-- On Wed, 6 Jan 2016, Michal Privoznik wrote --+
|      }
| -    if (!blk_is_inserted(conf->conf.blk)) {
| +
| +    dinfo = blk_legacy_dinfo(conf->conf.blk);
| +    if (!((dinfo && dinfo->media_cd) ||
| +          blk_is_inserted(conf->conf.blk))) {
|          error_setg(errp, "Device needs media, but drive is empty");
|          return;
|      }

  The if expression seems little confusing; Maybe

 ->  if (dinfo && !dinfo->media_cd && !blk_is_inserted(conf->conf.blk))

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] virtio-blk: Allow startup of empty cdroms

[Stefan Hajnoczi]

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

On Wed, Jan 06, 2016 at 11:35:43AM +0100, Michal Privoznik wrote:
> If you have an empty IDE cdrom we will start just fine:
> 
> -drive if=none,id=drive-ide0-0-0,readonly=on
> -device ide-cd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0
> 
> However, that's not the case with virtio disk:
> 
> -drive if=none,media=cdrom,id=drive-virtio-disk1,readonly=on
> -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x2,drive=drive-virtio-disk1,id=virtio-disk1
> 
> One will get the following error:
> 
> qemu-system-x86_64: -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x2,drive=drive-virtio-disk1,id=virtio-disk1: Device needs media, but drive is empty
> 
> The error comes from virtio_blk_device_realize() where we check
> if virtio block device has a media inserted. This should,
> however, be not required for cdroms.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  hw/block/virtio-blk.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

virtio-blk doesn't do CD-ROM emulation but it supports read-only block
devices.  The difference is that read-only block devices don't offer the
features that CD-ROMs support like media status and tray eject.

Also, virtio-blk does not support media change.  If we allow it to start
with an empty disk, is there an expectation that media can be changed
later?

In order to support empty media that can later be inserted virtio-blk.c
needs to implement BlockDevOps->change_media_cb() to notify the guest.
You'd need to check guest whether existing guest drivers handle a virtio
configuration interrupt properly (i.e. by invalidating cached pages from
the block devices).

Is this patch supposed to make virtio-blk support CD-ROMs?  In that case
a lot more work is necessary and it's probably better to use virtio-scsi
instead.

Is this patch purely supposed to make virtio-blk happy with empty
drives?  If so, then I'm not sure why that is a good idea since media
change isn't supported.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] virtio-blk: Allow startup of empty cdroms

[Michal Privoznik]

On 07.01.2016 05:48, Stefan Hajnoczi wrote:
> On Wed, Jan 06, 2016 at 11:35:43AM +0100, Michal Privoznik wrote:
>> If you have an empty IDE cdrom we will start just fine:
>>
>> -drive if=none,id=drive-ide0-0-0,readonly=on
>> -device ide-cd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0
>>
>> However, that's not the case with virtio disk:
>>
>> -drive if=none,media=cdrom,id=drive-virtio-disk1,readonly=on
>> -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x2,drive=drive-virtio-disk1,id=virtio-disk1
>>
>> One will get the following error:
>>
>> qemu-system-x86_64: -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x2,drive=drive-virtio-disk1,id=virtio-disk1: Device needs media, but drive is empty
>>
>> The error comes from virtio_blk_device_realize() where we check
>> if virtio block device has a media inserted. This should,
>> however, be not required for cdroms.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  hw/block/virtio-blk.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> virtio-blk doesn't do CD-ROM emulation but it supports read-only block
> devices.  The difference is that read-only block devices don't offer the
> features that CD-ROMs support like media status and tray eject.
> 
> Also, virtio-blk does not support media change.  If we allow it to start
> with an empty disk, is there an expectation that media can be changed
> later?
> 
> In order to support empty media that can later be inserted virtio-blk.c
> needs to implement BlockDevOps->change_media_cb() to notify the guest.
> You'd need to check guest whether existing guest drivers handle a virtio
> configuration interrupt properly (i.e. by invalidating cached pages from
> the block devices).
> 
> Is this patch supposed to make virtio-blk support CD-ROMs?  In that case
> a lot more work is necessary and it's probably better to use virtio-scsi
> instead.

Yeah, I realized I was aiming at CD-ROMs. But I'm not that familiar with
qemu to supply proper patch. It's just that after 39c4ae941ed992a3bb5 I
had to fix libvirt, because it was passing format= even for sourceless
devices. But while testing it I've noticed that virtio CD-ROMs still
don't work. This was my attempt to fix it. Well, maybe one day :)

Thanks!

Michal