set FEATURES_OK as per virtio 1.0 spec
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
include/uapi/linux/virtio_config.h | 2 ++
drivers/virtio/virtio.c | 29 ++++++++++++++++++++++-------
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/virtio_config.h
b/include/uapi/linux/virtio_config.h
index f3fe33a..a6d0cde 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -38,6 +38,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
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index d213567..a3df817 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -160,6 +160,7 @@ static int virtio_dev_probe(struct device *_d)
struct virtio_device *dev = dev_to_virtio(_d);
struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
u64 device_features;
+ unsigned status;
/* We have a driver! */
add_status(dev, VIRTIO_CONFIG_S_DRIVER);
@@ -183,18 +184,32 @@ static int virtio_dev_probe(struct device *_d)
dev->config->finalize_features(dev);
+ if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
+ add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
+ status = dev->config->get_status(dev);
+ if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
+ printk(KERN_ERR "virtio: device refuses features: %x\n",
+ status);
+ err = -ENODEV;
+ goto err;
+ }
+ }
+
err = drv->probe(dev);
if (err)
- add_status(dev, VIRTIO_CONFIG_S_FAILED);
- else {
- add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
- if (drv->scan)
- drv->scan(dev);
+ goto err;
- virtio_config_enable(dev);
- }
+ add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+ if (drv->scan)
+ drv->scan(dev);
+
+ virtio_config_enable(dev);
+ return 0;
+err:
+ add_status(dev, VIRTIO_CONFIG_S_FAILED);
return err;
+
}
static int virtio_dev_remove(struct device *_d)
--
MST
On Wed, 22 Oct 2014 21:44:44 +0300 "Michael S. Tsirkin" <mst at redhat.com> wrote:> set FEATURES_OK as per virtio 1.0 spec > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > include/uapi/linux/virtio_config.h | 2 ++ > drivers/virtio/virtio.c | 29 ++++++++++++++++++++++------- > 2 files changed, 24 insertions(+), 7 deletions(-) >> dev->config->finalize_features(dev); > > + if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) { > + add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); > + status = dev->config->get_status(dev); > + if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { > + printk(KERN_ERR "virtio: device refuses features: %x\n", > + status); > + err = -ENODEV; > + goto err; > + } > + } > +Ugh, I just realize that virtio-ccw has a problem with that mechanism :( Up to now, the driver only propagated status to the device: For virtio-ccw, this was easily implemented via a ccw that transmitted "status" to the device. However, the "read back status" part now actually requires that the driver can get "status" from the device, or has a comparable way to find out that the device won't accept the status it tried to write. I can think of two solutions: (1) Introduce a new ccw that actually reads the device status. (2) Make the WRITE_STATUS ccw fail (with a unit check) if the driver sets FEATURES_OK after it tried to set features the device won't accept. (1) is probably more generic, while (2) is more straightforward to implement. Good thing we actually try to finally implement this, I did not notice this problem during the review :(
On Thu, Oct 23, 2014 at 02:28:08PM +0200, Cornelia Huck wrote:> On Wed, 22 Oct 2014 21:44:44 +0300 > "Michael S. Tsirkin" <mst at redhat.com> wrote: > > > set FEATURES_OK as per virtio 1.0 spec > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > --- > > include/uapi/linux/virtio_config.h | 2 ++ > > drivers/virtio/virtio.c | 29 ++++++++++++++++++++++------- > > 2 files changed, 24 insertions(+), 7 deletions(-) > > > > > dev->config->finalize_features(dev); > > > > + if (virtio_has_feature(dev, VIRTIO_F_VERSION_1)) { > > + add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); > > + status = dev->config->get_status(dev); > > + if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { > > + printk(KERN_ERR "virtio: device refuses features: %x\n", > > + status); > > + err = -ENODEV; > > + goto err; > > + } > > + } > > + > > Ugh, I just realize that virtio-ccw has a problem with that mechanism :( > > Up to now, the driver only propagated status to the device: For > virtio-ccw, this was easily implemented via a ccw that transmitted > "status" to the device. However, the "read back status" part now > actually requires that the driver can get "status" from the device, or > has a comparable way to find out that the device won't accept the > status it tried to write.Ugh, it actually caches the status in the transport :(> I can think of two solutions: > > (1) Introduce a new ccw that actually reads the device status. > (2) Make the WRITE_STATUS ccw fail (with a unit check) if the driver > sets FEATURES_OK after it tried to set features the device won't > accept. > > (1) is probably more generic, while (2) is more straightforward to > implement. > > Good thing we actually try to finally implement this,> I did not notice > this problem during the review :(Well, it's a nuisance, but the spec is out. It seems to me a new command would be a substantive change so we can't do this in errata. Option (2) would require two statements for drivers and devices, but since it's clearly the case for correct drivers/devices that command does not fail, it follows that this is not a substantive change so it can be fixed in an errata. So the new command would have to be optional, please open two issues in the TC: one documenting that driver must check WRITE_STATUS and device can fail WRITE_STATUS, and another for adding READ_STATUS (which will have to wait until the next CS). -- MST