On Wed, Mar 31, 2021 at 11:49:14AM +0200, Igor Mammedov wrote: > On Wed, 31 Mar 2021 10:46:49 +1100 > 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 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. > > Yep, all we need to do is notify QEMU user, so it knows that unplug > has failed. Then It can decide on it's own what to do with it and also when. I'm not sure even that makes sense. I mean in the cases that the guest specifically signals failure, then yes, we should definitely notify the user. But for the cases the timeout was covering, I'm not convinced a notification is useful: we *don't* know the unplug has failed, we only suspect it, and I don't see that qemu really has any more information than the user about what the expected time for an unplug should be. > > > > 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. > > May be we can rename it to be ACPI agnostic (though I'm not sure how renaming > QAPI interfaces should be done (it might upset libvirt for example)). > > (My point was that ACPI spec had already gone through all the trouble defining > status of completion and documenting it. Also libvirt supports this notification. > It looks like worthwhile thing to consider if can somehow reuse it outside of > x86 world) Yeah, that's a fair point. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson