[PATCH] virtio-net: Read MAC only after initializing MSI-X

[PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

The MAC of a virtio-net device is located at the first field of the device
specific header. This header is located at offset 20 if the device doesn't
support MSI-X or offset 24 if it does.

Current code in virtnet_probe() used to probe the MAC before checking for
MSI-X, which means that the read was always made from offset 20 regardless
of whether MSI-X in enabled or not.

This patch moves the MAC probe to after the detection of whether MSI-X is
enabled. This way the MAC will be read from offset 24 if the device indeed
supports MSI-X.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 drivers/net/virtio_net.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..55ccf96 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -981,14 +981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
 		/* (!csum && gso) case will be fixed by register_netdev() */
 	}
 
-	/* Configuration may specify what MAC to use.  Otherwise random. */
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
-		vdev->config->get(vdev,
-				  offsetof(struct virtio_net_config, mac),
-				  dev->dev_addr, dev->addr_len);
-	} else
-		random_ether_addr(dev->dev_addr);
-
 	/* Set up our device-specific information */
 	vi = netdev_priv(dev);
 	netif_napi_add(dev, &vi->napi, virtnet_poll, napi_weight);
@@ -1032,6 +1024,14 @@ static int virtnet_probe(struct virtio_device *vdev)
 			dev->features |= NETIF_F_HW_VLAN_FILTER;
 	}
 
+	/* Configuration may specify what MAC to use.  Otherwise random. */
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
+		vdev->config->get(vdev,
+				  offsetof(struct virtio_net_config, mac),
+				  dev->dev_addr, dev->addr_len);
+	} else
+		random_ether_addr(dev->dev_addr);
+
 	err = register_netdev(dev);
 	if (err) {
 		pr_debug("virtio_net: registering device failed\n");
-- 
1.7.6

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> The MAC of a virtio-net device is located at the first field of the device
> specific header. This header is located at offset 20 if the device doesn't
> support MSI-X or offset 24 if it does.

Erk.  This means, in general, we have to do virtio_find_single_vq or
config->find_vqs before we examine any config options.

Look at virtio_blk, which has the same error.

Solutions in order of best to worst:
(1) Enable MSI-X before calling device probe.  This means reserving two
    vectors in virtio_pci_probe to ensure we *can* do this, I think.  Michael?

(2) Ensure ordering of "find_vqs then access config space" statically.  This
    probably means handing the vqs array to virtio_config_val, so noone
    can call it before they have their virtqueues.

(3) Ensure ordering dynamically, ie. BUG_ON() if they haven't done
    find_vqs when they call the config accessors.

If (1) is too invasive for -stable, then we should rearrange the drivers
in separate patches (and cc: -stable), then fix it properly.

Good catch Sasha!

Cheers,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> The MAC of a virtio-net device is located at the first field of the device
> specific header. This header is located at offset 20 if the device doesn't
> support MSI-X or offset 24 if it does.

Erk.  This means, in general, we have to do virtio_find_single_vq or
config->find_vqs before we examine any config options.

Look at virtio_blk, which has the same error.

Solutions in order of best to worst:
(1) Enable MSI-X before calling device probe.  This means reserving two
    vectors in virtio_pci_probe to ensure we *can* do this, I think.  Michael?

(2) Ensure ordering of "find_vqs then access config space" statically.  This
    probably means handing the vqs array to virtio_config_val, so noone
    can call it before they have their virtqueues.

(3) Ensure ordering dynamically, ie. BUG_ON() if they haven't done
    find_vqs when they call the config accessors.

If (1) is too invasive for -stable, then we should rearrange the drivers
in separate patches (and cc: -stable), then fix it properly.

Good catch Sasha!

Cheers,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > The MAC of a virtio-net device is located at the first field of the device
> > specific header. This header is located at offset 20 if the device doesn't
> > support MSI-X or offset 24 if it does.
> 
> Erk.  This means, in general, we have to do virtio_find_single_vq or
> config->find_vqs before we examine any config options.
> 
> Look at virtio_blk, which has the same error.
> 
> Solutions in order of best to worst:
> (1) Enable MSI-X before calling device probe.  This means reserving two
>     vectors in virtio_pci_probe to ensure we *can* do this, I think.  Michael?

Do you mean reserving the vectors even before we probed the device for
MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).

> (2) Ensure ordering of "find_vqs then access config space" statically.  This
>     probably means handing the vqs array to virtio_config_val, so noone
>     can call it before they have their virtqueues.

Just noticed that only virtio-blk uses virtio_config_val(), while the
others are still doing 'if(virtio_has_feature()) vdev->config->get()',
I'll send patches to fix that regardless of what we end up doing here.

Did you want to pass the vq array to virtio_config_val() just to check
that they were already found? 

> (3) Ensure ordering dynamically, ie. BUG_ON() if they haven't done
>     find_vqs when they call the config accessors.
> 
> If (1) is too invasive for -stable, then we should rearrange the drivers
> in separate patches (and cc: -stable), then fix it properly.
> 
> Good catch Sasha!
> 
> Cheers,
> Rusty.


-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Sun, 14 Aug 2011 16:57:32 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> > 
> > Erk.  This means, in general, we have to do virtio_find_single_vq or
> > config->find_vqs before we examine any config options.
> > 
> > Look at virtio_blk, which has the same error.
> > 
> > Solutions in order of best to worst:
> > (1) Enable MSI-X before calling device probe.  This means reserving two
> >     vectors in virtio_pci_probe to ensure we *can* do this, I think.  Michael?
> 
> Do you mean reserving the vectors even before we probed the device for
> MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).

We want three, but *need* two: see vp_find_vqs().  Also, the generic
code doesn't know how many virtqueues we have on the device.

