Re: [Qemu-block] [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Cornelia Huck]

On Mon, 13 Jul 2015 18:35:53 +0300
"Michael S. Tsirkin" <address@hidden> 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" <address@hidden> 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" <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > > > > > >> Cc: Kevin Wolf <address@hidden>
> > > > > > > > >> Cc: address@hidden
> > > > > > > > >> Signed-off-by: Jason Wang <address@hidden>
> > > > > > > > > Interesting, I noticed we have a field scsi - see
> > > > > > > > >       commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > > > > > > > >       Author: Paolo Bonzini <address@hidden>
> > > > > > > > >       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?