[PATCH 1/2] reset support: make net driver alloc/cleanup in probe and remove

[PATCH 1/2] reset support: make net driver alloc/cleanup in probe and remove

[Rusty Russell]

Since we want to reset the device to remove them, this is simpler
(device is reset for us on driver remove).

Signed-off-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>
---
 drivers/net/virtio_net.c |   44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff -r 7e5b3ff06f60 drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c	Wed Jan 23 22:53:14 2008 +1100
+++ b/drivers/net/virtio_net.c	Wed Jan 23 23:52:46 2008 +1100
@@ -300,12 +300,6 @@ static int virtnet_open(struct net_devic
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 
-	try_fill_recv(vi);
-
-	/* If we didn't even get one input buffer, we're useless. */
-	if (vi->num == 0)
-		return -ENOMEM;
-
 	napi_enable(&vi->napi);
 
 	/* If all buffers were filled by other side before we napi_enabled, we
@@ -320,22 +314,9 @@ static int virtnet_close(struct net_devi
 static int virtnet_close(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
-	struct sk_buff *skb;
 
 	napi_disable(&vi->napi);
 
-	/* networking core has neutered skb_xmit_done/skb_recv_done, so don't
-	 * worry about races vs. get(). */
-	vi->rvq->vq_ops->shutdown(vi->rvq);
-	while ((skb = __skb_dequeue(&vi->recv)) != NULL) {
-		kfree_skb(skb);
-		vi->num--;
-	}
-	vi->svq->vq_ops->shutdown(vi->svq);
-	while ((skb = __skb_dequeue(&vi->send)) != NULL)
-		kfree_skb(skb);
-
-	BUG_ON(vi->num != 0);
 	return 0;
 }
 
@@ -403,10 +384,22 @@ static int virtnet_probe(struct virtio_d
 		pr_debug("virtio_net: registering device failed\n");
 		goto free_send;
 	}
+
+	/* Last of all, set up some receive buffers. */
+	try_fill_recv(vi);
+
+	/* If we didn't even get one input buffer, we're useless. */
+	if (vi->num == 0) {
+		err = -ENOMEM;
+		goto unregister;
+	}
+
 	pr_debug("virtnet: registered device %s\n", dev->name);
 	vdev->priv = vi;
 	return 0;
 
+unregister:
+	unregister_netdev(dev);
 free_send:
 	vdev->config->del_vq(vi->svq);
 free_recv:
@@ -419,6 +412,19 @@ static void virtnet_remove(struct virtio
 static void virtnet_remove(struct virtio_device *vdev)
 {
 	struct virtnet_info *vi = vdev->priv;
+	struct sk_buff *skb;
+
+	/* Free our skbs in send and recv queues, if any. */
+	vi->rvq->vq_ops->shutdown(vi->rvq);
+	while ((skb = __skb_dequeue(&vi->recv)) != NULL) {
+		kfree_skb(skb);
+		vi->num--;
+	}
+	vi->svq->vq_ops->shutdown(vi->svq);
+	while ((skb = __skb_dequeue(&vi->send)) != NULL)
+		kfree_skb(skb);
+
+	BUG_ON(vi->num != 0);
 
 	vdev->config->del_vq(vi->svq);
 	vdev->config->del_vq(vi->rvq);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[PATCH 2/2] virtio reset support

[Rusty Russell]

(One big patch, I'll merge it in properly later).

A reset function solves three problems:

1) It allows us to renegotiate features, eg. if we want to upgrade a
   guest driver without rebooting the guest.

2) It gives us a clean way of shutting down virtqueues: after a reset,
   we know that the buffers won't be used by the guest, and

3) It helps the guest recover from messed-up drivers.

So we remove the ->shutdown hook, and the only way we now remove
feature bits is via reset.

Signed-off-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>
---
 drivers/char/hw_random/virtio-rng.c |    1 -
 drivers/lguest/lguest_device.c      |   15 ++++++++++++++-
 drivers/net/virtio_net.c            |    2 --
 drivers/virtio/virtio.c             |   13 ++++++++++---
 drivers/virtio/virtio_pci.c         |   10 ++++++++++
 drivers/virtio/virtio_ring.c        |   11 -----------
 include/linux/virtio.h              |    5 -----
 include/linux/virtio_config.h       |    4 ++++
 8 files changed, 38 insertions(+), 23 deletions(-)

