From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49426) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fTOQu-0001us-0o for qemu-devel@nongnu.org; Thu, 14 Jun 2018 05:20:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fTOQq-0003Ef-RU for qemu-devel@nongnu.org; Thu, 14 Jun 2018 05:20:43 -0400 Date: Thu, 14 Jun 2018 11:20:28 +0200 From: Igor Mammedov Message-ID: <20180614112028.768e1106@redhat.com> In-Reply-To: References: <20180530150806.061c26b9@redhat.com> <5dcb16e5-b9b9-d07d-dfe3-98724e911f83@redhat.com> <20180531161310.52ea9507@redhat.com> <20180607154455.6eab4e24@redhat.com> <5dda6237-88d8-f0f8-58d2-af996368cea4@redhat.com> <20180608142442.0d97b7c6@redhat.com> <397d37ee-b3a8-dae0-37d1-fd3c942448c2@redhat.com> <20180608155432-mutt-send-email-mst@kernel.org> <20180608181004-mutt-send-email-mst@kernel.org> <20180613174825.1eebef6c@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 04/14] pc: prepare for multi stage hotplug handlers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Hildenbrand Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, qemu-s390x@nongnu.org, Marcel Apfelbaum , Paolo Bonzini , Richard Henderson , Eduardo Habkost , David Gibson , Markus Armbruster , qemu-ppc@nongnu.org, Pankaj Gupta , Alexander Graf , Cornelia Huck , Christian Borntraeger , Luiz Capitulino On Wed, 13 Jun 2018 17:51:01 +0200 David Hildenbrand wrote: > On 13.06.2018 17:48, Igor Mammedov wrote: > > On Wed, 13 Jun 2018 12:58:46 +0200 > > David Hildenbrand wrote: > > > >> On 08.06.2018 17:12, Michael S. Tsirkin wrote: > >>> On Fri, Jun 08, 2018 at 03:07:53PM +0200, David Hildenbrand wrote: > >>>> On 08.06.2018 14:55, Michael S. Tsirkin wrote: > >>>>> On Fri, Jun 08, 2018 at 02:32:09PM +0200, David Hildenbrand wrote: > >>>>>> > >>>>>>>>> if (TYPE_PC_DIMM) { > >>>>>>>>> pc_dimm_plug() > >>>>>>>>> /* do here additional concrete machine specific things */ > >>>>>>>>> } else if (TYPE_VIRTIO_MEM) { > >>>>>>>>> virtio_mem_plug() <- do forwarding in there > >>>>>>>>> /* and do here additional concrete machine specific things */ > >>>>>>>>> } else if (TYPE_CPU) { > >>>>>>>>> cpu_plug() > >>>>>>>>> /* do here additional concrete machine specific things */ > >>>>>>>>> } > >>>>>>>> > >>>>>>>> That will result in a lot of duplicate code - for every machine we > >>>>>>>> support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - > >>>>>>>> virtio-mem and virtio-pmem could most probably share the code. > >>>>>>> maybe or maybe not, depending on if pmem endups as memory device or > >>>>>>> separate controller. And even with duplication, machine code would > >>>>>>> be easy to follow just down one explicit call chain. > >>>>>> > >>>>>> Not 100% convinced but I am now going into that direction. > >>>>> > >>>>> Can this go into DeviceClass? Failover has the same need to > >>>>> allocate/free resources for vfio without a full realize/unrealize. > >>>>> > >>>> > >>>> Conceptually, I would have called here something like > >>>> > >>>> virtio_mem_plug() ... > >>>> > >>>> Which would end up calling memory_device_plug() and triggering the > >>>> target hotplug handler. > >>>> > >>>> I assume this can also be done from a device class callback. > >>>> > >>>> So we would need a total of 3 callbacks for > >>>> > >>>> a) pre_plug > >>>> b) plug > >>>> c) unplug > >>>> > >>>> In addition, unplug requests might be necessary, so > >>>> > >>>> d) unplug_request > >>> > >>> > >>> Right - basically HotplugHandlerClass. > >> > >> Looking into this idea: > >> > >> What I would have right now (conceptually) > >> > >> if (TYPE_PC_DIMM) { > >> pc_dimm_plug(machine); > >> } else if (TYPE_CPU) { > >> cpu_plug(machine); > >> } else if (TYPE_VIRTIO_MEM) { > >> virtio_mem_plug(machine); > >> } > >> > >> Instead you want something like: > >> > >> if (TYPE_PC_DIMM) { > >> pc_dimm_plug(machine); > >> } else if (TYPE_CPU) { > >> cpu_plug(machine); > >> // igor requested an explicit list here, we could also check for > >> // DEVICE_CLASS(device)->plug and make it generic > >> } else if (TYPE_VIRTIO_MEM) { > >> DEVICE_CLASS(device)->plug(machine); > >> // call bus hotplug handler if necessary, or let the previous call > >> // handle it? > > not exactly this, I suggested following: > > > > [ ... specific to machine_foo wiring ...] > > > > virtio_mem_plug() { > > [... some machine specific wiring ...] > > > > bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() > > bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev) > > > > [... some more machine specific wiring ...] > > } > > > > [ ... specific to machine_foo wiring ...] > > > > i.e. device itself doesn't participate in attaching to external entities, > > those entities (machine or bus controller virtio device is attached to) > > do wiring on their own within their own domain. > > I am fine with this, but Michael asked if this ("virtio_mem_plug()") > could go via new DeviceClass functions. Michael, would that also work > for your use case? We can discuss DeviceClass functions when patches for failover surface and if it's really need.