From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54741) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkCNd-0005Dc-Ff for qemu-devel@nongnu.org; Mon, 20 Apr 2015 10:09:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkCNX-0007mT-PP for qemu-devel@nongnu.org; Mon, 20 Apr 2015 10:08:57 -0400 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:52741) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkCNX-0007m6-Gc for qemu-devel@nongnu.org; Mon, 20 Apr 2015 10:08:51 -0400 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 20 Apr 2015 15:08:49 +0100 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id B1B3C17D8042 for ; Mon, 20 Apr 2015 15:09:25 +0100 (BST) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t3KE8lSB58851562 for ; Mon, 20 Apr 2015 14:08:47 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t3KE8l4q021089 for ; Mon, 20 Apr 2015 10:08:47 -0400 Date: Mon, 20 Apr 2015 16:08:43 +0200 From: Cornelia Huck Message-ID: <20150420160843.4b8cbf1d.cornelia.huck@de.ibm.com> In-Reply-To: <55350004.1090107@linaro.org> References: <1429518001-1040-1-git-send-email-shannon.zhao@linaro.org> <1429518001-1040-2-git-send-email-shannon.zhao@linaro.org> <20150420133203.2fd6caff.cornelia.huck@de.ibm.com> <55350004.1090107@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shannon Zhao Cc: peter.maydell@linaro.org, hangaohuai@huawei.com, mst@redhat.com, peter.huangpeng@huawei.com, qemu-devel@nongnu.org, zhaoshenglong@huawei.com, pbonzini@redhat.com, christoffer.dall@linaro.org On Mon, 20 Apr 2015 21:32:52 +0800 Shannon Zhao wrote: > > > On 2015/4/20 19:32, Cornelia Huck wrote: > > On Mon, 20 Apr 2015 16:20:00 +0800 > > shannon.zhao@linaro.org wrote: > > > >> From: Shannon Zhao > >> > >> Move DEFINE_VIRTIO_NET_FEATURES to the backend virtio-net. > >> The transports just sync the host features from backend. > >> > >> Signed-off-by: Shannon Zhao > >> Signed-off-by: Shannon Zhao > >> --- > >> hw/net/virtio-net.c | 4 ++++ > >> hw/s390x/s390-virtio-bus.c | 1 - > >> hw/s390x/virtio-ccw.c | 1 - > >> hw/virtio/virtio-pci.c | 1 - > >> include/hw/virtio/virtio-net.h | 1 + > >> 5 files changed, 5 insertions(+), 3 deletions(-) > > > > I need the following change to make this work for virtio-ccw: > > > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > > index 2252789..7a2bdff 100644 > > --- a/hw/s390x/virtio-ccw.c > > +++ b/hw/s390x/virtio-ccw.c > > @@ -779,7 +779,6 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp) > > DeviceState *vdev = DEVICE(&dev->vdev); > > Error *err = NULL; > > > > - virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]); > > virtio_net_set_netclient_name(&dev->vdev, qdev->id, > > object_get_typename(OBJECT(qdev))); > > qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus)); > > @@ -790,6 +789,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp) > > } > > > > virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp); > > + virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]); > > } > > > > static void virtio_ccw_net_instance_init(Object *obj) > > > > host_features used to be statically populated, so > > virtio_net_set_config_size() was able to use the various feature bits > > for its decisions. > > > > It does not seem quite right, however, since the set of feature bits > > had not been through virtio-net's ->get_features() routine (or the > > feature bit manipulations in virtio-ccw's realize() routine) - it was > > just good enough. > > > > Maybe the right place for calling set_config_size() would be in a > > virtio-net specific ->plugged() callback? > > > > I'm not sure why virtio-pci works, but they have a different topology > > with pci device and virtio-pci device separate, so it might work out > > there. > > > > The virtio-pci works because it calls device_plugged hook to get the > features and this hook is called before realized function. When calling > virtio_net_set_config_size the features are already synced, so it works. > > I think maybe we should call device_plugged hook to get the features in > virtio-ccw other than get them in realized function. So the virtio-ccw, > virtio-pci and virtio-mmio use same ways. Hmm... isn't ->plugged() called after ->realize()? Maybe I'm just confused, so let's try to understand the callchain :) VirtIONetCcw is realized -> feature bits are used -> embedded VirtIODevice is realized -> VirtioCcwDevice is realized -> features are populated My understanding was that ->plugged() happened after step 3, but re-reading, it might already happen after step 2... very confusing. (This still would be too late for the feature bits, and we don't set up the parent bus before step 2.) virtio-pci might be slightly different due to a different topology, I think. I'm not opposed to moving setting up the features into ->plugged(), but I'm not sure it helps.