Re: [Qemu-devel] [PATCH v4 04/14] pc: prepare for multi stage hotplug handlers

[David Hildenbrand]

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