From 5ef827526fc01820a7a80827802e9fad3f34f937 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 May 2008 21:50:43 -0500 Subject: virtio: ignore corrupted virtqueues rather than spinning. A corrupt virtqueue (caused by the other end screwing up) can have strange results such as a driver spinning: just bail when we try to get a buffer from a known-broken queue. Signed-off-by: Rusty Russell --- drivers/virtio/virtio_ring.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index c2fa5c63081..937a49d6772 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -184,6 +184,11 @@ static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len) START_USE(vq); + if (unlikely(vq->broken)) { + END_USE(vq); + return NULL; + } + if (!more_used(vq)) { pr_debug("No more buffers in queue\n"); END_USE(vq); -- cgit v1.2.3 From 597d56e4b51fc3385e097e52d6e92bf596ff21ec Mon Sep 17 00:00:00 2001 From: Harvey Harrison Date: Mon, 31 Mar 2008 17:53:55 -0700 Subject: virtio: fix sparse return void-valued expression warnings drivers/virtio/virtio_pci.c:148:2: warning: returning void-valued expression drivers/virtio/virtio_pci.c:155:2: warning: returning void-valued expression Signed-off-by: Harvey Harrison Signed-off-by: Rusty Russell --- drivers/virtio/virtio_pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index c0df924766a..de102a614e9 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -145,14 +145,14 @@ static void vp_set_status(struct virtio_device *vdev, u8 status) struct virtio_pci_device *vp_dev = to_vp_device(vdev); /* We should never be setting status to 0. */ BUG_ON(status == 0); - return iowrite8(status, vp_dev->ioaddr + VIRTIO_PCI_STATUS); + iowrite8(status, vp_dev->ioaddr + VIRTIO_PCI_STATUS); } static void vp_reset(struct virtio_device *vdev) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); /* 0 status means a reset. */ - return iowrite8(0, vp_dev->ioaddr + VIRTIO_PCI_STATUS); + iowrite8(0, vp_dev->ioaddr + VIRTIO_PCI_STATUS); } /* the notify function used when creating a virt queue */ -- cgit v1.2.3 From 72e61eb40b55dd57031ec5971e810649f82b0259 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 May 2008 21:50:49 -0500 Subject: virtio: change config to guest endian. A recent proposed feature addition to the virtio block driver revealed some flaws in the API, in particular how easy it is to break big endian machines. The virtio config space was originally chosen to be little-endian, because we thought the config might be part of the PCI config space for virtio_pci. It's actually a separate mmio region, so that argument holds little water; as only x86 is currently using the virtio mechanism, we can change this (but must do so now, before the impending s390 merge). API changes: - __virtio_config_val() just becomes a striaght vdev->config_get() call. Signed-off-by: Rusty Russell --- drivers/virtio/virtio_balloon.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 0b3efc31ee6..fef88d84cef 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -155,9 +155,9 @@ static void virtballoon_changed(struct virtio_device *vdev) static inline s64 towards_target(struct virtio_balloon *vb) { u32 v; - __virtio_config_val(vb->vdev, - offsetof(struct virtio_balloon_config, num_pages), - &v); + vb->vdev->config->get(vb->vdev, + offsetof(struct virtio_balloon_config, num_pages), + &v, sizeof(v)); return v - vb->num_pages; } -- cgit v1.2.3 From c45a6816c19dee67b8f725e6646d428901a6dc24 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 May 2008 21:50:50 -0500 Subject: virtio: explicit advertisement of driver features A recent proposed feature addition to the virtio block driver revealed some flaws in the API: in particular, we assume that feature negotiation is complete once a driver's probe function returns. There is nothing in the API to require this, however, and even I didn't notice when it was violated. So instead, we require the driver to specify what features it supports in a table, we can then move the feature negotiation into the virtio core. The intersection of device and driver features are presented in a new 'features' bitmap in the struct virtio_device. Note that this highlights the difference between Linux unsigned-long bitmaps where each unsigned long is in native endian, and a straight-forward little-endian array of bytes. Drivers can still remove feature bits in their probe routine if they really have to. API changes: - dev->config->feature() no longer gets and acks a feature. - drivers should advertise their features in the 'feature_table' field - use virtio_has_feature() for extra sanity when checking feature bits Signed-off-by: Rusty Russell --- drivers/virtio/virtio.c | 38 ++++++++++++++++++++++++++++++++++++-- drivers/virtio/virtio_balloon.c | 6 +++++- drivers/virtio/virtio_pci.c | 30 +++++++++++++++--------------- 3 files changed, 56 insertions(+), 18 deletions(-) (limited to 'drivers/virtio') diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index b535483bc55..13866789b35 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -80,19 +80,51 @@ static void add_status(struct virtio_device *dev, unsigned status) dev->config->set_status(dev, dev->config->get_status(dev) | status); } +void virtio_check_driver_offered_feature(const struct virtio_device *vdev, + unsigned int fbit) +{ + unsigned int i; + struct virtio_driver *drv = container_of(vdev->dev.driver, + struct virtio_driver, driver); + + for (i = 0; i < drv->feature_table_size; i++) + if (drv->feature_table[i] == fbit) + return; + BUG(); +} +EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature); + static int virtio_dev_probe(struct device *_d) { - int err; + int err, i; struct virtio_device *dev = container_of(_d,struct virtio_device,dev); struct virtio_driver *drv = container_of(dev->dev.driver, struct virtio_driver, driver); + u32 device_features; + /* We have a driver! */ add_status(dev, VIRTIO_CONFIG_S_DRIVER); + + /* Figure out what features the device supports. */ + device_features = dev->config->get_features(dev); + + /* Features supported by both device and driver into dev->features. */ + memset(dev->features, 0, sizeof(dev->features)); + for (i = 0; i < drv->feature_table_size; i++) { + unsigned int f = drv->feature_table[i]; + BUG_ON(f >= 32); + if (device_features & (1 << f)) + set_bit(f, dev->features); + } + err = drv->probe(dev); if (err) add_status(dev, VIRTIO_CONFIG_S_FAILED); - else + else { add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); + /* They should never have set feature bits beyond 32 */ + dev->config->set_features(dev, dev->features[0]); + } return err; } @@ -114,6 +146,8 @@ static int virtio_dev_remove(struct device *_d) int register_virtio_driver(struct virtio_driver *driver) { + /* Catch this early. */ + BUG_ON(driver->feature_table_size && !driver->feature_table); driver->driver.bus = &virtio_bus; driver->driver.probe = virtio_dev_probe; driver->driver.remove = virtio_dev_remove; diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index fef88d84cef..bfef604160d 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -227,7 +227,7 @@ static int virtballoon_probe(struct virtio_device *vdev) } vb->tell_host_first - = vdev->config->feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST); + = virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST); return 0; @@ -259,7 +259,11 @@ static void virtballoon_remove(struct virtio_device *vdev) kfree(vb); } +static unsigned int features[] = { VIRTIO_BALLOON_F_MUST_TELL_HOST }; + static struct virtio_driver virtio_balloon = { + .feature_table = features, + .feature_table_size = ARRAY_SIZE(features), .driver.name = KBUILD_MODNAME, .driver.owner = THIS_MODULE, .id_table = id_table, diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c index de102a614e9..27e9fc9117c 100644 --- a/drivers/virtio/virtio_pci.c +++ b/drivers/virtio/virtio_pci.c @@ -87,23 +87,22 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) return container_of(vdev, struct virtio_pci_device, vdev); } -/* virtio config->feature() implementation */ -static bool vp_feature(struct virtio_device *vdev, unsigned bit) +/* virtio config->get_features() implementation */ +static u32 vp_get_features(struct virtio_device *vdev) +{ + struct virtio_pci_device *vp_dev = to_vp_device(vdev); + + /* When someone needs more than 32 feature bits, we'll need to + * steal a bit to indicate that the rest are somewhere else. */ + return ioread32(vp_dev->ioaddr + VIRTIO_PCI_HOST_FEATURES); +} + +/* virtio config->set_features() implementation */ +static void vp_set_features(struct virtio_device *vdev, u32 features) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); - u32 mask; - - /* Since this function is supposed to have the side effect of - * enabling a queried feature, we simulate that by doing a read - * from the host feature bitmask and then writing to the guest - * feature bitmask */ - mask = ioread32(vp_dev->ioaddr + VIRTIO_PCI_HOST_FEATURES); - if (mask & (1 << bit)) { - mask |= (1 << bit); - iowrite32(mask, vp_dev->ioaddr + VIRTIO_PCI_GUEST_FEATURES); - } - return !!(mask & (1 << bit)); + iowrite32(features, vp_dev->ioaddr + VIRTIO_PCI_GUEST_FEATURES); } /* virtio config->get() implementation */ @@ -293,7 +292,6 @@ static void vp_del_vq(struct virtqueue *vq) } static struct virtio_config_ops virtio_pci_config_ops = { - .feature = vp_feature, .get = vp_get, .set = vp_set, .get_status = vp_get_status, @@ -301,6 +299,8 @@ static struct virtio_config_ops virtio_pci_config_ops = { .reset = vp_reset, .find_vq = vp_find_vq, .del_vq = vp_del_vq, + .get_features = vp_get_features, + .set_features = vp_set_features, }; /* the PCI probing function */ -- cgit v1.2.3