diff -r f9464b21ed9c drivers/char/hw_random/virtio-rng.c
--- a/drivers/char/hw_random/virtio-rng.c	Wed Jan 23 23:52:47 2008 +1100
+++ b/drivers/char/hw_random/virtio-rng.c	Thu Jan 24 00:47:05 2008 +1100
@@ -92,7 +92,6 @@ static void virtrng_remove(struct virtio
 static void virtrng_remove(struct virtio_device *vdev)
 {
 	hwrng_unregister(&virtio_hwrng);
-	vq->vq_ops->shutdown(vq);
 	vdev->config->del_vq(vq);
 }
 
diff -r f9464b21ed9c drivers/lguest/lguest_device.c
--- a/drivers/lguest/lguest_device.c	Wed Jan 23 23:52:47 2008 +1100
+++ b/drivers/lguest/lguest_device.c	Thu Jan 24 00:47:05 2008 +1100
@@ -54,7 +54,7 @@ struct lguest_device {
  *
  * The configuration information for a device consists of one or more
  * virtqueues, a feature bitmaks, and some configuration bytes.  The
- * configuration bytes don't really matter to us: the Launcher set them up, and
+ * configuration bytes don't really matter to us: the Launcher sets them up, and
  * the driver will look at them during setup.
  *
  * A convenient routine to return the device's virtqueue config array:
@@ -139,7 +139,19 @@ static u8 lg_get_status(struct virtio_de
 
 static void lg_set_status(struct virtio_device *vdev, u8 status)
 {
+	BUG_ON(!status);
 	to_lgdev(vdev)->desc->status = status;
+}
+
+/* To reset the device, we (ab)use the NOTIFY hypercall, with the descriptor
+ * address of the device.  The Host will zero the status and all the
+ * features. */
+static void lg_reset(struct virtio_device *vdev)
+{
+	unsigned long offset = (void *)to_lgdev(vdev)->desc - lguest_devices;
+
+	printk("Resetting %i\n", vdev->index);
+	hcall(LHCALL_NOTIFY, (max_pfn<<PAGE_SHIFT) + offset, 0, 0);
 }
 
 /*
@@ -279,6 +291,7 @@ static struct virtio_config_ops lguest_c
 	.set = lg_set,
 	.get_status = lg_get_status,
 	.set_status = lg_set_status,
+	.reset = lg_reset,
 	.find_vq = lg_find_vq,
 	.del_vq = lg_del_vq,
 };
diff -r f9464b21ed9c drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c	Wed Jan 23 23:52:47 2008 +1100
+++ b/drivers/net/virtio_net.c	Thu Jan 24 00:47:05 2008 +1100
@@ -415,12 +415,10 @@ static void virtnet_remove(struct virtio
 	struct sk_buff *skb;
 
 	/* Free our skbs in send and recv queues, if any. */
-	vi->rvq->vq_ops->shutdown(vi->rvq);
 	while ((skb = __skb_dequeue(&vi->recv)) != NULL) {
 		kfree_skb(skb);
 		vi->num--;
 	}
-	vi->svq->vq_ops->shutdown(vi->svq);
 	while ((skb = __skb_dequeue(&vi->send)) != NULL)
 		kfree_skb(skb);
 
diff -r f9464b21ed9c drivers/virtio/virtio.c
--- a/drivers/virtio/virtio.c	Wed Jan 23 23:52:47 2008 +1100
+++ b/drivers/virtio/virtio.c	Thu Jan 24 00:47:05 2008 +1100
@@ -104,10 +104,13 @@ static int virtio_dev_remove(struct devi
 	struct virtio_driver *drv = container_of(dev->dev.driver,
 						 struct virtio_driver, driver);
 
-	dev->config->set_status(dev, dev->config->get_status(dev)
-				& ~(VIRTIO_CONFIG_S_DRIVER
-				    | VIRTIO_CONFIG_S_DRIVER_OK));
+	/* This shuts down all the virtqueues, so their buffers won't
+	 * be used. */
+	dev->config->reset(dev);
 	drv->remove(dev);
+
+	/* Acknowledge the device's existence again, after reset. */
+	add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
 	return 0;
 }
 
@@ -139,6 +142,10 @@ int register_virtio_device(struct virtio
 	dev->dev.release = virtio_device_release;
 	sprintf(dev->dev.bus_id, "%u", dev->index);
 
+	/* We always start by resetting the device, in case a previous
+	 * driver messed it up.  This also tests that code path a little. */
+	dev->config->reset(dev);
+
 	/* Acknowledge that we've seen the device. */
 	add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
 
diff -r f9464b21ed9c drivers/virtio/virtio_pci.c
--- a/drivers/virtio/virtio_pci.c	Wed Jan 23 23:52:47 2008 +1100
+++ b/drivers/virtio/virtio_pci.c	Thu Jan 24 00:47:05 2008 +1100
@@ -148,7 +148,16 @@ static void vp_set_status(struct virtio_
 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);
+}
+
+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);
 }
 
 /* the notify function used when creating a virt queue */
@@ -291,6 +300,7 @@ static struct virtio_config_ops virtio_p
 	.set		= vp_set,
 	.get_status	= vp_get_status,
 	.set_status	= vp_set_status,
+	.reset		= vp_reset,
 	.find_vq	= vp_find_vq,
 	.del_vq		= vp_del_vq,
 };
