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

[Markus Armbruster <armbru@redhat.com>]

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?