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
Michael S. Tsirkin
2014-Dec-15 21:23 UTC
[PATCH 1/6] virtio: set VIRTIO_CONFIG_S_FEATURES_OK on restore
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 <mst at redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>
---
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
Michael S. Tsirkin
2014-Dec-15 21:23 UTC
[PATCH 2/6] virtio_config: fix virtio_cread_bytes
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 <mst at redhat.com>
---
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
Michael S. Tsirkin
2014-Dec-15 21:23 UTC
[PATCH 3/6] virtio_pci_common.h: drop VIRTIO_PCI_NO_LEGACY
Legacy drivers use virtio_pci_common.h too, we should not define VIRTIO_PCI_NO_LEGACY there. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- 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
2014-Dec-15 21:23 UTC
[PATCH 4/6] virtio_pci: move probe to common file
It turns out this make everything easier.
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
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 <mst at redhat.com> --- 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
Michael S. Tsirkin
2014-Dec-15 21:23 UTC
[PATCH 6/6] virtio: core support for config generation
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 <mst at redhat.com>
---
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
"Michael S. Tsirkin" <mst at redhat.com> 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
Rusty Russell
2014-Dec-19 03:01 UTC
[PATCH 6/6] virtio: core support for config generation
"Michael S. Tsirkin" <mst at redhat.com> 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.