Re: [PATCH v4 4/4] target/loongarch: Set dest error with error_abort in virt_cpu_irq_init

[bibo mao]

On 2025/3/19 下午2:09, Markus Armbruster wrote:
Bibo Mao <maob...@loongson.cn> writes:

In function virt_cpu_irq_init(), there is notification with ipi and extioi
interrupt controller for cpu creation. Local variable with error type is
used, however there is no check with its return value.

Good catch.

When the first call fails, we pass non-null @err to the second call,
which is wrong.  If that one also fails, it'll likely trip
error_setv()'s assertion.

Here set dest error object with error_abort, rather than local variable, so
application will abort to run if there is error.

Why is failure impossible there?
In plug hanlder of extioi/ipi, there is only warn_report() if object is 
not TYPE_LOONGARCH_CPU, parameter errp is not changed.

With caller funciton virt_cpu_irq_init(), DEVICE(cs) is object with type 
TYPE_LOONGARCH_CPU always, so failure is impossible here.


If failure is impossible, the code before the patch is harmlessly wrong.
yes, it is harmlessly wrong.

Regards
Bibo Mao

If failure is possible, the code before the patch has a crash bug, and
the patch makes it crash harder, i.e. when either call fails instead of
when both fail.

Fixes: 50ebc3fc47fe (hw/intc/loongarch_ipi: Notify ipi object when cpu is 
plugged)
Fixes: 087a23a87c57 (hw/intc/loongarch_extioi: Use cpu plug notification)
Signed-off-by: Bibo Mao <maob...@loongson.cn>
---
  hw/loongarch/virt.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index a9fab39dd8..f10a4704ab 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -327,7 +327,6 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState 
*lvms)
      MachineClass *mc = MACHINE_GET_CLASS(ms);
      const CPUArchIdList *possible_cpus;
      CPUState *cs;
-    Error *err = NULL;
  
      /* cpu nodes */
      possible_cpus = mc->possible_cpu_arch_ids(ms);
@@ -337,8 +336,10 @@ static void virt_cpu_irq_init(LoongArchVirtMachineState 
*lvms)
              continue;
          }
  
-        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs), &err);
-        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs), &err);
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), DEVICE(cs),
+                             &error_abort);
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), DEVICE(cs),
+                             &error_abort);
      }
  }