[PATCH 0/2] kvm: Improve register failure reports for migration

[PATCH 0/2] kvm: Improve register failure reports for migration

[Julia Suvorova]

There were a few bugs with silent cpu failures during migrations [1].
The failures are no longer silent thanks to [2], but still
indistinguishable, making diagnostics difficult.

This patchset reworks kvm_arch_get/put_registers to handle Error** and
return a correct message, with x86_64 as a start.

[1] https://issues.redhat.com/browse/RHEL-7558
[2] 7191f24c7fcf "accel/kvm/kvm-all: Handle register access errors"

Julia Suvorova (2):
  kvm: Allow kvm_arch_get/put_registers to accept Error**
  target/i386/kvm: Report which action failed in
    kvm_arch_put/get_registers

 accel/kvm/kvm-all.c        | 41 +++++++++++++++++++++++++++++---------
 include/sysemu/kvm.h       |  4 ++--
 target/arm/kvm.c           |  4 ++--
 target/i386/kvm/kvm.c      | 27 +++++++++++++++++++++++--
 target/loongarch/kvm/kvm.c |  4 ++--
 target/mips/kvm.c          |  4 ++--
 target/ppc/kvm.c           |  2 +-
 target/riscv/kvm/kvm-cpu.c |  4 ++--
 target/s390x/kvm/kvm.c     |  4 ++--
 9 files changed, 70 insertions(+), 24 deletions(-)

-- 
2.45.0

[PATCH 1/2] kvm: Allow kvm_arch_get/put_registers to accept Error**

[Julia Suvorova]

This is necessary to provide discernible error messages to the caller.

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 accel/kvm/kvm-all.c        | 41 +++++++++++++++++++++++++++++---------
 include/sysemu/kvm.h       |  4 ++--
 target/arm/kvm.c           |  4 ++--
 target/i386/kvm/kvm.c      |  4 ++--
 target/loongarch/kvm/kvm.c |  4 ++--
 target/mips/kvm.c          |  4 ++--
 target/ppc/kvm.c           |  2 +-
 target/riscv/kvm/kvm-cpu.c |  4 ++--
 target/s390x/kvm/kvm.c     |  4 ++--
 9 files changed, 47 insertions(+), 24 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index fe4cd721d9..19e09067b3 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2762,9 +2762,15 @@ void kvm_flush_coalesced_mmio_buffer(void)
 static void do_kvm_cpu_synchronize_state(CPUState *cpu, run_on_cpu_data arg)
 {
     if (!cpu->vcpu_dirty && !kvm_state->guest_state_protected) {
-        int ret = kvm_arch_get_registers(cpu);
+        Error *err = NULL;
+        int ret = kvm_arch_get_registers(cpu, &err);
         if (ret) {
-            error_report("Failed to get registers: %s", strerror(-ret));
+            if (err) {
+                error_report_err(err);
+            } else {
+                error_report("Failed to get registers: %s", strerror(-ret));
+            }
+
             cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
             vm_stop(RUN_STATE_INTERNAL_ERROR);
         }
@@ -2782,9 +2788,15 @@ void kvm_cpu_synchronize_state(CPUState *cpu)
 
 static void do_kvm_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg)
 {
-    int ret = kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
+    Error *err = NULL;
+    int ret = kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE, &err);
     if (ret) {
-        error_report("Failed to put registers after reset: %s", strerror(-ret));
+        if (err) {
+            error_reportf_err(err, "Restoring resisters after reset");
+        } else {
+            error_report("Failed to put registers after reset: %s",
+                         strerror(-ret));
+        }
         cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
         vm_stop(RUN_STATE_INTERNAL_ERROR);
     }
@@ -2799,9 +2811,15 @@ void kvm_cpu_synchronize_post_reset(CPUState *cpu)
 
 static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg)
 {
-    int ret = kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
+    Error *err = NULL;
+    int ret = kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE, &err);
     if (ret) {
-        error_report("Failed to put registers after init: %s", strerror(-ret));
+        if (err) {
+            error_reportf_err(err, "Putting registers after init");
+        } else {
+            error_report("Failed to put registers after init: %s",
+                         strerror(-ret));
+        }
         exit(1);
     }
 
