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

[Cornelia Huck]

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
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > On Tue, 16 Feb 2021 12:18:30 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >     
> > > > The virtio standard specifies that any non-transitional device must
> > > > reject commands prior to revision setting (which we do) and else
> > > > assume revision 0 (legacy) if the driver sends a non-revision-setting
> > > > command first. We neglected to do the latter.      
> > > 
> > > Huh, I my opinion, it ain't very clear what is specified by the virtio
> > > standard (which starts with version 1.0) for the described situation.
> > > 
> > > The corresponding device normative section (4.3.2.1.1 Device
> > > Requirements: Setting the Virtio Revision) says that: "A device MUST
> > > treat the revision as unset from the time the associated subchannel has
> > > been enabled until a revision has been successfully set by the driver.
> > > This implies that revisions are not persistent across disabling and
> > > enabling of the associated subchannel.". It doesn't say anything more
> > > about the situation where the first command is not SET_VIRTIO_REV.
> > > 
> > > The section "4.3.2.1.3 Legacy Interfaces: A Note on Setting the Virtio
> > > Revision" which is to my best knowledge not normative, as none of the
> > > legacy-interface stuff is normative, but a mere advice on how to deal
> > > with legacy then says: "A legacy driver will not issue the
> > > CCW_CMD_SET_VIRTIO_REV prior to issuing other virtio-ccw specific
> > > channel commands." ... "A transitional device MUST assume
> > > in this case that the driver is a legacy driver and continue as if the
> > > driver selected revision 0. This implies that the device MUST reject any
> > > command not valid for revision 0, including a subsequent
> > > CCW_CMD_SET_VIRTIO_REV."
> > > 
> > > Do we agree that the legacy interface sections in general, and 4.3.2.1.3
> > > in particular is non-normative?    
> > 
> > IMHO, normative and non-normative are not something that applies to the
> > legacy sections. The legacy sections are supposed to give guidance on
> > how to write transitional devices/drivers that can deal with legacy
> > implementations. If you want to write something that strictly only
> > adheres to normative statements, you have to write a non-transitional
> > device/driver. Legacy support was never an official standard, so
> > 'normative' is meaningless in that context.  
> 
> 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.

> 
> >   
> > > 
> > > In my opinion the normative 'must threat as unset until set by driver'
> > > and 'if first cmd is not _REV, must continue as if the driver selected
> > > revision 0' is in a slight collision.    
> > 
> > I don't think there's a collision. If we want to accommodate legacy
> > drivers, we have to deal with their lack of revision handling, and
> > therefore treat non-_REV commands as implicitly selecting revision 0.
> > 
> > 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.

> 
> 3) It is OK-ish, that the device is free to do anything there, because
> a virtio 1.0 conform driver will never put the device in this situation. 
> 
> 4) The following, non-normative section recommends what a transitional,
> and a non-transitional device should do. There fore I think it would have
> been wiser to use should instead of MUST in that section.
> 
> >   
> > > 
> > >     
> > > > 
> > > > Fortunately, nearly everything worked as intended anyway; the only
> > > > problem was not properly rejecting revision setting after some other
> > > > command had been issued. Easy to fix by setting revision to 0 if
> > > > we see a non-revision command on a legacy-capable revision-less
> > > > device.
> > > > 
> > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>      
> > > 
> > > The change won't hurt so with a toned down commit message:
> > > Acked-by: Halil Pasic <pasic@linux.ibm.com>    
> > 
> > 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.