* [PATCH v2 0/3] target/loongarch: Solve some issues reported from coccinelle
@ 2025-03-14 8:41 Bibo Mao
2025-03-14 8:41 ` [PATCH v2 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Bibo Mao @ 2025-03-14 8:41 UTC (permalink / raw)
To: Song Gao; +Cc: Jiaxun Yang, qemu-devel, Markus Armbruster, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/3] target/loongarch: Fix error handling of KVM feature checks 2025-03-14 8:41 [PATCH v2 0/3] target/loongarch: Solve some issues reported from coccinelle Bibo Mao @ 2025-03-14 8:41 ` Bibo Mao 2025-03-14 11:31 ` Philippe Mathieu-Daudé 2025-03-14 8:42 ` [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking Bibo Mao 2025-03-14 8:42 ` [PATCH v2 3/3] target/loongarch: Remove unnecessary temporary variable assignment Bibo Mao 2 siblings, 1 reply; 9+ messages in thread From: Bibo Mao @ 2025-03-14 8:41 UTC (permalink / raw) To: Song Gao; +Cc: Jiaxun Yang, qemu-devel, Markus Armbruster, Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] target/loongarch: Fix error handling of KVM feature checks 2025-03-14 8:41 ` [PATCH v2 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao @ 2025-03-14 11:31 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2025-03-14 11:31 UTC (permalink / raw) To: Bibo Mao, Song Gao Cc: Jiaxun Yang, qemu-devel, Markus Armbruster, Paolo Bonzini 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking 2025-03-14 8:41 [PATCH v2 0/3] target/loongarch: Solve some issues reported from coccinelle Bibo Mao 2025-03-14 8:41 ` [PATCH v2 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao @ 2025-03-14 8:42 ` Bibo Mao 2025-03-14 9:11 ` Markus Armbruster 2025-03-14 8:42 ` [PATCH v2 3/3] target/loongarch: Remove unnecessary temporary variable assignment Bibo Mao 2 siblings, 1 reply; 9+ messages in thread From: Bibo Mao @ 2025-03-14 8:42 UTC (permalink / raw) To: Song Gao; +Cc: Jiaxun Yang, qemu-devel, Markus Armbruster, Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking 2025-03-14 8:42 ` [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking Bibo Mao @ 2025-03-14 9:11 ` Markus Armbruster 2025-03-14 9:25 ` bibo mao 0 siblings, 1 reply; 9+ messages in thread From: Markus Armbruster @ 2025-03-14 9:11 UTC (permalink / raw) To: Bibo Mao; +Cc: Song Gao, Jiaxun Yang, qemu-devel, Paolo Bonzini 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; } } } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking 2025-03-14 9:11 ` Markus Armbruster @ 2025-03-14 9:25 ` bibo mao 2025-03-14 9:38 ` Markus Armbruster 0 siblings, 1 reply; 9+ messages in thread From: bibo mao @ 2025-03-14 9:25 UTC (permalink / raw) To: Markus Armbruster; +Cc: Song Gao, Jiaxun Yang, qemu-devel, Paolo Bonzini 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 > } > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking 2025-03-14 9:25 ` bibo mao @ 2025-03-14 9:38 ` Markus Armbruster 2025-03-14 9:55 ` bibo mao 0 siblings, 1 reply; 9+ messages in thread From: Markus Armbruster @ 2025-03-14 9:38 UTC (permalink / raw) To: bibo mao; +Cc: Song Gao, Jiaxun Yang, qemu-devel, Paolo Bonzini 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking 2025-03-14 9:38 ` Markus Armbruster @ 2025-03-14 9:55 ` bibo mao 0 siblings, 0 replies; 9+ messages in thread From: bibo mao @ 2025-03-14 9:55 UTC (permalink / raw) To: Markus Armbruster; +Cc: Song Gao, Jiaxun Yang, qemu-devel, Paolo Bonzini 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] target/loongarch: Remove unnecessary temporary variable assignment 2025-03-14 8:41 [PATCH v2 0/3] target/loongarch: Solve some issues reported from coccinelle Bibo Mao 2025-03-14 8:41 ` [PATCH v2 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao 2025-03-14 8:42 ` [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking Bibo Mao @ 2025-03-14 8:42 ` Bibo Mao 2 siblings, 0 replies; 9+ messages in thread From: Bibo Mao @ 2025-03-14 8:42 UTC (permalink / raw) To: Song Gao Cc: Jiaxun Yang, qemu-devel, Markus Armbruster, Paolo Bonzini, Philippe Mathieu-Daudé 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-03-14 11:31 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-03-14 8:41 [PATCH v2 0/3] target/loongarch: Solve some issues reported from coccinelle Bibo Mao 2025-03-14 8:41 ` [PATCH v2 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao 2025-03-14 11:31 ` Philippe Mathieu-Daudé 2025-03-14 8:42 ` [PATCH v2 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer checking Bibo Mao 2025-03-14 9:11 ` Markus Armbruster 2025-03-14 9:25 ` bibo mao 2025-03-14 9:38 ` Markus Armbruster 2025-03-14 9:55 ` bibo mao 2025-03-14 8:42 ` [PATCH v2 3/3] target/loongarch: Remove unnecessary temporary variable assignment Bibo Mao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).