Fixes a couple of minor compliance issues in new virtio 1.0 code.
Plus, adds a couple of minor cleanups - not bugfixes,
but seem safe enough for 3.19.
Michael S. Tsirkin (6):
virtio: set VIRTIO_CONFIG_S_FEATURES_OK on restore
virtio_config: fix virtio_cread_bytes
virtio_pci_common.h: drop VIRTIO_PCI_NO_LEGACY
virtio_pci: move probe to common file
virtio_pci: add VIRTIO_PCI_NO_LEGACY
virtio: core support for config generation
drivers/virtio/virtio_pci_common.h | 7 +++----
include/linux/virtio_config.h | 29 ++++++++++++++++++++++++++++-
include/uapi/linux/virtio_pci.h | 15 ++++++++++-----
drivers/virtio/virtio.c | 37 +++++++++++++++++++++++--------------
drivers/virtio/virtio_pci_common.c | 34 +++++++++++++++++++++++++++++++++-
drivers/virtio/virtio_pci_legacy.c | 24 ++----------------------
6 files changed, 99 insertions(+), 47 deletions(-)
--
MST
virtio_cread_bytes is implemented incorrectly in case length happens to
be 2,4 or 8 bytes: transports and devices will assume it's an integer
value that has to be converted to LE format.
Let's just do multiple 1-byte reads: this also makes life easier
for transports who only need to implement 1,2,4 and 8 byte reads.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
include/linux/virtio_config.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 7979f85..a61cd37 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -305,7 +305,10 @@ static inline void virtio_cread_bytes(struct virtio_device *vdev,
unsigned int offset,
void *buf, size_t len)
{
- vdev->config->get(vdev, offset, buf, len);
+ int i;
+
+ for (i = 0; i < len; i++)
+ vdev->config->get(vdev, offset + i, buf + i, 1);
}
static inline void virtio_cwrite8(struct virtio_device *vdev,
--
MST
virtio 1.0 devices require that drivers set VIRTIO_CONFIG_S_FEATURES_OK
after finalizing features.
virtio core missed doing this on restore, fix it up.
Signed-off-by: Michael S. Tsirkin <[email protected]>
Reviewed-by: Cornelia Huck <[email protected]>
---
drivers/virtio/virtio.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index f226658..b9f70df 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -162,6 +162,27 @@ static void virtio_config_enable(struct virtio_device *dev)
spin_unlock_irq(&dev->config_lock);
}
+static int virtio_finalize_features(struct virtio_device *dev)
+{
+ int ret = dev->config->finalize_features(dev);
+ unsigned status;
+
+ if (ret)
+ return ret;
+
+ if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
+ return 0;
+
+ add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK);
+ status = dev->config->get_status(dev);
+ if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
+ dev_err(&dev->dev, "virtio: device refuses features: %x\n",
+ status);
+ return -ENODEV;
+ }
+ return 0;
+}
+
static int virtio_dev_probe(struct device *_d)
{
int err, i;
@@ -170,7 +191,6 @@ static int virtio_dev_probe(struct device *_d)
u64 device_features;
u64 driver_features;
u64 driver_features_legacy;
- unsigned status;
/* We have a driver! */
add_status(dev, VIRTIO_CONFIG_S_DRIVER);
@@ -208,21 +228,10 @@ static int virtio_dev_probe(struct device *_d)
if (device_features & (1ULL << i))
__virtio_set_bit(dev, i);
- err = dev->config->finalize_features(dev);
+ err = virtio_finalize_features(dev);
if (err)
goto err;
- 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)) {
- dev_err(_d, "virtio: device refuses features: %x\n",
- status);
- err = -ENODEV;
- goto err;
- }
- }
-
err = drv->probe(dev);
if (err)
goto err;
@@ -372,7 +381,7 @@ int virtio_device_restore(struct virtio_device *dev)
/* We have a driver! */
add_status(dev, VIRTIO_CONFIG_S_DRIVER);
- ret = dev->config->finalize_features(dev);
+ ret = virtio_finalize_features(dev);
if (ret)
goto err;
--
MST
It turns out this make everything easier.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/virtio/virtio_pci_common.h | 6 +++---
drivers/virtio/virtio_pci_common.c | 34 +++++++++++++++++++++++++++++++++-
drivers/virtio/virtio_pci_legacy.c | 24 ++----------------------
3 files changed, 38 insertions(+), 26 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 38d99ad..adddb64 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -128,8 +128,8 @@ const char *vp_bus_name(struct virtio_device *vdev);
int vp_set_vq_affinity(struct virtqueue *vq, int cpu);
void virtio_pci_release_dev(struct device *);
-#ifdef CONFIG_PM_SLEEP
-extern const struct dev_pm_ops virtio_pci_pm_ops;
-#endif
+int virtio_pci_legacy_probe(struct pci_dev *pci_dev,
+ const struct pci_device_id *id);
+void virtio_pci_legacy_remove(struct pci_dev *pci_dev);
#endif
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 953057d..59d3685 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -458,7 +458,39 @@ static int virtio_pci_restore(struct device *dev)
return virtio_device_restore(&vp_dev->vdev);
}
-const struct dev_pm_ops virtio_pci_pm_ops = {
+static const struct dev_pm_ops virtio_pci_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(virtio_pci_freeze, virtio_pci_restore)
};
#endif
+
+
+/* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
+static const struct pci_device_id virtio_pci_id_table[] = {
+ { PCI_DEVICE(0x1af4, PCI_ANY_ID) },
+ { 0 }
+};
+
+MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
+
+static int virtio_pci_probe(struct pci_dev *pci_dev,
+ const struct pci_device_id *id)
+{
+ return virtio_pci_legacy_probe(pci_dev, id);
+}
+
+static void virtio_pci_remove(struct pci_dev *pci_dev)
+{
+ virtio_pci_legacy_remove(pci_dev);
+}
+
+static struct pci_driver virtio_pci_driver = {
+ .name = "virtio-pci",
+ .id_table = virtio_pci_id_table,
+ .probe = virtio_pci_probe,
+ .remove = virtio_pci_remove,
+#ifdef CONFIG_PM_SLEEP
+ .driver.pm = &virtio_pci_pm_ops,
+#endif
+};
+
+module_pci_driver(virtio_pci_driver);
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 2588252..6c76f0f 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -19,14 +19,6 @@
#include "virtio_pci_common.h"
-/* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
-static const struct pci_device_id virtio_pci_id_table[] = {
- { PCI_DEVICE(0x1af4, PCI_ANY_ID) },
- { 0 }
-};
-
-MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
-
/* virtio config->get_features() implementation */
static u64 vp_get_features(struct virtio_device *vdev)
{
@@ -220,7 +212,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
};
/* the PCI probing function */
-static int virtio_pci_probe(struct pci_dev *pci_dev,
+int virtio_pci_legacy_probe(struct pci_dev *pci_dev,
const struct pci_device_id *id)
{
struct virtio_pci_device *vp_dev;
@@ -300,7 +292,7 @@ out:
return err;
}
-static void virtio_pci_remove(struct pci_dev *pci_dev)
+void virtio_pci_legacy_remove(struct pci_dev *pci_dev)
{
struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
@@ -312,15 +304,3 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
pci_disable_device(pci_dev);
kfree(vp_dev);
}
-
-static struct pci_driver virtio_pci_driver = {
- .name = "virtio-pci",
- .id_table = virtio_pci_id_table,
- .probe = virtio_pci_probe,
- .remove = virtio_pci_remove,
-#ifdef CONFIG_PM_SLEEP
- .driver.pm = &virtio_pci_pm_ops,
-#endif
-};
-
-module_pci_driver(virtio_pci_driver);
--
MST
Add macro to disable all legacy register defines.
Helpful to make sure legacy macros don't leak
through into modern code.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
include/uapi/linux/virtio_pci.h | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index e5ec1ca..35b552c 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -41,6 +41,8 @@
#include <linux/virtio_config.h>
+#ifndef VIRTIO_PCI_NO_LEGACY
+
/* A 32-bit r/o bitmask of the features supported by the host */
#define VIRTIO_PCI_HOST_FEATURES 0
@@ -67,16 +69,11 @@
* a read-and-acknowledge. */
#define VIRTIO_PCI_ISR 19
-/* The bit of the ISR which indicates a device configuration change. */
-#define VIRTIO_PCI_ISR_CONFIG 0x2
-
/* MSI-X registers: only enabled if MSI-X is enabled. */
/* A 16-bit vector for configuration changes. */
#define VIRTIO_MSI_CONFIG_VECTOR 20
/* A 16-bit vector for selected queue notifications. */
#define VIRTIO_MSI_QUEUE_VECTOR 22
-/* Vector value used to disable MSI for queue */
-#define VIRTIO_MSI_NO_VECTOR 0xffff
/* The remaining space is defined by each driver as the per-driver
* configuration space */
@@ -94,4 +91,12 @@
/* The alignment to use between consumer and producer parts of vring.
* x86 pagesize again. */
#define VIRTIO_PCI_VRING_ALIGN 4096
+
+#endif /* VIRTIO_PCI_NO_LEGACY */
+
+/* The bit of the ISR which indicates a device configuration change. */
+#define VIRTIO_PCI_ISR_CONFIG 0x2
+/* Vector value used to disable MSI for queue */
+#define VIRTIO_MSI_NO_VECTOR 0xffff
+
#endif
--
MST
virtio 1.0 spec says:
Drivers MUST NOT assume reads from fields greater than 32 bits wide are
atomic, nor are reads from multiple fields: drivers SHOULD read device
configuration space fields like so:
u32 before, after;
do {
before = get_config_generation(device);
// read config entry/entries.
after = get_config_generation(device);
} while (after != before);
Do exactly this, for transports that support it.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
include/linux/virtio_config.h | 32 ++++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index a61cd37..ca3ed78 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -19,6 +19,9 @@
* offset: the offset of the configuration field
* buf: the buffer to read the field value from.
* len: the length of the buffer
+ * @generation: config generation counter
+ * vdev: the virtio_device
+ * Returns the config generation counter
* @get_status: read the status byte
* vdev: the virtio_device
* Returns the status byte
@@ -60,6 +63,7 @@ struct virtio_config_ops {
void *buf, unsigned len);
void (*set)(struct virtio_device *vdev, unsigned offset,
const void *buf, unsigned len);
+ u32 (*generation)(struct virtio_device *vdev);
u8 (*get_status)(struct virtio_device *vdev);
void (*set_status)(struct virtio_device *vdev, u8 status);
void (*reset)(struct virtio_device *vdev);
@@ -301,14 +305,33 @@ static inline u8 virtio_cread8(struct virtio_device *vdev, unsigned int offset)
return ret;
}
+/* Read @count fields, @bytes each. */
+static inline void __virtio_cread_many(struct virtio_device *vdev,
+ unsigned int offset,
+ void *buf, size_t count, size_t bytes)
+{
+ u32 old, gen = vdev->config->generation ?
+ vdev->config->generation(vdev) : 0;
+ int i;
+
+ do {
+ old = gen;
+
+ for (i = 0; i < count; i++)
+ vdev->config->get(vdev, offset + bytes * i,
+ buf + i * bytes, bytes);
+
+ gen = vdev->config->generation ?
+ vdev->config->generation(vdev) : 0;
+ } while (gen != old);
+}
+
+
static inline void virtio_cread_bytes(struct virtio_device *vdev,
unsigned int offset,
void *buf, size_t len)
{
- int i;
-
- for (i = 0; i < len; i++)
- vdev->config->get(vdev, offset + i, buf + i, 1);
+ __virtio_cread_many(vdev, offset, buf, len, 1);
}
static inline void virtio_cwrite8(struct virtio_device *vdev,
@@ -352,6 +375,7 @@ static inline u64 virtio_cread64(struct virtio_device *vdev,
{
u64 ret;
vdev->config->get(vdev, offset, &ret, sizeof(ret));
+ __virtio_cread_many(vdev, offset, &ret, 1, sizeof(ret));
return virtio64_to_cpu(vdev, (__force __virtio64)ret);
}
--
MST
Legacy drivers use virtio_pci_common.h too, we should not
define VIRTIO_PCI_NO_LEGACY there.
Signed-off-by: Michael S. Tsirkin <[email protected]>
---
drivers/virtio/virtio_pci_common.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index d840dad..38d99ad 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -27,7 +27,6 @@
#include <linux/virtio.h>
#include <linux/virtio_config.h>
#include <linux/virtio_ring.h>
-#define VIRTIO_PCI_NO_LEGACY
#include <linux/virtio_pci.h>
#include <linux/highmem.h>
#include <linux/spinlock.h>
--
MST
"Michael S. Tsirkin" <[email protected]> writes:
> Fixes a couple of minor compliance issues in new virtio 1.0 code.
> Plus, adds a couple of minor cleanups - not bugfixes,
> but seem safe enough for 3.19.
OK, I've applied these.
Thanks,
Rusty.
> Michael S. Tsirkin (6):
> virtio: set VIRTIO_CONFIG_S_FEATURES_OK on restore
> virtio_config: fix virtio_cread_bytes
> virtio_pci_common.h: drop VIRTIO_PCI_NO_LEGACY
> virtio_pci: move probe to common file
> virtio_pci: add VIRTIO_PCI_NO_LEGACY
> virtio: core support for config generation
>
> drivers/virtio/virtio_pci_common.h | 7 +++----
> include/linux/virtio_config.h | 29 ++++++++++++++++++++++++++++-
> include/uapi/linux/virtio_pci.h | 15 ++++++++++-----
> drivers/virtio/virtio.c | 37 +++++++++++++++++++++++--------------
> drivers/virtio/virtio_pci_common.c | 34 +++++++++++++++++++++++++++++++++-
> drivers/virtio/virtio_pci_legacy.c | 24 ++----------------------
> 6 files changed, 99 insertions(+), 47 deletions(-)
>
> --
> MST
"Michael S. Tsirkin" <[email protected]> writes:
> virtio 1.0 spec says:
>
> Drivers MUST NOT assume reads from fields greater than 32 bits wide are
> atomic, nor are reads from multiple fields: drivers SHOULD read device
> configuration space fields like so:
> u32 before, after;
> do {
> before = get_config_generation(device);
> // read config entry/entries.
> after = get_config_generation(device);
> } while (after != before);
>
> Do exactly this, for transports that support it.
> static inline void virtio_cwrite8(struct virtio_device *vdev,
> @@ -352,6 +375,7 @@ static inline u64 virtio_cread64(struct virtio_device *vdev,
> {
> u64 ret;
> vdev->config->get(vdev, offset, &ret, sizeof(ret));
> + __virtio_cread_many(vdev, offset, &ret, 1, sizeof(ret));
> return virtio64_to_cpu(vdev, (__force __virtio64)ret);
> }
The "vdev->config->get(vdev, offset, &ret, sizeof(ret));" should
be deleted. Harmless if not, though.
Cheers,
Rusty.