On Wed, 17 Feb 2021 15:39:36 +0100 Halil Pasic <pa...@linux.ibm.com> wrote:
> On Tue, 16 Feb 2021 16:54:05 +0100 > Cornelia Huck <coh...@redhat.com> wrote: > > > On Tue, 16 Feb 2021 15:19:45 +0100 > > Halil Pasic <pa...@linux.ibm.com> wrote: > > > > > On Tue, 16 Feb 2021 12:18:30 +0100 > > > Cornelia Huck <coh...@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 <coh...@redhat.com> > > > > > > The change won't hurt so with a toned down commit message: > > > Acked-by: Halil Pasic <pa...@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.
