[PATCH v2 0/3] target/loongarch: Solve some issues reported from coccinelle

[PATCH v2 0/3] 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.

---
  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 (3):
  target/loongarch: Fix error handling of KVM feature checks
  hw/loongarch/virt: Remove unnecessary NULL pointer checking
  target/loongarch: Remove unnecessary temporary variable assignment

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


base-commit: 4c33c097f3a8a8093bcbaf097c3a178051e51b3e
-- 
2.39.3

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

[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.

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

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index a5840ff968..d82676d316 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -868,21 +868,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
             error_setg(&err,
                        "Invalid thread-id %u specified, must be in range 1:%u",
                        cpu->thread_id, ms->smp.threads - 1);
-            goto out;
+            error_propagate(errp, err);
+            return;
         }
 
         if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
             error_setg(&err,
                        "Invalid core-id %u specified, must be in range 1:%u",
                        cpu->core_id, ms->smp.cores - 1);
-            goto out;
+            error_propagate(errp, err);
+            return;
         }
 
         if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
             error_setg(&err,
                        "Invalid socket-id %u specified, must be in range 1:%u",
                        cpu->socket_id, ms->smp.sockets - 1);
-            goto out;
+            error_propagate(errp, err);
+            return;
         }
 
         topo.socket_id = cpu->socket_id;
@@ -895,7 +898,8 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
                        "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;
+            error_propagate(errp, err);
+            return;
         }
     } else {
         /* For cold-add cpu, find empty cpu slot */
@@ -912,10 +916,6 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
     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);
-    }
 }
 
 static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
@@ -935,9 +935,7 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
     }
 
     hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
@@ -1001,9 +999,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
 
     if (lvms->acpi_ged) {
         hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
-        if (err) {
-            error_propagate(errp, err);
-        }
+        error_propagate(errp, err);
+        return;
     }
 
     return;
-- 
2.39.3

[PATCH v2 3/3] 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

Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking

[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.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  hw/loongarch/virt.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index a5840ff968..d82676d316 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -868,21 +868,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>              error_setg(&err,
>                         "Invalid thread-id %u specified, must be in range 1:%u",
>                         cpu->thread_id, ms->smp.threads - 1);
> -            goto out;
> +            error_propagate(errp, err);
> +            return;

Make this

               error_setg(&err, ...);
               return;

>          }
>  
>          if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
>              error_setg(&err,
>                         "Invalid core-id %u specified, must be in range 1:%u",
>                         cpu->core_id, ms->smp.cores - 1);
> -            goto out;
> +            error_propagate(errp, err);
> +            return;

Likewise.

>          }
>  
>          if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
>              error_setg(&err,
>                         "Invalid socket-id %u specified, must be in range 1:%u",
>                         cpu->socket_id, ms->smp.sockets - 1);
> -            goto out;
> +            error_propagate(errp, err);
> +            return;

Likewise.

>          }
>  
>          topo.socket_id = cpu->socket_id;
> @@ -895,7 +898,8 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                         "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;
> +            error_propagate(errp, err);
> +            return;

Likewise.

>          }
>      } else {
>          /* For cold-add cpu, find empty cpu slot */
> @@ -912,10 +916,6 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      cpu->phy_id = cpu_slot->arch_id;
>      cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
>      numa_cpu_pre_plug(cpu_slot, dev, &err);

You need to pass errp instead of &err now.

> -out:
> -    if (err) {
> -        error_propagate(errp, err);
> -    }
>  }
>  
>  static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
> @@ -935,9 +935,7 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
>      }
>  
>      hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> -    }
> +    error_propagate(errp, err);
>  }

Correct, but I'd recomment to go one step further:

 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,
> @@ -1001,9 +999,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
       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) {
               error_propagate(errp, err);
               return;
           }
       }

       if (lvms->extioi) {
           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
           if (err) {
               error_propagate(errp, err);
               return;
           }
       }