@@ -2991,10 +3009,15 @@ int kvm_cpu_exec(CPUState *cpu)
         MemTxAttrs attrs;
 
         if (cpu->vcpu_dirty) {
-            ret = kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE);
+            Error *err = NULL;
+            ret = kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE, &err);
             if (ret) {
-                error_report("Failed to put registers after init: %s",
-                             strerror(-ret));
+                if (err) {
+                    error_reportf_err(err, "Putting registers after init");
+                } else {
+                    error_report("Failed to put registers after init: %s",
+                                 strerror(-ret));
+                }
                 ret = -1;
                 break;
             }
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 613d3f7581..c3a60b2890 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -359,7 +359,7 @@ int kvm_arch_handle_exit(CPUState *cpu, struct kvm_run *run);
 
 int kvm_arch_process_async_events(CPUState *cpu);
 
-int kvm_arch_get_registers(CPUState *cpu);
+int kvm_arch_get_registers(CPUState *cpu, Error **errp);
 
 /* state subset only touched by the VCPU itself during runtime */
 #define KVM_PUT_RUNTIME_STATE   1
@@ -368,7 +368,7 @@ int kvm_arch_get_registers(CPUState *cpu);
 /* full state set, modified during initialization or on vmload */
 #define KVM_PUT_FULL_STATE      3
 
-int kvm_arch_put_registers(CPUState *cpu, int level);
+int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp);
 
 int kvm_arch_get_default_type(MachineState *ms);
 
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 849e2e21b3..f1f1b5b375 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -2042,7 +2042,7 @@ static int kvm_arch_put_sve(CPUState *cs)
     return 0;
 }
 
-int kvm_arch_put_registers(CPUState *cs, int level)
+int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
 {
     uint64_t val;
     uint32_t fpr;
@@ -2226,7 +2226,7 @@ static int kvm_arch_get_sve(CPUState *cs)
     return 0;
 }
 
-int kvm_arch_get_registers(CPUState *cs)
+int kvm_arch_get_registers(CPUState *cs, Error **errp)
 {
     uint64_t val;
     unsigned int el;
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index ada581c5d6..039ed08825 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5118,7 +5118,7 @@ static int kvm_get_nested_state(X86CPU *cpu)
     return ret;
 }
 
-int kvm_arch_put_registers(CPUState *cpu, int level)
+int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
 {
     X86CPU *x86_cpu = X86_CPU(cpu);
     int ret;
@@ -5206,7 +5206,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
     return 0;
 }
 
-int kvm_arch_get_registers(CPUState *cs)
+int kvm_arch_get_registers(CPUState *cs, Error **errp)
 {
     X86CPU *cpu = X86_CPU(cs);
     int ret;
diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c
index 4786cd5efa..30ec16025d 100644
--- a/target/loongarch/kvm/kvm.c
+++ b/target/loongarch/kvm/kvm.c
@@ -588,7 +588,7 @@ static int kvm_loongarch_put_cpucfg(CPUState *cs)
     return ret;
 }
 
-int kvm_arch_get_registers(CPUState *cs)
+int kvm_arch_get_registers(CPUState *cs, Error **errp)
 {
     int ret;
 
@@ -616,7 +616,7 @@ int kvm_arch_get_registers(CPUState *cs)
     return ret;
 }
 
-int kvm_arch_put_registers(CPUState *cs, int level)
+int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
 {
     int ret;
 
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index a631ab544f..a98798c669 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -1172,7 +1172,7 @@ static int kvm_mips_get_cp0_registers(CPUState *cs)
     return ret;
 }
 
-int kvm_arch_put_registers(CPUState *cs, int level)
+int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
 {
     CPUMIPSState *env = cpu_env(cs);
     struct kvm_regs regs;
@@ -1207,7 +1207,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     return ret;
 }
 
-int kvm_arch_get_registers(CPUState *cs)
+int kvm_arch_get_registers(CPUState *cs, Error **errp)
 {
     CPUMIPSState *env = cpu_env(cs);
     int ret = 0;
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 907dba60d1..a361d9d23c 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -900,7 +900,7 @@ int kvmppc_put_books_sregs(PowerPCCPU *cpu)
     return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_SREGS, &sregs);
 }
 
-int kvm_arch_put_registers(CPUState *cs, int level)
+int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index f6e3156b8d..2bfb112be0 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1192,7 +1192,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_LAST_INFO
 };
 
