Re: [PATCH v6 3/6] hw/loongarch/virt: Fix error handling in cpu unplug

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 21 Mar 2025 15:35:37 +0800
bibo mao <maobibo@loongson.cn> wrote:

> On 2025/3/21 下午3:21, Markus Armbruster wrote:
> > bibo mao <maobibo@loongson.cn> writes:
> >   
> >> +Igor
> >>
> >>
> >> On 2025/3/21 下午2:47, Markus Armbruster wrote:  
> >>> Bibo Mao <maobibo@loongson.cn> writes:
> >>>  
> >>>> In function virt_cpu_unplug(), it will send cpu unplug message to
> >>>> interrupt controller extioi and ipi irqchip. If there is problem in
> >>>> this function, system should continue to run and keep state the same
> >>>> before cpu is removed.
> >>>>
> >>>> If error happends in cpu unplug stage, send cpu plug message to extioi
> >>>> and ipi irqchip to restore to previous stage, and then return immediately.
> >>>>
> >>>> Fixes: 2cd6857f6f5b (hw/loongarch/virt: Implement cpu unplug interface)
> >>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >>>> ---
> >>>>    hw/loongarch/virt.c | 6 ++++++
> >>>>    1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> >>>> index 8563967c8b..503362a69e 100644
> >>>> --- a/hw/loongarch/virt.c
> >>>> +++ b/hw/loongarch/virt.c
> >>>> @@ -958,6 +958,8 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
> >>>>        hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
> >>>>        if (err) {
> >>>>            error_propagate(errp, err);
> >>>> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev,
> >>>> +                             &error_abort);
> >>>>            return;
> >>>>        }
> >>>>    
> >>>> @@ -965,6 +967,10 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
> >>>>        hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
> >>>>        if (err) {
> >>>>            error_propagate(errp, err);
> >>>> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev,
> >>>> +                             &error_abort);
> >>>> +        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev,
> >>>> +                             &error_abort);
> >>>>            return;
> >>>>        }  
> >>>
> >>> virt_cpu_unplug() calls hotplug_handler_unplug() three times to notify
> >>> ipi, extioi, and acpi_get.  If any notification fails, virt_cpu_unplug()
> >>> calls hotplug_handler_plug() to "un-notify" the preceeding ones, if any.
> >>> This must not fail.
> >>>
> >>> virt_cpu_plug() does it the other way round (see previous patch).
> >>>
> >>> So, hotplug_handler_plug() must not fail in virt_cpu_unplug(), yet we
> >>> check for it to fail in virt_cpu_plug().
> >>>
> >>> Can it really fail in virt_cpu_plug()?
> >>>
> >>> If yes, why can't it fail in virt_cpu_unplug()?  
> >> you can check function acpi_cpu_plug_cb()/loongarch_ipi_cpu_plug(), that
> >> is cpuplug callback for acpi_ged and ipi. it will not fail.
> >>
> >> If *virt_cpu_pre_plug()* pass, it will succeed.
> >>
> >> Regards
> >> Bibo Mao
> >>  
> >>>
> >>> Same questions for hotplug_handler_unplug().  
> > 
> > Let me restate my argument.
> > 
> > We call hotplug_handler_plug() on the happy path, and on error recovery
> > paths.  Four cases:
> > 
> > 1. Can fail on the happy path
> > 
> >     Error recovery is required.
> > 
> > 1.1 Can fail on the error recovery path
> > 
> >      Error recovery is required, but broken.
> > 
> > 1.2 Can't fail on the error recovery path
> > 
> >      Error recovery is required and works, but why it works is not
> >      obvious.  Deserves a comment explaining why hotplug_handler_plug()
> >      can't fail here even though it can fail on the happy path next door.
> > 
> > 2. Can't fail on the happy path
> > 
> >     Error recovery is unreachable.
> > 
> > 2.1 Can fail on the error recovery path
> > 
> >      Error recovery is unreachable and broken.  Possibly a time bomb, and
> >      possibly misleading readers.
> > 
> > 2.2 Can't fail on the error recovery path
> > 
> >      Error recovery is unreachable and would work, but why it would work
> >      is again a not obvious.
> > 
> > Which of the four cases is it?  
> By my understanding, it is "2. Can't fail on the happy path",  and Error 
> recovery is unreachable.
> 
> I have said that it is impossible and recovery is only for future use.

_plug() handler can't fail hence error_abort.
the same likely goes for _unplug() though I haven't audited it's usage so I won't bet here.
In cpu/mem cases it shall not fail, but there were cases where device_del bypasses
_unplug_request and calls _unplug directly (and those should be re-checked)

handlers that can fail and require graceful recovery are _pre and _request variants.

wrt: _plug() handler, we shall drop errp argument across the tree so
it won't confuse anyone, smth like this:
   hotplug_handler_plug(otplugHandler *hotplug_dev, DeviceState *dev)
and then fixup callers to do the same.

Bibo,
can you take care of that?


Perhaps also check _unplug path and if it shall not fail either, clean it up as well.

> 
> do you mean recovery should be removed? And directly &error_abort is 
> used in virt_cpu_plug() such as:
> static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>                            DeviceState *dev, Error **errp)
> {
>    if (lvms->ipi) {
>      hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &error_abort);
> 
> Regards
> Bibo Mao
>