>  
>      if (lvms->acpi_ged) {
>          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
> -        if (err) {
> -            error_propagate(errp, err);
> -        }
> +        error_propagate(errp, err);
> +        return;
>      }
>  
>      return;

Better make this work exactly like the other checks above, and drop the
final return, which serves no purpose:

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

Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking

[bibo mao]

On 2025/3/14 下午5:11, 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.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/loongarch/virt.c | 25 +++++++++++--------------
>>   1 file changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index a5840ff968..d82676d316 100644
>> --- a/hw/loongarch/virt.c
>> +++ b/hw/loongarch/virt.c
>> @@ -868,21 +868,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>               error_setg(&err,
>>                          "Invalid thread-id %u specified, must be in range 1:%u",
>>                          cpu->thread_id, ms->smp.threads - 1);
>> -            goto out;
>> +            error_propagate(errp, err);
>> +            return;
> 
> Make this

> 
>                 error_setg(&err, ...);
>                 return;
My bad. I remember replacing error_propagate with error_setg(errp, ...) 
already. Will do
> 
>>           }
>>   
>>           if ((cpu->core_id < 0) || (cpu->core_id >= ms->smp.cores)) {
>>               error_setg(&err,
>>                          "Invalid core-id %u specified, must be in range 1:%u",
>>                          cpu->core_id, ms->smp.cores - 1);
>> -            goto out;
>> +            error_propagate(errp, err);
>> +            return;
> 
> Likewise.
> 
>>           }
>>   
>>           if ((cpu->socket_id < 0) || (cpu->socket_id >= ms->smp.sockets)) {
>>               error_setg(&err,
>>                          "Invalid socket-id %u specified, must be in range 1:%u",
>>                          cpu->socket_id, ms->smp.sockets - 1);
>> -            goto out;
>> +            error_propagate(errp, err);
>> +            return;
> 
> Likewise.
> 
>>           }
>>   
>>           topo.socket_id = cpu->socket_id;
>> @@ -895,7 +898,8 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>                          "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;
>> +            error_propagate(errp, err);
>> +            return;
> 
> Likewise.
> 
>>           }
>>       } else {
>>           /* For cold-add cpu, find empty cpu slot */
>> @@ -912,10 +916,6 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev,
>>       cpu->phy_id = cpu_slot->arch_id;
>>       cs->cpu_index = cpu_slot - ms->possible_cpus->cpus;
>>       numa_cpu_pre_plug(cpu_slot, dev, &err);
> 
> You need to pass errp instead of &err now.
> 
>> -out:
>> -    if (err) {
>> -        error_propagate(errp, err);
>> -    }
>>   }
>>   
>>   static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
>> @@ -935,9 +935,7 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev,
>>       }
>>   
>>       hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> -    }
>> +    error_propagate(errp, err);
>>   }
> 
> Correct, but I'd recomment to go one step further:
> 
>   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,
>> @@ -1001,9 +999,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>         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) {
>                 error_propagate(errp, err);
>                 return;
>             }
>         }
> 
>         if (lvms->extioi) {
>             hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
>             if (err) {
>                 error_propagate(errp, err);
>                 return;
>             }
>         }
>>   
>>       if (lvms->acpi_ged) {
>>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>> -        if (err) {
>> -            error_propagate(errp, err);
>> -        }
>> +        error_propagate(errp, err);
>> +        return;
>>       }
>>   
>>       return;
> 
> Better make this work exactly like the other checks above, and drop the
> final return, which serves no purpose:
> 
>             if (err) {
>                 error_propagate(errp, err);
>                 return;
>             }
>         }
Here is report from commandline, it say err NULL check is not necessary
spatch --sp-file scripts/coccinelle/error_propagate_null.cocci --dir 
hw/loongarch/

@@ -1001,9 +997,7 @@ static void virt_cpu_plug(HotplugHandler

      if (lvms->acpi_ged) {
          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
-        if (err) {
-            error_propagate(errp, err);
-        }
+        error_propagate(errp, err);
      }

Regards
Bibo Mao
>     }
> 

Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking

[Markus Armbruster]

bibo mao <maobibo@loongson.cn> writes:

On 2025/3/14 下午5:11, 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.
>>>
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>

[...]

>>> @@ -1001,9 +999,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>         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) {
>>                 error_propagate(errp, err);
>>                 return;
>>             }
>>         }
>> 
>>         if (lvms->extioi) {
>>             hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
>>             if (err) {
>>                 error_propagate(errp, err);
>>                 return;
>>             }
>>         }
>>>   
>>>       if (lvms->acpi_ged) {
>>>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>>> -        if (err) {
>>> -            error_propagate(errp, err);
>>> -        }
>>> +        error_propagate(errp, err);
>>> +        return;
>>>       }
>>>   
>>>       return;
>> 
>> Better make this work exactly like the other checks above, and drop the
>> final return, which serves no purpose:
>> 
>>             if (err) {
>>                 error_propagate(errp, err);
>>                 return;
>>             }
>>         }
> Here is report from commandline, it say err NULL check is not necessary
> spatch --sp-file scripts/coccinelle/error_propagate_null.cocci --dir 
> hw/loongarch/
>
> @@ -1001,9 +997,7 @@ static void virt_cpu_plug(HotplugHandler
>
>       if (lvms->acpi_ged) {
>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
> -        if (err) {
> -            error_propagate(errp, err);
> -        }
> +        error_propagate(errp, err);
>       }

This change is correct.  The change I suggests is also correct.  I
prefer mine because it's less easy to screw up.  If you add another
check at the end, my version keeps working, while yours needs an update,
which is easy to miss.

Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking

[bibo mao]

On 2025/3/14 下午5:38, Markus Armbruster wrote:
> bibo mao <maobibo@loongson.cn> writes:
> 
> On 2025/3/14 下午5:11, 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.
>>>>
>>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> 
> [...]
> 
>>>> @@ -1001,9 +999,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>>          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) {
>>>                  error_propagate(errp, err);
>>>                  return;
>>>              }
>>>          }
>>>
>>>          if (lvms->extioi) {
>>>              hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
>>>              if (err) {
>>>                  error_propagate(errp, err);
>>>                  return;
>>>              }
>>>          }
>>>>    
>>>>        if (lvms->acpi_ged) {
>>>>            hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>>>> -        if (err) {
>>>> -            error_propagate(errp, err);
>>>> -        }
>>>> +        error_propagate(errp, err);
>>>> +        return;
>>>>        }
>>>>    
>>>>        return;
>>>
>>> Better make this work exactly like the other checks above, and drop the
>>> final return, which serves no purpose:
>>>
>>>              if (err) {
>>>                  error_propagate(errp, err);
>>>                  return;
>>>              }
>>>          }
>> Here is report from commandline, it say err NULL check is not necessary
>> spatch --sp-file scripts/coccinelle/error_propagate_null.cocci --dir
>> hw/loongarch/
>>
>> @@ -1001,9 +997,7 @@ static void virt_cpu_plug(HotplugHandler
>>
>>        if (lvms->acpi_ged) {
>>            hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
>> -        if (err) {
>> -            error_propagate(errp, err);
>> -        }
>> +        error_propagate(errp, err);
>>        }
> 
> This change is correct.  The change I suggests is also correct.  I
> prefer mine because it's less easy to screw up.  If you add another
> check at the end, my version keeps working, while yours needs an update,
> which is easy to miss.
> 
I see until checking file scripts/coccinelle/error_propagate_null.cocci
-if (L) {
      error_propagate(E, L);
-}

you are error log expert, of source do with your suggestion -:)
And thanks for your earnest explanation.

Regards
Bibo Mao

Re: [PATCH v2 1/3] target/loongarch: Fix error handling of KVM feature checks

[Philippe Mathieu-Daudé]

On 14/3/25 09:41, Bibo Mao wrote:
> 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>
> ---
>   target/loongarch/kvm/kvm.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>