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

[Cornelia Huck]

On Wed, 15 Jul 2015 17:39:18 +0300
"Michael S. Tsirkin" <address@hidden> 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" <address@hidden> 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.