[PATCH v5 0/6] target/loongarch: Fix some issues reported from coccinelle

[PATCH v5 0/6] target/loongarch: Fix some issues reported from coccinelle

[Bibo Mao]

This patch set solves errors reported by coccinelle tool with commands:
  spatch --sp-file scripts/coccinelle/*.cocci --dir target/loongarch/
  spatch --sp-file scripts/coccinelle/*.cocci --dir hw/loongarch/

The main problem is that qemu should fail to run when feature is forced
to enabled however KVM does not support it, rather than report error and
continue to run.

Also there is fixup for cpu plug and unplug. If there is error when cpu
is plug/unplug at runtime,  system should restore to previous state and
continue to run.

---
  v4 ... v5:
    1. Split patch2 in v4 into three small patches, two are fixup for error
       handing when cpu plug/unplug fails so that system can continue to
       run, one is to remove error_propagate() and refresh title.
    2. Refresh changelog in last patch and remove fixes information
       since it is impossible to happen.

  v3 ... v4:
    1. Add missed this cleanup with error and remove some local error
       object.
    2. Replace local error object with error_abort object in
       virt_cpu_irq_init(), since its return value is not checked.

  v2 ... v3:
    1. Add missing modification replacing error_propagate() + error_setg()
      with error_setg().
    2. Some enhancement about error handling, handling error
       symmetrically in many places

  v1 ... v2:
    1. Add fixes tag and change title with fix prefix in patch 1.
    2. Replace error_propagate() with error_setg(), and return directly
       for any error.
---
Bibo Mao (6):
  target/loongarch: Fix error handling of KVM feature checks
  hw/loongarch/virt: Fix error handling in cpu plug
  hw/loongarch/virt: Fix error handling in cpu unplug
  hw/loongarch/virt: Eliminate error_propagate()
  target/loongarch: Remove unnecessary temporary variable assignment
  target/loongarch: Clean up virt_cpu_irq_init() error handling

 hw/loongarch/virt.c               | 60 +++++++++++++++++--------------
 target/loongarch/kvm/kvm.c        |  8 +++--
 target/loongarch/tcg/tlb_helper.c |  5 ++-
 3 files changed, 42 insertions(+), 31 deletions(-)


base-commit: 1dae461a913f9da88df05de6e2020d3134356f2e
-- 
2.39.3

[PATCH v5 1/6] target/loongarch: Fix error handling of KVM feature checks

[Bibo Mao]

For some paravirt KVM features, if user forces to enable it however
KVM does not support, qemu should fail to run and exit immediately,
rather than continue to run. Here set error message and return directly
in function kvm_arch_init_vcpu().

Fixes: 6edd2a9bec90 (target/loongarch/kvm: Implement LoongArch PMU extension)
Fixes: 936c3f4d7916 (target/loongarch: Use auto method with LSX feature)
Fixes: 5e360dabedb1 (target/loongarch: Use auto method with LASX feature)
Fixes: 620d9bd0022e (target/loongarch: Add paravirt ipi feature detection)
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/loongarch/kvm/kvm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c
index 28735c80be..7f63e7c8fe 100644
--- a/target/loongarch/kvm/kvm.c
+++ b/target/loongarch/kvm/kvm.c
@@ -1081,7 +1081,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
     int ret;
     Error *local_err = NULL;
 
-    ret = 0;
     qemu_add_vm_change_state_handler(kvm_loongarch_vm_stage_change, cs);
 
     if (!kvm_get_one_reg(cs, KVM_REG_LOONGARCH_DEBUG_INST, &val)) {
@@ -1091,29 +1090,34 @@ int kvm_arch_init_vcpu(CPUState *cs)
     ret = kvm_cpu_check_lsx(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
     ret = kvm_cpu_check_lasx(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
     ret = kvm_cpu_check_lbt(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
     ret = kvm_cpu_check_pmu(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
     ret = kvm_cpu_check_pv_features(cs, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
+        return ret;
     }
 
-    return ret;
+    return 0;
 }
 
 static bool loongarch_get_lbt(Object *obj, Error **errp)
-- 
2.39.3

[PATCH v5 2/6] hw/loongarch/virt: Fix error handling in cpu plug

[Bibo Mao]

In function virt_cpu_plug(), it will send cpu plug 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
added.

Object cpuslot::cpu is set at last only when there is no any error.
If there is, send cpu unplug message to extioi and ipi irqchip, and then
return immediately.

Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/virt.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index a5840ff968..5118f01e4b 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -981,8 +981,6 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
     Error *err = NULL;
 
-    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
-    cpu_slot->cpu = CPU(dev);
     if (lvms->ipi) {
         hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
         if (err) {
@@ -995,6 +993,10 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
         hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
         if (err) {
             error_propagate(errp, err);
+            if (lvms->ipi) {
+                /* Send unplug message to restore, discard error here */
+                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->ipi), dev, NULL);
+            }
             return;
         }
     }
