From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51570) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDdIX-0003hw-KP for qemu-devel@nongnu.org; Thu, 07 Mar 2013 11:04:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDdIS-0003yc-KF for qemu-devel@nongnu.org; Thu, 07 Mar 2013 11:04:01 -0500 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:50233) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDdIS-0003wf-2W for qemu-devel@nongnu.org; Thu, 07 Mar 2013 11:03:56 -0500 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 8 Mar 2013 01:53:50 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 0254F2BB0023 for ; Fri, 8 Mar 2013 03:03:39 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r27Fox8r6291952 for ; Fri, 8 Mar 2013 02:51:01 +1100 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r27G3bT9031534 for ; Fri, 8 Mar 2013 03:03:37 +1100 From: Anthony Liguori In-Reply-To: <51362575.2000908@de.ibm.com> References: <1360108037-9211-1-git-send-email-jlarrew@linux.vnet.ibm.com> <1360108037-9211-3-git-send-email-jlarrew@linux.vnet.ibm.com> <513621F7.9030403@suse.de> <51362575.2000908@de.ibm.com> Date: Thu, 07 Mar 2013 10:03:31 -0600 Message-ID: <87621319kc.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Borntraeger , Alexander Graf Cc: Cornelia Huck , Jens Freimann , Jesse Larrew , qemu-devel@nongnu.org Christian Borntraeger 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 >> >> 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 > Date: Tue Feb 5 17:47:15 2013 -0600 > > virtio-net: pass host features to virtio_net_init > > Signed-off-by: Anthony Liguori > > 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); 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 :-)