Re: [PATCH] virtio-ccw: commands on revision-less devices

[Halil Pasic]

On Thu, 18 Feb 2021 14:51:09 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 17 Feb 2021 15:39:36 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 16 Feb 2021 16:54:05 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Tue, 16 Feb 2021 15:19:45 +0100
[..]
> > 
> > Exactly, so the legacy stuff is not normative, and strictly speaking not
> > included in the standard (i.e. standardized).
> > 
> > But then I find usage of keywords like MUST in legacy interface sections
> > misreading. I believe some Oasis guy complained about keyword usage
> > outside of normative sections before.  
> 
> We can certainly discuss whether we want to change something in the
> legacy sections in the spec -- but that's outside the scope of this
> patch.
>

I agree. I just pointed out that those requirements are non-normative
despite the fact that MUST is used.

[..]

> > > If we want to implement a non-transitional device, the implicit
> > > selection of revision 0 goes away, of course. In fact, I'm currently
> > > trying to make legacy support optional for virtio-ccw, so that's why I
> > > had been looking at the revision handling :)    
> > 
> > Do you mean optional like build time configurable in QEMU? I think the
> > legacy support is already optional when it comes to the spec.
> > 
> > Let me explain how do I interpret device compliance with respect to
> > virtio revisions and first command is a non-_REV.
> > 
> > 1) If the first virtio command after the virtio-ccw device is enabled is
> > a non-_REV command, the virtio-ccw device not answering it with a
> > command reject does not preclude the device form being virtio 1.0
> > conform. I.e. this behavior is conform, because does not violate
> > any of the sections linked in "7.3.3 Clause 17: Channel I/O Device
> > Conformance" in general, and thus does not violate "4.3.2.1.1 Device
> > Requirements: Setting the Virtio Revision" in particular. If you disagree,
> > please point me to the corresponding device normative section.
> > 
> > 2) Rejecting the first command which which happens to be a non-_REV
> > however does not preclude virtio (1.0) conformance either. The device
> > is free to do whatever, because in my reading it ain't specified what
> > needs to be done.  
> 
> If it does that, however, it would be a pretty useless transitional
> device, as a legacy driver won't be able to work.
> 

Clear, a transitional device can't do that.

[..]

> > > 
> > > Replace 'and else' with 'a transitional device needs to'?    
> > 
> > Sounds good but, I would also replace 'The virtio standard specifies'
> > with 'The non-normative part of the virtio specification recommends'
> > and probably also replace 'MUST' with 'SHOULD'.
> > 
> > The current patch description sounds like, we are in violation of the
> > spec, and the change is necessary to have a spec conform device, but it
> > is not.  
> 
> I think you're reading too much into this patch description. The point
> of the legacy sections in the spec is to lay down what a device/driver
> needs to do if it also wants to support legacy drivers/devices. If we
> want to present a useful transitional device, we need (regardless of
> any MUST, or SHOULD) to operate in a way that a legacy driver can use
> it.
> 

I agree. And I was never disputing that. What I don't like is that the
patch description reads to me like a conformance fix, but it is not. The
QEMU implementation was VIRTIO spec conform before, and remains VIRTIO
spec conform later. Are you saying that a construct: 'The X
standard specifies that Y must Z, but our implementation does not Z.' does
not imply that our our implementation of Y is not conform to standard X?

In any case, the commit message ain't worth all this discussion. It
won't be the first or the last one that is not very precise. Please do
whatever you like.

Regards,
Halil