> > (2) Ensure ordering of "find_vqs then access config space" statically.  This
> >     probably means handing the vqs array to virtio_config_val, so noone
> >     can call it before they have their virtqueues.
> 
> Just noticed that only virtio-blk uses virtio_config_val(), while the
> others are still doing 'if(virtio_has_feature()) vdev->config->get()',
> I'll send patches to fix that regardless of what we end up doing here.

Thanks.

> Did you want to pass the vq array to virtio_config_val() just to check
> that they were already found? 

Not if we fix is using method #1...

Thanks,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Sun, 14 Aug 2011 16:57:32 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> > 
> > Erk.  This means, in general, we have to do virtio_find_single_vq or
> > config->find_vqs before we examine any config options.
> > 
> > Look at virtio_blk, which has the same error.
> > 
> > Solutions in order of best to worst:
> > (1) Enable MSI-X before calling device probe.  This means reserving two
> >     vectors in virtio_pci_probe to ensure we *can* do this, I think.  Michael?
> 
> Do you mean reserving the vectors even before we probed the device for
> MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).

We want three, but *need* two: see vp_find_vqs().  Also, the generic
code doesn't know how many virtqueues we have on the device.

> > (2) Ensure ordering of "find_vqs then access config space" statically.  This
> >     probably means handing the vqs array to virtio_config_val, so noone
> >     can call it before they have their virtqueues.
> 
> Just noticed that only virtio-blk uses virtio_config_val(), while the
> others are still doing 'if(virtio_has_feature()) vdev->config->get()',
> I'll send patches to fix that regardless of what we end up doing here.

Thanks.

> Did you want to pass the vq array to virtio_config_val() just to check
> that they were already found? 

Not if we fix is using method #1...

Thanks,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

On Mon, 2011-08-15 at 09:55 +0930, Rusty Russell wrote:
> On Sun, 14 Aug 2011 16:57:32 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> > > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > > > The MAC of a virtio-net device is located at the first field of the device
> > > > specific header. This header is located at offset 20 if the device doesn't
> > > > support MSI-X or offset 24 if it does.
> > > 
> > > Erk.  This means, in general, we have to do virtio_find_single_vq or
> > > config->find_vqs before we examine any config options.
> > > 
> > > Look at virtio_blk, which has the same error.
> > > 
> > > Solutions in order of best to worst:
> > > (1) Enable MSI-X before calling device probe.  This means reserving two
> > >     vectors in virtio_pci_probe to ensure we *can* do this, I think.  Michael?
> > 
> > Do you mean reserving the vectors even before we probed the device for
> > MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).
> 
> We want three, but *need* two: see vp_find_vqs().  Also, the generic
> code doesn't know how many virtqueues we have on the device.

We can just pci_enable_msix() and see if we can get 2 vectors, if we can
- we assume the device has msix on, right?


-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

On Mon, 2011-08-15 at 09:55 +0930, Rusty Russell wrote:
> On Sun, 14 Aug 2011 16:57:32 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> > > On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > > > The MAC of a virtio-net device is located at the first field of the device
> > > > specific header. This header is located at offset 20 if the device doesn't
> > > > support MSI-X or offset 24 if it does.
> > > 
> > > Erk.  This means, in general, we have to do virtio_find_single_vq or
> > > config->find_vqs before we examine any config options.
> > > 
> > > Look at virtio_blk, which has the same error.
> > > 
> > > Solutions in order of best to worst:
> > > (1) Enable MSI-X before calling device probe.  This means reserving two
> > >     vectors in virtio_pci_probe to ensure we *can* do this, I think.  Michael?
> > 
> > Do you mean reserving the vectors even before we probed the device for
> > MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).
> 
> We want three, but *need* two: see vp_find_vqs().  Also, the generic
> code doesn't know how many virtqueues we have on the device.

We can just pci_enable_msix() and see if we can get 2 vectors, if we can
- we assume the device has msix on, right?


-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

On Sun, 2011-08-14 at 12:23 +0930, Rusty Russell wrote:
> On Sat, 13 Aug 2011 11:51:01 +0300, Sasha Levin <levinsasha928@gmail.com> wrote:
> > The MAC of a virtio-net device is located at the first field of the device
> > specific header. This header is located at offset 20 if the device doesn't
> > support MSI-X or offset 24 if it does.
> 
> Erk.  This means, in general, we have to do virtio_find_single_vq or
> config->find_vqs before we examine any config options.
> 
> Look at virtio_blk, which has the same error.
> 
> Solutions in order of best to worst:
> (1) Enable MSI-X before calling device probe.  This means reserving two
>     vectors in virtio_pci_probe to ensure we *can* do this, I think.  Michael?

Do you mean reserving the vectors even before we probed the device for
MSI-X support? Wouldn't we need 3 vectors then? (config, input, output).

> (2) Ensure ordering of "find_vqs then access config space" statically.  This
>     probably means handing the vqs array to virtio_config_val, so noone
>     can call it before they have their virtqueues.

Just noticed that only virtio-blk uses virtio_config_val(), while the
others are still doing 'if(virtio_has_feature()) vdev->config->get()',
I'll send patches to fix that regardless of what we end up doing here.

Did you want to pass the vq array to virtio_config_val() just to check
that they were already found? 

> (3) Ensure ordering dynamically, ie. BUG_ON() if they haven't done
>     find_vqs when they call the config accessors.
> 
> If (1) is too invasive for -stable, then we should rearrange the drivers
> in separate patches (and cc: -stable), then fix it properly.
> 
> Good catch Sasha!
> 
> Cheers,
> Rusty.


-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> The MAC of a virtio-net device is located at the first field of the device
> specific header. This header is located at offset 20 if the device doesn't
> support MSI-X or offset 24 if it does.
> 
> Current code in virtnet_probe() used to probe the MAC before checking for
> MSI-X, which means that the read was always made from offset 20 regardless
> of whether MSI-X in enabled or not.
> 
> This patch moves the MAC probe to after the detection of whether MSI-X is
> enabled. This way the MAC will be read from offset 24 if the device indeed
> supports MSI-X.
> 
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: netdev@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>