-int kvm_arch_get_registers(CPUState *cs)
+int kvm_arch_get_registers(CPUState *cs, Error **errp)
 {
     int ret = 0;
 
@@ -1237,7 +1237,7 @@ int kvm_riscv_sync_mpstate_to_kvm(RISCVCPU *cpu, int state)
     return 0;
 }
 
-int kvm_arch_put_registers(CPUState *cs, int level)
+int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
 {
     int ret = 0;
 
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 94181d9281..8ffe0159d8 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -472,7 +472,7 @@ static int can_sync_regs(CPUState *cs, int regs)
 #define KVM_SYNC_REQUIRED_REGS (KVM_SYNC_GPRS | KVM_SYNC_ACRS | \
                                 KVM_SYNC_CRS | KVM_SYNC_PREFIX)
 
-int kvm_arch_put_registers(CPUState *cs, int level)
+int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
 {
     CPUS390XState *env = cpu_env(cs);
     struct kvm_fpu fpu = {};
@@ -598,7 +598,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     return 0;
 }
 
-int kvm_arch_get_registers(CPUState *cs)
+int kvm_arch_get_registers(CPUState *cs, Error **errp)
 {
     CPUS390XState *env = cpu_env(cs);
     struct kvm_fpu fpu;
-- 
2.45.0

[PATCH 2/2] target/i386/kvm: Report which action failed in kvm_arch_put/get_registers

[Julia Suvorova]

To help debug and triage future failure reports (akin to [1,2]) that
may occur during kvm_arch_put/get_registers, the error path of each
action is accompanied by unique error message.

[1] https://issues.redhat.com/browse/RHEL-7558
[2] https://issues.redhat.com/browse/RHEL-21761

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 target/i386/kvm/kvm.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 039ed08825..7cdc87f855 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5133,6 +5133,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
     if (level >= KVM_PUT_RESET_STATE) {
         ret = kvm_put_msr_feature_control(x86_cpu);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to set feature control MSR");
             return ret;
         }
     }
@@ -5140,12 +5141,14 @@ int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
     /* must be before kvm_put_nested_state so that EFER.SVME is set */
     ret = has_sregs2 ? kvm_put_sregs2(x86_cpu) : kvm_put_sregs(x86_cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to set special registers");
         return ret;
     }
 
     if (level >= KVM_PUT_RESET_STATE) {
         ret = kvm_put_nested_state(x86_cpu);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to set nested state");
             return ret;
         }
     }
@@ -5163,6 +5166,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
     if (xen_mode == XEN_EMULATE && level == KVM_PUT_FULL_STATE) {
         ret = kvm_put_xen_state(cpu);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to set Xen state");
             return ret;
         }
     }
@@ -5170,37 +5174,45 @@ int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
 
     ret = kvm_getput_regs(x86_cpu, 1);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to set general purpose registers");
         return ret;
     }
     ret = kvm_put_xsave(x86_cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to set XSAVE");
         return ret;
     }
     ret = kvm_put_xcrs(x86_cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to set XCRs");
         return ret;
     }
     ret = kvm_put_msrs(x86_cpu, level);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to set MSRs");
         return ret;
     }
     ret = kvm_put_vcpu_events(x86_cpu, level);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to set vCPU events");
         return ret;
     }
     if (level >= KVM_PUT_RESET_STATE) {
         ret = kvm_put_mp_state(x86_cpu);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to set MP state");
             return ret;
         }
     }
 
     ret = kvm_put_tscdeadline_msr(x86_cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to set TSC deadline MSR");
         return ret;
     }
     ret = kvm_put_debugregs(x86_cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to set debug registers");
         return ret;
     }
     return 0;
