Patchwork [2/3] hw/virtio-net.c: set config size using host features

login
register
mail settings
Submitter Alexander Graf
Date March 5, 2013, 4:48 p.m.
Message ID <513621F7.9030403@suse.de>
Download mbox | patch
Permalink /patch/225087/
State New
Headers show

Comments

Alexander Graf - March 5, 2013, 4:48 p.m.
On 02/06/2013 12:47 AM, Jesse Larrew wrote:
> Currently, the config size for virtio devices is hard coded. When a new
> feature is added that changes the config size, drivers that assume a static
> config size will break. For purposes of backward compatibility, there needs
> to be a way to inform drivers of the config size needed to accommodate the
> set of features enabled.
>
> Signed-off-by: Jesse Larrew<jlarrew@linux.vnet.ibm.com>

The following patch gets my s390 virtio guest working again, but I doubt 
it's the right fix.

What is the expected dependency chain of feature calls?


Alex
Christian Borntraeger - March 5, 2013, 5:03 p.m.
On 05/03/13 17:48, Alexander Graf wrote:
> On 02/06/2013 12:47 AM, Jesse Larrew wrote:
>> Currently, the config size for virtio devices is hard coded. When a new
>> feature is added that changes the config size, drivers that assume a static
>> config size will break. For purposes of backward compatibility, there needs
>> to be a way to inform drivers of the config size needed to accommodate the
>> set of features enabled.
>>
>> Signed-off-by: Jesse Larrew<jlarrew@linux.vnet.ibm.com>
> 
> The following patch gets my s390 virtio guest working again, but I doubt it's the right fix.
> 
> What is the expected dependency chain of feature calls?
> 
> 
> Alex
> 
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 089ed92..81be971 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -154,7 +154,7 @@ static int s390_virtio_net_init(VirtIOS390Device *dev)
>      VirtIODevice *vdev;
> 
>      vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net,
> -                           dev->host_features);
> +                           dev->host_features | (1 << VIRTIO_NET_F_MAC));
>      if (!vdev) {
>          return -1;
>      }
> 
> 

Actually this goes back to

commit 1e89ad5b00ba0426d4e949c9e6ce2926c15b81b7
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Tue Feb 5 17:47:15 2013 -0600

    virtio-net: pass host features to virtio_net_init
    
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

virtio-s390 calls  virtio_net_init  before it actually queries the dev->features into
the host_features. virtio-ccw does the same, but it does not BUG. (Its still wrong IMHO)

Same for virtio-pci:


static int virtio_net_init_pci(PCIDevice *pci_dev)
{
    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
    VirtIODevice *vdev;

    vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, &proxy->net,
                           proxy->host_features);   <--- use host_feature

    vdev->nvectors = proxy->nvectors;
    virtio_init_pci(proxy, vdev);   <------------- actually gets host_feature (!)
[..]


Good that my old rusty virtio-ccw transport has lots of BUG_ONS :-)
Christian Borntraeger - March 7, 2013, 12:28 p.m.
Ping.
Anthony, Jesse,

how is this supposed to work?


Christian
On 05/03/13 18:03, Christian Borntraeger wrote:
> On 05/03/13 17:48, Alexander Graf wrote:
>> On 02/06/2013 12:47 AM, Jesse Larrew wrote:
>>> Currently, the config size for virtio devices is hard coded. When a new
>>> feature is added that changes the config size, drivers that assume a static
>>> config size will break. For purposes of backward compatibility, there needs
>>> to be a way to inform drivers of the config size needed to accommodate the
>>> set of features enabled.
>>>
>>> Signed-off-by: Jesse Larrew<jlarrew@linux.vnet.ibm.com>
>>
>> The following patch gets my s390 virtio guest working again, but I doubt it's the right fix.
>>
>> What is the expected dependency chain of feature calls?
>>
>>
>> Alex
>>
>> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> index 089ed92..81be971 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -154,7 +154,7 @@ static int s390_virtio_net_init(VirtIOS390Device *dev)
>>      VirtIODevice *vdev;
>>
>>      vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net,
>> -                           dev->host_features);
>> +                           dev->host_features | (1 << VIRTIO_NET_F_MAC));
>>      if (!vdev) {
>>          return -1;
>>      }
>>
>>
> 
> Actually this goes back to
> 
> commit 1e89ad5b00ba0426d4e949c9e6ce2926c15b81b7
> Author: Anthony Liguori <aliguori@us.ibm.com>
> Date:   Tue Feb 5 17:47:15 2013 -0600
> 
>     virtio-net: pass host features to virtio_net_init
>     
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> virtio-s390 calls  virtio_net_init  before it actually queries the dev->features into
> the host_features. virtio-ccw does the same, but it does not BUG. (Its still wrong IMHO)
> 
> Same for virtio-pci:
> 
> 
> static int virtio_net_init_pci(PCIDevice *pci_dev)
> {
>     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>     VirtIODevice *vdev;
> 
>     vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, &proxy->net,
>                            proxy->host_features);   <--- use host_feature
> 
>     vdev->nvectors = proxy->nvectors;
>     virtio_init_pci(proxy, vdev);   <------------- actually gets host_feature (!)
> [..]
> 
> 
> Good that my old rusty virtio-ccw transport has lots of BUG_ONS :-)
> 
> 
>
Anthony Liguori - March 7, 2013, 4:03 p.m.
Christian Borntraeger <borntraeger@de.ibm.com> writes:

> On 05/03/13 17:48, Alexander Graf wrote:
>> On 02/06/2013 12:47 AM, Jesse Larrew wrote:
>>> Currently, the config size for virtio devices is hard coded. When a new
>>> feature is added that changes the config size, drivers that assume a static
>>> config size will break. For purposes of backward compatibility, there needs
>>> to be a way to inform drivers of the config size needed to accommodate the
>>> set of features enabled.
>>>
>>> Signed-off-by: Jesse Larrew<jlarrew@linux.vnet.ibm.com>
>> 
>> The following patch gets my s390 virtio guest working again, but I doubt it's the right fix.
>> 
>> What is the expected dependency chain of feature calls?
>> 
>> 
>> Alex
>> 
>> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> index 089ed92..81be971 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -154,7 +154,7 @@ static int s390_virtio_net_init(VirtIOS390Device *dev)
>>      VirtIODevice *vdev;
>> 
>>      vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net,
>> -                           dev->host_features);
>> +                           dev->host_features | (1 << VIRTIO_NET_F_MAC));
>>      if (!vdev) {
>>          return -1;
>>      }
>> 
>> 
>
> Actually this goes back to
>
> commit 1e89ad5b00ba0426d4e949c9e6ce2926c15b81b7
> Author: Anthony Liguori <aliguori@us.ibm.com>
> Date:   Tue Feb 5 17:47:15 2013 -0600
>
>     virtio-net: pass host features to virtio_net_init
>     
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> virtio-s390 calls  virtio_net_init  before it actually queries the dev->features into
> the host_features. virtio-ccw does the same, but it does not BUG. (Its still wrong IMHO)
>
> Same for virtio-pci:
>
>
> static int virtio_net_init_pci(PCIDevice *pci_dev)
> {
>     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>     VirtIODevice *vdev;
>
>     vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, &proxy->net,
>                            proxy->host_features);   <--- use host_feature
>
>     vdev->nvectors = proxy->nvectors;
>     virtio_init_pci(proxy, vdev);   <------------- actually gets
>     host_feature (!)

You're misreading how this works.

Host features are set based on command line arguments.  This is
advertised to the guest.  The vdev->get_config() call then sanitizes
features.  For instance, look at:

static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
{
    VirtIONet *n = to_virtio_net(vdev);
    NetClientState *nc = qemu_get_queue(n->nic);

    features |= (1 << VIRTIO_NET_F_MAC);

    if (!peer_has_vnet_hdr(n)) {
        features &= ~(0x1 << VIRTIO_NET_F_CSUM);
<snip>

This removes the VIRTIO_NET_F_CSUM feature if the peer doesn't support
it.  It's presupposing that the feature bit is set.

It's a bug in both virtio-ccw that features=0 when get_features is
called.  You can also tell this with:

[10:02 AM] anthony@titi:~/git/qemu/hw/s390x$ grep DEFINE_VIRTIO_NET_FEATURES *
virtio-ccw.c:    DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]),

So virtio-s390 is doing it wrong, but virtio-ccw looks like its doing it
right.

Regards,

Anthony Liguori

> [..]
>
>
> Good that my old rusty virtio-ccw transport has lots of BUG_ONS :-)

Patch

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 089ed92..81be971 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -154,7 +154,7 @@  static int s390_virtio_net_init(VirtIOS390Device *dev)
      VirtIODevice *vdev;

      vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net,
-                           dev->host_features);
+                           dev->host_features | (1 << VIRTIO_NET_F_MAC));
      if (!vdev) {
          return -1;
      }