@@ -1003,9 +1005,20 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
         hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
         if (err) {
             error_propagate(errp, err);
+            if (lvms->ipi) {
+                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->ipi), dev, NULL);
+            }
+
+            if (lvms->extioi) {
+                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->extioi),
+                                       dev, NULL);
+            }
+            return;
         }
     }
 
+    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
+    cpu_slot->cpu = CPU(dev);
     return;
 }
 
-- 
2.39.3

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

[Bibo Mao]

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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 5118f01e4b..8dd5d88c31 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -958,12 +958,16 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
     hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
     if (err) {
         error_propagate(errp, err);
+        /* Send plug message to restore, discard error here */
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, NULL);
         return;
     }
 
     /* Notify acpi ged CPU removed */
     hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
     if (err) {
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, NULL);
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, NULL);
         error_propagate(errp, err);
         return;
     }
-- 
2.39.3

[PATCH v5 4/6] hw/loongarch/virt: Eliminate error_propagate()

[Bibo Mao]

When there is an error, it is put into a local variable and then
propagated to somewhere else. Instead the error can be set right
away, error propagation can be removed.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/virt.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 8dd5d88c31..8a766181ec 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -859,30 +859,29 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
     LoongArchCPU *cpu = LOONGARCH_CPU(dev);
     CPUState *cs = CPU(dev);
     CPUArchId *cpu_slot;
