Re: [PATCH 1/3] target/loongarch: Return directly when detect KVM disabled features

[Markus Armbruster]

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 <maob...@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 <maob...@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 <arm...@redhat.com>

Thanks!