[PATCH] xen/xenbus: silence GCC warning
Compiling xenbus_xs.o triggers this GCC warning:
drivers/xen/xenbus/xenbus_xs.c:628:13: warning: function declaration isn’t
a prototype [-Wstrict-prototypes]
Add the obvious and trivial fix.
While we're touching this function add some equally obvious and trivial
whitespace fixes.
Signed-off-by: Paul Bolle
---
0) Triggered by compiling v3.7-rc1 using (basically) Fedora 17's current
config. Compile tested only.
1) Obligatory reference: https://lwn.net/Articles/487493/ .
drivers/xen/xenbus/xenbus_xs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index 48220e1..7a2b0da 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -619,13 +619,14 @@ static struct xenbus_watch *find_watch(const char *token)
return NULL;
}
+
/*
* Certain older XenBus toolstack cannot handle reading values that are
* not populated. Some Xen 3.4 installation are incapable of doing this
* so if we are running on anything older than 4 do not attempt to read
* control/platform-feature-xs_reset_watches.
*/
-static bool xen_strict_xenbus_quirk()
+static bool xen_strict_xenbus_quirk(void)
{
uint32_t eax, ebx, ecx, edx, base;
@@ -635,8 +636,8 @@ static bool xen_strict_xenbus_quirk()
if ((eax >> 16) < 4)
return true;
return false;
-
}
+
static void xs_reset_watches(void)
{
int err, supported = 0;
--
1.7.11.7
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.
On Wed, Oct 17, 2012 at 12:00:48AM +1030, Rusty Russell wrote:
> They're generic concepts, so hoist them. This also avoids accessor
> functions.
>
> This goes even further than Jason Wang's 17bb6d4088 patch
> ("virtio-ring: move queue_index to vring_virtqueue") which moved the
> queue_index from the specific transport.
>
> Signed-off-by: Rusty Russell
ACK series (with the corrected num_free description)
> ---
> drivers/virtio/virtio_mmio.c |4 ++--
> drivers/virtio/virtio_pci.c |6 ++
> drivers/virtio/virtio_ring.c | 34 +++---
> include/linux/virtio.h |6 --
> 4 files changed, 19 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 6b1b7e1..5a0e1d3 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -225,7 +225,7 @@ static void vm_notify(struct virtqueue *vq)
>
> /* We write the queue's selector into the notification register to
>* signal the other end */
> - writel(virtqueue_get_queue_index(vq), vm_dev->base +
> VIRTIO_MMIO_QUEUE_NOTIFY);
> + writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> }
>
> /* Notify all virtqueues on an interrupt. */
> @@ -266,7 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
> struct virtio_mmio_vq_info *info = vq->priv;
> unsigned long flags, size;
> - unsigned int index = virtqueue_get_queue_index(vq);
> + unsigned int index = vq->index;
>
> spin_lock_irqsave(&vm_dev->lock, flags);
> list_del(&info->node);
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index b59237c..e3ecc94 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -203,8 +203,7 @@ static void vp_notify(struct virtqueue *vq)
>
> /* we write the queue's selector into the notification register to
>* signal the other end */
> - iowrite16(virtqueue_get_queue_index(vq),
> - vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> + iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> }
>
> /* Handle a configuration change: Tell driver if it wants to know. */
> @@ -479,8 +478,7 @@ static void vp_del_vq(struct virtqueue *vq)
> list_del(&info->node);
> spin_unlock_irqrestore(&vp_dev->lock, flags);
>
> - iowrite16(virtqueue_get_queue_index(vq),
> - vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
> + iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
>
> if (vp_dev->msix_enabled) {
> iowrite16(VIRTIO_MSI_NO_VECTOR,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index e639584..335dcec 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -93,8 +93,6 @@ struct vring_virtqueue
> /* Host publishes avail event idx */
> bool event;
>
> - /* Number of free buffers */
> - unsigned int num_free;
> /* Head of free buffer list. */
> unsigned int free_head;
> /* Number we've added since last sync. */
> @@ -106,9 +104,6 @@ struct vring_virtqueue
> /* How to notify other side. FIXME: commonalize hcalls! */
> void (*notify)(struct virtqueue *vq);
>
> - /* Index of the queue */
> - int queue_index;
> -
> #ifdef DEBUG
> /* They're supposed to lock for us. */
> unsigned int in_use;
> @@ -160,7 +155,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> desc[i-1].next = 0;
>
> /* We're about to use a buffer */
> - vq->num_free--;
> + vq->vq.num_free--;
>
> /* Use a single buffer which doesn't continue */
> head = vq->free_head;
> @@ -174,13 +169,6 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> return head;
> }
>
> -int virtqueue_get_queue_index(struct virtqueue *_vq)
> -{
> - struct vring_virtqueue *vq = to_vvq(_vq);
> - return vq->queue_index;
> -}
> -EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
> -
> /**
> * virtqueue_add_buf - expose buffer to other end
> * @vq: the struct virtqueue we're talking about.
> @@ -228,7 +216,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>
> /* If the host supports indirect descriptor tables, and we have multiple
>* buffers, then go indirect. FIXME: tune this threshold */
> - if (vq->indirect && (out + in) > 1 && vq->num_free) {
> + if (vq->indirect && (out + in) > 1 && vq->vq.num_free) {
> head = vring_add_indirect(vq, sg, out, in, gfp);
> if (likely(head >= 0))
> goto add_head;
> @@ -237,9 +225,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
> BUG_ON(out + in > vq->vring.num);
> BUG_ON(out + in == 0);
>
> - if (vq->num_free < out + in) {
> + if (vq->vq.num_free < out + in) {
> pr_debug("Can't add buf len %i -
Re: [PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.
"Michael S. Tsirkin" writes: > On Wed, Oct 17, 2012 at 12:00:48AM +1030, Rusty Russell wrote: >> * @priv: a pointer for the virtqueue implementation to use. >> + * @index: the zero-based ordinal number for this queue. >> + * @num_free: number of buffers we expect to be able to fit. > > Only it's not exactly buffers: a single buffer can use > multiple s/g entries, right? > Maybe clarify as 'linear buffers'? Yes, it needs clarification, but I don't think I can do so tersely. Here's what I ended up with: * @num_free: number of elements we expect to be able to fit. * * A note on @num_free: with indirect buffers, each buffer needs one * element in the queue, otherwise a buffer will need one element per * sg element. Thanks, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Using PCI config space to indicate config location
"Michael S. Tsirkin" writes: > On Tue, Oct 16, 2012 at 11:45:41PM +1030, Rusty Russell wrote: >> "Michael S. Tsirkin" writes: >> I was assuming the implementation would keep two complete copies of the >> config space: writes go to the scratch version, which gets copied to the >> master version upon latch write. > > Yes but config space has some host modifiable registers too. > So host needs to be careful to avoid overwriting these. > > If accesses have side effects that of course breaks too ... Yes. > >> But I do wonder if we should just skip this for now, since we don't >> have any immediate need. >> >> Cheers, >> Rusty. > > MAC setting from guest needs this right now, no? Ah, I missed that in my table: Driver Config Device changesDriver writes... after init? net YY NN block YY YY console YY NN rng NN NN balloon YY YY scsiYN YN 9p YN NN First line should be: net YY YN So we could add a new cvq command (eg. #define VIRTIO_NET_CTRL_MAC_SET 1) and a VIRTIO_NET_F_CVQ_MAC_SET feature. Or go ahead with the latching scheme (which doesn't really help other busses). Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 5/5] virtio: make virtqueue_add_buf() returning 0 on success, not capacity.
Note: I also have minor cleanups to virtio drivers to change (ret >= 0) to (ret == 0) where it would otherwise be confusing for new readers, but I didn't clutter the list with them. See my virtio-wip tree for the gory details. Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.
On Wed, Oct 17, 2012 at 12:00:48AM +1030, Rusty Russell wrote:
> They're generic concepts, so hoist them. This also avoids accessor
> functions.
>
> This goes even further than Jason Wang's 17bb6d4088 patch
> ("virtio-ring: move queue_index to vring_virtqueue") which moved the
> queue_index from the specific transport.
>
> Signed-off-by: Rusty Russell
> ---
> drivers/virtio/virtio_mmio.c |4 ++--
> drivers/virtio/virtio_pci.c |6 ++
> drivers/virtio/virtio_ring.c | 34 +++---
> include/linux/virtio.h |6 --
> 4 files changed, 19 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 6b1b7e1..5a0e1d3 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -225,7 +225,7 @@ static void vm_notify(struct virtqueue *vq)
>
> /* We write the queue's selector into the notification register to
>* signal the other end */
> - writel(virtqueue_get_queue_index(vq), vm_dev->base +
> VIRTIO_MMIO_QUEUE_NOTIFY);
> + writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> }
>
> /* Notify all virtqueues on an interrupt. */
> @@ -266,7 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
> struct virtio_mmio_vq_info *info = vq->priv;
> unsigned long flags, size;
> - unsigned int index = virtqueue_get_queue_index(vq);
> + unsigned int index = vq->index;
>
> spin_lock_irqsave(&vm_dev->lock, flags);
> list_del(&info->node);
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index b59237c..e3ecc94 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -203,8 +203,7 @@ static void vp_notify(struct virtqueue *vq)
>
> /* we write the queue's selector into the notification register to
>* signal the other end */
> - iowrite16(virtqueue_get_queue_index(vq),
> - vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> + iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
> }
>
> /* Handle a configuration change: Tell driver if it wants to know. */
> @@ -479,8 +478,7 @@ static void vp_del_vq(struct virtqueue *vq)
> list_del(&info->node);
> spin_unlock_irqrestore(&vp_dev->lock, flags);
>
> - iowrite16(virtqueue_get_queue_index(vq),
> - vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
> + iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
>
> if (vp_dev->msix_enabled) {
> iowrite16(VIRTIO_MSI_NO_VECTOR,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index e639584..335dcec 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -93,8 +93,6 @@ struct vring_virtqueue
> /* Host publishes avail event idx */
> bool event;
>
> - /* Number of free buffers */
> - unsigned int num_free;
> /* Head of free buffer list. */
> unsigned int free_head;
> /* Number we've added since last sync. */
> @@ -106,9 +104,6 @@ struct vring_virtqueue
> /* How to notify other side. FIXME: commonalize hcalls! */
> void (*notify)(struct virtqueue *vq);
>
> - /* Index of the queue */
> - int queue_index;
> -
> #ifdef DEBUG
> /* They're supposed to lock for us. */
> unsigned int in_use;
> @@ -160,7 +155,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> desc[i-1].next = 0;
>
> /* We're about to use a buffer */
> - vq->num_free--;
> + vq->vq.num_free--;
>
> /* Use a single buffer which doesn't continue */
> head = vq->free_head;
> @@ -174,13 +169,6 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
> return head;
> }
>
> -int virtqueue_get_queue_index(struct virtqueue *_vq)
> -{
> - struct vring_virtqueue *vq = to_vvq(_vq);
> - return vq->queue_index;
> -}
> -EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
> -
> /**
> * virtqueue_add_buf - expose buffer to other end
> * @vq: the struct virtqueue we're talking about.
> @@ -228,7 +216,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>
> /* If the host supports indirect descriptor tables, and we have multiple
>* buffers, then go indirect. FIXME: tune this threshold */
> - if (vq->indirect && (out + in) > 1 && vq->num_free) {
> + if (vq->indirect && (out + in) > 1 && vq->vq.num_free) {
> head = vring_add_indirect(vq, sg, out, in, gfp);
> if (likely(head >= 0))
> goto add_head;
> @@ -237,9 +225,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
> BUG_ON(out + in > vq->vring.num);
> BUG_ON(out + in == 0);
>
> - if (vq->num_free < out + in) {
> + if (vq->vq.num_free < out + in) {
> pr_debug("Can't add buf len %i - avail = %i\n",
> - out + in, vq->
Re: [PATCH 4/5] virtio_net: don't rely on virtqueue_add_buf() returning capacity.
On Wed, Oct 17, 2012 at 12:00:51AM +1030, Rusty Russell wrote:
> Now we can easily use vq->num_free to determine if there are descriptors
> left in the queue, we're about to change virtqueue_add_buf() to return 0
> on success. The virtio_net driver is the only one which actually uses
> the return value, so change that.
>
> Signed-off-by: Rusty Russell
> ---
> drivers/net/virtio_net.c | 33 +
> 1 file changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6c094c8..7c7f5a9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -471,10 +471,11 @@ static bool try_fill_recv(struct virtnet_info *vi,
> gfp_t gfp)
> err = add_recvbuf_small(vi, gfp);
>
> oom = err == -ENOMEM;
> - if (err < 0)
> + if (err)
> break;
> ++vi->num;
> - } while (err > 0);
> + } while (vi->rvq->num_free);
> +
> if (unlikely(vi->num > vi->max))
> vi->max = vi->num;
> virtqueue_kick(vi->rvq);
I like this, it always bothered me that RX intentionally
triggers a failure path in virtqueue_add_buf.
> @@ -625,27 +626,20 @@ static int xmit_skb(struct virtnet_info *vi, struct
> sk_buff *skb)
> static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> - int capacity;
> + int err;
>
> /* Free up any pending old buffers before queueing new ones. */
> free_old_xmit_skbs(vi);
>
> /* Try to transmit */
> - capacity = xmit_skb(vi, skb);
> + err = xmit_skb(vi, skb);
>
> - /* This can happen with OOM and indirect buffers. */
> - if (unlikely(capacity < 0)) {
> - if (likely(capacity == -ENOMEM)) {
> - if (net_ratelimit())
> - dev_warn(&dev->dev,
> - "TX queue failure: out of memory\n");
> - } else {
> - dev->stats.tx_fifo_errors++;
> - if (net_ratelimit())
> - dev_warn(&dev->dev,
> - "Unexpected TX queue failure: %d\n",
> - capacity);
> - }
> + /* This should not happen! */
> + if (unlikely(err < 0)) {
> + dev->stats.tx_fifo_errors++;
> + if (net_ratelimit())
> + dev_warn(&dev->dev,
> + "Unexpected TX queue failure: %d\n", err);
> dev->stats.tx_dropped++;
> kfree_skb(skb);
> return NETDEV_TX_OK;
> @@ -658,13 +652,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb,
> struct net_device *dev)
>
> /* Apparently nice girls don't return TX_BUSY; stop the queue
>* before it gets out of hand. Naturally, this wastes entries. */
> - if (capacity < 2+MAX_SKB_FRAGS) {
> + if (vi->svq->num_free < 2+MAX_SKB_FRAGS) {
> netif_stop_queue(dev);
> if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
> /* More just got used, free them then recheck. */
> free_old_xmit_skbs(vi);
> - capacity = vi->svq->num_free;
> - if (capacity >= 2+MAX_SKB_FRAGS) {
> + if (vi->svq->num_free >= 2+MAX_SKB_FRAGS) {
> netif_start_queue(dev);
> virtqueue_disable_cb(vi->svq);
> }
> --
> 1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 3/5] virtio-net: correct capacity math on ring full
From: "Michael S. Tsirkin"
Capacity math on ring full is wrong: we are
looking at num_sg but that might be optimistic
because of indirect buffer use.
The implementation also penalizes fast path
with extra memory accesses for the benefit of
ring full condition handling which is slow path.
It's easy to query ring capacity so let's do just that.
This change also makes it easier to move vnet header
for tx around as follow-up patch does.
Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
---
drivers/net/virtio_net.c |9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 20880fb..6c094c8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -556,10 +556,10 @@ again:
return received;
}
-static unsigned int free_old_xmit_skbs(struct virtnet_info *vi)
+static void free_old_xmit_skbs(struct virtnet_info *vi)
{
struct sk_buff *skb;
- unsigned int len, tot_sgs = 0;
+ unsigned int len;
struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
while ((skb = virtqueue_get_buf(vi->svq, &len)) != NULL) {
@@ -570,10 +570,8 @@ static unsigned int free_old_xmit_skbs(struct virtnet_info
*vi)
stats->tx_packets++;
u64_stats_update_end(&stats->tx_syncp);
- tot_sgs += skb_vnet_hdr(skb)->num_sg;
dev_kfree_skb_any(skb);
}
- return tot_sgs;
}
static int xmit_skb(struct virtnet_info *vi, struct sk_buff *skb)
@@ -664,7 +662,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct
net_device *dev)
netif_stop_queue(dev);
if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
/* More just got used, free them then recheck. */
- capacity += free_old_xmit_skbs(vi);
+ free_old_xmit_skbs(vi);
+ capacity = vi->svq->num_free;
if (capacity >= 2+MAX_SKB_FRAGS) {
netif_start_queue(dev);
virtqueue_disable_cb(vi->svq);
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 5/5] virtio: make virtqueue_add_buf() returning 0 on success, not capacity.
Now noone relies on this behavior, we simplify virtqueue_add_buf() so it
return 0 or -errno.
Signed-off-by: Rusty Russell
---
drivers/virtio/virtio_ring.c |7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 335dcec..5fde312 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -181,10 +181,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
- * Returns remaining capacity of queue or a negative error
- * (ie. ENOSPC). Note that it only really makes sense to treat all
- * positive return values as "available": indirect buffers mean that
- * we can put an entire sg[] array inside a single queue entry.
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM).
*/
int virtqueue_add_buf(struct virtqueue *_vq,
struct scatterlist sg[],
@@ -284,7 +281,7 @@ add_head:
pr_debug("Added buffer head %i to %p\n", head, vq);
END_USE(vq);
- return vq->vq.num_free;
+ return 0;
}
EXPORT_SYMBOL_GPL(virtqueue_add_buf);
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/5] virtio: move queue_index and num_free fields into core struct virtqueue.
They're generic concepts, so hoist them. This also avoids accessor
functions.
This goes even further than Jason Wang's 17bb6d4088 patch
("virtio-ring: move queue_index to vring_virtqueue") which moved the
queue_index from the specific transport.
Signed-off-by: Rusty Russell
---
drivers/virtio/virtio_mmio.c |4 ++--
drivers/virtio/virtio_pci.c |6 ++
drivers/virtio/virtio_ring.c | 34 +++---
include/linux/virtio.h |6 --
4 files changed, 19 insertions(+), 31 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 6b1b7e1..5a0e1d3 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -225,7 +225,7 @@ static void vm_notify(struct virtqueue *vq)
/* We write the queue's selector into the notification register to
* signal the other end */
- writel(virtqueue_get_queue_index(vq), vm_dev->base +
VIRTIO_MMIO_QUEUE_NOTIFY);
+ writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
}
/* Notify all virtqueues on an interrupt. */
@@ -266,7 +266,7 @@ static void vm_del_vq(struct virtqueue *vq)
struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
struct virtio_mmio_vq_info *info = vq->priv;
unsigned long flags, size;
- unsigned int index = virtqueue_get_queue_index(vq);
+ unsigned int index = vq->index;
spin_lock_irqsave(&vm_dev->lock, flags);
list_del(&info->node);
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index b59237c..e3ecc94 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -203,8 +203,7 @@ static void vp_notify(struct virtqueue *vq)
/* we write the queue's selector into the notification register to
* signal the other end */
- iowrite16(virtqueue_get_queue_index(vq),
- vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+ iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
}
/* Handle a configuration change: Tell driver if it wants to know. */
@@ -479,8 +478,7 @@ static void vp_del_vq(struct virtqueue *vq)
list_del(&info->node);
spin_unlock_irqrestore(&vp_dev->lock, flags);
- iowrite16(virtqueue_get_queue_index(vq),
- vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
+ iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL);
if (vp_dev->msix_enabled) {
iowrite16(VIRTIO_MSI_NO_VECTOR,
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e639584..335dcec 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -93,8 +93,6 @@ struct vring_virtqueue
/* Host publishes avail event idx */
bool event;
- /* Number of free buffers */
- unsigned int num_free;
/* Head of free buffer list. */
unsigned int free_head;
/* Number we've added since last sync. */
@@ -106,9 +104,6 @@ struct vring_virtqueue
/* How to notify other side. FIXME: commonalize hcalls! */
void (*notify)(struct virtqueue *vq);
- /* Index of the queue */
- int queue_index;
-
#ifdef DEBUG
/* They're supposed to lock for us. */
unsigned int in_use;
@@ -160,7 +155,7 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
desc[i-1].next = 0;
/* We're about to use a buffer */
- vq->num_free--;
+ vq->vq.num_free--;
/* Use a single buffer which doesn't continue */
head = vq->free_head;
@@ -174,13 +169,6 @@ static int vring_add_indirect(struct vring_virtqueue *vq,
return head;
}
-int virtqueue_get_queue_index(struct virtqueue *_vq)
-{
- struct vring_virtqueue *vq = to_vvq(_vq);
- return vq->queue_index;
-}
-EXPORT_SYMBOL_GPL(virtqueue_get_queue_index);
-
/**
* virtqueue_add_buf - expose buffer to other end
* @vq: the struct virtqueue we're talking about.
@@ -228,7 +216,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
/* If the host supports indirect descriptor tables, and we have multiple
* buffers, then go indirect. FIXME: tune this threshold */
- if (vq->indirect && (out + in) > 1 && vq->num_free) {
+ if (vq->indirect && (out + in) > 1 && vq->vq.num_free) {
head = vring_add_indirect(vq, sg, out, in, gfp);
if (likely(head >= 0))
goto add_head;
@@ -237,9 +225,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
BUG_ON(out + in > vq->vring.num);
BUG_ON(out + in == 0);
- if (vq->num_free < out + in) {
+ if (vq->vq.num_free < out + in) {
pr_debug("Can't add buf len %i - avail = %i\n",
-out + in, vq->num_free);
+out + in, vq->vq.num_free);
/* FIXME: for historical reasons, we force a notify here if
* there are outgoing parts to the buffer
[PATCH 4/5] virtio_net: don't rely on virtqueue_add_buf() returning capacity.
Now we can easily use vq->num_free to determine if there are descriptors
left in the queue, we're about to change virtqueue_add_buf() to return 0
on success. The virtio_net driver is the only one which actually uses
the return value, so change that.
Signed-off-by: Rusty Russell
---
drivers/net/virtio_net.c | 33 +
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6c094c8..7c7f5a9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -471,10 +471,11 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t
gfp)
err = add_recvbuf_small(vi, gfp);
oom = err == -ENOMEM;
- if (err < 0)
+ if (err)
break;
++vi->num;
- } while (err > 0);
+ } while (vi->rvq->num_free);
+
if (unlikely(vi->num > vi->max))
vi->max = vi->num;
virtqueue_kick(vi->rvq);
@@ -625,27 +626,20 @@ static int xmit_skb(struct virtnet_info *vi, struct
sk_buff *skb)
static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct virtnet_info *vi = netdev_priv(dev);
- int capacity;
+ int err;
/* Free up any pending old buffers before queueing new ones. */
free_old_xmit_skbs(vi);
/* Try to transmit */
- capacity = xmit_skb(vi, skb);
+ err = xmit_skb(vi, skb);
- /* This can happen with OOM and indirect buffers. */
- if (unlikely(capacity < 0)) {
- if (likely(capacity == -ENOMEM)) {
- if (net_ratelimit())
- dev_warn(&dev->dev,
-"TX queue failure: out of memory\n");
- } else {
- dev->stats.tx_fifo_errors++;
- if (net_ratelimit())
- dev_warn(&dev->dev,
-"Unexpected TX queue failure: %d\n",
-capacity);
- }
+ /* This should not happen! */
+ if (unlikely(err < 0)) {
+ dev->stats.tx_fifo_errors++;
+ if (net_ratelimit())
+ dev_warn(&dev->dev,
+"Unexpected TX queue failure: %d\n", err);
dev->stats.tx_dropped++;
kfree_skb(skb);
return NETDEV_TX_OK;
@@ -658,13 +652,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct
net_device *dev)
/* Apparently nice girls don't return TX_BUSY; stop the queue
* before it gets out of hand. Naturally, this wastes entries. */
- if (capacity < 2+MAX_SKB_FRAGS) {
+ if (vi->svq->num_free < 2+MAX_SKB_FRAGS) {
netif_stop_queue(dev);
if (unlikely(!virtqueue_enable_cb_delayed(vi->svq))) {
/* More just got used, free them then recheck. */
free_old_xmit_skbs(vi);
- capacity = vi->svq->num_free;
- if (capacity >= 2+MAX_SKB_FRAGS) {
+ if (vi->svq->num_free >= 2+MAX_SKB_FRAGS) {
netif_start_queue(dev);
virtqueue_disable_cb(vi->svq);
}
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/5] virtio-net: remove unused skb_vnet_hdr->num_sg field
From: "Michael S. Tsirkin"
[Split from "correct capacity math on ring full" -- Rusty]
Signed-off-by: Michael S. Tsirkin
Signed-off-by: Rusty Russell
---
drivers/net/virtio_net.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index cbf8b06..20880fb 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -95,7 +95,6 @@ struct skb_vnet_hdr {
struct virtio_net_hdr hdr;
struct virtio_net_hdr_mrg_rxbuf mhdr;
};
- unsigned int num_sg;
};
struct padded_vnet_hdr {
@@ -581,6 +580,7 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff
*skb)
{
struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
const unsigned char *dest = ((struct ethhdr *)skb->data)->h_dest;
+ unsigned num_sg;
pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest);
@@ -619,8 +619,8 @@ static int xmit_skb(struct virtnet_info *vi, struct sk_buff
*skb)
else
sg_set_buf(vi->tx_sg, &hdr->hdr, sizeof hdr->hdr);
- hdr->num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
- return virtqueue_add_buf(vi->svq, vi->tx_sg, hdr->num_sg,
+ num_sg = skb_to_sgvec(skb, vi->tx_sg + 1, 0, skb->len) + 1;
+ return virtqueue_add_buf(vi->svq, vi->tx_sg, num_sg,
0, skb, GFP_ATOMIC);
}
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Using PCI config space to indicate config location
On Tue, Oct 16, 2012 at 11:45:41PM +1030, Rusty Russell wrote: > "Michael S. Tsirkin" writes: > > On Fri, Oct 12, 2012 at 08:21:50PM +1030, Rusty Russell wrote: > >> "Michael S. Tsirkin" writes: > >> > On Fri, Oct 12, 2012 at 08:59:36AM +1030, Rusty Russell wrote: > >> >> >> For writes, the standard seems to be a commit latch. We could abuse > >> >> >> the > >> >> >> generation count for this: the driver writes to it to commit config > >> >> >> changes. > >> >> > > >> >> > I think this will work. There are a couple of things that bother me: > >> >> > > >> >> > This assumes read accesses have no side effects, and these are > >> >> > sometimes handy. > >> >> > Also the semantics for write aren't very clear to me. > >> >> > I guess device must buffer data until generation count write? > >> >> > This assumes the device has a buffer to store writes, > >> >> > and it must track each byte written. I kind of dislike this > >> >> > tracking of accessed bytes. Also, device would need to resolve > >> >> > conflicts > >> >> > if any in some device specific way. > >> >> > >> >> It should be trivial to implement: you keep a scratch copy of the config > >> >> space, and copy it to the master copy when they hit the latch. > >> >> > >> >> Implementation of this will show whether I've missed anything here, I > >> >> think. > >> > > >> > What I refer to: what happens if driver does: > >> > - write offset 1 > >> > - write offset 3 > >> > - hit commit latch > >> > >> - nothing > >> - nothing > >> - effect of offset 1 and offset 3 writes > > > > OK so this means that you also need to track which bytes where written > > in order to know to skip byte 2. > > This is what I referred to. If instead we ask driver to specify > > offset/length explicitly device only needs to remember that. > > I was assuming the implementation would keep two complete copies of the > config space: writes go to the scratch version, which gets copied to the > master version upon latch write. Yes but config space has some host modifiable registers too. So host needs to be careful to avoid overwriting these. If accesses have side effects that of course breaks too ... > But I do wonder if we should just skip this for now, since we don't > have any immediate need. > > Cheers, > Rusty. MAC setting from guest needs this right now, no? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Using PCI config space to indicate config location
"Michael S. Tsirkin" writes: > On Fri, Oct 12, 2012 at 08:21:50PM +1030, Rusty Russell wrote: >> "Michael S. Tsirkin" writes: >> > On Fri, Oct 12, 2012 at 08:59:36AM +1030, Rusty Russell wrote: >> >> >> For writes, the standard seems to be a commit latch. We could abuse >> >> >> the >> >> >> generation count for this: the driver writes to it to commit config >> >> >> changes. >> >> > >> >> > I think this will work. There are a couple of things that bother me: >> >> > >> >> > This assumes read accesses have no side effects, and these are >> >> > sometimes handy. >> >> > Also the semantics for write aren't very clear to me. >> >> > I guess device must buffer data until generation count write? >> >> > This assumes the device has a buffer to store writes, >> >> > and it must track each byte written. I kind of dislike this >> >> > tracking of accessed bytes. Also, device would need to resolve conflicts >> >> > if any in some device specific way. >> >> >> >> It should be trivial to implement: you keep a scratch copy of the config >> >> space, and copy it to the master copy when they hit the latch. >> >> >> >> Implementation of this will show whether I've missed anything here, I >> >> think. >> > >> > What I refer to: what happens if driver does: >> > - write offset 1 >> > - write offset 3 >> > - hit commit latch >> >> - nothing >> - nothing >> - effect of offset 1 and offset 3 writes > > OK so this means that you also need to track which bytes where written > in order to know to skip byte 2. > This is what I referred to. If instead we ask driver to specify > offset/length explicitly device only needs to remember that. I was assuming the implementation would keep two complete copies of the config space: writes go to the scratch version, which gets copied to the master version upon latch write. But I do wonder if we should just skip this for now, since we don't have any immediate need. Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] memory corruption in HYPERVISOR_physdev_op()
>>> On 16.10.12 at 11:07, "Jan Beulich" wrote:
On 15.10.12 at 12:27, Ian Campbell wrote:
>>> My static analyzer complains about potential memory corruption in
>>> HYPERVISOR_physdev_op()
>>>
>>> arch/x86/include/asm/xen/hypercall.h
>>>389 static inline int
>>>390 HYPERVISOR_physdev_op(int cmd, void *arg)
>>>391 {
>>>392 int rc = _hypercall2(int, physdev_op, cmd, arg);
>>>393 if (unlikely(rc == -ENOSYS)) {
>>>394 struct physdev_op op;
>>>395 op.cmd = cmd;
>>>396 memcpy(&op.u, arg, sizeof(op.u));
>>>397 rc = _hypercall1(int, physdev_op_compat, &op);
>>>398 memcpy(arg, &op.u, sizeof(op.u));
>>> ^
>>> Some of the arg buffers are not as large as sizeof(op.u) which is either
>>> 12 or 16 depending on the size of longs in struct physdev_apic.
>>
>> Nasty!
>
> Doesn't the same problem also exist for
> HYPERVISOR_event_channel_op() then, at least theoretically
> (EVTCHNOP_reset is apparently the only addition here so far,
> but is being used by the tools only afaics)?
Actually, the problem isn't tied to new additions of sub-hypercalls
(I was wrongly implying this from the example originally provided),
and should be visible e.g. on any use of EVTCHNOP_unmask.
Jan
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] memory corruption in HYPERVISOR_physdev_op()
>>> On 15.10.12 at 12:27, Ian Campbell wrote:
>> My static analyzer complains about potential memory corruption in
>> HYPERVISOR_physdev_op()
>>
>> arch/x86/include/asm/xen/hypercall.h
>>389 static inline int
>>390 HYPERVISOR_physdev_op(int cmd, void *arg)
>>391 {
>>392 int rc = _hypercall2(int, physdev_op, cmd, arg);
>>393 if (unlikely(rc == -ENOSYS)) {
>>394 struct physdev_op op;
>>395 op.cmd = cmd;
>>396 memcpy(&op.u, arg, sizeof(op.u));
>>397 rc = _hypercall1(int, physdev_op_compat, &op);
>>398 memcpy(arg, &op.u, sizeof(op.u));
>> ^
>> Some of the arg buffers are not as large as sizeof(op.u) which is either
>> 12 or 16 depending on the size of longs in struct physdev_apic.
>
> Nasty!
Doesn't the same problem also exist for
HYPERVISOR_event_channel_op() then, at least theoretically
(EVTCHNOP_reset is apparently the only addition here so far,
but is being used by the tools only afaics)?
Jan
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Xen-devel] potential integer overflow in xenbus_file_write()
>>> On 15.10.12 at 12:25, Ian Campbell wrote:
> On Thu, 2012-09-13 at 19:00 +0300, Dan Carpenter wrote:
>> Hi,
>
> Thanks Dan. I'm not sure anyone from Xen-land really monitors
> virtualization@. Adding xen-devel and Konrad.
>
>>
>> I was reading some code and had a question in xenbus_file_write()
>>
>> drivers/xen/xenbus/xenbus_dev_frontend.c
>>461 if ((len + u->len) > sizeof(u->u.buffer)) {
>>
>> Can this addition overflow?
>
> len is a size_t and u->len is an unsigned int, so I expect so.
>
>> Should the test be something like:
>>
>> if (len > sizeof(u->u.buffer) || len + u->len > sizeof(u->u.buffer)) {
>
> I think that would do it.
Actually, it can remain a single range check:
if (len > sizeof(u->u.buffer) - u->len) {
because the rest of the code guarantees u->len to be less or
equal to sizeof(u->u.buffer) at all times.
Jan
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
