Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Alexander Graf]

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>

This patch breaks virtio-s390:

------------[ cut here ]------------
kernel BUG at 
/usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
illegal operation: 0001 [#1] SMP
Modules linked in: virtio_net(F+) scsi_dh_emc(F) scsi_dh_hp_sw(F) 
scsi_dh_rdac(F) scsi_dh_alua(F) scsi_dh(F) scsi_mod(F) dm_snapshot(F) 
dm_mod(F) ext3(F) mbcache(F) jbd(F)
Supported: No, Unsupported modules are loaded
CPU: 0 Tainted: GF             3.0.58-52-default #1
Process modprobe (pid: 160, task: 0000000005c1a238, ksp: 0000000005c1feb8)
Krnl PSW : 0704200180000000 00000000004172ca (kvm_get+0x86/0x90)
            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:0 CC:2 PM:0 EA:3
Krnl GPRS: 0000000000000000 0000000000000000 0000000000000006 
0000000000000000
            000000000108d710 0000000000000006 0000000007dbf198 
0000000007dbf000
            0000000000001000 0000000007dbf19e 00000000008499d0 
00000000013e6000
            0000000008000000 000003c0004e80e8 000003c0004e6be8 
0000000005c1fb78
Krnl Code: 00000000004172be: a737fff9        brctg    %r3,4172b0
            00000000004172c2: a7f4ffee        brc    15,41729e
            00000000004172c6: a7f40001        brc    15,4172c8
 >00000000004172ca: a7f40000        brc    15,4172ca
            00000000004172ce: d20040001000    mvc    0(1,%r4),0(%r1)
            00000000004172d4: ebaff0680024    stmg    %r10,%r15,104(%r15)
            00000000004172da: c0d00009ca03    larl    %r13,5506e0
            00000000004172e0: a7f13fc0        tmll    %r15,16320
Call Trace:
([<000003c0004e6b54>] virtnet_probe+0x50/0x4f0 [virtio_net])
  [<00000000003a7df0>] virtio_dev_probe+0x16c/0x1c0
  [<00000000003c80ce>] really_probe+0x8a/0x25c
  [<00000000003c8416>] __driver_attach+0xca/0xd0
  [<00000000003c74cc>] bus_for_each_dev+0x80/0xd4
  [<00000000003c6b82>] bus_add_driver+0x22a/0x2f4
  [<00000000003c8c0e>] driver_register+0x9e/0x1a0
  [<00000000001000ba>] do_one_initcall+0x3a/0x170
  [<000000000019e292>] SyS_init_module+0xe6/0x234
  [<00000000004f9c40>] sysc_noemu+0x16/0x1c
  [<000003fffd4a1046>] 0x3fffd4a1046
Last Breaking-Event-Address:
  [<00000000004172c6>] kvm_get+0x82/0x90

---[ end trace 22332d4912971f08 ]---


> ---
>   hw/virtio-net.c | 44 ++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index f1c2884..8f521b3 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -73,8 +73,31 @@ typedef struct VirtIONet
>       int multiqueue;
>       uint16_t max_queues;
>       uint16_t curr_queues;
> +    int config_size;
>   } VirtIONet;
>
> +/*
> + * Calculate the number of bytes up to and including the given 'field' of
> + * 'container'.
> + */
> +#define endof(container, field) \
> +    ((intptr_t)(&(((container *)0)->field)+1))
> +
> +typedef struct VirtIOFeature {
> +    uint32_t flags;
> +    size_t end;
> +} VirtIOFeature;
> +
> +static VirtIOFeature feature_sizes[] = {
> +    {.flags = 1<<  VIRTIO_NET_F_MAC,
> +     .end = endof(struct virtio_net_config, mac)},
> +    {.flags = 1<<  VIRTIO_NET_F_STATUS,
> +     .end = endof(struct virtio_net_config, status)},
> +    {.flags = 1<<  VIRTIO_NET_F_MQ,
> +     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
> +    {}
> +};
> +
>   static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
>   {
>       VirtIONet *n = qemu_get_nic_opaque(nc);
> @@ -104,15 +127,15 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>       stw_p(&netcfg.status, n->status);
>       stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
>       memcpy(netcfg.mac, n->mac, ETH_ALEN);
> -    memcpy(config,&netcfg, sizeof(netcfg));
> +    memcpy(config,&netcfg, n->config_size);
>   }
>
>   static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>   {
>       VirtIONet *n = to_virtio_net(vdev);
> -    struct virtio_net_config netcfg;
> +    struct virtio_net_config netcfg = {};
>
> -    memcpy(&netcfg, config, sizeof(netcfg));
> +    memcpy(&netcfg, config, n->config_size);
>
>       if (!(n->vdev.guest_features>>  VIRTIO_NET_F_CTRL_MAC_ADDR&  1)&&
>           memcmp(netcfg.mac, n->mac, ETH_ALEN)) {
> @@ -1279,16 +1302,21 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>   }
>
>   VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
> -                              virtio_net_conf *net,
> -                              uint32_t host_features)
> +                              virtio_net_conf *net, uint32_t host_features)
>   {
>       VirtIONet *n;
> -    int i;
> +    int i, config_size = 0;
> +
> +    for (i = 0; feature_sizes[i].flags != 0; i++) {
> +        if (host_features&  feature_sizes[i].flags) {
> +            config_size = MAX(feature_sizes[i].end, config_size);
> +        }
> +    }

Because down here, host_features doesn't include any features yet. They 
only get set later by calling get_config(0) ...

>
>       n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
> -                                        sizeof(struct virtio_net_config),
> -                                        sizeof(VirtIONet));
> +                                        config_size, sizeof(VirtIONet));
>
> +    n->config_size = config_size;
>       n->vdev.get_config = virtio_net_get_config;

