[PATCH v4 0/4] target/loongarch: Solve some issues reported from coccinelle

[PATCH v4 0/4] target/loongarch: Solve 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.

---
  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 (4):
  target/loongarch: Fix error handling of KVM feature checks
  hw/loongarch/virt: Remove unnecessary NULL pointer
  target/loongarch: Remove unnecessary temporary variable assignment
  target/loongarch: Set dest error with error_abort in virt_cpu_irq_init

 hw/loongarch/virt.c               | 40 +++++++++++++------------------
 target/loongarch/kvm/kvm.c        |  8 +++++--
 target/loongarch/tcg/tlb_helper.c |  5 ++--
 3 files changed, 24 insertions(+), 29 deletions(-)


base-commit: 1dae461a913f9da88df05de6e2020d3134356f2e
-- 
2.39.3

[PATCH v4 1/4] 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 v4 2/4] hw/loongarch/virt: Remove unnecessary NULL pointer

[Bibo Mao]

There is NULL pointer checking function error_propagate() already,
it is not necessary to add checking for function parameter. Here remove
NULL pointer checking with function parameter.

Since function will return directly when there is error report, this
patch removes combination about error_setg() and error_propagate(),
error_setg() with dest error object is used directly such as:

  error_setg(err);                 -------->      error_setg(errp);
  error_propagate(errp, err);                     return;
  return;

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

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index a5840ff968..a9fab39dd8 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,
@@ -1003,6 +993,7 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
         hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
         if (err) {
             error_propagate(errp, err);
+            return;
         }
     }
 
-- 
2.39.3

[PATCH v4 3/4] 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 v4 4/4] target/loongarch: Set dest error with error_abort in virt_cpu_irq_init

[Bibo Mao]

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.

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

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 <maobibo@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);
     }
 }
 
-- 
2.39.3

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

[Markus Armbruster]

Bibo Mao <maobibo@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?

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

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 <maobibo@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);
>      }
>  }

Re: [PATCH v4 2/4] hw/loongarch/virt: Remove unnecessary NULL pointer

[Markus Armbruster]

Bibo Mao <maobibo@loongson.cn> writes:

> There is NULL pointer checking function error_propagate() already,
> it is not necessary to add checking for function parameter. Here remove
> NULL pointer checking with function parameter.

I believe the title "Remove unnecessary NULL pointer" and this paragraph
are remnants of your initial version, which transformed

    if (err) {
        error_propagate(errp, err);
    }

to just

    error_propagate(errp, err);

However, the patch doesn't do that anymore.

I think you should drop the paragraph, and replace the title.

I apologize for not noticing this earlier.

> Since function will return directly when there is error report, this
> patch removes combination about error_setg() and error_propagate(),
> error_setg() with dest error object is used directly such as:
>
>   error_setg(err);                 -------->      error_setg(errp);
>   error_propagate(errp, err);                     return;
>   return;

Yes, much of the patch does this or equivalent transformations.
However, there's more; see [*] below.

> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  hw/loongarch/virt.c | 33 ++++++++++++---------------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index a5840ff968..a9fab39dd8 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,
> @@ -1003,6 +993,7 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>          if (err) {
>              error_propagate(errp, err);
> +            return;
>          }
>      }

       cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
       cpu_slot->cpu = NULL;
       return;
   }

[*] This is something else.  Before the patch, we clear cpu_slot->cpu
evem when the last hotplug_handler() fails.  Afterwards, we don't.
Looks like a bug fix to me.  Either mention the fix in the commit
message, or split it off into a separate patch.  I'd do the latter.

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 <maobibo@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 <maobibo@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);
>>       }
>>   }
> 

Re: [PATCH v4 2/4] hw/loongarch/virt: Remove unnecessary NULL pointer

[bibo mao]

On 2025/3/19 下午2:50, Markus Armbruster wrote:
> Bibo Mao <maobibo@loongson.cn> writes:
> 
>> There is NULL pointer checking function error_propagate() already,
>> it is not necessary to add checking for function parameter. Here remove
>> NULL pointer checking with function parameter.
> 
> I believe the title "Remove unnecessary NULL pointer" and this paragraph
> are remnants of your initial version, which transformed
> 
>      if (err) {
>          error_propagate(errp, err);
>      }
> 
> to just
> 
>      error_propagate(errp, err);
> 
> However, the patch doesn't do that anymore.
> 
> I think you should drop the paragraph, and replace the title.
yes, the title is misleading. Originally the problem is found with 
script scripts/coccinelle/remove_local_err.cocci, so here is the title.

How about "Remove local error object" or something else. Could you 
please provide some suggestions since English is your mother language?

> 
> I apologize for not noticing this earlier.
It is not necessary for the apologize. I appreciate your review 
comments. With effective communication, the quality of code is better.
> 
>> Since function will return directly when there is error report, this
>> patch removes combination about error_setg() and error_propagate(),
>> error_setg() with dest error object is used directly such as:
>>
>>    error_setg(err);                 -------->      error_setg(errp);
>>    error_propagate(errp, err);                     return;
>>    return;
> 
> Yes, much of the patch does this or equivalent transformations.
> However, there's more; see [*] below.
> 
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/loongarch/virt.c | 33 ++++++++++++---------------------
>>   1 file changed, 12 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index a5840ff968..a9fab39dd8 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,
>> @@ -1003,6 +993,7 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>>           if (err) {
>>               error_propagate(errp, err);
>> +            return;
>>           }
>>       }
> 
>         cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>         cpu_slot->cpu = NULL;
>         return;
>     }
> 
> [*] This is something else.  Before the patch, we clear cpu_slot->cpu
> evem when the last hotplug_handler() fails.  Afterwards, we don't.
> Looks like a bug fix to me.  Either mention the fix in the commit
> message, or split it off into a separate patch.  I'd do the latter.
> 
yes, will split it into two patches. The latter is bugfix, when cpu 
fails to unplug and it should return immediately, so that system can 
continue to run , and cpu_slot->cpu should not be cleared.

Regards
Bibo Mao

Re: [PATCH v4 2/4] hw/loongarch/virt: Remove unnecessary NULL pointer

[Markus Armbruster]

bibo mao <maobibo@loongson.cn> writes:

On 2025/3/19 下午2:50, Markus Armbruster wrote:
>> Bibo Mao <maobibo@loongson.cn> writes:
>> 
>>> There is NULL pointer checking function error_propagate() already,
>>> it is not necessary to add checking for function parameter. Here remove
>>> NULL pointer checking with function parameter.
>> 
>> I believe the title "Remove unnecessary NULL pointer" and this paragraph
>> are remnants of your initial version, which transformed
>> 
>>      if (err) {
>>          error_propagate(errp, err);
>>      }
>> 
>> to just
>> 
>>      error_propagate(errp, err);
>> 
>> However, the patch doesn't do that anymore.
>> 
>> I think you should drop the paragraph, and replace the title.
>
> yes, the title is misleading. Originally the problem is found with 
> script scripts/coccinelle/remove_local_err.cocci, so here is the title.
>
> How about "Remove local error object" or something else. Could you 
> please provide some suggestions since English is your mother language?

My first language is German, but I've practiced writing English commit
messages for a while :)

Here's what I've used for similar patches before, adapted to this one:

    hw/loongarch/virt: Eliminate error_propagate()
    
    When all we do with an Error we receive into a local variable is
    propagating to somewhere else, we can just as well receive it there
    right away.


>> I apologize for not noticing this earlier.
>
> It is not necessary for the apologize. I appreciate your review 
> comments. With effective communication, the quality of code is better.

Thank you!

>>> Since function will return directly when there is error report, this
>>> patch removes combination about error_setg() and error_propagate(),
>>> error_setg() with dest error object is used directly such as:
>>>
>>>    error_setg(err);                 -------->      error_setg(errp);
>>>    error_propagate(errp, err);                     return;
>>>    return;
>> 
>> Yes, much of the patch does this or equivalent transformations.
>> However, there's more; see [*] below.
>> 
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>> ---
>>>   hw/loongarch/virt.c | 33 ++++++++++++---------------------
>>>   1 file changed, 12 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>>> index a5840ff968..a9fab39dd8 100644
>>> --- a/hw/loongarch/virt.c
>>> +++ b/hw/loongarch/virt.c

[...]

>>> @@ -1003,6 +993,7 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>>          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>>>          if (err) {
>>>              error_propagate(errp, err);
>>> +            return;
>>>          }
>>>      }
>> 
>>       cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>>       cpu_slot->cpu = NULL;
>>       return;
>>   }
>> 
>> [*] This is something else.  Before the patch, we clear cpu_slot->cpu
>> evem when the last hotplug_handler() fails.  Afterwards, we don't.
>> Looks like a bug fix to me.  Either mention the fix in the commit
>> message, or split it off into a separate patch.  I'd do the latter.
>> 
> yes, will split it into two patches. The latter is bugfix, when cpu 
> fails to unplug and it should return immediately, so that system can 
> continue to run , and cpu_slot->cpu should not be cleared.

Thanks again!

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

[Markus Armbruster]

bibo mao <maobibo@loongson.cn> writes:

On 2025/3/19 下午2:09, Markus Armbruster wrote:
>> Bibo Mao <maobibo@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.

Could use something like

    target/loongarch: Clean up virt_cpu_irq_init() error handling

    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 clearing passing &error_abort instead.

    Signed-off-by: Bibo Mao <maobibo@loongson.cn>

Note: I replaced the Fixes: tags by a "Messed up ..." paragraph, because
there is no bug to fix according to your explanation.

With something like that:
Acked-by: Markus Armbruster <armbru@redhat.com>

> 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 <maobibo@loongson.cn>