Re: [Qemu-devel] [PATCH RFC] pseries: define coldplugged devices as "configured"

[Laurent Vivier]

On 23/08/2015 21:08, Michael Roth wrote:
> Quoting Laurent Vivier (2015-08-14 02:46:49)
>>
>>
>> On 14/08/2015 07:20, Bharata B Rao wrote:
>>> On Thu, Aug 13, 2015 at 02:53:02PM +0200, Laurent Vivier wrote:
>>>> When a device is hotplugged, attach() sets "configured" to
>>>> false, waiting an action from the OS to configure it and then
>>>> to call ibm,configure-connector. On ibm,configure-connector,
>>>> the hypervisor sets "configured" to true.
>>>>
>>>> In case of coldplugged device, attach() sets "configured" to
>>>> false, but firmware and OS never call the ibm,configure-connector
>>>> in this case, so it remains set to false.
>>>>
>>>> It could be harmless, but when we unplug a device, hypervisor
>>>> waits the device becomes configured because for it, a not configured
>>>> device is a device being configured, so it waits the end of configuration
>>>> to unplug it... and it never happens, so it is never unplugged.
>>>
>>> Not true for at least logical DR device like CPU. I am able to cleanly
>>> unplug a cold plugged CPU in the patchset I posted at:
>>>
>>> https://lists.gnu.org/archive/html/qemu-ppc/2015-08/msg00041.html
>>>
>>> And this is how the state transitions work for cold plugged CPU devices:
>>>
>>> - Cold plugged CPU DRC is explicitly set with allocation_state=USABLE
>>>   and isolation_state=UNISOLATED.
>>> - device_del results in drck->detach() that just returns by setting
>>>   drc->awaiting_release to true.
>>> - Unplug notification is sent to guest.
>>> - Guest comes back with set_indicator RTAS call for setting isolation_state
>>>   to ISOLATED. set_isolation_state() sets drc->configured to false.
>>> - Guest comes back again with set_indicator RTAS call for setting allocation
>>>   state to UNUSABLE. set_allocation_state() finalizes the device removal by
>>>   calling drck->detach()
>>
>> It doesn't work for PCI, because (QEMU 2.4.0):
>>
>> static int set_allocation_state(sPAPRDRConnector *drc,
>>                                 sPAPRDRAllocationState state)
>> ...
>>     if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>> ...
>>             drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
>>                          drc->detach_cb_opaque, NULL);
>> ...
> 
> Ok, that makes sense then:
> 
> the is_configured() checks were added due to a race specifically with
> PCI devices: when we plug the device we hand control over to OS and set
> state to unisolated as a result. The guest assumes 'interactive' hotplug
> where it sets a slot back to isolated and waits for the user to actually
> plug it in. Once plugged in, state is moved back to isolated, and guest
> starts configuring device. We use a flag in guest drmgr invocation to skip
> the wait, but it *still* does the change to isolated state. So there's an
> extra unisolated->isolated->unisolated transition for PCI in guest code.
> 
> Because of that check, if management does a quick device_add+device_del,
> there's a race where we mark the device as awaiting_release as soon as
> the device_del comes in (even though device_add event might still be
> getting processed by guest). That would fine normally, but in this state
> a transition to isolated state results in the device getting immediately
> finalized and then disappearing while the guest is trying to configure
> it, so the extra transition in the PCI case races with device_del.
> 
> The is_configured() check removes that race window, and the check was
> added in set_isolation(). 'logical' resources (lmb/cpu/phb) get
> finalized via set_allocation() however, which is why they didn't appear
> affected by this bug. And from what I can tell, cpu/lmb don't make extra
> 'isolated'/'unallocated' transitions, just the ones at the end unplug,
> so the fact that we're missing the check in set_allocation() shouldn't
> be a problem. Makes sense to set the configured flag appropriately for
> those case as well though for consistency.
> 
> Reviewed-by: Michael Roth <address@hidden>

David or Alex, are you ready to take this patch to your -next branch ?

> 
>>     }
>>
>>> - drck->detach() now calls drc->detach_cb() that truly releases the
>>>   CPU resource by getting rid of vCPU thread in QEMU.
>>
>> Laurent
>>
>