Re: [Qemu-devel] [PATCH V3 2/3] virtio-blk: fail get_features when both scsi and 1.0 were set

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote:
> On Wed, 22 Jul 2015 13:32:17 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote:
> > > On Wed, 22 Jul 2015 12:21:32 +0300
> > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > 
> > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote:
> > > > > On Wed, 22 Jul 2015 13:59:51 +0800
> > > > > Jason Wang <address@hidden> wrote:
> > > > > 
> > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this 
> > > > > > patch
> > > > > > fail the get_features() when both 1.0 and scsi is set. And also only
> > > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device.
> > > > > > 
> > > > > > Signed-off-by: Jason Wang <address@hidden>
> > > > > > ---
> > > > > >  hw/block/virtio-blk.c | 9 ++++++++-
> > > > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > > index 4c27974..4716c3e 100644
> > > > > > --- a/hw/block/virtio-blk.c
> > > > > > +++ b/hw/block/virtio-blk.c
> > > > > > @@ -731,7 +731,14 @@ 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)) {
> > > > > > +        if (s->conf.scsi) {
> > > > > > +            error_setg(errp, "Virtio 1.0 does not support scsi 
> > > > > > passthrough!");
> > > > > > +            return 0;
> > > > > > +        }
> > > > > > +    } else {
> > > > > > +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > > > > +    }
> > > > > > 
> > > > > >      if (s->conf.config_wce) {
> > > > > >          virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
> > > > > 
> > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep
> > > > > the same bits as before? I'm afraid I don't remember, that thread was
> > > > > long :/
> > > > > 
> > > > > I'm asking because I'd like to depend on that bit to decide whether I
> > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It
> > > > > would be an easy thing to do, and I'd like to avoid mucking around 
> > > > > with
> > > > > device-specific configuration from the transport.
> > > > > 
> > > > > To illustrate what I'm talking about, my current patchset for virtio-1
> > > > > on ccw is here:
> > > > > 
> > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5
> > > > 
> > > > 
> > > > I still think you are over-engineering it.
> > > > 
> > > > Just add a property to disable the modern interface.
> > > > Anyone using scsi passthrough will have to set that,
> > > > if not - above patch will make initialization fail.
> > > 
> > > And I still think requiring user action and not having this work
> > > transparently is a bad idea...
> > 
> > Having what work transparently? SCSI passthrough?
> > Look, either you agree with Paolo or not.
> > Paolo thinks it's a deprecated hack not really worth supporting long term.
> > If you agree, I don't see why is asking for an extra property
> > such a bit deal. If you don't agree - please open a new thread
> > and argue about that.
> 
> I sometimes wonder whether we're arguing about the same thing :(
> 
> Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is
> not. If I upgrade the host, I want the guests to be able to continue
> using scsi without needing to fence virtio-1 off manually.

Paolo's argument is that no one should be using scsi passthrough.

If the feature has users, we should bring it back into virtio 1.

If almost one uses it, then no one will suffer too much from getting
an error message saying "please set disable-modern=on".

And there's no reason to make it behave differently
between ccw and pci.


> > 
> > 
> > > Moreover, I will need a revision-fencing mechanism in any case, when we
> > > introduce further revisions.
> > 
> > Why? Assuming we drop more features in the future?
> 
> Revisions != features. Think new or changed channel commands, for
> example.

You likely can just add these unconditionally.
How is this related to virtio blk at all?

-- 
MST