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 = 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? >>> >>> It's not virtio specifically, I'm interested in how this will work for >>> 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? My >> assumption is that the PCI bridge hotplug handler handles this. > > Well given how things work in qemu that's not exactly > the case. See below. > >> 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. > > 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. > >> And we only seem to be sending events in the PCI >> bridge hotplug handlers, so my assumption is that this is fine. > > For core PCI, it's mostly just this line: > > bus->devices[devfn] = 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). > > which makes it accessible to pci config cycles. > > But failover also cares about vfio, which seems to set up > e.g. irqfs on realize. -- Thanks, David / dhildenb