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

[Anthony Liguori <aliguori@us.ibm.com>]

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