@@ -5215,6 +5227,7 @@ int kvm_arch_get_registers(CPUState *cs, Error **errp)
 
     ret = kvm_get_vcpu_events(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get vCPU events");
         goto out;
     }
     /*
@@ -5223,44 +5236,54 @@ int kvm_arch_get_registers(CPUState *cs, Error **errp)
      */
     ret = kvm_get_mp_state(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get MP state");
         goto out;
     }
     ret = kvm_getput_regs(cpu, 0);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get general purpose registers");
         goto out;
     }
     ret = kvm_get_xsave(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get XSAVE");
         goto out;
     }
     ret = kvm_get_xcrs(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get XCRs");
         goto out;
     }
     ret = has_sregs2 ? kvm_get_sregs2(cpu) : kvm_get_sregs(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get special registers");
         goto out;
     }
     ret = kvm_get_msrs(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get MSRs");
         goto out;
     }
     ret = kvm_get_apic(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get APIC");
         goto out;
     }
     ret = kvm_get_debugregs(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get debug registers");
         goto out;
     }
     ret = kvm_get_nested_state(cpu);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to get nested state");
         goto out;
     }
 #ifdef CONFIG_XEN_EMU
     if (xen_mode == XEN_EMULATE) {
         ret = kvm_get_xen_state(cs);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to get Xen state");
             goto out;
         }
     }
-- 
2.45.0

Re: [PATCH 1/2] kvm: Allow kvm_arch_get/put_registers to accept Error**

[Peter Xu]

On Wed, Sep 25, 2024 at 05:36:22PM +0200, Julia Suvorova wrote:
> This is necessary to provide discernible error messages to the caller.
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

One nitpick below:

> ---
>  accel/kvm/kvm-all.c        | 41 +++++++++++++++++++++++++++++---------
>  include/sysemu/kvm.h       |  4 ++--
>  target/arm/kvm.c           |  4 ++--
>  target/i386/kvm/kvm.c      |  4 ++--
>  target/loongarch/kvm/kvm.c |  4 ++--
>  target/mips/kvm.c          |  4 ++--
>  target/ppc/kvm.c           |  2 +-
>  target/riscv/kvm/kvm-cpu.c |  4 ++--
>  target/s390x/kvm/kvm.c     |  4 ++--
>  9 files changed, 47 insertions(+), 24 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index fe4cd721d9..19e09067b3 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2762,9 +2762,15 @@ void kvm_flush_coalesced_mmio_buffer(void)
>  static void do_kvm_cpu_synchronize_state(CPUState *cpu, run_on_cpu_data arg)
>  {
>      if (!cpu->vcpu_dirty && !kvm_state->guest_state_protected) {
> -        int ret = kvm_arch_get_registers(cpu);
> +        Error *err = NULL;
> +        int ret = kvm_arch_get_registers(cpu, &err);
>          if (ret) {
> -            error_report("Failed to get registers: %s", strerror(-ret));
> +            if (err) {
> +                error_report_err(err);
> +            } else {
> +                error_report("Failed to get registers: %s", strerror(-ret));
> +            }
> +
>              cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
>              vm_stop(RUN_STATE_INTERNAL_ERROR);
>          }
> @@ -2782,9 +2788,15 @@ void kvm_cpu_synchronize_state(CPUState *cpu)
>  
>  static void do_kvm_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg)
>  {
> -    int ret = kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
> +    Error *err = NULL;
> +    int ret = kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE, &err);
>      if (ret) {
> -        error_report("Failed to put registers after reset: %s", strerror(-ret));
> +        if (err) {
> +            error_reportf_err(err, "Restoring resisters after reset");

IMO it'll be nice to use the same error report pattern, either
error_report_err() or error_reportf_err() for all the four call sites.

If to use error_reportf_err(), better add comma at the end (e.g. "PREFIX: "
instead of "PREFIX") to make the print looks slightly better.

Thanks!

> +        } else {
> +            error_report("Failed to put registers after reset: %s",
> +                         strerror(-ret));
> +        }
>          cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
>          vm_stop(RUN_STATE_INTERNAL_ERROR);
>      }
> @@ -2799,9 +2811,15 @@ void kvm_cpu_synchronize_post_reset(CPUState *cpu)
>  
>  static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg)
>  {
> -    int ret = kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
> +    Error *err = NULL;
> +    int ret = kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE, &err);
>      if (ret) {
> -        error_report("Failed to put registers after init: %s", strerror(-ret));
> +        if (err) {
> +            error_reportf_err(err, "Putting registers after init");
> +        } else {
> +            error_report("Failed to put registers after init: %s",
> +                         strerror(-ret));
> +        }
>          exit(1);
>      }
>  
> @@ -2991,10 +3009,15 @@ int kvm_cpu_exec(CPUState *cpu)
>          MemTxAttrs attrs;
>  
>          if (cpu->vcpu_dirty) {
> -            ret = kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE);
> +            Error *err = NULL;
> +            ret = kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE, &err);
>              if (ret) {
> -                error_report("Failed to put registers after init: %s",
> -                             strerror(-ret));
> +                if (err) {
> +                    error_reportf_err(err, "Putting registers after init");
> +                } else {
> +                    error_report("Failed to put registers after init: %s",
> +                                 strerror(-ret));
> +                }
>                  ret = -1;
>                  break;
>              }
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 613d3f7581..c3a60b2890 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -359,7 +359,7 @@ int kvm_arch_handle_exit(CPUState *cpu, struct kvm_run *run);
>  
>  int kvm_arch_process_async_events(CPUState *cpu);
>  
> -int kvm_arch_get_registers(CPUState *cpu);
> +int kvm_arch_get_registers(CPUState *cpu, Error **errp);
>  
>  /* state subset only touched by the VCPU itself during runtime */
>  #define KVM_PUT_RUNTIME_STATE   1
> @@ -368,7 +368,7 @@ int kvm_arch_get_registers(CPUState *cpu);
>  /* full state set, modified during initialization or on vmload */
>  #define KVM_PUT_FULL_STATE      3
>  
> -int kvm_arch_put_registers(CPUState *cpu, int level);
> +int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp);
>  
>  int kvm_arch_get_default_type(MachineState *ms);
>  
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 849e2e21b3..f1f1b5b375 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -2042,7 +2042,7 @@ static int kvm_arch_put_sve(CPUState *cs)
>      return 0;
>  }
>  
> -int kvm_arch_put_registers(CPUState *cs, int level)
> +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
>  {
>      uint64_t val;
>      uint32_t fpr;
> @@ -2226,7 +2226,7 @@ static int kvm_arch_get_sve(CPUState *cs)
>      return 0;
>  }
>  
> -int kvm_arch_get_registers(CPUState *cs)
> +int kvm_arch_get_registers(CPUState *cs, Error **errp)
>  {
>      uint64_t val;
>      unsigned int el;
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index ada581c5d6..039ed08825 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5118,7 +5118,7 @@ static int kvm_get_nested_state(X86CPU *cpu)
>      return ret;
>  }
>  
> -int kvm_arch_put_registers(CPUState *cpu, int level)
> +int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
>  {
>      X86CPU *x86_cpu = X86_CPU(cpu);
>      int ret;
> @@ -5206,7 +5206,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
>      return 0;
>  }
>  
> -int kvm_arch_get_registers(CPUState *cs)
> +int kvm_arch_get_registers(CPUState *cs, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      int ret;
> diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c
> index 4786cd5efa..30ec16025d 100644
> --- a/target/loongarch/kvm/kvm.c
> +++ b/target/loongarch/kvm/kvm.c
> @@ -588,7 +588,7 @@ static int kvm_loongarch_put_cpucfg(CPUState *cs)
>      return ret;
>  }
>  
> -int kvm_arch_get_registers(CPUState *cs)
> +int kvm_arch_get_registers(CPUState *cs, Error **errp)
>  {
>      int ret;
>  
> @@ -616,7 +616,7 @@ int kvm_arch_get_registers(CPUState *cs)
>      return ret;
>  }
>  
> -int kvm_arch_put_registers(CPUState *cs, int level)
> +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
>  {
>      int ret;
>  
> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> index a631ab544f..a98798c669 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -1172,7 +1172,7 @@ static int kvm_mips_get_cp0_registers(CPUState *cs)
>      return ret;
>  }
>  
> -int kvm_arch_put_registers(CPUState *cs, int level)
> +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
>  {
>      CPUMIPSState *env = cpu_env(cs);
>      struct kvm_regs regs;
> @@ -1207,7 +1207,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      return ret;
>  }
>  
> -int kvm_arch_get_registers(CPUState *cs)
> +int kvm_arch_get_registers(CPUState *cs, Error **errp)
>  {
>      CPUMIPSState *env = cpu_env(cs);
>      int ret = 0;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 907dba60d1..a361d9d23c 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -900,7 +900,7 @@ int kvmppc_put_books_sregs(PowerPCCPU *cpu)
>      return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_SREGS, &sregs);
>  }
>  
> -int kvm_arch_put_registers(CPUState *cs, int level)
> +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index f6e3156b8d..2bfb112be0 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1192,7 +1192,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>      KVM_CAP_LAST_INFO
>  };
>  
> -int kvm_arch_get_registers(CPUState *cs)
> +int kvm_arch_get_registers(CPUState *cs, Error **errp)
>  {
>      int ret = 0;
>  
> @@ -1237,7 +1237,7 @@ int kvm_riscv_sync_mpstate_to_kvm(RISCVCPU *cpu, int state)
>      return 0;
>  }
>  
> -int kvm_arch_put_registers(CPUState *cs, int level)
> +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
>  {
>      int ret = 0;
>  
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 94181d9281..8ffe0159d8 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -472,7 +472,7 @@ static int can_sync_regs(CPUState *cs, int regs)
>  #define KVM_SYNC_REQUIRED_REGS (KVM_SYNC_GPRS | KVM_SYNC_ACRS | \
>                                  KVM_SYNC_CRS | KVM_SYNC_PREFIX)
>  
> -int kvm_arch_put_registers(CPUState *cs, int level)
> +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
>  {
>      CPUS390XState *env = cpu_env(cs);
>      struct kvm_fpu fpu = {};
> @@ -598,7 +598,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      return 0;
>  }
>  
> -int kvm_arch_get_registers(CPUState *cs)
> +int kvm_arch_get_registers(CPUState *cs, Error **errp)
>  {
>      CPUS390XState *env = cpu_env(cs);
>      struct kvm_fpu fpu;
> -- 
> 2.45.0
> 

-- 
Peter Xu

Re: [PATCH 2/2] target/i386/kvm: Report which action failed in kvm_arch_put/get_registers

[Peter Xu]

On Wed, Sep 25, 2024 at 05:36:23PM +0200, Julia Suvorova wrote:
> To help debug and triage future failure reports (akin to [1,2]) that
> may occur during kvm_arch_put/get_registers, the error path of each
> action is accompanied by unique error message.
> 
> [1] https://issues.redhat.com/browse/RHEL-7558
> [2] https://issues.redhat.com/browse/RHEL-21761
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 1/2] kvm: Allow kvm_arch_get/put_registers to accept Error**

[Julia Suvorova]

On Wed, Sep 25, 2024 at 8:53 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Sep 25, 2024 at 05:36:22PM +0200, Julia Suvorova wrote:
> > This is necessary to provide discernible error messages to the caller.
> >
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> One nitpick below:
>
> > ---
> >  accel/kvm/kvm-all.c        | 41 +++++++++++++++++++++++++++++---------
> >  include/sysemu/kvm.h       |  4 ++--
> >  target/arm/kvm.c           |  4 ++--
> >  target/i386/kvm/kvm.c      |  4 ++--
> >  target/loongarch/kvm/kvm.c |  4 ++--
> >  target/mips/kvm.c          |  4 ++--
> >  target/ppc/kvm.c           |  2 +-
> >  target/riscv/kvm/kvm-cpu.c |  4 ++--
> >  target/s390x/kvm/kvm.c     |  4 ++--
> >  9 files changed, 47 insertions(+), 24 deletions(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index fe4cd721d9..19e09067b3 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -2762,9 +2762,15 @@ void kvm_flush_coalesced_mmio_buffer(void)
> >  static void do_kvm_cpu_synchronize_state(CPUState *cpu, run_on_cpu_data arg)
> >  {
> >      if (!cpu->vcpu_dirty && !kvm_state->guest_state_protected) {
> > -        int ret = kvm_arch_get_registers(cpu);
> > +        Error *err = NULL;
> > +        int ret = kvm_arch_get_registers(cpu, &err);
> >          if (ret) {
> > -            error_report("Failed to get registers: %s", strerror(-ret));
> > +            if (err) {
> > +                error_report_err(err);
> > +            } else {
> > +                error_report("Failed to get registers: %s", strerror(-ret));
> > +            }
> > +
> >              cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
> >              vm_stop(RUN_STATE_INTERNAL_ERROR);
> >          }
> > @@ -2782,9 +2788,15 @@ void kvm_cpu_synchronize_state(CPUState *cpu)
> >
> >  static void do_kvm_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg)
> >  {
> > -    int ret = kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
> > +    Error *err = NULL;
> > +    int ret = kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE, &err);
> >      if (ret) {
> > -        error_report("Failed to put registers after reset: %s", strerror(-ret));
> > +        if (err) {
> > +            error_reportf_err(err, "Restoring resisters after reset");
>
> IMO it'll be nice to use the same error report pattern, either
> error_report_err() or error_reportf_err() for all the four call sites.
>
> If to use error_reportf_err(), better add comma at the end (e.g. "PREFIX: "
> instead of "PREFIX") to make the print looks slightly better.

Good call, I'll fix it :)

Best regards, Julia Suvorova.

> Thanks!
>
> > +        } else {
> > +            error_report("Failed to put registers after reset: %s",
> > +                         strerror(-ret));
> > +        }
> >          cpu_dump_state(cpu, stderr, CPU_DUMP_CODE);
> >          vm_stop(RUN_STATE_INTERNAL_ERROR);
> >      }
> > @@ -2799,9 +2811,15 @@ void kvm_cpu_synchronize_post_reset(CPUState *cpu)
> >
> >  static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg)
> >  {
> > -    int ret = kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
> > +    Error *err = NULL;
> > +    int ret = kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE, &err);
> >      if (ret) {
> > -        error_report("Failed to put registers after init: %s", strerror(-ret));
> > +        if (err) {
> > +            error_reportf_err(err, "Putting registers after init");
> > +        } else {
> > +            error_report("Failed to put registers after init: %s",
> > +                         strerror(-ret));
> > +        }
> >          exit(1);
> >      }
> >
> > @@ -2991,10 +3009,15 @@ int kvm_cpu_exec(CPUState *cpu)
> >          MemTxAttrs attrs;
> >
> >          if (cpu->vcpu_dirty) {
> > -            ret = kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE);
> > +            Error *err = NULL;
> > +            ret = kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE, &err);
> >              if (ret) {
> > -                error_report("Failed to put registers after init: %s",
> > -                             strerror(-ret));
> > +                if (err) {
> > +                    error_reportf_err(err, "Putting registers after init");
> > +                } else {
> > +                    error_report("Failed to put registers after init: %s",
> > +                                 strerror(-ret));
> > +                }
> >                  ret = -1;
> >                  break;
> >              }
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 613d3f7581..c3a60b2890 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -359,7 +359,7 @@ int kvm_arch_handle_exit(CPUState *cpu, struct kvm_run *run);
> >
> >  int kvm_arch_process_async_events(CPUState *cpu);
> >
> > -int kvm_arch_get_registers(CPUState *cpu);
> > +int kvm_arch_get_registers(CPUState *cpu, Error **errp);
> >
> >  /* state subset only touched by the VCPU itself during runtime */
> >  #define KVM_PUT_RUNTIME_STATE   1
> > @@ -368,7 +368,7 @@ int kvm_arch_get_registers(CPUState *cpu);
> >  /* full state set, modified during initialization or on vmload */
> >  #define KVM_PUT_FULL_STATE      3
> >
> > -int kvm_arch_put_registers(CPUState *cpu, int level);
> > +int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp);
> >
> >  int kvm_arch_get_default_type(MachineState *ms);
> >
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 849e2e21b3..f1f1b5b375 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -2042,7 +2042,7 @@ static int kvm_arch_put_sve(CPUState *cs)
> >      return 0;
> >  }
> >
> > -int kvm_arch_put_registers(CPUState *cs, int level)
> > +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
> >  {
> >      uint64_t val;
> >      uint32_t fpr;
> > @@ -2226,7 +2226,7 @@ static int kvm_arch_get_sve(CPUState *cs)
> >      return 0;
> >  }
> >
> > -int kvm_arch_get_registers(CPUState *cs)
> > +int kvm_arch_get_registers(CPUState *cs, Error **errp)
> >  {
> >      uint64_t val;
> >      unsigned int el;
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index ada581c5d6..039ed08825 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -5118,7 +5118,7 @@ static int kvm_get_nested_state(X86CPU *cpu)
> >      return ret;
> >  }
> >
> > -int kvm_arch_put_registers(CPUState *cpu, int level)
> > +int kvm_arch_put_registers(CPUState *cpu, int level, Error **errp)
> >  {
> >      X86CPU *x86_cpu = X86_CPU(cpu);
> >      int ret;
> > @@ -5206,7 +5206,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
> >      return 0;
> >  }
> >
> > -int kvm_arch_get_registers(CPUState *cs)
> > +int kvm_arch_get_registers(CPUState *cs, Error **errp)
> >  {
> >      X86CPU *cpu = X86_CPU(cs);
> >      int ret;
> > diff --git a/target/loongarch/kvm/kvm.c b/target/loongarch/kvm/kvm.c
> > index 4786cd5efa..30ec16025d 100644
> > --- a/target/loongarch/kvm/kvm.c
> > +++ b/target/loongarch/kvm/kvm.c
> > @@ -588,7 +588,7 @@ static int kvm_loongarch_put_cpucfg(CPUState *cs)
> >      return ret;
> >  }
> >
> > -int kvm_arch_get_registers(CPUState *cs)
> > +int kvm_arch_get_registers(CPUState *cs, Error **errp)
> >  {
> >      int ret;
> >
> > @@ -616,7 +616,7 @@ int kvm_arch_get_registers(CPUState *cs)
> >      return ret;
> >  }
> >
> > -int kvm_arch_put_registers(CPUState *cs, int level)
> > +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
> >  {
> >      int ret;
> >
> > diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> > index a631ab544f..a98798c669 100644
> > --- a/target/mips/kvm.c
> > +++ b/target/mips/kvm.c
> > @@ -1172,7 +1172,7 @@ static int kvm_mips_get_cp0_registers(CPUState *cs)
> >      return ret;
> >  }
> >
> > -int kvm_arch_put_registers(CPUState *cs, int level)
> > +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
> >  {
> >      CPUMIPSState *env = cpu_env(cs);
> >      struct kvm_regs regs;
> > @@ -1207,7 +1207,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> >      return ret;
> >  }
> >
> > -int kvm_arch_get_registers(CPUState *cs)
> > +int kvm_arch_get_registers(CPUState *cs, Error **errp)
> >  {
> >      CPUMIPSState *env = cpu_env(cs);
> >      int ret = 0;
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 907dba60d1..a361d9d23c 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -900,7 +900,7 @@ int kvmppc_put_books_sregs(PowerPCCPU *cpu)
> >      return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_SREGS, &sregs);
> >  }
> >
> > -int kvm_arch_put_registers(CPUState *cs, int level)
> > +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
> >  {
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >      CPUPPCState *env = &cpu->env;
> > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > index f6e3156b8d..2bfb112be0 100644
> > --- a/target/riscv/kvm/kvm-cpu.c
> > +++ b/target/riscv/kvm/kvm-cpu.c
> > @@ -1192,7 +1192,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
> >      KVM_CAP_LAST_INFO
> >  };
> >
> > -int kvm_arch_get_registers(CPUState *cs)
> > +int kvm_arch_get_registers(CPUState *cs, Error **errp)
> >  {
> >      int ret = 0;
> >
> > @@ -1237,7 +1237,7 @@ int kvm_riscv_sync_mpstate_to_kvm(RISCVCPU *cpu, int state)
> >      return 0;
> >  }
> >
> > -int kvm_arch_put_registers(CPUState *cs, int level)
> > +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
> >  {
> >      int ret = 0;
> >
> > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> > index 94181d9281..8ffe0159d8 100644
> > --- a/target/s390x/kvm/kvm.c
> > +++ b/target/s390x/kvm/kvm.c
> > @@ -472,7 +472,7 @@ static int can_sync_regs(CPUState *cs, int regs)
> >  #define KVM_SYNC_REQUIRED_REGS (KVM_SYNC_GPRS | KVM_SYNC_ACRS | \
> >                                  KVM_SYNC_CRS | KVM_SYNC_PREFIX)
> >
> > -int kvm_arch_put_registers(CPUState *cs, int level)
> > +int kvm_arch_put_registers(CPUState *cs, int level, Error **errp)
> >  {
> >      CPUS390XState *env = cpu_env(cs);
> >      struct kvm_fpu fpu = {};
> > @@ -598,7 +598,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> >      return 0;
> >  }
> >
> > -int kvm_arch_get_registers(CPUState *cs)
> > +int kvm_arch_get_registers(CPUState *cs, Error **errp)
> >  {
> >      CPUS390XState *env = cpu_env(cs);
> >      struct kvm_fpu fpu;
> > --
> > 2.45.0
> >
>
> --
> Peter Xu
>