| Message ID | 81110f82ab57277a4200d66da5c6d438f61aef93.1452076543.git.mprivozn@redhat.com |
|---|---|
| State | New |
| Headers | show |
+-- 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
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
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
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; }
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(-)