From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41906) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkEoD-0005nZ-IA for qemu-devel@nongnu.org; Mon, 20 Apr 2015 12:44:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkEoA-0005Sk-Aa for qemu-devel@nongnu.org; Mon, 20 Apr 2015 12:44:33 -0400 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:35212) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkEoA-0005SJ-1b for qemu-devel@nongnu.org; Mon, 20 Apr 2015 12:44:30 -0400 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 20 Apr 2015 17:44:28 +0100 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 9DCCA1B0804B for ; Mon, 20 Apr 2015 17:45:02 +0100 (BST) Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t3KGiQpg58654810 for ; Mon, 20 Apr 2015 16:44:26 GMT Received: from d06av08.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t3KGiQxZ008011 for ; Mon, 20 Apr 2015 10:44:26 -0600 Date: Mon, 20 Apr 2015 18:44:22 +0200 From: Cornelia Huck Message-ID: <20150420184422.28d66195.cornelia.huck@de.ibm.com> In-Reply-To: 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> <20150420160843.4b8cbf1d.cornelia.huck@de.ibm.com> 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: Peter Maydell Cc: hangaohuai@huawei.com, "Michael S. Tsirkin" , "Huangpeng (Peter)" , QEMU Developers , Shannon Zhao , Shannon Zhao , Paolo Bonzini , Christoffer Dall On Mon, 20 Apr 2015 15:34:06 +0100 Peter Maydell wrote: > On 20 April 2015 at 15:08, Cornelia Huck wrote: > > 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.) > > plugged gets called when the virtio backend device is realized > (from the hw/virtio/virtio.c base class realize method). > For virtio-ccw, your virtio_ccw_net_realize function does this > (by setting the 'realized' property on the backend vdev to true). > Since it does this before it calls virtio_ccw_device_realize() > you get the ordering: > * virtio_ccw_net_realize early stuff > * virtio-net backend realize > * virtio_ccw plugged method called (if you had one) > * virtio_ccw_device_realize called (manually by the subclass) > > That's probably not a very helpful order... Indeed. > > > virtio-pci might be slightly different due to a different topology, I > > think. > > virtio-pci has three differences: > (1) its generic 'virtio_pci_realize' is a method on a common > base class, which then invokes the subclass realize > (rather than having the subclass realize call the parent > realize function as virtio-ccw does) That actually makes a lot of sense. I'll put checking if I can do something similar for virtio-ccw on my todo list. > (2) it implements a plugged method and a lot of work is done there I'm not sure how much we can actually do in a plugged method for virtio-ccw, but it's probably worth checking out. > (3) the virtio_net_pci_realize realizes the backend as its > final action, not in the middle of doing other things > > So the order here is: > * virtio_pci_realize (as base class realize method) > * virtio_net_pci_realize > * virtio-net backend realize > * virtio_pci plugged method called So if the features are propagated in the plugged method, virtio-pci should have the same problem? > > > I'm not opposed to moving setting up the features into ->plugged(), but > > I'm not sure it helps. > > Conceptually I think if you have code which relies on the > backend existing, it is better placed in the plugged() method > rather than trying to implement the realize method as a sort > of two-stage thing with the backend-realize done in the middle > and manual calls from the subclass back into the base class > done afterwards. > > You can probably fix the specific weirdnesses here by > being a bit more careful about what all the > virtio_ccw_net_realize &c functions do before realizing > the backend and what they do afterwards. But it might > be long term cleaner to structure things like virtio-pci. Let me see what makes sense. One of the problems is that we don't have a clean split between the hardware device (along the lines of a pci device) and the virtio proxy - which means that the virtio-ccw realize method does a lot of things that have more to do with channel devices than with virtio. Modelling on the old s390-virtio transport is still another problem, and I don't want to do anything there beyond the minimum changes to make it work.