Re: [PATCH 0/4] DEVICE_NOT_DELETED/DEVICE_UNPLUG_ERROR QAPI events

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 3/30/21 8:46 PM, David Gibson wrote:
> On Tue, Mar 30, 2021 at 01:28:31AM +0200, Igor Mammedov wrote:
>> On Wed, 24 Mar 2021 16:09:59 -0300
>> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>>
>>> On 3/23/21 10:40 PM, David Gibson wrote:
>>>> On Tue, Mar 23, 2021 at 02:10:22PM -0300, Daniel Henrique Barboza wrote:
>>>>>
>>>>>
>>>>> On 3/22/21 10:12 PM, David Gibson wrote:
>>>>>> On Fri, Mar 12, 2021 at 05:07:36PM -0300, Daniel Henrique Barboza wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> This series adds 2 new QAPI events, DEVICE_NOT_DELETED and
>>>>>>> DEVICE_UNPLUG_ERROR. They were (and are still being) discussed in [1].
>>>>>>>
>>>>>>> Patches 1 and 3 are independent of the ppc patches and can be applied
>>>>>>> separately. Patches 2 and 4 are based on David's ppc-for-6.0 branch and
>>>>>>> are dependent on the QAPI patches.
>>>>>>
>>>>>> Implementation looks fine, but I think there's a bit more to discuss
>>>>>> before we can apply.
>>>>>>
>>>>>> I think it would make sense to re-order this and put UNPLUG_ERROR
>>>>>> first.  Its semantics are clearer, and I think there's a stronger case
>>>>>> for it.
>>>>>
>>>>> Alright
>>>>>   
>>>>>>
>>>>>> I'm a bit less sold on DEVICE_NOT_DELETED, after consideration.  Does
>>>>>> it really tell the user/management anything useful beyond what
>>>>>> receiving neither a DEVICE_DELETED nor a DEVICE_UNPLUG_ERROR does?
>>>>>
>>>>>
>>>>> It informs that the hotunplug operation exceed the timeout that QEMU
>>>>> internals considers adequate, but QEMU can't assert that it was caused
>>>>> by an error or an unexpected delay. The end result is that the device
>>>>> is not going to be deleted from QMP, so DEVICE_NOT_DELETED.
>>>>
>>>> Is it, though?  I mean, it is with this implementation for papr:
>>>> because we clear the unplug_requested flag, even if the guest later
>>>> tries to complete the unplug, it will fail.
>>>>
>>>> But if I understand what Markus was saying correctly, that might not
>>>> be possible for all hotplug systems.  I believe Markus was suggesting
>>>> that DEVICE_NOT_DELETED could just mean that we haven't deleted the
>>>> device yet, but it could still happen later.
>>>>
>>>> And in that case, I'm not yet sold on the value of a message that
>>>> essentially just means "Ayup, still dunno what's happening, sorry".
>>>>    
>>>>> Perhaps we should just be straightforward and create a DEVICE_UNPLUG_TIMEOUT
>>>>> event.
>>>>
>>>> Hm... what if we added a "reason" field to UNPLUG_ERROR.  That could
>>>> be "guest rejected hotplug", or something more specific, in the rare
>>>> case that the guest has a way of signalling something more specific,
>>>> or "timeout" - but the later *only* to be sent in cases where on the
>>>> timeout we're able to block any later completion of the unplug (as we
>>>> can on papr).
>>
>> Is canceling unplug on timeout documented somewhere (like some spec)?
> 
> Uh.. not as such.  In the PAPR model, hotplugs and unplugs are mostly
> guest directed, so the question doesn't really arise.
> 
>> If not it might (theoretically) confuse guest when it tries to unplug
>> after timeout and leave guest in some unexpected state.
> 
> Possible, but probably not that likely.  The mechanism we use to
> "cancel" the hotplugs is that we just fail the hypercalls that the
> guest will need to call to actually complete the hotplug.  We also
> fail those in some other situations, and that seems to work.
> 
> That said, I no longer think this cancelling on timeout is a good
> idea, since it mismatches what happens on other platforms more than I
> think we need to.
> 
> My now preferred approach is to revert the timeout changes, but
> instead allow retries of unplugs to be issued.  I think that's just a
> matter of resending the unplug message to the guest, while making it
> otherwise a no-op on the qemu side.

I used this approach in a patch I sent back in January:

"[PATCH v2 1/1] spapr.c: always pulse guest IRQ in spapr_core_unplug_request()"

https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg04399.html


Let me know and I'll revert the timeout mechanism and re-post this one.
I guess there's still time to make this change in the 6.0.0 window, avoiding
releasing a mechanism we're not happy with.



Thanks,


DHB

> 
>>> I believe that's already covered by the existing API:
>>>
>>>
>>> +# @DEVICE_UNPLUG_ERROR:
>>> +#
>>> +# Emitted when a device hot unplug error occurs.
>>> +#
>>> +# @device: device name
>>> +#
>>> +# @msg: Informative message
>>>
>>> The 'informative message' would be the reason the event occurred. In patch
>>> 4/4, for the memory hotunplug refused by the guest, it is being set as:
>>>
>>>        qapi_error = g_strdup_printf("Memory hotunplug rejected by the guest "
>>>                                     "for device %s", dev->id);
>>>        qapi_event_send_device_unplug_error(dev->id, qapi_error);
>>>
>>>
>>>
>>> We could use the same DEVICE_UNPLUG_ERROR event in the CPU hotunplug timeout
>>> case (currently on patch 2/4) by just changing 'msg', e.g.:
>>>
>>>
>>>        qapi_error = g_strdup_printf("CPU hotunplug timeout for device %s", dev->id);
>>>        qapi_event_send_device_unplug_error(dev->id, qapi_error);
>>>
>>
>> lets make everything support ACPI (just kidding).
> 
> Heh.  If nothing else, doesn't help us with existing guests.
> 
>> maybe we can reuse already existing ACPI_DEVICE_OST instead of DEVICE_UNPLUG_ERROR
>> which sort of does the same thing (and more) but instead of strings uses status codes
>> defined by spec.
> 
> Hmm.  I'm a bit dubious about issuing ACPI messages for a non ACPI
> guest, but maybe that could work.
> 
>> Idea similar to DEVICE_UNPLUG_ERROR was considered back then, but instead of QEMU being
>> a poor translator of status codes to non machine-readable strings we went with
>> exposing well documented status codes to user. This way user can implement
>> specific reactions to particular errors just looking at JSON + spec.
>>
>>> Thanks,
>>>
>>> DHB
>>>
>>>
>>>>
>>>> Thoughs, Markus?
>>>>    
>>>
>>
>