| Message ID | 20250313091350.3770394-2-maobibo@loongson.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/loongarch: Solve some issues reported from coccinelle | expand |
Suggest something like arget/loongarch: Fix error handling of KVM feature checks That way, the nature of the patch (it's an error handling bug fix) is obvious at a glance. Bibo Mao <maobibo@loongson.cn> writes: > For some paravirt KVM features, if user forces to enable it however > KVM does not support, qemu should fail to run. Here set error message > and return directly in function kvm_arch_init_vcpu(). > Please add 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> > --- > target/loongarch/kvm/kvm.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c > index 28735c80be..b7f370ba97 100644 > --- a/target/loongarch/kvm/kvm.c > +++ b/target/loongarch/kvm/kvm.c int kvm_arch_init_vcpu(CPUState *cs) { uint64_t val; int ret; Error *local_err = NULL; ret = 0; Please drop this assignment, it's useless. qemu_add_vm_change_state_handler(kvm_loongarch_vm_stage_change, cs); if (!kvm_get_one_reg(cs, KVM_REG_LOONGARCH_DEBUG_INST, &val)) { brk_insn = val; } > @@ -1091,21 +1091,25 @@ 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; > } > Recommend to > ret = kvm_cpu_check_pv_features(cs, &local_err); if (ret < 0) { error_report_err(local_err); + return ret; } - return ret; return 0; } Why? Have a look at commit 6edd2a9bec90: @@ -793,6 +828,12 @@ int kvm_arch_init_vcpu(CPUState *cs) if (ret < 0) { error_report_err(local_err); } + + ret = kvm_cpu_check_pmu(cs, &local_err); + if (ret < 0) { + error_report_err(local_err); + } + return ret; } The new call broke the previous call's error handling. Easy mistake to make. Less easy with my version. With that Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks!
On 2025/3/13 下午6:26, Markus Armbruster wrote: > Suggest something like > > target/loongarch: Fix error handling of KVM feature checks > > That way, the nature of the patch (it's an error handling bug fix) is > obvious at a glance. yeap, will modify title like this. > > Bibo Mao <maobibo@loongson.cn> writes: > >> For some paravirt KVM features, if user forces to enable it however >> KVM does not support, qemu should fail to run. Here set error message >> and return directly in function kvm_arch_init_vcpu(). >> > > Please add > > 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) OK, will add these fixes tag. > >> Signed-off-by: Bibo Mao <maobibo@loongson.cn> >> --- >> target/loongarch/kvm/kvm.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c >> index 28735c80be..b7f370ba97 100644 >> --- a/target/loongarch/kvm/kvm.c >> +++ b/target/loongarch/kvm/kvm.c > int kvm_arch_init_vcpu(CPUState *cs) > { > uint64_t val; > int ret; > Error *local_err = NULL; > > ret = 0; > > Please drop this assignment, it's useless. Sure, will do. > > qemu_add_vm_change_state_handler(kvm_loongarch_vm_stage_change, cs); > > if (!kvm_get_one_reg(cs, KVM_REG_LOONGARCH_DEBUG_INST, &val)) { > brk_insn = val; > } > >> @@ -1091,21 +1091,25 @@ 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; >> } >> > > Recommend to > >> ret = kvm_cpu_check_pv_features(cs, &local_err); > if (ret < 0) { > error_report_err(local_err); > + return ret; > } > > - return ret; > return 0; > } > > Why? Have a look at commit 6edd2a9bec90: I agree. That is simple and easy to understand, will do like this. Regards Bibo Mao > > @@ -793,6 +828,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > if (ret < 0) { > error_report_err(local_err); > } > + > + ret = kvm_cpu_check_pmu(cs, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > + > return ret; > } > > The new call broke the previous call's error handling. Easy mistake to > make. Less easy with my version. > > With that > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Thanks! >
diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c index 28735c80be..b7f370ba97 100644 --- a/target/loongarch/kvm/kvm.c +++ b/target/loongarch/kvm/kvm.c @@ -1091,21 +1091,25 @@ 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);
For some paravirt KVM features, if user forces to enable it however KVM does not support, qemu should fail to run. Here set error message and return directly in function kvm_arch_init_vcpu(). Signed-off-by: Bibo Mao <maobibo@loongson.cn> --- target/loongarch/kvm/kvm.c | 4 ++++ 1 file changed, 4 insertions(+)