... which only works after get_config got set.


Alex

>       n->vdev.set_config = virtio_net_set_config;
>       n->vdev.get_features = virtio_net_get_features;

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Alexander Graf]

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;
      }

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Christian Borntraeger]

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 :-)

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Christian Borntraeger]

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 :-)
> 
> 
> 

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Anthony Liguori]

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 :-)

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Christian Borntraeger]

> 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.

There is a  parse error in your statement (error in both virtio-ccw).
Is virtio-ccw ok or not?


At least, this patch seems to work. (That also implies, that a transport
must not hide virtio feature bits).


From: Christian Borntraeger <borntraeger@de.ibm.com>
Date: Thu, 7 Mar 2013 17:21:41 +0100
Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus

Enable all virtio-net features for the legacy s390 virtio bus.
This also fixes
kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/s390-virtio-bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 1200691..a8a8e19 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = {
 
 static Property s390_virtio_net_properties[] = {
     DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
+    DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features),
     DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device,
                        net.txtimer, TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIOS390Device,
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Andreas Färber]

Am 07.03.2013 17:27, schrieb Christian Borntraeger:
>> 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.
> 
> At least, this patch seems to work. (That also implies, that a transport
> must not hide virtio feature bits).

To me it indicates that the use of the old qdev property setters is
hiding errors resulting from trying to set not-existing properties.
If we would set the properties in a way that gets us an Error* on
failure like the object_property_set_*() do, we would notice on machine
creation (or device_add).

Andreas

> 
> 
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> Date: Thu, 7 Mar 2013 17:21:41 +0100
> Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus
> 
> Enable all virtio-net features for the legacy s390 virtio bus.
> This also fixes
> kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/s390-virtio-bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 1200691..a8a8e19 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = {
>  
>  static Property s390_virtio_net_properties[] = {
>      DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features),
>      DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device,
>                         net.txtimer, TX_TIMER_INTERVAL),
>      DEFINE_PROP_INT32("x-txburst", VirtIOS390Device,
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Alexander Graf]

On 07.03.2013, at 17:27, Christian Borntraeger wrote:

>> 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.
> 
> There is a  parse error in your statement (error in both virtio-ccw).
> Is virtio-ccw ok or not?
> 
> 
> At least, this patch seems to work. (That also implies, that a transport
> must not hide virtio feature bits).
> 
> 
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> Date: Thu, 7 Mar 2013 17:21:41 +0100
> Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus
> 
> Enable all virtio-net features for the legacy s390 virtio bus.
> This also fixes
> kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks, applied to s390-next.


Alex

