| Message ID | 513621F7.9030403@suse.de |
|---|---|
| State | New |
| Headers | show |
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 :-)
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 :-) > > >
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 :-)
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; }