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

[Halil Pasic]

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.

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

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.

Regards,
Halil