* [PATCH v3 0/3] target/loongarch: Solve some issues reported from coccinelle
@ 2025-03-17 2:29 Bibo Mao
2025-03-17 2:29 ` [PATCH v3 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Bibo Mao @ 2025-03-17 2:29 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.
---
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 (3):
target/loongarch: Fix error handling of KVM feature checks
hw/loongarch/virt: Remove unnecessary NULL pointer
target/loongarch: Remove unnecessary temporary variable assignment
hw/loongarch/virt.c | 28 +++++++++++-----------------
target/loongarch/kvm/kvm.c | 8 ++++++--
target/loongarch/tcg/tlb_helper.c | 5 ++---
3 files changed, 19 insertions(+), 22 deletions(-)
base-commit: aa90f1161bb17a4863e16ec2f75104cff0752d4e
--
2.39.3
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3 1/3] target/loongarch: Fix error handling of KVM feature checks 2025-03-17 2:29 [PATCH v3 0/3] target/loongarch: Solve some issues reported from coccinelle Bibo Mao @ 2025-03-17 2:29 ` Bibo Mao 2025-03-17 2:29 ` [PATCH v3 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer Bibo Mao 2025-03-17 2:29 ` [PATCH v3 3/3] target/loongarch: Remove unnecessary temporary variable assignment Bibo Mao 2 siblings, 0 replies; 6+ messages in thread From: Bibo Mao @ 2025-03-17 2:29 UTC (permalink / raw) To: Song Gao Cc: Jiaxun Yang, qemu-devel, Markus Armbruster, Paolo Bonzini, Philippe Mathieu-Daudé 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer 2025-03-17 2:29 [PATCH v3 0/3] target/loongarch: Solve some issues reported from coccinelle Bibo Mao 2025-03-17 2:29 ` [PATCH v3 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao @ 2025-03-17 2:29 ` Bibo Mao 2025-03-17 6:08 ` Markus Armbruster 2025-03-17 2:29 ` [PATCH v3 3/3] target/loongarch: Remove unnecessary temporary variable assignment Bibo Mao 2 siblings, 1 reply; 6+ messages in thread From: Bibo Mao @ 2025-03-17 2:29 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. 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 | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c index a5840ff968..1fd91f94b5 100644 --- a/hw/loongarch/virt.c +++ b/hw/loongarch/virt.c @@ -865,24 +865,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, 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 +891,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 */ @@ -912,10 +912,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, @@ -927,17 +923,14 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_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); - } + error_propagate(errp, err); } static void virt_cpu_unplug(HotplugHandler *hotplug_dev, @@ -1003,6 +996,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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer 2025-03-17 2:29 ` [PATCH v3 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer Bibo Mao @ 2025-03-17 6:08 ` Markus Armbruster 2025-03-17 6:48 ` bibo mao 0 siblings, 1 reply; 6+ messages in thread From: Markus Armbruster @ 2025-03-17 6:08 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. > > 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 | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > > diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c > index a5840ff968..1fd91f94b5 100644 > --- a/hw/loongarch/virt.c > +++ b/hw/loongarch/virt.c > @@ -865,24 +865,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, > > 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 +891,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 */ > @@ -912,10 +912,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, > @@ -927,17 +923,14 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_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); > - } > + error_propagate(errp, err); Missed this cleanup: hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, errp); } > } > > static void virt_cpu_unplug(HotplugHandler *hotplug_dev, > @@ -1003,6 +996,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; > } > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer 2025-03-17 6:08 ` Markus Armbruster @ 2025-03-17 6:48 ` bibo mao 0 siblings, 0 replies; 6+ messages in thread From: bibo mao @ 2025-03-17 6:48 UTC (permalink / raw) To: Markus Armbruster; +Cc: Song Gao, Jiaxun Yang, qemu-devel, Paolo Bonzini On 2025/3/17 下午2:08, 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. >> >> 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 | 28 +++++++++++----------------- >> 1 file changed, 11 insertions(+), 17 deletions(-) >> >> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c >> index a5840ff968..1fd91f94b5 100644 >> --- a/hw/loongarch/virt.c >> +++ b/hw/loongarch/virt.c >> @@ -865,24 +865,24 @@ static void virt_cpu_pre_plug(HotplugHandler *hotplug_dev, >> >> 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 +891,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 */ >> @@ -912,10 +912,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. yes, should pass errp and remove local variable Error err. > >> -out: >> - if (err) { >> - error_propagate(errp, err); >> - } >> } >> >> static void virt_cpu_unplug_request(HotplugHandler *hotplug_dev, >> @@ -927,17 +923,14 @@ static void virt_cpu_unplug_request(HotplugHandler *hotplug_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); >> - } >> + error_propagate(errp, err); > > Missed this cleanup: > > hotplug_handler_unplug_request(HOTPLUG_HANDLER(lvms->acpi_ged), dev, errp); > } will do. With that, almost all local variable Error *err is removed. And that is cleaner. Regards Bibo Mao > >> } >> >> static void virt_cpu_unplug(HotplugHandler *hotplug_dev, >> @@ -1003,6 +996,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; >> } >> } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 3/3] target/loongarch: Remove unnecessary temporary variable assignment 2025-03-17 2:29 [PATCH v3 0/3] target/loongarch: Solve some issues reported from coccinelle Bibo Mao 2025-03-17 2:29 ` [PATCH v3 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao 2025-03-17 2:29 ` [PATCH v3 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer Bibo Mao @ 2025-03-17 2:29 ` Bibo Mao 2 siblings, 0 replies; 6+ messages in thread From: Bibo Mao @ 2025-03-17 2:29 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] 6+ messages in thread
end of thread, other threads:[~2025-03-17 6:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-03-17 2:29 [PATCH v3 0/3] target/loongarch: Solve some issues reported from coccinelle Bibo Mao 2025-03-17 2:29 ` [PATCH v3 1/3] target/loongarch: Fix error handling of KVM feature checks Bibo Mao 2025-03-17 2:29 ` [PATCH v3 2/3] hw/loongarch/virt: Remove unnecessary NULL pointer Bibo Mao 2025-03-17 6:08 ` Markus Armbruster 2025-03-17 6:48 ` bibo mao 2025-03-17 2:29 ` [PATCH v3 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).