-    Error *err = NULL;
     LoongArchCPUTopo topo;
     int arch_id;
 
     if (lvms->acpi_ged) {
         if ((cpu->thread_id < 0) || (cpu->thread_id >= ms->smp.threads)) {
-            error_setg(&err,
+            error_setg(errp,
                        "Invalid thread-id %u specified, must be in range 1:%u",
                        cpu->thread_id, ms->smp.threads - 1);
-            goto out;
+            return;
         }
 
         if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
-            error_setg(&err,
+            error_setg(errp,
                        "Invalid core-id %u specified, must be in range 1:%u",
                        cpu->core_id, ms->smp.cores - 1);
-            goto out;
+            return;
         }
 
         if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
-            error_setg(&err,
+            error_setg(errp,
                        "Invalid socket-id %u specified, must be in range 1:%u",
                        cpu->socket_id, ms->smp.sockets - 1);
-            goto out;
+            return;
         }
 
         topo.socket_id = cpu->socket_id;
@@ -891,11 +890,11 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
         arch_id =  virt_get_arch_id_from_topo(ms, &topo);
         cpu_slot = virt_find_cpu_slot(ms, arch_id);
         if (CPU(cpu_slot->cpu)) {
-            error_setg(&err,
+            error_setg(errp,
                        "cpu(id%d=%d:%d:%d) with arch-id %" PRIu64 " exists",
                        cs->cpu_index, cpu->socket_id, cpu->core_id,
                        cpu->thread_id, cpu_slot->arch_id);
-            goto out;
+            return;
         }
     } else {
         /* For cold-add cpu, find empty cpu slot */
@@ -911,33 +910,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
     cpu->env.address_space_iocsr = &lvms->as_iocsr;
     cpu->phy_id = cpu_slot->arch_id;
     cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
-    numa_cpu_pre_plug(cpu_slot, dev, &err);
-out:
-    if (err) {
-        error_propagate(errp, err);
-    }
+    numa_cpu_pre_plug(cpu_slot, dev, errp);
 }
 
 static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
                                     DeviceState *dev, Error **errp)
 {
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
-    Error *err = NULL;
     LoongArchCPU *cpu = LOONGARCH_CPU(dev);
     CPUState *cs = CPU(dev);
 
     if (cs->cpu_index == 0) {
-        error_setg(&err, "hot-unplug of boot cpu(id%d=%d:%d:%d) not supported",
+        error_setg(errp, "hot-unplug of boot cpu(id%d=%d:%d:%d) not supported",
                    cs->cpu_index, cpu->socket_id,
                    cpu->core_id, cpu->thread_id);
-        error_propagate(errp, err);
         return;
     }
 
-    hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, errp);
 }
 
 static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
-- 
2.39.3

[PATCH v5 5/6] target/loongarch: Remove unnecessary temporary variable assignment

[Bibo Mao]

Temporary variable ret is assigned at last line and return, it can
be removed and return directly.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/loongarch/tcg/tlb_helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/loongarch/tcg/tlb_helper.c b/target/loongarch/tcg/tlb_helper.c
index 646dbf59de..182881a237 100644
--- a/target/loongarch/tcg/tlb_helper.c
+++ b/target/loongarch/tcg/tlb_helper.c
@@ -543,7 +543,7 @@ target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
                           target_ulong level, uint32_t mem_idx)
 {
     CPUState *cs = env_cpu(env);
-    target_ulong badvaddr, index, phys, ret;
+    target_ulong badvaddr, index, phys;
     uint64_t dir_base, dir_width;
 
     if (unlikely((level == 0) || (level > 4))) {
@@ -571,8 +571,7 @@ target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
     get_dir_base_width(env, &dir_base, &dir_width, level);
     index = (badvaddr >> dir_base) & ((1 << dir_width) - 1);
     phys = base | index << 3;
-    ret = ldq_phys(cs->as, phys) & TARGET_PHYS_MASK;
-    return ret;
+    return ldq_phys(cs->as, phys) & TARGET_PHYS_MASK;
 }
 
 void helper_ldpte(CPULoongArchState *env, target_ulong base, target_ulong odd,
-- 
2.39.3

[PATCH v5 6/6] target/loongarch: Clean up virt_cpu_irq_init() error handling

[Bibo Mao]

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

virt_cpu_irq_init() is wrong that way: it passes &err to
hotplug_handler_plug() twice.  If both calls failed, this could trip
error_setv()'s assertion.  Moreover, if just one fails, the Error
object leaks. Fortunately, these calls can't actually fail.

Messed up in commit 50ebc3fc47f7 (hw/intc/loongarch_ipi: Notify ipi
object when cpu is plugged) and commit 087a23a87c57
(hw/intc/loongarch_extioi: Use cpu plug notification).

Clean this up by passing &error_abort instead.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Acked-by: Markus Armbruster <armbru@redhat.com>
---
 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 8a766181ec..66da6b241f 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);
     }
 }
 
-- 
2.39.3

Re: [PATCH v5 4/6] hw/loongarch/virt: Eliminate error_propagate()

[Markus Armbruster]

Bibo Mao <maobibo@loongson.cn> writes:

> When there is an error, it is put into a local variable and then
> propagated to somewhere else. Instead the error can be set right
> away, error propagation can be removed.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v5 2/6] hw/loongarch/virt: Fix error handling in cpu plug

[Markus Armbruster]

Bibo Mao <maobibo@loongson.cn> writes:

