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

[Michael S. Tsirkin]

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?
            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