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

[bibo mao]

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 <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)

OK, will add these fixes tag.

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.

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

Thanks!