> In function virt_cpu_plug(), it will send cpu plug 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
> added.
>
> Object cpuslot::cpu is set at last only when there is no any error.
> If there is, send cpu unplug message to extioi and ipi irqchip, and then
> return immediately.
>
> Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  hw/loongarch/virt.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index a5840ff968..5118f01e4b 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -981,8 +981,6 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>      LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>      Error *err = NULL;
>  
> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
> -    cpu_slot->cpu = CPU(dev);
>      if (lvms->ipi) {
>          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
>          if (err) {
> @@ -995,6 +993,10 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
>          if (err) {
>              error_propagate(errp, err);
> +            if (lvms->ipi) {
> +                /* Send unplug message to restore, discard error here */
> +                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->ipi), dev, NULL);
> +            }
>              return;
>          }
>      }
> @@ -1003,9 +1005,20 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>          if (err) {
>              error_propagate(errp, err);
> +            if (lvms->ipi) {
> +                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->ipi), dev, NULL);
> +            }
> +
> +            if (lvms->extioi) {
> +                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->extioi),
> +                                       dev, NULL);
> +            }
> +            return;
>          }
>      }
>  
> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
> +    cpu_slot->cpu = CPU(dev);
>      return;
>  }

Hmm.

You're right about the problem: virt_cpu_plug() neglects to revert
changes when it fails.

You're probably right to move the assignment to cpu_slot->cpu to the
end.  Anything you can delay until success is assured you don't have to
revert.  I say "probably" because the code that now runs before the
assignment might theoretically "see" the assignment, and I didn't
examine it to exclude that.

Where I have doubts is the code to revert changes.

The hotplug_handler_plug() error checkign suggests it can fail.

Can hotplug_handler_unplug() fail, too?  The error checking in
virt_cpu_unplug() right above suggests it can.

What happens if it fails in virt_cpu_plug()?

Re: [PATCH v5 2/6] hw/loongarch/virt: Fix error handling in cpu plug

[bibo mao]

On 2025/3/20 下午2:16, Markus Armbruster wrote:
> Bibo Mao <maobibo@loongson.cn> writes:
> 
>> In function virt_cpu_plug(), it will send cpu plug 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
>> added.
>>
>> Object cpuslot::cpu is set at last only when there is no any error.
>> If there is, send cpu unplug message to extioi and ipi irqchip, and then
>> return immediately.
>>
>> Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/loongarch/virt.c | 17 +++++++++++++++--
>>   1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index a5840ff968..5118f01e4b 100644
>> --- a/hw/loongarch/virt.c
>> +++ b/hw/loongarch/virt.c
>> @@ -981,8 +981,6 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>>       Error *err = NULL;
>>   
>> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>> -    cpu_slot->cpu = CPU(dev);
>>       if (lvms->ipi) {
>>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
>>           if (err) {
>> @@ -995,6 +993,10 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
>>           if (err) {
>>               error_propagate(errp, err);
>> +            if (lvms->ipi) {
>> +                /* Send unplug message to restore, discard error here */
>> +                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->ipi), dev, NULL);
>> +            }
>>               return;
>>           }
>>       }
>> @@ -1003,9 +1005,20 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>>           if (err) {
>>               error_propagate(errp, err);
>> +            if (lvms->ipi) {
>> +                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->ipi), dev, NULL);
>> +            }
>> +
>> +            if (lvms->extioi) {
>> +                hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->extioi),
>> +                                       dev, NULL);
>> +            }
>> +            return;
>>           }
>>       }
>>   
>> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>> +    cpu_slot->cpu = CPU(dev);
>>       return;
>>   }
> 
> Hmm.
> 
> You're right about the problem: virt_cpu_plug() neglects to revert
> changes when it fails.
> 
> You're probably right to move the assignment to cpu_slot->cpu to the
> end.  Anything you can delay until success is assured you don't have to
> revert.  I say "probably" because the code that now runs before the
> assignment might theoretically "see" the assignment, and I didn't
> examine it to exclude that.
> 
> Where I have doubts is the code to revert changes.
> 
> The hotplug_handler_plug() error checkign suggests it can fail.
> 
> Can hotplug_handler_unplug() fail, too?  The error checking in
> virt_cpu_unplug() right above suggests it can.
Basically from existing code about ipi/extioi hotplug handler, it is
impossible to there is error, here is only for future use.

If there is error in function virt_cpu_plug(), undo() such as 
hotplug_handler_unplug() should be called. However if undo() reports 
error, I do not know how to handle it, here just discard error in 
function undo().

Regards
Bibo Mao
> 
> What happens if it fails in virt_cpu_plug()?
> 

Re: [PATCH v5 0/6] target/loongarch: Fix some issues reported from coccinelle

[Markus Armbruster]

Bibo Mao <maobibo@loongson.cn> writes:

> This patch set solves errors reported by coccinelle tool with commands:
>   spatch --sp-file scripts/coccinelle/*.cocci --dir target/loongarch/
>   spatch --sp-file scripts/coccinelle/*.cocci --dir hw/loongarch/
>
> The main problem is that qemu should fail to run when feature is forced
> to enabled however KVM does not support it, rather than report error and
> continue to run.
>
> Also there is fixup for cpu plug and unplug. If there is error when cpu
> is plug/unplug at runtime,  system should restore to previous state and
> continue to run.

PACTH 2 and 3 are still being discussed.

Since I already have a few error-handling patches queued up, I'm queuing
the remainder of this series for 10.0.  Thanks!

Re: [PATCH v5 0/6] target/loongarch: Fix some issues reported from coccinelle

[bibo mao]

On 2025/3/20 下午3:01, Markus Armbruster wrote:
> Bibo Mao <maobibo@loongson.cn> writes:
> 
>> This patch set solves errors reported by coccinelle tool with commands:
>>    spatch --sp-file scripts/coccinelle/*.cocci --dir target/loongarch/
>>    spatch --sp-file scripts/coccinelle/*.cocci --dir hw/loongarch/
>>
>> The main problem is that qemu should fail to run when feature is forced
>> to enabled however KVM does not support it, rather than report error and
>> continue to run.
>>
>> Also there is fixup for cpu plug and unplug. If there is error when cpu
>> is plug/unplug at runtime,  system should restore to previous state and
>> continue to run.
> 
> PACTH 2 and 3 are still being discussed.
> 
> Since I already have a few error-handling patches queued up, I'm queuing
> the remainder of this series for 10.0.  Thanks!
Sure, that is ok.
And thanks for taking time to review this.

Regards
Bibo Mao

Re: [PATCH v5 2/6] hw/loongarch/virt: Fix error handling in cpu plug

[Markus Armbruster]

bibo mao <maobibo@loongson.cn> writes:

On 2025/3/20 下午2:16, Markus Armbruster wrote:
>> Bibo Mao <maobibo@loongson.cn> writes:
>> 
>>> In function virt_cpu_plug(), it will send cpu plug 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
>>> added.
>>>
>>> Object cpuslot::cpu is set at last only when there is no any error.
>>> If there is, send cpu unplug message to extioi and ipi irqchip, and then
>>> return immediately.
>>>
>>> Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>

[...]

>> Hmm.
>> 
>> You're right about the problem: virt_cpu_plug() neglects to revert
>> changes when it fails.
>> 
>> You're probably right to move the assignment to cpu_slot->cpu to the
>> end.  Anything you can delay until success is assured you don't have to
>> revert.  I say "probably" because the code that now runs before the
>> assignment might theoretically "see" the assignment, and I didn't
>> examine it to exclude that.
>> 
>> Where I have doubts is the code to revert changes.
>> 
>> The hotplug_handler_plug() error checkign suggests it can fail.
>> 
>> Can hotplug_handler_unplug() fail, too?  The error checking in
>> virt_cpu_unplug() right above suggests it can.
>
> Basically from existing code about ipi/extioi hotplug handler, it is
> impossible to there is error, here is only for future use.

Aha.  More at the end of my reply.

> If there is error in function virt_cpu_plug(), undo() such as 
> hotplug_handler_unplug() should be called. However if undo() reports 
> error, I do not know how to handle it, here just discard error in 
> function undo().

Steinbach's Guideline for Systems Programming: Never test for an error
condition you don't know how to handle.

This old quip is a funny way to say that errors we don't know how to
handle are *bad*, and should be avoided.

> Regards
> Bibo Mao
>> 
>> What happens if it fails in virt_cpu_plug()?

You assure us this can't happen today.  Because of that, broken error
recovery is not an actual problem.

However, if things change some day so it can happen, broken error
recovery becomes an actual problem.

so, broken error recovery just "for future use" is actually just for
silent future breakage.

But is it broken?  This is what I'm trying to find out with my "what
happens if" question.

If it is broken, then passing &error_abort would likely be less bad:
crash instead of silent breakage.  Also makes it completely obvious in
the code that these errors are not handled, whereas broken error
handling looks like it is until you actually think about it.

Re: [PATCH v5 2/6] hw/loongarch/virt: Fix error handling in cpu plug

[bibo mao]

On 2025/3/20 下午3:25, Markus Armbruster wrote:
> bibo mao <maobibo@loongson.cn> writes:
> 
> On 2025/3/20 下午2:16, Markus Armbruster wrote:
>>> Bibo Mao <maobibo@loongson.cn> writes:
>>>
>>>> In function virt_cpu_plug(), it will send cpu plug 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
>>>> added.
>>>>
>>>> Object cpuslot::cpu is set at last only when there is no any error.
>>>> If there is, send cpu unplug message to extioi and ipi irqchip, and then
>>>> return immediately.
>>>>
>>>> Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> 
> [...]
> 
>>> Hmm.
>>>
>>> You're right about the problem: virt_cpu_plug() neglects to revert
>>> changes when it fails.
>>>
>>> You're probably right to move the assignment to cpu_slot->cpu to the
>>> end.  Anything you can delay until success is assured you don't have to
>>> revert.  I say "probably" because the code that now runs before the
>>> assignment might theoretically "see" the assignment, and I didn't
>>> examine it to exclude that.
>>>
>>> Where I have doubts is the code to revert changes.
>>>
>>> The hotplug_handler_plug() error checkign suggests it can fail.
>>>
>>> Can hotplug_handler_unplug() fail, too?  The error checking in
>>> virt_cpu_unplug() right above suggests it can.
>>
>> Basically from existing code about ipi/extioi hotplug handler, it is
>> impossible to there is error, here is only for future use.
> 
> Aha.  More at the end of my reply.
> 
>> If there is error in function virt_cpu_plug(), undo() such as
>> hotplug_handler_unplug() should be called. However if undo() reports
>> error, I do not know how to handle it, here just discard error in
>> function undo().
> 
> Steinbach's Guideline for Systems Programming: Never test for an error
> condition you don't know how to handle.
> 
> This old quip is a funny way to say that errors we don't know how to
> handle are *bad*, and should be avoided.
> 
>> Regards
>> Bibo Mao
>>>
>>> What happens if it fails in virt_cpu_plug()?
> 
> You assure us this can't happen today.  Because of that, broken error
> recovery is not an actual problem.
> 
> However, if things change some day so it can happen, broken error
> recovery becomes an actual problem.
> 
> so, broken error recovery just "for future use" is actually just for
> silent future breakage.
> 
> But is it broken?  This is what I'm trying to find out with my "what
> happens if" question.
> 
> If it is broken, then passing &error_abort would likely be less bad:
> crash instead of silent breakage.  Also makes it completely obvious in
> the code that these errors are not handled, whereas broken error
> handling looks like it is until you actually think about it.
> 
yes, it seems that &error_abort is better than NULL, it is better than 
slient and do nothing. If error really happens, we had to solve it then.

And I will refresh the patch in next version.

Regards
Bibo Mao