| Message ID | 1418304322-7546-13-git-send-email-cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 11 Dec 2014 14:25:14 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > For virtio-1 devices, the driver must not attempt to set feature bits > after it set FEATURES_OK in the device status. Simply reject it in > that case. > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > hw/virtio/virtio.c | 16 ++++++++++++++-- > include/hw/virtio/virtio.h | 2 ++ > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 57190ba..a3dd67b 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > vmstate_save_state(f, &vmstate_virtio, vdev); > } > > -int virtio_set_features(VirtIODevice *vdev, uint64_t val) > +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val) Maybe avoid the double underscores here? But unfortunately, I also fail to come up with a better suggestion for a name here ... > { > BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); > VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus); > @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) > return bad ? -1 : 0; > } > > +int virtio_set_features(VirtIODevice *vdev, uint64_t val) > +{ > + /* > + * The driver must not attempt to set features after feature negotiation > + * has finished. > + */ > + if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) { > + return -EINVAL; > + } Hmm, according to your patch description, the FEATURES_OK check only applies to virtio-1.0 devices ... so shouldn't there be a check for virtio-1 here? Or did I miss something? > + return __virtio_set_features(vdev, val); > +} Thomas
On Fri, 12 Dec 2014 11:55:38 +0100 Thomas Huth <thuth@linux.vnet.ibm.com> wrote: > On Thu, 11 Dec 2014 14:25:14 +0100 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > For virtio-1 devices, the driver must not attempt to set feature bits > > after it set FEATURES_OK in the device status. Simply reject it in > > that case. > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > --- > > hw/virtio/virtio.c | 16 ++++++++++++++-- > > include/hw/virtio/virtio.h | 2 ++ > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index 57190ba..a3dd67b 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > > vmstate_save_state(f, &vmstate_virtio, vdev); > > } > > > > -int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val) > > Maybe avoid the double underscores here? But unfortunately, I also fail > to come up with a better suggestion for a name here ... virtio_set_features_nocheck()? This function is only called within virtio.c anyway... > > > { > > BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); > > VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus); > > @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > return bad ? -1 : 0; > > } > > > > +int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > +{ > > + /* > > + * The driver must not attempt to set features after feature negotiation > > + * has finished. > > + */ > > + if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) { > > + return -EINVAL; > > + } > > Hmm, according to your patch description, the FEATURES_OK check only > applies to virtio-1.0 devices ... so shouldn't there be a check for > virtio-1 here? Or did I miss something? A device in legacy mode will never have FEATURES_OK set. But it is a bit non-obvious - maybe adding a check for VERSION_1 does not hurt. > > > + return __virtio_set_features(vdev, val); > > +} > > Thomas
On Fri, 12 Dec 2014 12:18:25 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Fri, 12 Dec 2014 11:55:38 +0100 > Thomas Huth <thuth@linux.vnet.ibm.com> wrote: > > > On Thu, 11 Dec 2014 14:25:14 +0100 > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > > > For virtio-1 devices, the driver must not attempt to set feature bits > > > after it set FEATURES_OK in the device status. Simply reject it in > > > that case. > > > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > > --- > > > hw/virtio/virtio.c | 16 ++++++++++++++-- > > > include/hw/virtio/virtio.h | 2 ++ > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > > index 57190ba..a3dd67b 100644 > > > --- a/hw/virtio/virtio.c > > > +++ b/hw/virtio/virtio.c > > > @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > > > vmstate_save_state(f, &vmstate_virtio, vdev); > > > } > > > > > > -int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > > +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val) > > > > Maybe avoid the double underscores here? But unfortunately, I also fail > > to come up with a better suggestion for a name here ... > > virtio_set_features_nocheck()? Sounds ok to me. > This function is only called within virtio.c anyway... Right, so the double underscores should be ok here, too. (I still do not like them very much, but that's just my personal taste in this case) > > > { > > > BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); > > > VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus); > > > @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > > return bad ? -1 : 0; > > > } > > > > > > +int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > > +{ > > > + /* > > > + * The driver must not attempt to set features after feature negotiation > > > + * has finished. > > > + */ > > > + if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) { > > > + return -EINVAL; > > > + } > > > > Hmm, according to your patch description, the FEATURES_OK check only > > applies to virtio-1.0 devices ... so shouldn't there be a check for > > virtio-1 here? Or did I miss something? > > A device in legacy mode will never have FEATURES_OK set. But it is a > bit non-obvious - maybe adding a check for VERSION_1 does not hurt. Ah, ok, right, and if it is a legacy device and has FEATURES_OK set, it is certainly a misbehavior wrt the legacy protocol. So it really should be ok or even good to _not_ check for virtio-1.0 here. So sorry for the confusion, I think now the patch is good as it is: Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
On Fri, Dec 12, 2014 at 12:25:47PM +0100, Thomas Huth wrote: > On Fri, 12 Dec 2014 12:18:25 +0100 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > On Fri, 12 Dec 2014 11:55:38 +0100 > > Thomas Huth <thuth@linux.vnet.ibm.com> wrote: > > > > > On Thu, 11 Dec 2014 14:25:14 +0100 > > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > > > > > For virtio-1 devices, the driver must not attempt to set feature bits > > > > after it set FEATURES_OK in the device status. Simply reject it in > > > > that case. > > > > > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > > > --- > > > > hw/virtio/virtio.c | 16 ++++++++++++++-- > > > > include/hw/virtio/virtio.h | 2 ++ > > > > 2 files changed, 16 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > > > index 57190ba..a3dd67b 100644 > > > > --- a/hw/virtio/virtio.c > > > > +++ b/hw/virtio/virtio.c > > > > @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > > > > vmstate_save_state(f, &vmstate_virtio, vdev); > > > > } > > > > > > > > -int virtio_set_features(VirtIODevice *vdev, uint64_t val) > > > > +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val) > > > > > > Maybe avoid the double underscores here? But unfortunately, I also fail > > > to come up with a better suggestion for a name here ... > > > > virtio_set_features_nocheck()? > > Sounds ok to me. > > > This function is only called within virtio.c anyway... > > Right, so the double underscores should be ok here, too. (I still do > not like them very much, but that's just my personal taste in this case) C99 "7.1.3 Reserved identifiers" says: All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use [by the standard library] You can use a trailing underscore or useless word like "do", e.g. virtio_do_set_features(), for internal functions.
On Thu, Dec 11, 2014 at 02:25:14PM +0100, Cornelia Huck wrote: > For virtio-1 devices, the driver must not attempt to set feature bits > after it set FEATURES_OK in the device status. Simply reject it in > that case. > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 57190ba..a3dd67b 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -978,7 +978,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) vmstate_save_state(f, &vmstate_virtio, vdev); } -int virtio_set_features(VirtIODevice *vdev, uint64_t val) +static int __virtio_set_features(VirtIODevice *vdev, uint64_t val) { BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus); @@ -994,6 +994,18 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val) return bad ? -1 : 0; } +int virtio_set_features(VirtIODevice *vdev, uint64_t val) +{ + /* + * The driver must not attempt to set features after feature negotiation + * has finished. + */ + if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) { + return -EINVAL; + } + return __virtio_set_features(vdev, val); +} + int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) { int i, ret; @@ -1026,7 +1038,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) qemu_get_be32s(f, &features); /* XXX features >= 32 */ - if (virtio_set_features(vdev, features) < 0) { + if (__virtio_set_features(vdev, features) < 0) { supported_features = k->get_features(qbus->parent); error_report("Features 0x%x unsupported. Allowed features: 0x%lx", features, supported_features); diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index b63ced3..a24e403 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -32,6 +32,8 @@ #define VIRTIO_CONFIG_S_DRIVER 2 /* Driver has used its parts of the config, and is happy */ #define VIRTIO_CONFIG_S_DRIVER_OK 4 +/* Driver has finished configuring features */ +#define VIRTIO_CONFIG_S_FEATURES_OK 8 /* We've given up on this device. */ #define VIRTIO_CONFIG_S_FAILED 0x80
For virtio-1 devices, the driver must not attempt to set feature bits after it set FEATURES_OK in the device status. Simply reject it in that case. Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> --- hw/virtio/virtio.c | 16 ++++++++++++++-- include/hw/virtio/virtio.h | 2 ++ 2 files changed, 16 insertions(+), 2 deletions(-)