From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40925) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fTLWZ-0008SQ-5b for qemu-devel@nongnu.org; Thu, 14 Jun 2018 02:14:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fTLWV-0002Kf-4t for qemu-devel@nongnu.org; Thu, 14 Jun 2018 02:14:23 -0400 References: <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> <20180613212702-mutt-send-email-mst@kernel.org> <5d03c321-4121-a814-ef4f-c649125c0231@redhat.com> <20180614005745-mutt-send-email-mst@kernel.org> From: David Hildenbrand Message-ID: <1a5dc38f-0bc7-75f6-f68f-d327e18c8934@redhat.com> Date: Thu, 14 Jun 2018 08:14:05 +0200 MIME-Version: 1.0 In-Reply-To: <20180614005745-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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: "Michael S. Tsirkin" Cc: Igor Mammedov , 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 14.06.2018 00:05, Michael S. Tsirkin wrote: > On Wed, Jun 13, 2018 at 09:37:54PM +0200, David Hildenbrand wrote: >>>>> [ ... specific to machine_foo wiring ...] >>>>> >>>>> virtio_mem_plug() { >>>>> [... some machine specific wiring ...] >>>>> >>>>> bus_hotplug_ctrl =3D 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 ent= ities, >>>>> 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 wor= k >>>> for your use case? >>> >>> It's not virtio specifically, I'm interested in how this will work fo= r >>> PCI generally. Right now we call do_pci_register_device which >>> immediately makes it guest visible. >> >> So you're telling me that a PCI device exposes itself to the system in >> pci_qdev_realize() instead of letting a hotplug handler handle that? M= y >> assumption is that the PCI bridge hotplug handler handles this. >=20 > Well given how things work in qemu that's not exactly > the case. See below. >=20 >> What am >> I missing? >> >> I can see that e.g. for a virtio device the realize call chain is >> >> pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize = -> >> virtio_XXX_realize() >> >> If any realization in pci_qdev_realize() fails, we do a >> do_pci_unregister_device(). >> >> So if it is true what you're saying than we're already exposing >> partially realized (and possibly unrealized again) devices via PCI. I >> *guess* because we're holding the iothread mutex this is okay and >> actually not visible. >=20 > For now but we need ability to have separate new commands for > realize and plug, so we will drop the mutex. If you want to actually drop the mutex, I assume that quite some rework will be necessary (not only for this specific PCI hotplug handler case), most probably also in other code parts (to) - all of the hotplug/realize code seems to rely on the mutex being locked and not being dropped temporarily. >=20 >> And we only seem to be sending events in the PCI >> bridge hotplug handlers, so my assumption is that this is fine. >=20 > For core PCI, it's mostly just this line: >=20 > bus->devices[devfn] =3D pci_dev; This should go into the hotplug handler if I am not wrong. From what I learned from Igor, this is a layer violation. Resource assignment should happen during pre_plug / plug. But then you might need a different way to "reserve" a function (if there could be races then with the lock temporarily dropped). >=20 > which makes it accessible to pci config cycles. >=20 > But failover also cares about vfio, which seems to set up > e.g. irqfs on realize. --=20 Thanks, David / dhildenb