| Message ID | 1436766411-29144-2-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: qemu-block@nongnu.org > Signed-off-by: Jason Wang <jasowang@redhat.com> Interesting, I noticed we have a field scsi - see commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Fri Dec 23 15:39:03 2011 +0100 virtio-blk: refuse SG_IO requests with scsi=off but it doesn't seem to be propagated to guest features in any way. Maybe we should fix that, making that flag AutoOnOff? Then, if user explicitly requested scsi=on with a modern interface then we can error out cleanly. Given scsi flag is currently ignored, I think this can be a patch on top. > --- > hw/block/virtio-blk.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 6aefda4..f30ad25 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -730,7 +730,8 @@ 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); > + if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1)) > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > if (s->conf.config_wce) { > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > -- > 2.1.4
On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. >> >> Cc: Stefan Hajnoczi <stefanha@redhat.com> >> Cc: Kevin Wolf <kwolf@redhat.com> >> Cc: qemu-block@nongnu.org >> Signed-off-by: Jason Wang <jasowang@redhat.com> > Interesting, I noticed we have a field scsi - see > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > Author: Paolo Bonzini <pbonzini@redhat.com> > Date: Fri Dec 23 15:39:03 2011 +0100 > > virtio-blk: refuse SG_IO requests with scsi=off > > but it doesn't seem to be propagated to guest features in > any way. > > Maybe we should fix that, making that flag AutoOnOff? Looks ok but auto may need some compat work since default is true. > Then, if user explicitly requested scsi=on with a modern > interface then we can error out cleanly. > > Given scsi flag is currently ignored, I think > this can be a patch on top. Looks like virtio_blk_handle_scsi_req() check this: if (!blk->conf.scsi) { status = VIRTIO_BLK_S_UNSUPP; goto fail; } > >> --- >> hw/block/virtio-blk.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 6aefda4..f30ad25 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -730,7 +730,8 @@ 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); >> + if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1)) >> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); >> >> if (s->conf.config_wce) { >> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); >> -- >> 2.1.4
Am 13.07.2015 um 11:00 hat Jason Wang geschrieben: > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > >> > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > >> Cc: Kevin Wolf <kwolf@redhat.com> > >> Cc: qemu-block@nongnu.org > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > Interesting, I noticed we have a field scsi - see > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > Author: Paolo Bonzini <pbonzini@redhat.com> > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > but it doesn't seem to be propagated to guest features in > > any way. > > > > Maybe we should fix that, making that flag AutoOnOff? > > Looks ok but auto may need some compat work since default is true. > > > Then, if user explicitly requested scsi=on with a modern > > interface then we can error out cleanly. > > > > Given scsi flag is currently ignored, I think > > this can be a patch on top. > > Looks like virtio_blk_handle_scsi_req() check this: > > if (!blk->conf.scsi) { > status = VIRTIO_BLK_S_UNSUPP; > goto fail; > } So we should be checking the same condition for the feature flag and error out in the init function if we have a VERSION_1 device and blk->conf.scsi is set. > >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > >> index 6aefda4..f30ad25 100644 > >> --- a/hw/block/virtio-blk.c > >> +++ b/hw/block/virtio-blk.c > >> @@ -730,7 +730,8 @@ 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); > >> + if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1)) > >> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); Coding style: Braces are needed here. (Same in the other patch you CCed me on). Kevin
On Mon, Jul 13, 2015 at 05:00:51PM +0800, Jason Wang wrote: > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > >> > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > >> Cc: Kevin Wolf <kwolf@redhat.com> > >> Cc: qemu-block@nongnu.org > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > Interesting, I noticed we have a field scsi - see > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > Author: Paolo Bonzini <pbonzini@redhat.com> > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > but it doesn't seem to be propagated to guest features in > > any way. > > > > Maybe we should fix that, making that flag AutoOnOff? > > Looks ok but auto may need some compat work since default is true. Right. Auto would then mean "!modern". > > Then, if user explicitly requested scsi=on with a modern > > interface then we can error out cleanly. > > > > Given scsi flag is currently ignored, I think > > this can be a patch on top. > > Looks like virtio_blk_handle_scsi_req() check this: > > if (!blk->conf.scsi) { > status = VIRTIO_BLK_S_UNSUPP; > goto fail; > } > > > > >> --- > >> hw/block/virtio-blk.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > >> index 6aefda4..f30ad25 100644 > >> --- a/hw/block/virtio-blk.c > >> +++ b/hw/block/virtio-blk.c > >> @@ -730,7 +730,8 @@ 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); > >> + if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1)) > >> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > >> > >> if (s->conf.config_wce) { > >> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > >> -- > >> 2.1.4
On Mon, 13 Jul 2015 11:56:51 +0200 Kevin Wolf <kwolf@redhat.com> wrote: > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben: > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > > >> > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > > >> Cc: Kevin Wolf <kwolf@redhat.com> > > >> Cc: qemu-block@nongnu.org > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > > Interesting, I noticed we have a field scsi - see > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > > > but it doesn't seem to be propagated to guest features in > > > any way. > > > > > > Maybe we should fix that, making that flag AutoOnOff? > > > > Looks ok but auto may need some compat work since default is true. > > > > > Then, if user explicitly requested scsi=on with a modern > > > interface then we can error out cleanly. > > > > > > Given scsi flag is currently ignored, I think > > > this can be a patch on top. > > > > Looks like virtio_blk_handle_scsi_req() check this: > > > > if (!blk->conf.scsi) { > > status = VIRTIO_BLK_S_UNSUPP; > > goto fail; > > } > > So we should be checking the same condition for the feature flag and > error out in the init function if we have a VERSION_1 device and > blk->conf.scsi is set. Hm, I wonder how this plays with transports that want to make the virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically only want to offer VERSION_1 if the driver negotiated revision >= 1. I'd need to check for !scsi as well before I can add this feature bit then? Have the init function set a blocker for VERSION_1 so that the driver may only negotiate revision 0?
On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote: > On Mon, 13 Jul 2015 11:56:51 +0200 > Kevin Wolf <kwolf@redhat.com> wrote: > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben: > > > > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > > > >> > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > > > >> Cc: Kevin Wolf <kwolf@redhat.com> > > > >> Cc: qemu-block@nongnu.org > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > Interesting, I noticed we have a field scsi - see > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > > > > > but it doesn't seem to be propagated to guest features in > > > > any way. > > > > > > > > Maybe we should fix that, making that flag AutoOnOff? > > > > > > Looks ok but auto may need some compat work since default is true. > > > > > > > Then, if user explicitly requested scsi=on with a modern > > > > interface then we can error out cleanly. > > > > > > > > Given scsi flag is currently ignored, I think > > > > this can be a patch on top. > > > > > > Looks like virtio_blk_handle_scsi_req() check this: > > > > > > if (!blk->conf.scsi) { > > > status = VIRTIO_BLK_S_UNSUPP; > > > goto fail; > > > } > > > > So we should be checking the same condition for the feature flag and > > error out in the init function if we have a VERSION_1 device and > > blk->conf.scsi is set. > > Hm, I wonder how this plays with transports that want to make the > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically > only want to offer VERSION_1 if the driver negotiated revision >= 1. > I'd need to check for !scsi as well before I can add this feature bit > then? Have the init function set a blocker for VERSION_1 so that the > driver may only negotiate revision 0? We already handle this, do we not? if (features.index == 0) { features.features = (uint32_t)vdev->host_features; } else if (features.index == 1) { features.features = (uint32_t)(vdev->host_features >> 32); /* * Don't offer version 1 to the guest if it did not * negotiate at least revision 1. */ if (dev->revision <= 0) { features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32)); } } else { /* Return zeroes if the guest supports more feature bits. */ features.features = 0; } So guest that doesn't negotiate revision >= 1 never gets to see VIRTIO_F_VERSION_1. Maybe we should go further and additionally all bits >= 32 if VIRTIO_F_VERSION_1 is clear, but that can wait and we have no bits like that in 2.4.
On Mon, 13 Jul 2015 15:22:52 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote: > > On Mon, 13 Jul 2015 11:56:51 +0200 > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben: > > > > > > > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > > > > >> > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > > > > >> Cc: Kevin Wolf <kwolf@redhat.com> > > > > >> Cc: qemu-block@nongnu.org > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > Interesting, I noticed we have a field scsi - see > > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > > > > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > > > > > > > but it doesn't seem to be propagated to guest features in > > > > > any way. > > > > > > > > > > Maybe we should fix that, making that flag AutoOnOff? > > > > > > > > Looks ok but auto may need some compat work since default is true. > > > > > > > > > Then, if user explicitly requested scsi=on with a modern > > > > > interface then we can error out cleanly. > > > > > > > > > > Given scsi flag is currently ignored, I think > > > > > this can be a patch on top. > > > > > > > > Looks like virtio_blk_handle_scsi_req() check this: > > > > > > > > if (!blk->conf.scsi) { > > > > status = VIRTIO_BLK_S_UNSUPP; > > > > goto fail; > > > > } > > > > > > So we should be checking the same condition for the feature flag and > > > error out in the init function if we have a VERSION_1 device and > > > blk->conf.scsi is set. > > > > Hm, I wonder how this plays with transports that want to make the > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically > > only want to offer VERSION_1 if the driver negotiated revision >= 1. > > I'd need to check for !scsi as well before I can add this feature bit > > then? Have the init function set a blocker for VERSION_1 so that the > > driver may only negotiate revision 0? > > > We already handle this, do we not? (...) > So guest that doesn't negotiate revision >= 1 never gets to see > VIRTIO_F_VERSION_1. Not my question :) I was wondering about scsi vs. virtio-1 devices. And as I basically only want to make the decision on whether to offer VERSION_1 when the guest negotiated a revision, I cannot fence scsi during init, no? > > Maybe we should go further and additionally all bits >= 32 if > VIRTIO_F_VERSION_1 is clear, but that can wait > and we have no bits like that in 2.4. > Spec says bits >= 32 are only valid if we have VERSION_1, doesn't it? Sounds sensible.
On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote: > On Mon, 13 Jul 2015 15:22:52 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote: > > > On Mon, 13 Jul 2015 11:56:51 +0200 > > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben: > > > > > > > > > > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > > > > > >> > > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > > > > > >> Cc: Kevin Wolf <kwolf@redhat.com> > > > > > >> Cc: qemu-block@nongnu.org > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > Interesting, I noticed we have a field scsi - see > > > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > > > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > > > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > > > > > > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > > > > > > > > > but it doesn't seem to be propagated to guest features in > > > > > > any way. > > > > > > > > > > > > Maybe we should fix that, making that flag AutoOnOff? > > > > > > > > > > Looks ok but auto may need some compat work since default is true. > > > > > > > > > > > Then, if user explicitly requested scsi=on with a modern > > > > > > interface then we can error out cleanly. > > > > > > > > > > > > Given scsi flag is currently ignored, I think > > > > > > this can be a patch on top. > > > > > > > > > > Looks like virtio_blk_handle_scsi_req() check this: > > > > > > > > > > if (!blk->conf.scsi) { > > > > > status = VIRTIO_BLK_S_UNSUPP; > > > > > goto fail; > > > > > } > > > > > > > > So we should be checking the same condition for the feature flag and > > > > error out in the init function if we have a VERSION_1 device and > > > > blk->conf.scsi is set. > > > > > > Hm, I wonder how this plays with transports that want to make the > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically > > > only want to offer VERSION_1 if the driver negotiated revision >= 1. > > > I'd need to check for !scsi as well before I can add this feature bit > > > then? Have the init function set a blocker for VERSION_1 so that the > > > driver may only negotiate revision 0? > > > > > > We already handle this, do we not? > (...) > > So guest that doesn't negotiate revision >= 1 never gets to see > > VIRTIO_F_VERSION_1. > > Not my question :) I was wondering about scsi vs. virtio-1 devices. And > as I basically only want to make the decision on whether to offer > VERSION_1 when the guest negotiated a revision, I cannot fence scsi > during init, no? No, I don't think there's a lot of value in offering scsi only to old guests that don't negotiate revision >= 1. If user asked for virtio 1 support then that by proxy implies scsi passthrough does not work, and it won't work for legacy guests too. > > > > Maybe we should go further and additionally all bits >= 32 if > > VIRTIO_F_VERSION_1 is clear, but that can wait > > and we have no bits like that in 2.4. > > > Spec says bits >= 32 are only valid if we have VERSION_1, doesn't it? > Sounds sensible.
On Mon, 13 Jul 2015 15:36:00 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote: > > On Mon, 13 Jul 2015 15:22:52 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote: > > > > On Mon, 13 Jul 2015 11:56:51 +0200 > > > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben: > > > > > > > > > > > > > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > > > > > > >> > > > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > > > > > > >> Cc: Kevin Wolf <kwolf@redhat.com> > > > > > > >> Cc: qemu-block@nongnu.org > > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > Interesting, I noticed we have a field scsi - see > > > > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > > > > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > > > > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > > > > > > > > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > > > > > > > > > > > but it doesn't seem to be propagated to guest features in > > > > > > > any way. > > > > > > > > > > > > > > Maybe we should fix that, making that flag AutoOnOff? > > > > > > > > > > > > Looks ok but auto may need some compat work since default is true. > > > > > > > > > > > > > Then, if user explicitly requested scsi=on with a modern > > > > > > > interface then we can error out cleanly. > > > > > > > > > > > > > > Given scsi flag is currently ignored, I think > > > > > > > this can be a patch on top. > > > > > > > > > > > > Looks like virtio_blk_handle_scsi_req() check this: > > > > > > > > > > > > if (!blk->conf.scsi) { > > > > > > status = VIRTIO_BLK_S_UNSUPP; > > > > > > goto fail; > > > > > > } > > > > > > > > > > So we should be checking the same condition for the feature flag and > > > > > error out in the init function if we have a VERSION_1 device and > > > > > blk->conf.scsi is set. > > > > > > > > Hm, I wonder how this plays with transports that want to make the > > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically > > > > only want to offer VERSION_1 if the driver negotiated revision >= 1. > > > > I'd need to check for !scsi as well before I can add this feature bit > > > > then? Have the init function set a blocker for VERSION_1 so that the > > > > driver may only negotiate revision 0? > > > > > > > > > We already handle this, do we not? > > (...) > > > So guest that doesn't negotiate revision >= 1 never gets to see > > > VIRTIO_F_VERSION_1. > > > > Not my question :) I was wondering about scsi vs. virtio-1 devices. And > > as I basically only want to make the decision on whether to offer > > VERSION_1 when the guest negotiated a revision, I cannot fence scsi > > during init, no? > > No, I don't think there's a lot of value in offering scsi only to > old guests that don't negotiate revision >= 1. > > If user asked for virtio 1 support then that by proxy implies scsi > passthrough does not work, and it won't work for legacy > guests too. This would imply that any transitional device cannot offer scsi, doesn't it? We have two layers interacting here: virtio-blk which may or may not offer scsi support, and the transport layer which may or may not offer VERSION_1 support. Failing scsi commands if VERSION_1 has been negotiated makes sense to me; but I don't want to disable scsi config a priori because the driver might negotiate VERSION_1. This would imply that virtio-blk over virtio-ccw would never offer scsi once we enable virtio-1 support, and it kind of defeats the purpose of a transitional device for me. (The other way round - fail negotiating revison 1 if the device was configured with scsi support - makes more sense to me.)
On 13/07/2015 15:20, Cornelia Huck wrote: > This would imply that any transitional device cannot offer scsi, > doesn't it? > > We have two layers interacting here: virtio-blk which may or may not > offer scsi support, and the transport layer which may or may not offer > VERSION_1 support. Failing scsi commands if VERSION_1 has been > negotiated makes sense to me; but I don't want to disable scsi config a > priori because the driver might negotiate VERSION_1. This would imply > that virtio-blk over virtio-ccw would never offer scsi once we enable > virtio-1 support, and it kind of defeats the purpose of a transitional > device for me. > > (The other way round - fail negotiating revison 1 if the device was > configured with scsi support - makes more sense to me.) For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI altogether if !blk->conf.scsi. Would that fix the problem for you too? Paolo
On Mon, 13 Jul 2015 16:34:30 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 13/07/2015 15:20, Cornelia Huck wrote: > > This would imply that any transitional device cannot offer scsi, > > doesn't it? > > > > We have two layers interacting here: virtio-blk which may or may not > > offer scsi support, and the transport layer which may or may not offer > > VERSION_1 support. Failing scsi commands if VERSION_1 has been > > negotiated makes sense to me; but I don't want to disable scsi config a > > priori because the driver might negotiate VERSION_1. This would imply > > that virtio-blk over virtio-ccw would never offer scsi once we enable > > virtio-1 support, and it kind of defeats the purpose of a transitional > > device for me. > > > > (The other way round - fail negotiating revison 1 if the device was > > configured with scsi support - makes more sense to me.) > > For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI > altogether if !blk->conf.scsi. Would that fix the problem for you too? This is probably a sensible approach, and it can be contained in virtio-block, no?
On 13/07/2015 16:41, Cornelia Huck wrote: > On Mon, 13 Jul 2015 16:34:30 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 13/07/2015 15:20, Cornelia Huck wrote: >>> This would imply that any transitional device cannot offer scsi, >>> doesn't it? >>> >>> We have two layers interacting here: virtio-blk which may or may not >>> offer scsi support, and the transport layer which may or may not offer >>> VERSION_1 support. Failing scsi commands if VERSION_1 has been >>> negotiated makes sense to me; but I don't want to disable scsi config a >>> priori because the driver might negotiate VERSION_1. This would imply >>> that virtio-blk over virtio-ccw would never offer scsi once we enable >>> virtio-1 support, and it kind of defeats the purpose of a transitional >>> device for me. >>> >>> (The other way round - fail negotiating revison 1 if the device was >>> configured with scsi support - makes more sense to me.) >> >> For newer machine types, it would make sense to block VIRTIO_BLK_F_SCSI >> altogether if !blk->conf.scsi. Would that fix the problem for you too? > > This is probably a sensible approach, and it can be contained in > virtio-block, no? Yes, I think so. Paolo
On Mon, Jul 13, 2015 at 03:20:59PM +0200, Cornelia Huck wrote: > On Mon, 13 Jul 2015 15:36:00 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote: > > > On Mon, 13 Jul 2015 15:22:52 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote: > > > > > On Mon, 13 Jul 2015 11:56:51 +0200 > > > > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben: > > > > > > > > > > > > > > > > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > > > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > > > > > > > >> > > > > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > > > > > > > >> Cc: Kevin Wolf <kwolf@redhat.com> > > > > > > > >> Cc: qemu-block@nongnu.org > > > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > Interesting, I noticed we have a field scsi - see > > > > > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > > > > > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > > > > > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > > > > > > > > > > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > > > > > > > > > > > > > but it doesn't seem to be propagated to guest features in > > > > > > > > any way. > > > > > > > > > > > > > > > > Maybe we should fix that, making that flag AutoOnOff? > > > > > > > > > > > > > > Looks ok but auto may need some compat work since default is true. > > > > > > > > > > > > > > > Then, if user explicitly requested scsi=on with a modern > > > > > > > > interface then we can error out cleanly. > > > > > > > > > > > > > > > > Given scsi flag is currently ignored, I think > > > > > > > > this can be a patch on top. > > > > > > > > > > > > > > Looks like virtio_blk_handle_scsi_req() check this: > > > > > > > > > > > > > > if (!blk->conf.scsi) { > > > > > > > status = VIRTIO_BLK_S_UNSUPP; > > > > > > > goto fail; > > > > > > > } > > > > > > > > > > > > So we should be checking the same condition for the feature flag and > > > > > > error out in the init function if we have a VERSION_1 device and > > > > > > blk->conf.scsi is set. > > > > > > > > > > Hm, I wonder how this plays with transports that want to make the > > > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically > > > > > only want to offer VERSION_1 if the driver negotiated revision >= 1. > > > > > I'd need to check for !scsi as well before I can add this feature bit > > > > > then? Have the init function set a blocker for VERSION_1 so that the > > > > > driver may only negotiate revision 0? > > > > > > > > > > > > We already handle this, do we not? > > > (...) > > > > So guest that doesn't negotiate revision >= 1 never gets to see > > > > VIRTIO_F_VERSION_1. > > > > > > Not my question :) I was wondering about scsi vs. virtio-1 devices. And > > > as I basically only want to make the decision on whether to offer > > > VERSION_1 when the guest negotiated a revision, I cannot fence scsi > > > during init, no? > > > > No, I don't think there's a lot of value in offering scsi only to > > old guests that don't negotiate revision >= 1. > > > > If user asked for virtio 1 support then that by proxy implies scsi > > passthrough does not work, and it won't work for legacy > > guests too. > > This would imply that any transitional device cannot offer scsi, > doesn't it? Yes, and that's because as written, transitional devices must set ANY_LAYOUT, but that's incompatible with scsi. > We have two layers interacting here: virtio-blk which may or may not > offer scsi support, and the transport layer which may or may not offer > VERSION_1 support. Failing scsi commands if VERSION_1 has been > negotiated makes sense to me; but I don't want to disable scsi config a > priori because the driver might negotiate VERSION_1. This would imply > that virtio-blk over virtio-ccw would never offer scsi once we enable > virtio-1 support, and it kind of defeats the purpose of a transitional > device for me. > > (The other way round - fail negotiating revison 1 if the device was > configured with scsi support - makes more sense to me.)
On Mon, 13 Jul 2015 18:35:53 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Jul 13, 2015 at 03:20:59PM +0200, Cornelia Huck wrote: > > On Mon, 13 Jul 2015 15:36:00 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Mon, Jul 13, 2015 at 02:30:24PM +0200, Cornelia Huck wrote: > > > > On Mon, 13 Jul 2015 15:22:52 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote: > > > > > > On Mon, 13 Jul 2015 11:56:51 +0200 > > > > > > Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > > > > > > > Am 13.07.2015 um 11:00 hat Jason Wang geschrieben: > > > > > > > > > > > > > > > > > > > > > > > > On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote: > > > > > > > > > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote: > > > > > > > > >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. > > > > > > > > >> > > > > > > > > >> Cc: Stefan Hajnoczi <stefanha@redhat.com> > > > > > > > > >> Cc: Kevin Wolf <kwolf@redhat.com> > > > > > > > > >> Cc: qemu-block@nongnu.org > > > > > > > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > Interesting, I noticed we have a field scsi - see > > > > > > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > > > > > > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > > > > > > > Date: Fri Dec 23 15:39:03 2011 +0100 > > > > > > > > > > > > > > > > > > virtio-blk: refuse SG_IO requests with scsi=off > > > > > > > > > > > > > > > > > > but it doesn't seem to be propagated to guest features in > > > > > > > > > any way. > > > > > > > > > > > > > > > > > > Maybe we should fix that, making that flag AutoOnOff? > > > > > > > > > > > > > > > > Looks ok but auto may need some compat work since default is true. > > > > > > > > > > > > > > > > > Then, if user explicitly requested scsi=on with a modern > > > > > > > > > interface then we can error out cleanly. > > > > > > > > > > > > > > > > > > Given scsi flag is currently ignored, I think > > > > > > > > > this can be a patch on top. > > > > > > > > > > > > > > > > Looks like virtio_blk_handle_scsi_req() check this: > > > > > > > > > > > > > > > > if (!blk->conf.scsi) { > > > > > > > > status = VIRTIO_BLK_S_UNSUPP; > > > > > > > > goto fail; > > > > > > > > } > > > > > > > > > > > > > > So we should be checking the same condition for the feature flag and > > > > > > > error out in the init function if we have a VERSION_1 device and > > > > > > > blk->conf.scsi is set. > > > > > > > > > > > > Hm, I wonder how this plays with transports that want to make the > > > > > > virtio-1 vs. legacy decision post-init? For virtio-ccw, I basically > > > > > > only want to offer VERSION_1 if the driver negotiated revision >= 1. > > > > > > I'd need to check for !scsi as well before I can add this feature bit > > > > > > then? Have the init function set a blocker for VERSION_1 so that the > > > > > > driver may only negotiate revision 0? > > > > > > > > > > > > > > > We already handle this, do we not? > > > > (...) > > > > > So guest that doesn't negotiate revision >= 1 never gets to see > > > > > VIRTIO_F_VERSION_1. > > > > > > > > Not my question :) I was wondering about scsi vs. virtio-1 devices. And > > > > as I basically only want to make the decision on whether to offer > > > > VERSION_1 when the guest negotiated a revision, I cannot fence scsi > > > > during init, no? > > > > > > No, I don't think there's a lot of value in offering scsi only to > > > old guests that don't negotiate revision >= 1. > > > > > > If user asked for virtio 1 support then that by proxy implies scsi > > > passthrough does not work, and it won't work for legacy > > > guests too. > > > > This would imply that any transitional device cannot offer scsi, > > doesn't it? > > Yes, and that's because as written, transitional devices must set > ANY_LAYOUT, but that's incompatible with scsi. Hm, I had a patch before that dynamically allowed different feature sets for legacy or modern, not only a subset. Probably won't apply anymore, but I'd like to able to do the following: - driver reads features without negotiating a revision: driver is legacy, offer legacy bits - driver negotiates revision 0: dito - driver negotiates revision >= 1: driver is modern, offer modern bits That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the first two cases, and a new qemu could still offer scsi to old guests. Would it be worth pursuing that idea?
On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: > > Yes, and that's because as written, transitional devices must set > > ANY_LAYOUT, but that's incompatible with scsi. > > Hm, I had a patch before that dynamically allowed different feature > sets for legacy or modern, not only a subset. Probably won't apply > anymore, but I'd like to able to do the following: > > - driver reads features without negotiating a revision: driver is > legacy, offer legacy bits > - driver negotiates revision 0: dito > - driver negotiates revision >= 1: driver is modern, offer modern bits > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the > first two cases, and a new qemu could still offer scsi to old guests. > > Would it be worth pursuing that idea? Frankly, I don't think so: I don't see why it makes sense to expose more features on the legacy interface than on the modern one. Imagine updating drivers to fix a bug and losing some features. How does this make sense? I think the virtio TC's assumption was that the scsi passthrough was a bad idea, so in QEMU we only keep it around for legacy devices to avoid regressions. If you disagree and think transitional devices need the SCSI feature, either try to convince pbonzini or rewrite the spec youself to support it in the virtio 1 mode.
On Wed, 15 Jul 2015 13:59:00 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: > > > Yes, and that's because as written, transitional devices must set > > > ANY_LAYOUT, but that's incompatible with scsi. > > > > Hm, I had a patch before that dynamically allowed different feature > > sets for legacy or modern, not only a subset. Probably won't apply > > anymore, but I'd like to able to do the following: > > > > - driver reads features without negotiating a revision: driver is > > legacy, offer legacy bits > > - driver negotiates revision 0: dito > > - driver negotiates revision >= 1: driver is modern, offer modern bits > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the > > first two cases, and a new qemu could still offer scsi to old guests. > > > > Would it be worth pursuing that idea? > > Frankly, I don't think so: I don't see why it makes sense > to expose more features on the legacy interface than > on the modern one. Imagine updating drivers to fix a bug > and losing some features. How does this make sense? I don't think one should be a strict subset of the other. But I think we don't want to withdraw features from legacy guests on qemu updates either? > > I think the virtio TC's assumption was that the scsi passthrough was a > bad idea, so in QEMU we only keep it around for legacy devices to avoid > regressions. I'm not opposing this :) > > If you disagree and think transitional devices need the SCSI feature, > either try to convince pbonzini or rewrite the spec youself > to support it in the virtio 1 mode. This seems to boil down to the different meaning of "transitional" for ccw and pci, see the other thread.
On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: > On Wed, 15 Jul 2015 13:59:00 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: > > > > Yes, and that's because as written, transitional devices must set > > > > ANY_LAYOUT, but that's incompatible with scsi. > > > > > > Hm, I had a patch before that dynamically allowed different feature > > > sets for legacy or modern, not only a subset. Probably won't apply > > > anymore, but I'd like to able to do the following: > > > > > > - driver reads features without negotiating a revision: driver is > > > legacy, offer legacy bits > > > - driver negotiates revision 0: dito > > > - driver negotiates revision >= 1: driver is modern, offer modern bits > > > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the > > > first two cases, and a new qemu could still offer scsi to old guests. > > > > > > Would it be worth pursuing that idea? > > > > Frankly, I don't think so: I don't see why it makes sense > > to expose more features on the legacy interface than > > on the modern one. Imagine updating drivers to fix a bug > > and losing some features. How does this make sense? > > I don't think one should be a strict subset of the other. But I think > we don't want to withdraw features from legacy guests on qemu updates > either? Absolutely. For now one has to enable the modern interface explicitly. Around 2.5 we might switch that around, we'll need to think hard about compatibility at that point. In any case, we must definitely keep the old capability for old machine types. > > > > I think the virtio TC's assumption was that the scsi passthrough was a > > bad idea, so in QEMU we only keep it around for legacy devices to avoid > > regressions. > > I'm not opposing this :) > > > > > If you disagree and think transitional devices need the SCSI feature, > > either try to convince pbonzini or rewrite the spec youself > > to support it in the virtio 1 mode. > > This seems to boil down to the different meaning of "transitional" for > ccw and pci, see the other thread. Before the revision is negotiated, ccw won't know whether it's a legacy driver - is that the difference? Fine, but revision is negotiated way before features are probed so why does it make a practical difference?
On Wed, 15 Jul 2015 15:01:01 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: > > On Wed, 15 Jul 2015 13:59:00 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: > > > > > Yes, and that's because as written, transitional devices must set > > > > > ANY_LAYOUT, but that's incompatible with scsi. > > > > > > > > Hm, I had a patch before that dynamically allowed different feature > > > > sets for legacy or modern, not only a subset. Probably won't apply > > > > anymore, but I'd like to able to do the following: > > > > > > > > - driver reads features without negotiating a revision: driver is > > > > legacy, offer legacy bits > > > > - driver negotiates revision 0: dito > > > > - driver negotiates revision >= 1: driver is modern, offer modern bits > > > > > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the > > > > first two cases, and a new qemu could still offer scsi to old guests. > > > > > > > > Would it be worth pursuing that idea? > > > > > > Frankly, I don't think so: I don't see why it makes sense > > > to expose more features on the legacy interface than > > > on the modern one. Imagine updating drivers to fix a bug > > > and losing some features. How does this make sense? > > > > I don't think one should be a strict subset of the other. But I think > > we don't want to withdraw features from legacy guests on qemu updates > > either? > > Absolutely. For now one has to enable the modern interface > explicitly. Around 2.5 we might switch that around, we'll > need to think hard about compatibility at that point. > In any case, we must definitely keep the old capability for old machine > types. ccw only offers revision 0 (legacy) in 2.4. I plan to introduce revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4 is the first versioned ccw machine). > > > > > > > I think the virtio TC's assumption was that the scsi passthrough was a > > > bad idea, so in QEMU we only keep it around for legacy devices to avoid > > > regressions. > > > > I'm not opposing this :) > > > > > > > > If you disagree and think transitional devices need the SCSI feature, > > > either try to convince pbonzini or rewrite the spec youself > > > to support it in the virtio 1 mode. > > > > This seems to boil down to the different meaning of "transitional" for > > ccw and pci, see the other thread. > > Before the revision is negotiated, ccw won't know whether > it's a legacy driver - is that the difference? I'd say it doesn't know whether the driver intends to use the modern interface. > Fine, but revision is negotiated way before features are > probed so why does it make a practical difference? Legacy drivers (that don't know about the set-revision command) will read features without revision negotiation - we need to offer them the legacy feature set.
On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote: > On Wed, 15 Jul 2015 15:01:01 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: > > > On Wed, 15 Jul 2015 13:59:00 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: > > > > > > Yes, and that's because as written, transitional devices must set > > > > > > ANY_LAYOUT, but that's incompatible with scsi. > > > > > > > > > > Hm, I had a patch before that dynamically allowed different feature > > > > > sets for legacy or modern, not only a subset. Probably won't apply > > > > > anymore, but I'd like to able to do the following: > > > > > > > > > > - driver reads features without negotiating a revision: driver is > > > > > legacy, offer legacy bits > > > > > - driver negotiates revision 0: dito > > > > > - driver negotiates revision >= 1: driver is modern, offer modern bits > > > > > > > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the > > > > > first two cases, and a new qemu could still offer scsi to old guests. > > > > > > > > > > Would it be worth pursuing that idea? > > > > > > > > Frankly, I don't think so: I don't see why it makes sense > > > > to expose more features on the legacy interface than > > > > on the modern one. Imagine updating drivers to fix a bug > > > > and losing some features. How does this make sense? > > > > > > I don't think one should be a strict subset of the other. But I think > > > we don't want to withdraw features from legacy guests on qemu updates > > > either? > > > > Absolutely. For now one has to enable the modern interface > > explicitly. Around 2.5 we might switch that around, we'll > > need to think hard about compatibility at that point. > > In any case, we must definitely keep the old capability for old machine > > types. > > ccw only offers revision 0 (legacy) in 2.4. I plan to introduce > revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4 > is the first versioned ccw machine). I was talking about pci here actually. > > > > > > > > > > I think the virtio TC's assumption was that the scsi passthrough was a > > > > bad idea, so in QEMU we only keep it around for legacy devices to avoid > > > > regressions. > > > > > > I'm not opposing this :) > > > > > > > > > > > If you disagree and think transitional devices need the SCSI feature, > > > > either try to convince pbonzini or rewrite the spec youself > > > > to support it in the virtio 1 mode. > > > > > > This seems to boil down to the different meaning of "transitional" for > > > ccw and pci, see the other thread. > > > > Before the revision is negotiated, ccw won't know whether > > it's a legacy driver - is that the difference? > > I'd say it doesn't know whether the driver intends to use the modern > interface. That's also the case for pci. > > Fine, but revision is negotiated way before features are > > probed so why does it make a practical difference? > > Legacy drivers (that don't know about the set-revision command) will > read features without revision negotiation - we need to offer them the > legacy feature set. Right. So simply do if (revision < 1) return features & 0xffffffff and that will do this, will it not?
On Wed, 15 Jul 2015 16:16:07 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote: > > On Wed, 15 Jul 2015 15:01:01 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: > > > > On Wed, 15 Jul 2015 13:59:00 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: > > > > > > > Yes, and that's because as written, transitional devices must set > > > > > > > ANY_LAYOUT, but that's incompatible with scsi. > > > > > > > > > > > > Hm, I had a patch before that dynamically allowed different feature > > > > > > sets for legacy or modern, not only a subset. Probably won't apply > > > > > > anymore, but I'd like to able to do the following: > > > > > > > > > > > > - driver reads features without negotiating a revision: driver is > > > > > > legacy, offer legacy bits > > > > > > - driver negotiates revision 0: dito > > > > > > - driver negotiates revision >= 1: driver is modern, offer modern bits > > > > > > > > > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the > > > > > > first two cases, and a new qemu could still offer scsi to old guests. > > > > > > > > > > > > Would it be worth pursuing that idea? > > > > > > > > > > Frankly, I don't think so: I don't see why it makes sense > > > > > to expose more features on the legacy interface than > > > > > on the modern one. Imagine updating drivers to fix a bug > > > > > and losing some features. How does this make sense? > > > > > > > > I don't think one should be a strict subset of the other. But I think > > > > we don't want to withdraw features from legacy guests on qemu updates > > > > either? > > > > > > Absolutely. For now one has to enable the modern interface > > > explicitly. Around 2.5 we might switch that around, we'll > > > need to think hard about compatibility at that point. > > > In any case, we must definitely keep the old capability for old machine > > > types. > > > > ccw only offers revision 0 (legacy) in 2.4. I plan to introduce > > revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4 > > is the first versioned ccw machine). > > I was talking about pci here actually. Sure, and these are my plans for ccw ;) > > > > > > > > > > > > > > I think the virtio TC's assumption was that the scsi passthrough was a > > > > > bad idea, so in QEMU we only keep it around for legacy devices to avoid > > > > > regressions. > > > > > > > > I'm not opposing this :) > > > > > > > > > > > > > > If you disagree and think transitional devices need the SCSI feature, > > > > > either try to convince pbonzini or rewrite the spec youself > > > > > to support it in the virtio 1 mode. > > > > > > > > This seems to boil down to the different meaning of "transitional" for > > > > ccw and pci, see the other thread. > > > > > > Before the revision is negotiated, ccw won't know whether > > > it's a legacy driver - is that the difference? > > > > I'd say it doesn't know whether the driver intends to use the modern > > interface. > > That's also the case for pci. But does pci know the moment it first tries to get the device's features? And does pci assume modern as default for transitional devices? > > > > Fine, but revision is negotiated way before features are > > > probed so why does it make a practical difference? > > > > Legacy drivers (that don't know about the set-revision command) will > > read features without revision negotiation - we need to offer them the > > legacy feature set. > > Right. So simply do if (revision < 1) return features & 0xffffffff > and that will do this, will it not? Not for bits that we want to offer for legacy but not for modern.
On Wed, Jul 15, 2015 at 03:40:22PM +0200, Cornelia Huck wrote: > On Wed, 15 Jul 2015 16:16:07 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 15, 2015 at 02:43:51PM +0200, Cornelia Huck wrote: > > > On Wed, 15 Jul 2015 15:01:01 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Jul 15, 2015 at 01:46:38PM +0200, Cornelia Huck wrote: > > > > > On Wed, 15 Jul 2015 13:59:00 +0300 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > On Tue, Jul 14, 2015 at 07:43:44PM +0200, Cornelia Huck wrote: > > > > > > > > Yes, and that's because as written, transitional devices must set > > > > > > > > ANY_LAYOUT, but that's incompatible with scsi. > > > > > > > > > > > > > > Hm, I had a patch before that dynamically allowed different feature > > > > > > > sets for legacy or modern, not only a subset. Probably won't apply > > > > > > > anymore, but I'd like to able to do the following: > > > > > > > > > > > > > > - driver reads features without negotiating a revision: driver is > > > > > > > legacy, offer legacy bits > > > > > > > - driver negotiates revision 0: dito > > > > > > > - driver negotiates revision >= 1: driver is modern, offer modern bits > > > > > > > > > > > > > > That way we could offer SCSI and !ANY_LAYOUT (if scsi is enabled) in the > > > > > > > first two cases, and a new qemu could still offer scsi to old guests. > > > > > > > > > > > > > > Would it be worth pursuing that idea? > > > > > > > > > > > > Frankly, I don't think so: I don't see why it makes sense > > > > > > to expose more features on the legacy interface than > > > > > > on the modern one. Imagine updating drivers to fix a bug > > > > > > and losing some features. How does this make sense? > > > > > > > > > > I don't think one should be a strict subset of the other. But I think > > > > > we don't want to withdraw features from legacy guests on qemu updates > > > > > either? > > > > > > > > Absolutely. For now one has to enable the modern interface > > > > explicitly. Around 2.5 we might switch that around, we'll > > > > need to think hard about compatibility at that point. > > > > In any case, we must definitely keep the old capability for old machine > > > > types. > > > > > > ccw only offers revision 0 (legacy) in 2.4. I plan to introduce > > > revision 1 in 2.5 and force revision to 0 for 2.4 compatibility (as 2.4 > > > is the first versioned ccw machine). > > > > I was talking about pci here actually. > > Sure, and these are my plans for ccw ;) > > > > > > > > > > > > > > > > > > > I think the virtio TC's assumption was that the scsi passthrough was a > > > > > > bad idea, so in QEMU we only keep it around for legacy devices to avoid > > > > > > regressions. > > > > > > > > > > I'm not opposing this :) > > > > > > > > > > > > > > > > > If you disagree and think transitional devices need the SCSI feature, > > > > > > either try to convince pbonzini or rewrite the spec youself > > > > > > to support it in the virtio 1 mode. > > > > > > > > > > This seems to boil down to the different meaning of "transitional" for > > > > > ccw and pci, see the other thread. > > > > > > > > Before the revision is negotiated, ccw won't know whether > > > > it's a legacy driver - is that the difference? > > > > > > I'd say it doesn't know whether the driver intends to use the modern > > > interface. > > > > That's also the case for pci. > > But does pci know the moment it first tries to get the device's > features? And does pci assume modern as default for transitional > devices? I don't think it does. > > > > > > Fine, but revision is negotiated way before features are > > > > probed so why does it make a practical difference? > > > > > > Legacy drivers (that don't know about the set-revision command) will > > > read features without revision negotiation - we need to offer them the > > > legacy feature set. > > > > Right. So simply do if (revision < 1) return features & 0xffffffff > > and that will do this, will it not? > > Not for bits that we want to offer for legacy but not for modern. I don't think this selective offering works at least for scsi. scsi is a backend feature, if you connect a modern device in front the device simply does not work. It therefore makes no sense to attach a transitional device to such a backend.
On Wed, 15 Jul 2015 17:11:57 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > Fine, but revision is negotiated way before features are > > > > > probed so why does it make a practical difference? > > > > > > > > Legacy drivers (that don't know about the set-revision command) will > > > > read features without revision negotiation - we need to offer them the > > > > legacy feature set. > > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff > > > and that will do this, will it not? > > > > Not for bits that we want to offer for legacy but not for modern. > > I don't think this selective offering works at least for scsi. > scsi is a backend feature, if you connect a modern device > in front the device simply does not work. > It therefore makes no sense to attach a transitional device > to such a backend. My point is that we're losing legacy features with that approach, and it would not be possible to offer them to legacy guests with newer qemus (at least with ccw). What about the other way around (i.e. scsi is configured, therefore the device is legacy-only)? We'd only retain the scsi bit if it is actually wanted by the user's configuration. I would need to enforce a max revision of 0 for such a device in ccw, and pci could disable modern for it.
On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: > On Wed, 15 Jul 2015 17:11:57 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > Fine, but revision is negotiated way before features are > > > > > > probed so why does it make a practical difference? > > > > > > > > > > Legacy drivers (that don't know about the set-revision command) will > > > > > read features without revision negotiation - we need to offer them the > > > > > legacy feature set. > > > > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff > > > > and that will do this, will it not? > > > > > > Not for bits that we want to offer for legacy but not for modern. > > > > I don't think this selective offering works at least for scsi. > > scsi is a backend feature, if you connect a modern device > > in front the device simply does not work. > > It therefore makes no sense to attach a transitional device > > to such a backend. > > My point is that we're losing legacy features with that approach, and > it would not be possible to offer them to legacy guests with newer > qemus (at least with ccw). What's wrong with adding a disable-modern flag, like pci has? User can set that to get a legacy device. > What about the other way around (i.e. scsi is configured, therefore the > device is legacy-only)? We'd only retain the scsi bit if it is actually > wanted by the user's configuration. I would need to enforce a max > revision of 0 for such a device in ccw, and pci could disable modern > for it. Will have to think about it. But I think a flag to disable/enable modern is useful in any case, and it seems sufficient.
On Wed, 15 Jul 2015 17:39:18 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: > > On Wed, 15 Jul 2015 17:11:57 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > Fine, but revision is negotiated way before features are > > > > > > > probed so why does it make a practical difference? > > > > > > > > > > > > Legacy drivers (that don't know about the set-revision command) will > > > > > > read features without revision negotiation - we need to offer them the > > > > > > legacy feature set. > > > > > > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff > > > > > and that will do this, will it not? > > > > > > > > Not for bits that we want to offer for legacy but not for modern. > > > > > > I don't think this selective offering works at least for scsi. > > > scsi is a backend feature, if you connect a modern device > > > in front the device simply does not work. > > > It therefore makes no sense to attach a transitional device > > > to such a backend. > > > > My point is that we're losing legacy features with that approach, and > > it would not be possible to offer them to legacy guests with newer > > qemus (at least with ccw). > > What's wrong with adding a disable-modern flag, like pci has? > User can set that to get a legacy device. The whole idea behind the revision-stuff was that we don't need something like disable-modern. If the device is able to figure out on its own if it is to act as a modern or a legacy device, why require user intervention? > > > What about the other way around (i.e. scsi is configured, therefore the > > device is legacy-only)? We'd only retain the scsi bit if it is actually > > wanted by the user's configuration. I would need to enforce a max > > revision of 0 for such a device in ccw, and pci could disable modern > > for it. > > Will have to think about it. > But I think a flag to disable/enable modern is useful in any case, > and it seems sufficient. I don't like the idea of disabling modern or legacy for ccw, where the differences between both are very minor. I also don't think requiring the user to specify a new flag on upgrade just to present the same features as before is a good idea: it is something that is easily missed and may lead to much headscratching.
On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote: > On Wed, 15 Jul 2015 17:39:18 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: > > > On Wed, 15 Jul 2015 17:11:57 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > Fine, but revision is negotiated way before features are > > > > > > > > probed so why does it make a practical difference? > > > > > > > > > > > > > > Legacy drivers (that don't know about the set-revision command) will > > > > > > > read features without revision negotiation - we need to offer them the > > > > > > > legacy feature set. > > > > > > > > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff > > > > > > and that will do this, will it not? > > > > > > > > > > Not for bits that we want to offer for legacy but not for modern. > > > > > > > > I don't think this selective offering works at least for scsi. > > > > scsi is a backend feature, if you connect a modern device > > > > in front the device simply does not work. > > > > It therefore makes no sense to attach a transitional device > > > > to such a backend. > > > > > > My point is that we're losing legacy features with that approach, and > > > it would not be possible to offer them to legacy guests with newer > > > qemus (at least with ccw). > > > > What's wrong with adding a disable-modern flag, like pci has? > > User can set that to get a legacy device. > > The whole idea behind the revision-stuff was that we don't need > something like disable-modern. If the device is able to figure out on > its own if it is to act as a modern or a legacy device, why require > user intervention? It's about compatibility, e.g. being able to test legacy mode in transitional drivers in guests. Consider also bugs, e.g. the fact that linux guests lack WCE in modern mode ATM. > > > > > What about the other way around (i.e. scsi is configured, therefore the > > > device is legacy-only)? We'd only retain the scsi bit if it is actually > > > wanted by the user's configuration. I would need to enforce a max > > > revision of 0 for such a device in ccw, and pci could disable modern > > > for it. > > > > Will have to think about it. > > But I think a flag to disable/enable modern is useful in any case, > > and it seems sufficient. > > I don't like the idea of disabling modern or legacy for ccw, where the > differences between both are very minor. > > I also don't think requiring the user to specify a new flag on upgrade > just to present the same features as before is a good idea: it is > something that is easily missed and may lead to much headscratching. And doing this on a driver upgrade won't? As I said, if you believe this feature has value, argue that we shouldn't drop scsi off in virtio 1.0 then.
On Wed, 15 Jul 2015 21:51:32 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote: > > On Wed, 15 Jul 2015 17:39:18 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: > > > > On Wed, 15 Jul 2015 17:11:57 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > Fine, but revision is negotiated way before features are > > > > > > > > > probed so why does it make a practical difference? > > > > > > > > > > > > > > > > Legacy drivers (that don't know about the set-revision command) will > > > > > > > > read features without revision negotiation - we need to offer them the > > > > > > > > legacy feature set. > > > > > > > > > > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff > > > > > > > and that will do this, will it not? > > > > > > > > > > > > Not for bits that we want to offer for legacy but not for modern. > > > > > > > > > > I don't think this selective offering works at least for scsi. > > > > > scsi is a backend feature, if you connect a modern device > > > > > in front the device simply does not work. > > > > > It therefore makes no sense to attach a transitional device > > > > > to such a backend. > > > > > > > > My point is that we're losing legacy features with that approach, and > > > > it would not be possible to offer them to legacy guests with newer > > > > qemus (at least with ccw). > > > > > > What's wrong with adding a disable-modern flag, like pci has? > > > User can set that to get a legacy device. > > > > The whole idea behind the revision-stuff was that we don't need > > something like disable-modern. If the device is able to figure out on > > its own if it is to act as a modern or a legacy device, why require > > user intervention? > > It's about compatibility, e.g. being able to test legacy mode > in transitional drivers in guests. Let's step back a bit. Why do we need legacy? To support old hosts and old guests. So what we need is a test matrix combining old/modern hosts with old/modern guests. I don't think forcing to an old version is something we should spend time on except during development. > Consider also bugs, e.g. the fact that linux guests lack WCE > in modern mode ATM. If we consider this a bug, shouldn't VERSION_1 be forced off unconditionally until specs and codebase are fixed? > > > > > > > > What about the other way around (i.e. scsi is configured, therefore the > > > > device is legacy-only)? We'd only retain the scsi bit if it is actually > > > > wanted by the user's configuration. I would need to enforce a max > > > > revision of 0 for such a device in ccw, and pci could disable modern > > > > for it. > > > > > > Will have to think about it. > > > But I think a flag to disable/enable modern is useful in any case, > > > and it seems sufficient. > > > > I don't like the idea of disabling modern or legacy for ccw, where the > > differences between both are very minor. > > > > I also don't think requiring the user to specify a new flag on upgrade > > just to present the same features as before is a good idea: it is > > something that is easily missed and may lead to much headscratching. > > And doing this on a driver upgrade won't? As I said, if you believe > this feature has value, argue that we shouldn't drop scsi off in virtio > 1.0 then. I seem to have problems explaining myself :( I don't care about the feature, except for supporting old guests that should continue working as before even if the host updated. And without special configuration on the host, as this is too easily forgotten. What else is legacy good for, other than providing backwards compatibility?
On Thu, Jul 16, 2015 at 02:37:12PM +0200, Cornelia Huck wrote: > On Wed, 15 Jul 2015 21:51:32 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 15, 2015 at 05:38:53PM +0200, Cornelia Huck wrote: > > > On Wed, 15 Jul 2015 17:39:18 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Jul 15, 2015 at 04:30:51PM +0200, Cornelia Huck wrote: > > > > > On Wed, 15 Jul 2015 17:11:57 +0300 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > > > Fine, but revision is negotiated way before features are > > > > > > > > > > probed so why does it make a practical difference? > > > > > > > > > > > > > > > > > > Legacy drivers (that don't know about the set-revision command) will > > > > > > > > > read features without revision negotiation - we need to offer them the > > > > > > > > > legacy feature set. > > > > > > > > > > > > > > > > Right. So simply do if (revision < 1) return features & 0xffffffff > > > > > > > > and that will do this, will it not? > > > > > > > > > > > > > > Not for bits that we want to offer for legacy but not for modern. > > > > > > > > > > > > I don't think this selective offering works at least for scsi. > > > > > > scsi is a backend feature, if you connect a modern device > > > > > > in front the device simply does not work. > > > > > > It therefore makes no sense to attach a transitional device > > > > > > to such a backend. > > > > > > > > > > My point is that we're losing legacy features with that approach, and > > > > > it would not be possible to offer them to legacy guests with newer > > > > > qemus (at least with ccw). > > > > > > > > What's wrong with adding a disable-modern flag, like pci has? > > > > User can set that to get a legacy device. > > > > > > The whole idea behind the revision-stuff was that we don't need > > > something like disable-modern. If the device is able to figure out on > > > its own if it is to act as a modern or a legacy device, why require > > > user intervention? > > > > It's about compatibility, e.g. being able to test legacy mode > > in transitional drivers in guests. > > Let's step back a bit. Why do we need legacy? To support old hosts and > old guests. So what we need is a test matrix combining old/modern hosts > with old/modern guests. I don't think forcing to an old version is > something we should spend time on except during development. > > > Consider also bugs, e.g. the fact that linux guests lack WCE > > in modern mode ATM. > > If we consider this a bug, shouldn't VERSION_1 be forced off > unconditionally until specs and codebase are fixed? I don't see why. We'll never fix all bugs. It causes a performance regression but otherwise things work correctly. modern is off by default for now, this seems sufficient, if we never even ship it as an option we'll never find all bugs. > > > > > > > > > > > What about the other way around (i.e. scsi is configured, therefore the > > > > > device is legacy-only)? We'd only retain the scsi bit if it is actually > > > > > wanted by the user's configuration. I would need to enforce a max > > > > > revision of 0 for such a device in ccw, and pci could disable modern > > > > > for it. > > > > > > > > Will have to think about it. > > > > But I think a flag to disable/enable modern is useful in any case, > > > > and it seems sufficient. > > > > > > I don't like the idea of disabling modern or legacy for ccw, where the > > > differences between both are very minor. > > > > > > I also don't think requiring the user to specify a new flag on upgrade > > > just to present the same features as before is a good idea: it is > > > something that is easily missed and may lead to much headscratching. > > > > And doing this on a driver upgrade won't? As I said, if you believe > > this feature has value, argue that we shouldn't drop scsi off in virtio > > 1.0 then. > > I seem to have problems explaining myself :( > > I don't care about the feature, except for supporting old guests that > should continue working as before even if the host updated. And without > special configuration on the host, as this is too easily forgotten. > What else is legacy good for, other than providing backwards > compatibility? Maybe it's me having trouble explaining. There are two kind of blk devices: - those that allow scsi passthrough Such devices can't work properly through the modern interface. We could add a modern interface but device can not operate through it. - those that don't allow scsi passthrough Such devices can't work properly through the modern interface. We could add a modern interface and we'll get a transitional device. I think for 2.4 it's a good idea to avoid enabling modern interface by default. Therefore, for 2.4 we can keep scsi enabled unless modern is requested by user. I am also fine with just doing if (modern && scsi) exit;
On 16/07/2015 14:47, Michael S. Tsirkin wrote: > I think for 2.4 it's a good idea to avoid enabling modern interface > by default. Therefore, for 2.4 we can keep scsi enabled unless modern > is requested by user. I agree. > I am also fine with just doing > > if (modern && scsi) > exit; exit is evil. Paolo
On Thu, 16 Jul 2015 19:22:21 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 16/07/2015 14:47, Michael S. Tsirkin wrote: > > I think for 2.4 it's a good idea to avoid enabling modern interface > > by default. Therefore, for 2.4 we can keep scsi enabled unless modern > > is requested by user. > > I agree. For 2.4, ccw isn't supporting revisions > 0 anyway, so keeping scsi enabled for 2.4 sounds good to me as well.
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 6aefda4..f30ad25 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -730,7 +730,8 @@ 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); + if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1)) + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); if (s->conf.config_wce) { virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it. Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: qemu-block@nongnu.org Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/block/virtio-blk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)