Re: [qemu-s390x] [Qemu-devel] [PATCH v2 4/6] s390x/pci: Introduce unplug requests and split unplug handler

[David Hildenbrand]

On 31.01.19 21:40, Collin Walling wrote:
> On 1/30/19 10:57 AM, David Hildenbrand wrote:
>> PCI on s390x is really weird and how it was modeled in QEMU might not have
>> been the right choice. Anyhow, right now it is the case that:
>> - Hotplugging a PCI device will silently create a zPCI device
>>    (if none is provided)
>> - Hotunplugging a zPCI device will unplug the PCI device (if any)
>> - Hotunplugging a PCI device will unplug also the zPCI device
>> As far as I can see, we can no longer change this behavior. But we
>> should fix it.
>>
>> Both device types are handled via a single hotplug handler call. This
>> is problematic for various reasons:
>> 1. Unplugging via the zPCI device allows to unplug devices that are not
>>     hot removable. (check performed in qdev_unplug()) - bad.
>> 2. Hotplug handler chains are not possible for the unplug case. In the
>>     future, the machine might want to override hotplug handlers, to
>>     process device specific stuff and to then branch off to the actual
>>     hotplug handler. We need separate hotplug handler calls for both the
>>     PCI and zPCI device to make this work reliably. All other PCI
>>     implementations are already prepared to handle this correctly, only
>>     s390x is missing.
>>
>> Therefore, introduce the unplug_request handler and properly perform
>> unplug checks by redirecting to the separate unplug_request handlers.
>> When finally unplugging, perform two separate hotplug_handler_unplug()
>> calls, first for the PCI device, followed by the zPCI device. This now
>> nicely splits unplugging paths for both devices.
>>
>> The redirect part is a little hairy, as the user is allowed to trigger
>> unplug either via the PCI or the zPCI device. So redirect always to the
>> PCI unplug request handler first and remember if that check has been
>> performed in the zPCI device. Redirect then to the zPCI device unplug
>> request handler to perform the magic. Remembering that we already
>> checked the PCI device breaks the redirect loop.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
> 
> Thanks for clearing up some of the areas I was having troubles following
> during the last post. I think this looks a lot better than what we had
> before. Patches 4, 5, and 6 make the code a lot easier to understand
> what is going on here.
> 
> I assume you were able to squeeze in an easy test? At least making sure
> we can still hot unplug a device from a guest successfully?

I only tested under TCG, but I did quite some tests
- Plug/unplug a device
- Disable slot in guest, unplug device
- Plug bridges, try to unplug
- Create hierarchy of bridges (statically and dynamically)
- Plug devices to bridge hierarchies ...

Thanks!

> 
>  From the code perspective:
> 
> Reviewed-by: Collin Walling <address@hidden>
> 


-- 

Thanks,

David / dhildenb