| Message ID | 20210216111830.1087847-1-cohuck@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-ccw: commands on revision-less devices | expand |
On 16/02/2021 12.18, Cornelia Huck 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. > > 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> > --- > hw/s390x/virtio-ccw.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 4582e94ae7dc..06c06056814b 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > ccw.cmd_code); > check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC)); > > - if (dev->force_revision_1 && dev->revision < 0 && > - ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { > - /* > - * virtio-1 drivers must start with negotiating to a revision >= 1, > - * so post a command reject for all other commands > - */ > - return -ENOSYS; > + if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { > + if (dev->force_revision_1) { > + /* > + * virtio-1 drivers must start with negotiating to a revision >= 1, > + * so post a command reject for all other commands > + */ > + return -ENOSYS; > + } else { > + /* > + * If the driver issues any command that is not SET_VIRTIO_REV, > + * we'll have to operate the device in legacy mode. > + */ > + dev->revision = 0; > + } > } > > /* Look at the command. */ > Reviewed-by: Thomas Huth <thuth@redhat.com>
On Tue, Feb 16, 2021 at 12:18:30PM +0100, Cornelia Huck 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. > > 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> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Feel free to merge. Curious how was this discovered. > --- > hw/s390x/virtio-ccw.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 4582e94ae7dc..06c06056814b 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > ccw.cmd_code); > check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC)); > > - if (dev->force_revision_1 && dev->revision < 0 && > - ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { > - /* > - * virtio-1 drivers must start with negotiating to a revision >= 1, > - * so post a command reject for all other commands > - */ > - return -ENOSYS; > + if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { > + if (dev->force_revision_1) { > + /* > + * virtio-1 drivers must start with negotiating to a revision >= 1, > + * so post a command reject for all other commands > + */ > + return -ENOSYS; > + } else { > + /* > + * If the driver issues any command that is not SET_VIRTIO_REV, > + * we'll have to operate the device in legacy mode. > + */ > + dev->revision = 0; > + } > } > > /* Look at the command. */ > -- > 2.26.2
On Tue, 16 Feb 2021 06:40:41 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Feb 16, 2021 at 12:18:30PM +0100, Cornelia Huck 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. > > > > 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> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Feel free to merge. > > Curious how was this discovered. Code reading :) I have not seen any errors in the wild.
On Tue, Feb 16, 2021 at 12:51:50PM +0100, Cornelia Huck wrote: > On Tue, 16 Feb 2021 06:40:41 -0500 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Feb 16, 2021 at 12:18:30PM +0100, Cornelia Huck 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. > > > > > > 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> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > Feel free to merge. > > > > Curious how was this discovered. > > Code reading :) > > I have not seen any errors in the wild. Worth including in the commit log.
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? 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. > > 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> > --- > hw/s390x/virtio-ccw.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 4582e94ae7dc..06c06056814b 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > ccw.cmd_code); > check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC)); > > - if (dev->force_revision_1 && dev->revision < 0 && > - ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { > - /* > - * virtio-1 drivers must start with negotiating to a revision >= 1, > - * so post a command reject for all other commands > - */ > - return -ENOSYS; > + if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { > + if (dev->force_revision_1) { > + /* > + * virtio-1 drivers must start with negotiating to a revision >= 1, > + * so post a command reject for all other commands > + */ > + return -ENOSYS; > + } else { > + /* > + * If the driver issues any command that is not SET_VIRTIO_REV, > + * we'll have to operate the device in legacy mode. > + */ > + dev->revision = 0; > + } > } > > /* Look at the command. */
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. > > 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 :) > > > > > > 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'? > > > --- > > hw/s390x/virtio-ccw.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > > index 4582e94ae7dc..06c06056814b 100644 > > --- a/hw/s390x/virtio-ccw.c > > +++ b/hw/s390x/virtio-ccw.c > > @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) > > ccw.cmd_code); > > check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC)); > > > > - if (dev->force_revision_1 && dev->revision < 0 && > > - ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { > > - /* > > - * virtio-1 drivers must start with negotiating to a revision >= 1, > > - * so post a command reject for all other commands > > - */ > > - return -ENOSYS; > > + if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { > > + if (dev->force_revision_1) { > > + /* > > + * virtio-1 drivers must start with negotiating to a revision >= 1, > > + * so post a command reject for all other commands > > + */ > > + return -ENOSYS; > > + } else { > > + /* > > + * If the driver issues any command that is not SET_VIRTIO_REV, > > + * we'll have to operate the device in legacy mode. > > + */ > > + dev->revision = 0; > > + } > > } > > > > /* Look at the command. */ >
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
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.
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
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. > > 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> > --- > hw/s390x/virtio-ccw.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) I now have: Author: Cornelia Huck <cohuck@redhat.com> Date: Tue Feb 16 12:18:30 2021 +0100 virtio-ccw: commands on revision-less devices The virtio standard specifies that any non-transitional device must reject commands prior to revision setting (which we do). Devices that are transitional need to assume revision 0 (legacy) if the driver sends a non-revision-setting command first in order to support legacy drivers. We neglected to do the latter. 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. Found by code inspection, not observed in the wild. Signed-off-by: Cornelia Huck <cohuck@redhat.com> Reviewed-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Acked-by: Halil Pasic <pasic@linux.ibm.com> Message-Id: <20210216111830.1087847-1-cohuck@redhat.com> Any objections?
On Fri, 19 Feb 2021 12:21:36 +0100 Cornelia Huck <cohuck@redhat.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. > > > > 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> > > --- > > hw/s390x/virtio-ccw.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > I now have: > > Author: Cornelia Huck <cohuck@redhat.com> > Date: Tue Feb 16 12:18:30 2021 +0100 > > virtio-ccw: commands on revision-less devices > > The virtio standard specifies that any non-transitional device must > reject commands prior to revision setting (which we do). Devices > that are transitional need to assume revision 0 (legacy) if the > driver sends a non-revision-setting command first in order to > support legacy drivers. We neglected to do the latter. > > 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. > > Found by code inspection, not observed in the wild. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Acked-by: Halil Pasic <pasic@linux.ibm.com> > Message-Id: <20210216111830.1087847-1-cohuck@redhat.com> > > Any objections? Queued now with this description.
On Fri, Feb 19, 2021 at 12:21:36PM +0100, Cornelia Huck 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. > > > > 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> > > --- > > hw/s390x/virtio-ccw.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > I now have: > > Author: Cornelia Huck <cohuck@redhat.com> > Date: Tue Feb 16 12:18:30 2021 +0100 > > virtio-ccw: commands on revision-less devices > > The virtio standard specifies that any non-transitional device must > reject commands prior to revision setting (which we do). Devices > that are transitional need to assume revision 0 (legacy) if the > driver sends a non-revision-setting command first in order to > support legacy drivers. We neglected to do the latter. > > 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. > > Found by code inspection, not observed in the wild. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Acked-by: Halil Pasic <pasic@linux.ibm.com> > Message-Id: <20210216111830.1087847-1-cohuck@redhat.com> > > Any objections? Acked-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 4582e94ae7dc..06c06056814b 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw) ccw.cmd_code); check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC)); - if (dev->force_revision_1 && dev->revision < 0 && - ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { - /* - * virtio-1 drivers must start with negotiating to a revision >= 1, - * so post a command reject for all other commands - */ - return -ENOSYS; + if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) { + if (dev->force_revision_1) { + /* + * virtio-1 drivers must start with negotiating to a revision >= 1, + * so post a command reject for all other commands + */ + return -ENOSYS; + } else { + /* + * If the driver issues any command that is not SET_VIRTIO_REV, + * we'll have to operate the device in legacy mode. + */ + dev->revision = 0; + } } /* Look at the command. */
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. 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> --- hw/s390x/virtio-ccw.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)