[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host
From: |
Anthony Liguori |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features |
Date: |
Thu, 07 Mar 2013 10:03:31 -0600 |
User-agent: |
Notmuch/0.13.2+93~ged93d79 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) |
Christian Borntraeger <address@hidden> 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<address@hidden>
>>
>> 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 <address@hidden>
> Date: Tue Feb 5 17:47:15 2013 -0600
>
> virtio-net: pass host features to virtio_net_init
>
> Signed-off-by: Anthony Liguori <address@hidden>
>
> 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] address@hidden:~/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, Alexander Graf, 2013/03/05
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Christian Borntraeger, 2013/03/05
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Christian Borntraeger, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features,
Anthony Liguori <=
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Christian Borntraeger, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Andreas Färber, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Anthony Liguori, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Michael S. Tsirkin, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Anthony Liguori, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Andreas Färber, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Anthony Liguori, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Alexander Graf, 2013/03/07
- Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features, Anthony Liguori, 2013/03/07