I am not sure I see a bug in virtio: the config pace layout simply
changes as msix is enabled and disabled (and if you look at the latest
draft, also on whether 64 bit features are enabled).
It doesn't depend on msix capability being present in device.

The spec seems to be explicit enough:
	If MSI-X is enabled for the device, two additional fields immediately
	follow this header.

So I'm guessing the bug is in kvm tools which assume
same layout for when msix is enabled and disabled.
qemu-kvm seems to do the right thing so the device
seems to get the correct mac.

> ---
>  drivers/net/virtio_net.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0c7321c..55ccf96 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -981,14 +981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
>  		/* (!csum && gso) case will be fixed by register_netdev() */
>  	}
>  
> -	/* Configuration may specify what MAC to use.  Otherwise random. */
> -	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> -		vdev->config->get(vdev,
> -				  offsetof(struct virtio_net_config, mac),
> -				  dev->dev_addr, dev->addr_len);
> -	} else
> -		random_ether_addr(dev->dev_addr);
> -
>  	/* Set up our device-specific information */
>  	vi = netdev_priv(dev);
>  	netif_napi_add(dev, &vi->napi, virtnet_poll, napi_weight);
> @@ -1032,6 +1024,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>  			dev->features |= NETIF_F_HW_VLAN_FILTER;
>  	}
>  
> +	/* Configuration may specify what MAC to use.  Otherwise random. */
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> +		vdev->config->get(vdev,
> +				  offsetof(struct virtio_net_config, mac),
> +				  dev->dev_addr, dev->addr_len);
> +	} else
> +		random_ether_addr(dev->dev_addr);
> +
>  	err = register_netdev(dev);
>  	if (err) {
>  		pr_debug("virtio_net: registering device failed\n");
> -- 
> 1.7.6

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> The MAC of a virtio-net device is located at the first field of the device
> specific header. This header is located at offset 20 if the device doesn't
> support MSI-X or offset 24 if it does.
> 
> Current code in virtnet_probe() used to probe the MAC before checking for
> MSI-X, which means that the read was always made from offset 20 regardless
> of whether MSI-X in enabled or not.
> 
> This patch moves the MAC probe to after the detection of whether MSI-X is
> enabled. This way the MAC will be read from offset 24 if the device indeed
> supports MSI-X.
> 
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Cc: netdev@vger.kernel.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>

I am not sure I see a bug in virtio: the config pace layout simply
changes as msix is enabled and disabled (and if you look at the latest
draft, also on whether 64 bit features are enabled).
It doesn't depend on msix capability being present in device.

The spec seems to be explicit enough:
	If MSI-X is enabled for the device, two additional fields immediately
	follow this header.

So I'm guessing the bug is in kvm tools which assume
same layout for when msix is enabled and disabled.
qemu-kvm seems to do the right thing so the device
seems to get the correct mac.

> ---
>  drivers/net/virtio_net.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0c7321c..55ccf96 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -981,14 +981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
>  		/* (!csum && gso) case will be fixed by register_netdev() */
>  	}
>  
> -	/* Configuration may specify what MAC to use.  Otherwise random. */
> -	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> -		vdev->config->get(vdev,
> -				  offsetof(struct virtio_net_config, mac),
> -				  dev->dev_addr, dev->addr_len);
> -	} else
> -		random_ether_addr(dev->dev_addr);
> -
>  	/* Set up our device-specific information */
>  	vi = netdev_priv(dev);
>  	netif_napi_add(dev, &vi->napi, virtnet_poll, napi_weight);
> @@ -1032,6 +1024,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>  			dev->features |= NETIF_F_HW_VLAN_FILTER;
>  	}
>  
> +	/* Configuration may specify what MAC to use.  Otherwise random. */
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
> +		vdev->config->get(vdev,
> +				  offsetof(struct virtio_net_config, mac),
> +				  dev->dev_addr, dev->addr_len);
> +	} else
> +		random_ether_addr(dev->dev_addr);
> +
>  	err = register_netdev(dev);
>  	if (err) {
>  		pr_debug("virtio_net: registering device failed\n");
> -- 
> 1.7.6

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote:
> On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > The MAC of a virtio-net device is located at the first field of the device
> > specific header. This header is located at offset 20 if the device doesn't
> > support MSI-X or offset 24 if it does.
> > 
> > Current code in virtnet_probe() used to probe the MAC before checking for
> > MSI-X, which means that the read was always made from offset 20 regardless
> > of whether MSI-X in enabled or not.
> > 
> > This patch moves the MAC probe to after the detection of whether MSI-X is
> > enabled. This way the MAC will be read from offset 24 if the device indeed
> > supports MSI-X.
> > 
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: netdev@vger.kernel.org
> > Cc: kvm@vger.kernel.org
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> I am not sure I see a bug in virtio: the config pace layout simply
> changes as msix is enabled and disabled (and if you look at the latest
> draft, also on whether 64 bit features are enabled).
> It doesn't depend on msix capability being present in device.
> 
> The spec seems to be explicit enough:
> 	If MSI-X is enabled for the device, two additional fields immediately
> 	follow this header.
> 
> So I'm guessing the bug is in kvm tools which assume
> same layout for when msix is enabled and disabled.
> qemu-kvm seems to do the right thing so the device
> seems to get the correct mac.

We assumed that PCI config space has a static layout like most other
devices. Having a behavior of "First bit 20 does something, but after
enabling MSI-X it does something completely different" sounds strange.

I'm wondering why offsets of the config structure change during run time
and are not statically defined when the device is started.

It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled,
or MSI-X can be simply disabled during run time.

Maybe this is better solved by copying the way it was done in PCI itself
with capability linked list?

-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote:
> On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > The MAC of a virtio-net device is located at the first field of the device
> > specific header. This header is located at offset 20 if the device doesn't
> > support MSI-X or offset 24 if it does.
> > 
> > Current code in virtnet_probe() used to probe the MAC before checking for
> > MSI-X, which means that the read was always made from offset 20 regardless
> > of whether MSI-X in enabled or not.
> > 
> > This patch moves the MAC probe to after the detection of whether MSI-X is
> > enabled. This way the MAC will be read from offset 24 if the device indeed
> > supports MSI-X.
> > 
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: netdev@vger.kernel.org
> > Cc: kvm@vger.kernel.org
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> I am not sure I see a bug in virtio: the config pace layout simply
> changes as msix is enabled and disabled (and if you look at the latest
> draft, also on whether 64 bit features are enabled).
> It doesn't depend on msix capability being present in device.
> 
> The spec seems to be explicit enough:
> 	If MSI-X is enabled for the device, two additional fields immediately
> 	follow this header.
> 
> So I'm guessing the bug is in kvm tools which assume
> same layout for when msix is enabled and disabled.
> qemu-kvm seems to do the right thing so the device
> seems to get the correct mac.

We assumed that PCI config space has a static layout like most other
devices. Having a behavior of "First bit 20 does something, but after
enabling MSI-X it does something completely different" sounds strange.

I'm wondering why offsets of the config structure change during run time
and are not statically defined when the device is started.

It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled,
or MSI-X can be simply disabled during run time.

Maybe this is better solved by copying the way it was done in PCI itself
with capability linked list?

-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote:
> > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> > > 
> > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > MSI-X, which means that the read was always made from offset 20 regardless
> > > of whether MSI-X in enabled or not.
> > > 
> > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > supports MSI-X.
> > > 
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: netdev@vger.kernel.org
> > > Cc: kvm@vger.kernel.org
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > 
> > I am not sure I see a bug in virtio: the config pace layout simply
> > changes as msix is enabled and disabled (and if you look at the latest
> > draft, also on whether 64 bit features are enabled).
> > It doesn't depend on msix capability being present in device.
> > 
> > The spec seems to be explicit enough:
> > 	If MSI-X is enabled for the device, two additional fields immediately
> > 	follow this header.
> > 
> > So I'm guessing the bug is in kvm tools which assume
> > same layout for when msix is enabled and disabled.
> > qemu-kvm seems to do the right thing so the device
> > seems to get the correct mac.
> 
> We assumed that PCI config space has a static layout like most other
> devices. Having a behavior of "First bit 20 does something, but after
> enabling MSI-X it does something completely different" sounds strange.

The layout is always virtio header followed by device specific header.
We started with a small header so when more data was added, we could not
extend the header unconditionally.

We can't change that behaviour for MSI-X now, guests and
hosts rely on it.

>
> I'm wondering why offsets of the config structure change during run time
> and are not statically defined when the device is started.

That's because of backwards compatibility with old guests.
When we know the guest is new, we expose new layout,
but old guests must see old layout.

> It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled,

Yes it can, e.g. at guest reset. Generally features can be tweaked
any way guest likes until status is set to OK.

> or MSI-X can be simply disabled during run time.

Not sure what you mean by 'run time'. Guest can reset
or disable the device, change any parameters,
then re-enable.

> Maybe this is better solved by copying the way it was done in PCI itself
> with capability linked list?
> 
> -- 
> 
> Sasha.

There are any number of ways to lay out the structure.  I went for what
seemed a simplest one.  For MSI-X the train has left the station.  We
can probably still tweak where the high 32 bit features
for 64 bit features are.  No idea if it's worth it.


-- 
MST

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> > Maybe this is better solved by copying the way it was done in PCI itself
> > with capability linked list?
> 
> There are any number of ways to lay out the structure.  I went for what
> seemed a simplest one.  For MSI-X the train has left the station.  We
> can probably still tweak where the high 32 bit features
> for 64 bit features are.  No idea if it's worth it.

Sorry, this has been in the back of my mind.  I think it's a good idea;
can we use the capability linked list for pre-device specific stuff from
now on?

Thanks,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote:
> On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> > > Maybe this is better solved by copying the way it was done in PCI itself
> > > with capability linked list?
> > 
> > There are any number of ways to lay out the structure.  I went for what
> > seemed a simplest one.  For MSI-X the train has left the station.  We
> > can probably still tweak where the high 32 bit features
> > for 64 bit features are.  No idea if it's worth it.
> 
> Sorry, this has been in the back of my mind.  I think it's a good idea;
> can we use the capability linked list for pre-device specific stuff from
> now on?
> 
> Thanks,
> Rusty.

Do we even want capability bits then?
We can give each capability an ack flag ...

-- 
MST

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote:
> > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> > > > Maybe this is better solved by copying the way it was done in PCI itself
> > > > with capability linked list?
> > > 
> > > There are any number of ways to lay out the structure.  I went for what
> > > seemed a simplest one.  For MSI-X the train has left the station.  We
> > > can probably still tweak where the high 32 bit features
> > > for 64 bit features are.  No idea if it's worth it.
> > 
> > Sorry, this has been in the back of my mind.  I think it's a good idea;
> > can we use the capability linked list for pre-device specific stuff from
> > now on?
> > 
> > Thanks,
> > Rusty.
> 
> Do we even want capability bits then?
> We can give each capability an ack flag ...

We could have, and if I'd known PCI when I designed virtio I might have.

But it's not easy now to map structure offsets to that scheme, and we
can't really force such a change on the non-PCI users.  So I'd say we
should only do it for the non-device-specific options.  ie. we'll still
have the MSI-X case move the device-specific config, but we'll use a
linked list from now on, eg. for the next 32 features bits...

Thoughts?
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

On Mon, 2011-09-19 at 17:19 +0930, Rusty Russell wrote:
> On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote:
> > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> > > > > Maybe this is better solved by copying the way it was done in PCI itself
> > > > > with capability linked list?
> > > > 
> > > > There are any number of ways to lay out the structure.  I went for what
> > > > seemed a simplest one.  For MSI-X the train has left the station.  We
> > > > can probably still tweak where the high 32 bit features
> > > > for 64 bit features are.  No idea if it's worth it.
> > > 
> > > Sorry, this has been in the back of my mind.  I think it's a good idea;
> > > can we use the capability linked list for pre-device specific stuff from
> > > now on?
> > > 
> > > Thanks,
> > > Rusty.
> > 
> > Do we even want capability bits then?
> > We can give each capability an ack flag ...
> 
> We could have, and if I'd known PCI when I designed virtio I might have.
> 
> But it's not easy now to map structure offsets to that scheme, and we
> can't really force such a change on the non-PCI users.  So I'd say we
> should only do it for the non-device-specific options.  ie. we'll still
> have the MSI-X case move the device-specific config, but we'll use a
> linked list from now on, eg. for the next 32 features bits...
> 
> Thoughts?
> Rusty.

What if we create a capability list but place it in the virtio-pci
config space instead of the PCI space?

It'll work fine with non-PCI users and would leave MSI-X as the only
thing that changes offsets (and we could probably deprecate and remove
it at some point in the future).

-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Wed, Sep 28, 2011 at 09:30:51PM +0300, Sasha Levin wrote:
> On Mon, 2011-09-19 at 17:19 +0930, Rusty Russell wrote:
> > On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote:
> > > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> > > > > > Maybe this is better solved by copying the way it was done in PCI itself
> > > > > > with capability linked list?
> > > > > 
> > > > > There are any number of ways to lay out the structure.  I went for what
> > > > > seemed a simplest one.  For MSI-X the train has left the station.  We
> > > > > can probably still tweak where the high 32 bit features
> > > > > for 64 bit features are.  No idea if it's worth it.
> > > > 
> > > > Sorry, this has been in the back of my mind.  I think it's a good idea;
> > > > can we use the capability linked list for pre-device specific stuff from
> > > > now on?
> > > > 
> > > > Thanks,
> > > > Rusty.
> > > 
> > > Do we even want capability bits then?
> > > We can give each capability an ack flag ...
> > 
> > We could have, and if I'd known PCI when I designed virtio I might have.
> > 
> > But it's not easy now to map structure offsets to that scheme, and we
> > can't really force such a change on the non-PCI users.  So I'd say we
> > should only do it for the non-device-specific options.  ie. we'll still
> > have the MSI-X case move the device-specific config, but we'll use a
> > linked list from now on, eg. for the next 32 features bits...
> > 
> > Thoughts?
> > Rusty.
> 
> What if we create a capability list but place it in the virtio-pci
> config space instead of the PCI space?

Pls note that virtio-pci config space is io so it is very constrained,
we do need to pack it densely.  If we want to add a lot of stuff there
we probably should move it to memory space. It's slower than io
on kvm, but most uses of it aren't on data path.

> It'll work fine with non-PCI users and would leave MSI-X as the only
> thing that changes offsets (and we could probably deprecate and remove
> it at some point in the future).
> 
> -- 
> 
> Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Mon, Sep 19, 2011 at 05:19:49PM +0930, Rusty Russell wrote:
> On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote:
> > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> > > > > Maybe this is better solved by copying the way it was done in PCI itself
> > > > > with capability linked list?
> > > > 
> > > > There are any number of ways to lay out the structure.  I went for what
> > > > seemed a simplest one.  For MSI-X the train has left the station.  We
> > > > can probably still tweak where the high 32 bit features
> > > > for 64 bit features are.  No idea if it's worth it.
> > > 
> > > Sorry, this has been in the back of my mind.  I think it's a good idea;
> > > can we use the capability linked list for pre-device specific stuff from
> > > now on?
> > > 
> > > Thanks,
> > > Rusty.
> > 
> > Do we even want capability bits then?
> > We can give each capability an ack flag ...
> 
> We could have, and if I'd known PCI when I designed virtio I might have.
> 
> But it's not easy now to map structure offsets to that scheme, and we
> can't really force such a change on the non-PCI users.  So I'd say we
> should only do it for the non-device-specific options.  ie. we'll still
> have the MSI-X case move the device-specific config, but we'll use a
> linked list from now on, eg. for the next 32 features bits...
> 
> Thoughts?
> Rusty.

So I thought about this some more. It probably makes sense to
stop adding info in the io space, and start adding new stuff in
memory space which is much less contrained.

As we need to keep compatibility, and also because memory access is
much slower with kvm so we prefer io for datapath, we could have the
first X bytes still mirrored in io space.

MSIX is there, and it's pointed to from config space,
so we could just put our stuff at offset 0.

Or if we wanted a lot of flexibility, we could add pointers, in config
space, to device specific and non device specific regions in memory
space.

Makes sense?

-- 
MST

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Sun, 2 Oct 2011 11:09:00 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Sep 19, 2011 at 05:19:49PM +0930, Rusty Russell wrote:
> > On Mon, 19 Sep 2011 09:01:50 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Mon, Sep 19, 2011 at 01:05:17PM +0930, Rusty Russell wrote:
> > > > On Sat, 20 Aug 2011 23:00:44 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> > > > > > Maybe this is better solved by copying the way it was done in PCI itself
> > > > > > with capability linked list?
> > > > > 
> > > > > There are any number of ways to lay out the structure.  I went for what
> > > > > seemed a simplest one.  For MSI-X the train has left the station.  We
> > > > > can probably still tweak where the high 32 bit features
> > > > > for 64 bit features are.  No idea if it's worth it.
> > > > 
> > > > Sorry, this has been in the back of my mind.  I think it's a good idea;
> > > > can we use the capability linked list for pre-device specific stuff from
> > > > now on?
> > > > 
> > > > Thanks,
> > > > Rusty.
> > > 
> > > Do we even want capability bits then?
> > > We can give each capability an ack flag ...
> > 
> > We could have, and if I'd known PCI when I designed virtio I might have.
> > 
> > But it's not easy now to map structure offsets to that scheme, and we
> > can't really force such a change on the non-PCI users.  So I'd say we
> > should only do it for the non-device-specific options.  ie. we'll still
> > have the MSI-X case move the device-specific config, but we'll use a
> > linked list from now on, eg. for the next 32 features bits...
> > 
> > Thoughts?
> > Rusty.
> 
> So I thought about this some more. It probably makes sense to
> stop adding info in the io space, and start adding new stuff in
> memory space which is much less contrained.
> 
> As we need to keep compatibility, and also because memory access is
> much slower with kvm so we prefer io for datapath, we could have the
> first X bytes still mirrored in io space.
> 
> MSIX is there, and it's pointed to from config space,
> so we could just put our stuff at offset 0.
> 
> Or if we wanted a lot of flexibility, we could add pointers, in config
> space, to device specific and non device specific regions in memory
> space.
> 
> Makes sense?

I've cc'd David Gibson who has noted in the past that I/O space on PPC
is icky.

I think a linked list for future virtio-pci extensions makes sense (not
device-specific stuff, that's great as a simple structure).  I think
mirroring everything in mem space makes sense, too.

Thanks,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Fri, Aug 19, 2011 at 07:33:07PM +0300, Sasha Levin wrote:
> On Fri, 2011-08-19 at 18:23 +0300, Michael S. Tsirkin wrote:
> > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> > > 
> > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > MSI-X, which means that the read was always made from offset 20 regardless
> > > of whether MSI-X in enabled or not.
> > > 
> > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > supports MSI-X.
> > > 
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: netdev@vger.kernel.org
> > > Cc: kvm@vger.kernel.org
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > 
> > I am not sure I see a bug in virtio: the config pace layout simply
> > changes as msix is enabled and disabled (and if you look at the latest
> > draft, also on whether 64 bit features are enabled).
> > It doesn't depend on msix capability being present in device.
> > 
> > The spec seems to be explicit enough:
> > 	If MSI-X is enabled for the device, two additional fields immediately
> > 	follow this header.
> > 
> > So I'm guessing the bug is in kvm tools which assume
> > same layout for when msix is enabled and disabled.
> > qemu-kvm seems to do the right thing so the device
> > seems to get the correct mac.
> 
> We assumed that PCI config space has a static layout like most other
> devices. Having a behavior of "First bit 20 does something, but after
> enabling MSI-X it does something completely different" sounds strange.

The layout is always virtio header followed by device specific header.
We started with a small header so when more data was added, we could not
extend the header unconditionally.

We can't change that behaviour for MSI-X now, guests and
hosts rely on it.

>
> I'm wondering why offsets of the config structure change during run time
> and are not statically defined when the device is started.

That's because of backwards compatibility with old guests.
When we know the guest is new, we expose new layout,
but old guests must see old layout.

> It's not like VIRTIO_F_FEATURES_HI can be disabled after it was enabled,

Yes it can, e.g. at guest reset. Generally features can be tweaked
any way guest likes until status is set to OK.

> or MSI-X can be simply disabled during run time.

Not sure what you mean by 'run time'. Guest can reset
or disable the device, change any parameters,
then re-enable.

> Maybe this is better solved by copying the way it was done in PCI itself
> with capability linked list?
> 
> -- 
> 
> Sasha.

There are any number of ways to lay out the structure.  I went for what
seemed a simplest one.  For MSI-X the train has left the station.  We
can probably still tweak where the high 32 bit features
for 64 bit features are.  No idea if it's worth it.


-- 
MST

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > The MAC of a virtio-net device is located at the first field of the device
> > specific header. This header is located at offset 20 if the device doesn't
> > support MSI-X or offset 24 if it does.
> > 
> > Current code in virtnet_probe() used to probe the MAC before checking for
> > MSI-X, which means that the read was always made from offset 20 regardless
> > of whether MSI-X in enabled or not.
> > 
> > This patch moves the MAC probe to after the detection of whether MSI-X is
> > enabled. This way the MAC will be read from offset 24 if the device indeed
> > supports MSI-X.
> > 
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: netdev@vger.kernel.org
> > Cc: kvm@vger.kernel.org
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> I am not sure I see a bug in virtio: the config pace layout simply
> changes as msix is enabled and disabled (and if you look at the latest
> draft, also on whether 64 bit features are enabled).
> It doesn't depend on msix capability being present in device.
> 
> The spec seems to be explicit enough:
> 	If MSI-X is enabled for the device, two additional fields immediately
> 	follow this header.
> 
> So I'm guessing the bug is in kvm tools which assume
> same layout for when msix is enabled and disabled.
> qemu-kvm seems to do the right thing so the device
> seems to get the correct mac.

So, the config space moves once MSI-X is enabled?  In which case, it
should say "ONCE MSI-X is enabled..."

Thanks,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Mon, Aug 22, 2011 at 09:54:50AM +0930, Rusty Russell wrote:
> On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> > > 
> > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > MSI-X, which means that the read was always made from offset 20 regardless
> > > of whether MSI-X in enabled or not.
> > > 
> > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > supports MSI-X.
> > > 
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: netdev@vger.kernel.org
> > > Cc: kvm@vger.kernel.org
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > 
> > I am not sure I see a bug in virtio: the config pace layout simply
> > changes as msix is enabled and disabled (and if you look at the latest
> > draft, also on whether 64 bit features are enabled).
> > It doesn't depend on msix capability being present in device.
> > 
> > The spec seems to be explicit enough:
> > 	If MSI-X is enabled for the device, two additional fields immediately
> > 	follow this header.
> > 
> > So I'm guessing the bug is in kvm tools which assume
> > same layout for when msix is enabled and disabled.
> > qemu-kvm seems to do the right thing so the device
> > seems to get the correct mac.
> 
> So, the config space moves once MSI-X is enabled?  In which case, it
> should say "ONCE MSI-X is enabled..."
> 
> Thanks,
> Rusty.

Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back.
Let's update the spec to make it clearer?

-- 
MST

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Michael S. Tsirkin]

On Mon, Aug 22, 2011 at 09:54:50AM +0930, Rusty Russell wrote:
> On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > > The MAC of a virtio-net device is located at the first field of the device
> > > specific header. This header is located at offset 20 if the device doesn't
> > > support MSI-X or offset 24 if it does.
> > > 
> > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > MSI-X, which means that the read was always made from offset 20 regardless
> > > of whether MSI-X in enabled or not.
> > > 
> > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > supports MSI-X.
> > > 
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: virtualization@lists.linux-foundation.org
> > > Cc: netdev@vger.kernel.org
> > > Cc: kvm@vger.kernel.org
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > 
> > I am not sure I see a bug in virtio: the config pace layout simply
> > changes as msix is enabled and disabled (and if you look at the latest
> > draft, also on whether 64 bit features are enabled).
> > It doesn't depend on msix capability being present in device.
> > 
> > The spec seems to be explicit enough:
> > 	If MSI-X is enabled for the device, two additional fields immediately
> > 	follow this header.
> > 
> > So I'm guessing the bug is in kvm tools which assume
> > same layout for when msix is enabled and disabled.
> > qemu-kvm seems to do the right thing so the device
> > seems to get the correct mac.
> 
> So, the config space moves once MSI-X is enabled?  In which case, it
> should say "ONCE MSI-X is enabled..."
> 
> Thanks,
> Rusty.

Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back.
Let's update the spec to make it clearer?

-- 
MST

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Mon, 22 Aug 2011 11:36:13 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back.
> Let's update the spec to make it clearer?

I left the language as is, but added a footnote at the end of the
sentence:

If MSI-X is enabled for the device, two additional fields immediately
follow this header:
[footnote: ie. once you enable MSI-X on the device, the other
           fields move. If you turn it off again, they move back!]

Thanks,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Mon, 22 Aug 2011 11:36:13 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back.
> Let's update the spec to make it clearer?

I left the language as is, but added a footnote at the end of the
sentence:

If MSI-X is enabled for the device, two additional fields immediately
follow this header:
[footnote: ie. once you enable MSI-X on the device, the other
           fields move. If you turn it off again, they move back!]

Thanks,
Rusty.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

I'm wondering if we can switch to using a linked list 'capabilities'
structure similar to whats being done with PCI capabilities.

Here are the pros and the cons as I see them:

Pros:
 * Simpler code - currently before each access to the virtio config
space we have to check whether MSI-X is on and whether the device has
64bit features. This isn't necessarily slow, but it complicates the
code.

 * Future proof - code will be a mess once 5-6 features can change the
config space.

 * 'Concept reuse' - using same concepts in virtio-pci as the ones used
in PCI ('PCI Capabilities list') would make it easier to understand, and
would implement the same method to use optional features as in the layer
above.


Cons:
 * MSI-X is already moving the config space, we'll need to keep
supporting it for a while, but it would mean that it's the only thing we
need to keep backwards compatible.

 * 64bit features also move config space, but they're brand new in the
spec and aren't implemented in the kernel yet - I doubt anyone
implemented it anywhere else yet.

On Mon, 2011-08-22 at 11:36 +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 22, 2011 at 09:54:50AM +0930, Rusty Russell wrote:
> > On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > > > The MAC of a virtio-net device is located at the first field of the device
> > > > specific header. This header is located at offset 20 if the device doesn't
> > > > support MSI-X or offset 24 if it does.
> > > > 
> > > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > > MSI-X, which means that the read was always made from offset 20 regardless
> > > > of whether MSI-X in enabled or not.
> > > > 
> > > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > > supports MSI-X.
> > > > 
> > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: virtualization@lists.linux-foundation.org
> > > > Cc: netdev@vger.kernel.org
> > > > Cc: kvm@vger.kernel.org
> > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > 
> > > I am not sure I see a bug in virtio: the config pace layout simply
> > > changes as msix is enabled and disabled (and if you look at the latest
> > > draft, also on whether 64 bit features are enabled).
> > > It doesn't depend on msix capability being present in device.
> > > 
> > > The spec seems to be explicit enough:
> > > 	If MSI-X is enabled for the device, two additional fields immediately
> > > 	follow this header.
> > > 
> > > So I'm guessing the bug is in kvm tools which assume
> > > same layout for when msix is enabled and disabled.
> > > qemu-kvm seems to do the right thing so the device
> > > seems to get the correct mac.
> > 
> > So, the config space moves once MSI-X is enabled?  In which case, it
> > should say "ONCE MSI-X is enabled..."
> > 
> > Thanks,
> > Rusty.
> 
> Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back.
> Let's update the spec to make it clearer?
> 

-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

I'm wondering if we can switch to using a linked list 'capabilities'
structure similar to whats being done with PCI capabilities.

Here are the pros and the cons as I see them:

Pros:
 * Simpler code - currently before each access to the virtio config
space we have to check whether MSI-X is on and whether the device has
64bit features. This isn't necessarily slow, but it complicates the
code.

 * Future proof - code will be a mess once 5-6 features can change the
config space.

 * 'Concept reuse' - using same concepts in virtio-pci as the ones used
in PCI ('PCI Capabilities list') would make it easier to understand, and
would implement the same method to use optional features as in the layer
above.


Cons:
 * MSI-X is already moving the config space, we'll need to keep
supporting it for a while, but it would mean that it's the only thing we
need to keep backwards compatible.

 * 64bit features also move config space, but they're brand new in the
spec and aren't implemented in the kernel yet - I doubt anyone
implemented it anywhere else yet.

On Mon, 2011-08-22 at 11:36 +0300, Michael S. Tsirkin wrote:
> On Mon, Aug 22, 2011 at 09:54:50AM +0930, Rusty Russell wrote:
> > On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > > > The MAC of a virtio-net device is located at the first field of the device
> > > > specific header. This header is located at offset 20 if the device doesn't
> > > > support MSI-X or offset 24 if it does.
> > > > 
> > > > Current code in virtnet_probe() used to probe the MAC before checking for
> > > > MSI-X, which means that the read was always made from offset 20 regardless
> > > > of whether MSI-X in enabled or not.
> > > > 
> > > > This patch moves the MAC probe to after the detection of whether MSI-X is
> > > > enabled. This way the MAC will be read from offset 24 if the device indeed
> > > > supports MSI-X.
> > > > 
> > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: virtualization@lists.linux-foundation.org
> > > > Cc: netdev@vger.kernel.org
> > > > Cc: kvm@vger.kernel.org
> > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > 
> > > I am not sure I see a bug in virtio: the config pace layout simply
> > > changes as msix is enabled and disabled (and if you look at the latest
> > > draft, also on whether 64 bit features are enabled).
> > > It doesn't depend on msix capability being present in device.
> > > 
> > > The spec seems to be explicit enough:
> > > 	If MSI-X is enabled for the device, two additional fields immediately
> > > 	follow this header.
> > > 
> > > So I'm guessing the bug is in kvm tools which assume
> > > same layout for when msix is enabled and disabled.
> > > qemu-kvm seems to do the right thing so the device
> > > seems to get the correct mac.
> > 
> > So, the config space moves once MSI-X is enabled?  In which case, it
> > should say "ONCE MSI-X is enabled..."
> > 
> > Thanks,
> > Rusty.
> 
> Yes. Or maybe 'WHEN' - since if MSI-X is disabled again, it moves back.
> Let's update the spec to make it clearer?
> 

-- 

Sasha.

Re: [PATCH] virtio-net: Read MAC only after initializing MSI-X

[Rusty Russell]

On Fri, 19 Aug 2011 18:23:35 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sat, Aug 13, 2011 at 11:51:01AM +0300, Sasha Levin wrote:
> > The MAC of a virtio-net device is located at the first field of the device
> > specific header. This header is located at offset 20 if the device doesn't
> > support MSI-X or offset 24 if it does.
> > 
> > Current code in virtnet_probe() used to probe the MAC before checking for
> > MSI-X, which means that the read was always made from offset 20 regardless
> > of whether MSI-X in enabled or not.
> > 
> > This patch moves the MAC probe to after the detection of whether MSI-X is
> > enabled. This way the MAC will be read from offset 24 if the device indeed
> > supports MSI-X.
> > 
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Cc: netdev@vger.kernel.org
> > Cc: kvm@vger.kernel.org
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> I am not sure I see a bug in virtio: the config pace layout simply
> changes as msix is enabled and disabled (and if you look at the latest
> draft, also on whether 64 bit features are enabled).
> It doesn't depend on msix capability being present in device.
> 
> The spec seems to be explicit enough:
> 	If MSI-X is enabled for the device, two additional fields immediately
> 	follow this header.
> 
> So I'm guessing the bug is in kvm tools which assume
> same layout for when msix is enabled and disabled.
> qemu-kvm seems to do the right thing so the device
> seems to get the correct mac.

So, the config space moves once MSI-X is enabled?  In which case, it
should say "ONCE MSI-X is enabled..."

Thanks,
Rusty.

[PATCH] virtio-net: Read MAC only after initializing MSI-X

[Sasha Levin]

The MAC of a virtio-net device is located at the first field of the device
specific header. This header is located at offset 20 if the device doesn't
support MSI-X or offset 24 if it does.

Current code in virtnet_probe() used to probe the MAC before checking for
MSI-X, which means that the read was always made from offset 20 regardless
of whether MSI-X in enabled or not.

This patch moves the MAC probe to after the detection of whether MSI-X is
enabled. This way the MAC will be read from offset 24 if the device indeed
supports MSI-X.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 drivers/net/virtio_net.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0c7321c..55ccf96 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -981,14 +981,6 @@ static int virtnet_probe(struct virtio_device *vdev)
 		/* (!csum && gso) case will be fixed by register_netdev() */
 	}
 
-	/* Configuration may specify what MAC to use.  Otherwise random. */
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
-		vdev->config->get(vdev,
-				  offsetof(struct virtio_net_config, mac),
-				  dev->dev_addr, dev->addr_len);
-	} else
-		random_ether_addr(dev->dev_addr);
-
 	/* Set up our device-specific information */
 	vi = netdev_priv(dev);
 	netif_napi_add(dev, &vi->napi, virtnet_poll, napi_weight);
@@ -1032,6 +1024,14 @@ static int virtnet_probe(struct virtio_device *vdev)
 			dev->features |= NETIF_F_HW_VLAN_FILTER;
 	}
 
+	/* Configuration may specify what MAC to use.  Otherwise random. */
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_MAC)) {
+		vdev->config->get(vdev,
+				  offsetof(struct virtio_net_config, mac),
+				  dev->dev_addr, dev->addr_len);
+	} else
+		random_ether_addr(dev->dev_addr);
+
 	err = register_netdev(dev);
 	if (err) {
 		pr_debug("virtio_net: registering device failed\n");
-- 
1.7.6