> ---
> hw/s390x/s390-virtio-bus.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 1200691..a8a8e19 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = {
> 
> static Property s390_virtio_net_properties[] = {
>     DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features),
>     DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device,
>                        net.txtimer, TX_TIMER_INTERVAL),
>     DEFINE_PROP_INT32("x-txburst", VirtIOS390Device,
> -- 
> 1.8.0.1
> 

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Anthony Liguori]

Christian Borntraeger <borntraeger@de.ibm.com> writes:

>> 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.
>
> There is a  parse error in your statement (error in both virtio-ccw).
> Is virtio-ccw ok or not?

Sorry, virtio-ccw is okay.

> At least, this patch seems to work. (That also implies, that a transport
> must not hide virtio feature bits).

Looks correct to me.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
>
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> Date: Thu, 7 Mar 2013 17:21:41 +0100
> Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus
>
> Enable all virtio-net features for the legacy s390 virtio bus.
> This also fixes
> kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/s390-virtio-bus.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 1200691..a8a8e19 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = {
>  
>  static Property s390_virtio_net_properties[] = {
>      DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features),
>      DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device,
>                         net.txtimer, TX_TIMER_INTERVAL),
>      DEFINE_PROP_INT32("x-txburst", VirtIOS390Device,
> -- 
> 1.8.0.1

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 07.03.2013 17:27, schrieb Christian Borntraeger:
>>> 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.
>> 
>> At least, this patch seems to work. (That also implies, that a transport
>> must not hide virtio feature bits).
>
> To me it indicates that the use of the old qdev property setters is
> hiding errors resulting from trying to set not-existing properties.
> If we would set the properties in a way that gets us an Error* on
> failure like the object_property_set_*() do, we would notice on machine
> creation (or device_add).

Hrm, I don't understand your statement.

Can you elaborate?

Regards,

Anthony Liguori

>
> Andreas
>
>> 
>> 
>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>> Date: Thu, 7 Mar 2013 17:21:41 +0100
>> Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus
>> 
>> Enable all virtio-net features for the legacy s390 virtio bus.
>> This also fixes
>> kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
>> 
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-bus.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> index 1200691..a8a8e19 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = {
>>  
>>  static Property s390_virtio_net_properties[] = {
>>      DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
>> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features),
>>      DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device,
>>                         net.txtimer, TX_TIMER_INTERVAL),
>>      DEFINE_PROP_INT32("x-txburst", VirtIOS390Device,
>> 
>
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Michael S. Tsirkin]

On Thu, Mar 07, 2013 at 10:44:18AM -0600, Anthony Liguori wrote:
> Andreas Färber <afaerber@suse.de> writes:
> 
> > Am 07.03.2013 17:27, schrieb Christian Borntraeger:
> >>> 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.
> >> 
> >> At least, this patch seems to work. (That also implies, that a transport
> >> must not hide virtio feature bits).
> >
> > To me it indicates that the use of the old qdev property setters is
> > hiding errors resulting from trying to set not-existing properties.
> > If we would set the properties in a way that gets us an Error* on
> > failure like the object_property_set_*() do, we would notice on machine
> > creation (or device_add).
> 
> Hrm, I don't understand your statement.
> 
> Can you elaborate?
> 
> Regards,
> 
> Anthony Liguori


Well to me this indicates that s390 virtio is buggy.
It's not supposed to crash whatever set of features
we specify.

> >
> > Andreas
> >
> >> 
> >> 
> >> From: Christian Borntraeger <borntraeger@de.ibm.com>
> >> Date: Thu, 7 Mar 2013 17:21:41 +0100
> >> Subject: [PATCH] Allow virtio-net features for legacy s390 virtio bus
> >> 
> >> Enable all virtio-net features for the legacy s390 virtio bus.
> >> This also fixes
> >> kernel BUG at /usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
> >> 
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>  hw/s390x/s390-virtio-bus.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> >> index 1200691..a8a8e19 100644
> >> --- a/hw/s390x/s390-virtio-bus.c
> >> +++ b/hw/s390x/s390-virtio-bus.c
> >> @@ -399,6 +399,7 @@ static const VirtIOBindings virtio_s390_bindings = {
> >>  
> >>  static Property s390_virtio_net_properties[] = {
> >>      DEFINE_NIC_PROPERTIES(VirtIOS390Device, nic),
> >> +    DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features),
> >>      DEFINE_PROP_UINT32("x-txtimer", VirtIOS390Device,
> >>                         net.txtimer, TX_TIMER_INTERVAL),
> >>      DEFINE_PROP_INT32("x-txburst", VirtIOS390Device,
> >> 
> >
> >
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Andreas Färber]

Am 07.03.2013 17:44, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 07.03.2013 17:27, schrieb Christian Borntraeger:
>>>> 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.
>>>
>>> At least, this patch seems to work. (That also implies, that a transport
>>> must not hide virtio feature bits).
>>
>> To me it indicates that the use of the old qdev property setters is
>> hiding errors resulting from trying to set not-existing properties.
>> If we would set the properties in a way that gets us an Error* on
>> failure like the object_property_set_*() do, we would notice on machine
>> creation (or device_add).
> 
> Hrm, I don't understand your statement.
> 
> Can you elaborate?

<afaerber> aliguori_, borntraeger added new qdev static properties as
bug fix
<afaerber> aliguori_, I was saying if errors setting such properties
were not silently ignored, we would notice such bugs earlier

I.e., no new field was added, no new value set, so apparently something
somewhere is setting some of these properties that are defined via
virtio-net.h:DEFINE_VIRTIO_NET_FEATURES() using DEFINE_PROP_BIT().

And if code expects these properties to be settable, failing to set them
should be treated as error and an appropriate API for setting individual
bits with Error **errp argument should be provided.

Unfortunately I don't see any property matching Alex' VIRTIO_NET_F_MAC,
so I can't draft a patch for illustration.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Mar 07, 2013 at 10:44:18AM -0600, Anthony Liguori wrote:
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>> > Am 07.03.2013 17:27, schrieb Christian Borntraeger:
>> >>> 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.
>> >> 
>> >> At least, this patch seems to work. (That also implies, that a transport
>> >> must not hide virtio feature bits).
>> >
>> > To me it indicates that the use of the old qdev property setters is
>> > hiding errors resulting from trying to set not-existing properties.
>> > If we would set the properties in a way that gets us an Error* on
>> > failure like the object_property_set_*() do, we would notice on machine
>> > creation (or device_add).
>> 
>> Hrm, I don't understand your statement.
>> 
>> Can you elaborate?
>> 
>> Regards,
>> 
>> Anthony Liguori
>
>
> Well to me this indicates that s390 virtio is buggy.
> It's not supposed to crash whatever set of features
> we specify.

Ack.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 07.03.2013 17:44, schrieb Anthony Liguori:
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>>> Am 07.03.2013 17:27, schrieb Christian Borntraeger:
>>>>> 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.
>>>>
>>>> At least, this patch seems to work. (That also implies, that a transport
>>>> must not hide virtio feature bits).
>>>
>>> To me it indicates that the use of the old qdev property setters is
>>> hiding errors resulting from trying to set not-existing properties.
>>> If we would set the properties in a way that gets us an Error* on
>>> failure like the object_property_set_*() do, we would notice on machine
>>> creation (or device_add).
>> 
>> Hrm, I don't understand your statement.
>> 
>> Can you elaborate?
>
> <afaerber> aliguori_, borntraeger added new qdev static properties as
> bug fix
> <afaerber> aliguori_, I was saying if errors setting such properties
> were not silently ignored, we would notice such bugs earlier
>
> I.e., no new field was added, no new value set, so apparently something
> somewhere is setting some of these properties that are defined via
> virtio-net.h:DEFINE_VIRTIO_NET_FEATURES() using DEFINE_PROP_BIT().

No code is explicitly setting the property.  Each property has a default
value (usually true) and that's how we get the initial non-zero value.

So in the absence of the define, we end up with no properties and no
value for this field.

Regards,

Anthony Liguori
>
> And if code expects these properties to be settable, failing to set them
> should be treated as error and an appropriate API for setting individual
> bits with Error **errp argument should be provided.
>
> Unfortunately I don't see any property matching Alex' VIRTIO_NET_F_MAC,
> so I can't draft a patch for illustration.
>
> Regards,
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg