On Mon, Jul 13, 2015 at 01:51:56PM +0200, Cornelia Huck wrote: > On Mon, 13 Jul 2015 11:56:51 +0200 > Kevin Wolf <kw...@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 <stefa...@redhat.com> > > > >> Cc: Kevin Wolf <kw...@redhat.com> > > > >> Cc: qemu-bl...@nongnu.org > > > >> Signed-off-by: Jason Wang <jasow...@redhat.com> > > > > Interesting, I noticed we have a field scsi - see > > > > commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2 > > > > Author: Paolo Bonzini <pbonz...@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. -- MST