diff -r f9464b21ed9c drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c	Wed Jan 23 23:52:47 2008 +1100
+++ b/drivers/virtio/virtio_ring.c	Thu Jan 24 00:47:05 2008 +1100
@@ -173,16 +173,6 @@ static void detach_buf(struct vring_virt
 	vq->num_free++;
 }
 
-/* FIXME: We need to tell other side about removal, to synchronize. */
-static void vring_shutdown(struct virtqueue *_vq)
-{
-	struct vring_virtqueue *vq = to_vvq(_vq);
-	unsigned int i;
-
-	for (i = 0; i < vq->vring.num; i++)
-		detach_buf(vq, i);
-}
-
 static inline bool more_used(const struct vring_virtqueue *vq)
 {
 	return vq->last_used_idx != vq->vring.used->idx;
@@ -287,7 +277,6 @@ static struct virtqueue_ops vring_vq_ops
 	.kick = vring_kick,
 	.disable_cb = vring_disable_cb,
 	.enable_cb = vring_enable_cb,
-	.shutdown = vring_shutdown,
 };
 
 struct virtqueue *vring_new_virtqueue(unsigned int num,
diff -r f9464b21ed9c include/linux/virtio.h
--- a/include/linux/virtio.h	Wed Jan 23 23:52:47 2008 +1100
+++ b/include/linux/virtio.h	Thu Jan 24 00:47:05 2008 +1100
@@ -45,9 +45,6 @@ struct virtqueue
  *	vq: the struct virtqueue we're talking about.
  *	This returns "false" (and doesn't re-enable) if there are pending
  *	buffers in the queue, to avoid a race.
- * @shutdown: "unadd" all buffers.
- *	vq: the struct virtqueue we're talking about.
- *	Remove everything from the queue.
  *
  * Locking rules are straightforward: the driver is responsible for
  * locking.  No two operations may be invoked simultaneously.
@@ -67,8 +64,6 @@ struct virtqueue_ops {
 
 	void (*disable_cb)(struct virtqueue *vq);
 	bool (*enable_cb)(struct virtqueue *vq);
-
-	void (*shutdown)(struct virtqueue *vq);
 };
 
 /**
diff -r f9464b21ed9c include/linux/virtio_config.h
--- a/include/linux/virtio_config.h	Wed Jan 23 23:52:47 2008 +1100
+++ b/include/linux/virtio_config.h	Thu Jan 24 00:47:05 2008 +1100
@@ -43,6 +43,9 @@ struct virtio_device;
  * @set_status: write the status byte
  *	vdev: the virtio_device
  *	status: the new status byte
+ * @reset: reset the device
+ *	vdev: the virtio device
+ *	After this, status and feature negotiation must be done again
  * @find_vq: find a virtqueue and instantiate it.
  *	vdev: the virtio_device
  *	index: the 0-based virtqueue number in case there's more than one.
@@ -59,6 +62,7 @@ struct virtio_config_ops
 		    const void *buf, unsigned len);
 	u8 (*get_status)(struct virtio_device *vdev);
 	void (*set_status)(struct virtio_device *vdev, u8 status);
+	void (*reset)(struct virtio_device *vdev);
 	struct virtqueue *(*find_vq)(struct virtio_device *vdev,
 				     unsigned index,
 				     void (*callback)(struct virtqueue *));

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 2/2] virtio reset support

[Dor Laor]

The patches really fix/simplify things :) 
Did you test device open->close->open? It was broken in my last test,
we cad add link indications too.

Besides that I also remembered that adding 'vq->num_free =vq->ring.num;'
after reset fixed part of the problems, I don't remember why (gonna
re-check it soon), I think it was the reset calls for (..) detach_buf 
for all buffers regardless if they were free already.
Cheers,
Dor


On Thu, 2008-01-24 at 01:18 +1100, Rusty Russell wrote:
> diff -r f9464b21ed9c drivers/net/virtio_net.c
> --- a/drivers/net/virtio_net.c  Wed Jan 23 23:52:47 2008 +1100
> +++ b/drivers/net/virtio_net.c  Thu Jan 24 00:47:05 2008 +1100
> @@ -415,12 +415,10 @@ static void virtnet_remove(struct virtio
>         struct sk_buff *skb;
>  
>         /* Free our skbs in send and recv queues, if any. */
> -       vi->rvq->vq_ops->shutdown(vi->rvq);
>         while ((skb = __skb_dequeue(&vi->recv)) != NULL) {
>                 kfree_skb(skb);
>                 vi->num--;
>         }
> -       vi->svq->vq_ops->shutdown(vi->svq);
>         while ((skb = __skb_dequeue(&vi->send)) != NULL)
>                 kfree_skb(skb);


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 2/2] virtio reset support

[Rusty Russell]

On Friday 25 January 2008 00:09:07 Dor Laor wrote:
> The patches really fix/simplify things :)

Yes, that's why I like reset, it solves multiple problems.

> Did you test device open->close->open? It was broken in my last test,

Well, it's not really fair, since I moved the buffer allocation and 
deallocation to the probe and remove routines (otherwise we'd have to reset 
on every close).

> we cad add link indications too.

Definitely.  We'll use the config_change callback.  It should be quite neat.

> Besides that I also remembered that adding 'vq->num_free =vq->ring.num;'
> after reset fixed part of the problems, I don't remember why (gonna
> re-check it soon), I think it was the reset calls for (..) detach_buf
> for all buffers regardless if they were free already.

My initial reaction was that we don't need to do cleanup, because it's 
nonsensical to use a virtqueue after reset (you can only really do del_vq: in 
fact we may eventually change "reset" to "kill" which also deletes the 
virtqueues).

However, a shared interrupt might make us look at the ring again, and we'll 
get confused.  reset() must at least reset the 'last seen' index of the 
virtqueues (I had the same issue in the guest).

BTW, here's what lguest does on the host side for reset:

/* Resetting a device is fairly easy. */
static void reset_device(struct device *dev)
{
	struct virtqueue *vq;

	verbose("Resetting device %s\n", dev->name);
	/* Clear the status. */
	dev->desc->status = 0;

	/* Clear any features they've acked. */
	memset(get_feature_bits(dev) + dev->desc->feature_len, 0,
	       dev->desc->feature_len);

	/* Zero out the virtqueues. */
	for (vq = dev->vq; vq; vq = vq->next) {
		memset(vq->vring.desc, 0,
		       vring_size(vq->config.num, getpagesize()));
		vq->last_avail_idx = 0;
	}
}

Cheers,
Rusty.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 2/2] virtio reset support

[Avi Kivity]

Rusty Russell wrote:
> +
> +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);
>  }
>   

pci has something called FLR for function-level reset.  If we use that 
as the reset mechanism, then reset can be initiated from outside the 
virtio layer, if the guest OS supports that.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 2/2] virtio reset support

[Anthony Liguori]

Avi Kivity wrote:
> Rusty Russell wrote:
>> +
>> +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);
>>  }
>>   
>
> pci has something called FLR for function-level reset.  If we use that 
> as the reset mechanism, then reset can be initiated from outside the 
> virtio layer, if the guest OS supports that.

PCI-e has a common reset concept (warm and cold).  I've been looking 
around and I can't seem to find any common reset mechanism for PCI.  Is 
FLR something that is per-device or a standard PCI mechanism?  If it's 
the former, than we've basically implemented FLR using this bit in the 
config space.

Regards,

Anthony Liguori


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 2/2] virtio reset support

[Avi Kivity]

Anthony Liguori wrote:
>
> PCI-e has a common reset concept (warm and cold).  I've been looking 
> around and I can't seem to find any common reset mechanism for PCI.  
> Is FLR something that is per-device or a standard PCI mechanism?  If 
> it's the former, than we've basically implemented FLR using this bit 
> in the config space.
>

I believe it is a standard mechanism, albeit new, so perhaps many 
devices don't implement it.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 2/2] virtio reset support

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>>
>> PCI-e has a common reset concept (warm and cold).  I've been looking 
>> around and I can't seem to find any common reset mechanism for PCI.  
>> Is FLR something that is per-device or a standard PCI mechanism?  If 
>> it's the former, than we've basically implemented FLR using this bit 
>> in the config space.
>>
>
> I believe it is a standard mechanism, albeit new, so perhaps many 
> devices don't implement it.

I don't have a copy of the PCI specification handy.  Can you dig up how 
it's implemented?  I don't see any references in my local documentation 
and I couldn't find anything in Linux that referenced it at the PCI level.

Regards,

Anthony Liguori



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 2/2] virtio reset support

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>>
>>> PCI-e has a common reset concept (warm and cold).  I've been looking 
>>> around and I can't seem to find any common reset mechanism for PCI.  
>>> Is FLR something that is per-device or a standard PCI mechanism?  If 
>>> it's the former, than we've basically implemented FLR using this bit 
>>> in the config space.
>>>
>>
>> I believe it is a standard mechanism, albeit new, so perhaps many 
>> devices don't implement it.
>
> I don't have a copy of the PCI specification handy.  Can you dig up 
> how it's implemented?  I don't see any references in my local 
> documentation and I couldn't find anything in Linux that referenced it 
> at the PCI level.
>

I think it's NDA material, so even if I found it, I couldn't publish 
it.  It can probably be reverse-engineered from the Xen patches, or 
perhaps your employer is a PCI-SIG member so you can get access to it.

-- 
error compiling committee.c: too many arguments to function