| Message ID | 1437990561-22134-3-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 27/07/2015 11:49, Jason Wang wrote: > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { No double underscores in userspace code. Longstanding so it can be fixed after 2.4 is out---but please remember to do it. > + if (s->conf.scsi) { > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); Unclear error message, as one would expect SCSI passthrough not to work anyway for e.g. a disk backed by a file. It's not a big deal as long as you will disable VIRTIO_BLK_F_SCSI by default in the same release that enables VIRTIO_F_VERSION_1 by default. Paolo > + return 0; > + } > + virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT); > + } else { > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > + }
On Mon, 27 Jul 2015 12:28:23 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 27/07/2015 11:49, Jason Wang wrote: > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > No double underscores in userspace code. Longstanding so it can be > fixed after 2.4 is out---but please remember to do it. There's https://marc.info/?l=qemu-devel&m=142599436726514&w=2 which seems to have been lost in all that virtio rebasing.
On Mon, Jul 27, 2015 at 12:39:44PM +0200, Cornelia Huck wrote: > On Mon, 27 Jul 2015 12:28:23 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 27/07/2015 11:49, Jason Wang wrote: > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > No double underscores in userspace code. Longstanding so it can be > > fixed after 2.4 is out---but please remember to do it. > > There's https://marc.info/?l=qemu-devel&m=142599436726514&w=2 > which seems to have been lost in all that virtio rebasing. Right. Pls repost after 2.4.
On Mon, Jul 27, 2015 at 12:28:23PM +0200, Paolo Bonzini wrote: > > > On 27/07/2015 11:49, Jason Wang wrote: > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > No double underscores in userspace code. Longstanding so it can be > fixed after 2.4 is out---but please remember to do it. > > > + if (s->conf.scsi) { > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > Unclear error message, as one would expect SCSI passthrough not to work > anyway for e.g. a disk backed by a file. Right - so I suggested: Virtio modern does not support scsi passthrough - please set disable-modern=on or switch to virtio-scsi. With that change - ACK? > > It's not a big deal as long as you will disable VIRTIO_BLK_F_SCSI by > default in the same release that enables VIRTIO_F_VERSION_1 by default. > > Paolo > > + return 0; > > + } > > + virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT); > > + } else { > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > + }
On 27/07/2015 13:26, Michael S. Tsirkin wrote: >>> > > + if (s->conf.scsi) { >>> > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); >> > >> > Unclear error message, as one would expect SCSI passthrough not to work >> > anyway for e.g. a disk backed by a file. > Right - so I suggested: > Virtio modern does not support scsi passthrough - please set disable-modern=on or switch to virtio-scsi. > With that change - ACK? scsi=on by default, so everyone is getting the message until they disable virtio 1.0. Suggesting a switch to virtio-scsi doesn't make sense. Your proposal makes sense once scsi=off by default. Until then, what about "please set scsi=off for virtio-blk devices in order to use virtio 1.0"? Paolo
On 07/27/2015 07:26 PM, Michael S. Tsirkin wrote: > On Mon, Jul 27, 2015 at 12:28:23PM +0200, Paolo Bonzini wrote: >> >> On 27/07/2015 11:49, Jason Wang wrote: >>> + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { >> No double underscores in userspace code. Longstanding so it can be >> fixed after 2.4 is out---but please remember to do it. >> >>> + if (s->conf.scsi) { >>> + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); >> Unclear error message, as one would expect SCSI passthrough not to work >> anyway for e.g. a disk backed by a file. > Right - so I suggested: > Virtio modern does not support scsi passthrough - please set disable-modern=on or switch to virtio-scsi. > With that change - ACK? I've considered this, but a little problem is 'disable-modern' only works for pci. >> It's not a big deal as long as you will disable VIRTIO_BLK_F_SCSI by >> default in the same release that enables VIRTIO_F_VERSION_1 by default. >> >> Paolo >>> + return 0; >>> + } >>> + virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT); >>> + } else { >>> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); >>> + }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index a6cf008..9acbc3a 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -731,8 +731,16 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT); + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { + if (s->conf.scsi) { + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); + return 0; + } + virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT); + } else { + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); + } if (s->conf.config_wce) { virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
SCSI passthrough was no longer supported in virtio 1.0, so this patch fail the get_features() when both 1.0 and scsi is set. And also only advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/block/virtio-blk.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)