On 2025/3/21 下午3:21, Markus Armbruster wrote:
bibo mao <maob...@loongson.cn> writes:
+Igor
On 2025/3/21 下午2:47, Markus Armbruster wrote:
Bibo Mao <maob...@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 <maob...@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.
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
