All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] target/arm: Propagate errno when writing list
@ 2022-12-01 10:33 ` Akihiko Odaki
  0 siblings, 0 replies; 4+ messages in thread
From: Akihiko Odaki @ 2022-12-01 10:33 UTC (permalink / raw)
  Cc: qemu-devel, qemu-arm, kvm, Peter Maydell, Paolo Bonzini, Akihiko Odaki

Before this change, write_kvmstate_to_list() and
write_list_to_kvmstate() tolerated even if it failed to access some
register, and returned a bool indicating whether one of the register
accesses failed. However, it does not make sen not to fail early as the
the callers check the returned value and fail early anyway.

So let write_kvmstate_to_list() and write_list_to_kvmstate() fail early
too. This will allow to propagate errno to the callers and log it if
appropriate.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/arm/kvm-stub.c |  4 ++--
 target/arm/kvm.c      | 27 +++++++++++----------------
 target/arm/kvm64.c    | 10 ++++++----
 target/arm/kvm_arm.h  | 15 ++++-----------
 target/arm/machine.c  | 13 ++++++++-----
 5 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/target/arm/kvm-stub.c b/target/arm/kvm-stub.c
index 965a486b32..f659afd7b8 100644
--- a/target/arm/kvm-stub.c
+++ b/target/arm/kvm-stub.c
@@ -13,12 +13,12 @@
 #include "cpu.h"
 #include "kvm_arm.h"
 
-bool write_kvmstate_to_list(ARMCPU *cpu)
+int write_kvmstate_to_list(ARMCPU *cpu)
 {
     g_assert_not_reached();
 }
 
-bool write_list_to_kvmstate(ARMCPU *cpu, int level)
+int write_list_to_kvmstate(ARMCPU *cpu, int level)
 {
     g_assert_not_reached();
 }
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index f022c644d2..4cef0efc96 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -501,12 +501,12 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
     }
     assert(cpu->cpreg_array_len == arraylen);
 
-    if (!write_kvmstate_to_list(cpu)) {
+    ret = write_kvmstate_to_list(cpu);
+    if (ret) {
         /* Shouldn't happen unless kernel is inconsistent about
          * what registers exist.
          */
         fprintf(stderr, "Initial read of kernel register state failed\n");
-        ret = -EINVAL;
         goto out;
     }
 
@@ -515,11 +515,10 @@ out:
     return ret;
 }
 
-bool write_kvmstate_to_list(ARMCPU *cpu)
+int write_kvmstate_to_list(ARMCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
     int i;
-    bool ok = true;
 
     for (i = 0; i < cpu->cpreg_array_len; i++) {
         struct kvm_one_reg r;
@@ -545,17 +544,16 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
             g_assert_not_reached();
         }
         if (ret) {
-            ok = false;
+            return ret;
         }
     }
-    return ok;
+    return 0;
 }
 
-bool write_list_to_kvmstate(ARMCPU *cpu, int level)
+int write_list_to_kvmstate(ARMCPU *cpu, int level)
 {
     CPUState *cs = CPU(cpu);
     int i;
-    bool ok = true;
 
     for (i = 0; i < cpu->cpreg_array_len; i++) {
         struct kvm_one_reg r;
@@ -581,14 +579,10 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
         }
         ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &r);
         if (ret) {
-            /* We might fail for "unknown register" and also for
-             * "you tried to set a register which is constant with
-             * a different value from what it actually contains".
-             */
-            ok = false;
+            return ret;
         }
     }
-    return ok;
+    return 0;
 }
 
 void kvm_arm_cpu_pre_save(ARMCPU *cpu)
@@ -620,8 +614,9 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
         fprintf(stderr, "kvm_arm_vcpu_init failed: %s\n", strerror(-ret));
         abort();
     }
-    if (!write_kvmstate_to_list(cpu)) {
-        fprintf(stderr, "write_kvmstate_to_list failed\n");
+    ret = write_kvmstate_to_list(cpu);
+    if (ret < 0) {
+        fprintf(stderr, "write_kvmstate_to_list failed: %s\n", strerror(-ret));
         abort();
     }
     /*
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 1197253d12..f7879194f9 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1202,8 +1202,9 @@ int kvm_arch_put_registers(CPUState *cs, int level)
 
     write_cpustate_to_list(cpu, true);
 
-    if (!write_list_to_kvmstate(cpu, level)) {
-        return -EINVAL;
+    ret = write_list_to_kvmstate(cpu, level);
+    if (ret) {
+        return ret;
     }
 
    /*
@@ -1418,8 +1419,9 @@ int kvm_arch_get_registers(CPUState *cs)
         return ret;
     }
 
-    if (!write_kvmstate_to_list(cpu)) {
-        return -EINVAL;
+    ret = write_kvmstate_to_list(cpu);
+    if (ret) {
+        return ret;
     }
     /* Note that it's OK to have registers which aren't in CPUState,
      * so we can ignore a failure return here.
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 99017b635c..05fce74baf 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -106,13 +106,9 @@ int kvm_arm_cpreg_level(uint64_t regidx);
  * This updates KVM's working data structures from TCG data or
  * from incoming migration state.
  *
- * Returns: true if all register values were updated correctly,
- * false if some register was unknown to the kernel or could not
- * be written (eg constant register with the wrong value).
- * Note that we do not stop early on failure -- we will attempt
- * writing all registers in the list.
+ * Returns: 0 if success, else < 0 error code
  */
-bool write_list_to_kvmstate(ARMCPU *cpu, int level);
+int write_list_to_kvmstate(ARMCPU *cpu, int level);
 
 /**
  * write_kvmstate_to_list:
@@ -123,12 +119,9 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level);
  * copy info from KVM's working data structures into TCG or
  * for outbound migration.
  *
- * Returns: true if all register values were read correctly,
- * false if some register was unknown or could not be read.
- * Note that we do not stop early on failure -- we will attempt
- * reading all registers in the list.
+ * Returns: 0 if success, else < 0 error code
  */
-bool write_kvmstate_to_list(ARMCPU *cpu);
+int write_kvmstate_to_list(ARMCPU *cpu);
 
 /**
  * kvm_arm_cpu_pre_save:
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 54c5c62433..8a5f3d53ff 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -686,15 +686,16 @@ static const VMStateInfo vmstate_powered_off = {
 static int cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
+    int ret;
 
     if (!kvm_enabled()) {
         pmu_op_start(&cpu->env);
     }
 
     if (kvm_enabled()) {
-        if (!write_kvmstate_to_list(cpu)) {
-            /* This should never fail */
-            g_assert_not_reached();
+        ret = write_kvmstate_to_list(cpu);
+        if (ret) {
+            return ret;
         }
 
         /*
@@ -752,7 +753,7 @@ static int cpu_post_load(void *opaque, int version_id)
 {
     ARMCPU *cpu = opaque;
     CPUARMState *env = &cpu->env;
-    int i, v;
+    int i, v, ret;
 
     /*
      * Handle migration compatibility from old QEMU which didn't
@@ -796,7 +797,9 @@ static int cpu_post_load(void *opaque, int version_id)
     }
 
     if (kvm_enabled()) {
-        if (!write_list_to_kvmstate(cpu, KVM_PUT_FULL_STATE)) {
+        ret = write_list_to_kvmstate(cpu, KVM_PUT_FULL_STATE);
+        if (ret) {
+            error_report("Failed to set KVM register: %s", strerror(-ret));
             return -1;
         }
         /* Note that it's OK for the TCG side not to know about
-- 
2.38.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH] target/arm: Propagate errno when writing list
@ 2022-12-01 10:33 ` Akihiko Odaki
  0 siblings, 0 replies; 4+ messages in thread
From: Akihiko Odaki @ 2022-12-01 10:33 UTC (permalink / raw)
  Cc: qemu-devel, qemu-arm, kvm, Peter Maydell, Paolo Bonzini, Akihiko Odaki

Before this change, write_kvmstate_to_list() and
write_list_to_kvmstate() tolerated even if it failed to access some
register, and returned a bool indicating whether one of the register
accesses failed. However, it does not make sen not to fail early as the
the callers check the returned value and fail early anyway.

So let write_kvmstate_to_list() and write_list_to_kvmstate() fail early
too. This will allow to propagate errno to the callers and log it if
appropriate.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/arm/kvm-stub.c |  4 ++--
 target/arm/kvm.c      | 27 +++++++++++----------------
 target/arm/kvm64.c    | 10 ++++++----
 target/arm/kvm_arm.h  | 15 ++++-----------
 target/arm/machine.c  | 13 ++++++++-----
 5 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/target/arm/kvm-stub.c b/target/arm/kvm-stub.c
index 965a486b32..f659afd7b8 100644
--- a/target/arm/kvm-stub.c
+++ b/target/arm/kvm-stub.c
@@ -13,12 +13,12 @@
 #include "cpu.h"
 #include "kvm_arm.h"
 
-bool write_kvmstate_to_list(ARMCPU *cpu)
+int write_kvmstate_to_list(ARMCPU *cpu)
 {
     g_assert_not_reached();
 }
 
-bool write_list_to_kvmstate(ARMCPU *cpu, int level)
+int write_list_to_kvmstate(ARMCPU *cpu, int level)
 {
     g_assert_not_reached();
 }
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index f022c644d2..4cef0efc96 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -501,12 +501,12 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
     }
     assert(cpu->cpreg_array_len == arraylen);
 
-    if (!write_kvmstate_to_list(cpu)) {
+    ret = write_kvmstate_to_list(cpu);
+    if (ret) {
         /* Shouldn't happen unless kernel is inconsistent about
          * what registers exist.
          */
         fprintf(stderr, "Initial read of kernel register state failed\n");
-        ret = -EINVAL;
         goto out;
     }
 
@@ -515,11 +515,10 @@ out:
     return ret;
 }
 
-bool write_kvmstate_to_list(ARMCPU *cpu)
+int write_kvmstate_to_list(ARMCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
     int i;
-    bool ok = true;
 
     for (i = 0; i < cpu->cpreg_array_len; i++) {
         struct kvm_one_reg r;
@@ -545,17 +544,16 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
             g_assert_not_reached();
         }
         if (ret) {
-            ok = false;
+            return ret;
         }
     }
-    return ok;
+    return 0;
 }
 
-bool write_list_to_kvmstate(ARMCPU *cpu, int level)
+int write_list_to_kvmstate(ARMCPU *cpu, int level)
 {
     CPUState *cs = CPU(cpu);
     int i;
-    bool ok = true;
 
     for (i = 0; i < cpu->cpreg_array_len; i++) {
         struct kvm_one_reg r;
@@ -581,14 +579,10 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
         }
         ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &r);
         if (ret) {
-            /* We might fail for "unknown register" and also for
-             * "you tried to set a register which is constant with
-             * a different value from what it actually contains".
-             */
-            ok = false;
+            return ret;
         }
     }
-    return ok;
+    return 0;
 }
 
 void kvm_arm_cpu_pre_save(ARMCPU *cpu)
@@ -620,8 +614,9 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
         fprintf(stderr, "kvm_arm_vcpu_init failed: %s\n", strerror(-ret));
         abort();
     }
-    if (!write_kvmstate_to_list(cpu)) {
-        fprintf(stderr, "write_kvmstate_to_list failed\n");
+    ret = write_kvmstate_to_list(cpu);
+    if (ret < 0) {
+        fprintf(stderr, "write_kvmstate_to_list failed: %s\n", strerror(-ret));
         abort();
     }
     /*
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 1197253d12..f7879194f9 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1202,8 +1202,9 @@ int kvm_arch_put_registers(CPUState *cs, int level)
 
     write_cpustate_to_list(cpu, true);
 
-    if (!write_list_to_kvmstate(cpu, level)) {
-        return -EINVAL;
+    ret = write_list_to_kvmstate(cpu, level);
+    if (ret) {
+        return ret;
     }
 
    /*
@@ -1418,8 +1419,9 @@ int kvm_arch_get_registers(CPUState *cs)
         return ret;
     }
 
-    if (!write_kvmstate_to_list(cpu)) {
-        return -EINVAL;
+    ret = write_kvmstate_to_list(cpu);
+    if (ret) {
+        return ret;
     }
     /* Note that it's OK to have registers which aren't in CPUState,
      * so we can ignore a failure return here.
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 99017b635c..05fce74baf 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -106,13 +106,9 @@ int kvm_arm_cpreg_level(uint64_t regidx);
  * This updates KVM's working data structures from TCG data or
  * from incoming migration state.
  *
- * Returns: true if all register values were updated correctly,
- * false if some register was unknown to the kernel or could not
- * be written (eg constant register with the wrong value).
- * Note that we do not stop early on failure -- we will attempt
- * writing all registers in the list.
+ * Returns: 0 if success, else < 0 error code
  */
-bool write_list_to_kvmstate(ARMCPU *cpu, int level);
+int write_list_to_kvmstate(ARMCPU *cpu, int level);
 
 /**
  * write_kvmstate_to_list:
@@ -123,12 +119,9 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level);
  * copy info from KVM's working data structures into TCG or
  * for outbound migration.
  *
- * Returns: true if all register values were read correctly,
- * false if some register was unknown or could not be read.
- * Note that we do not stop early on failure -- we will attempt
- * reading all registers in the list.
+ * Returns: 0 if success, else < 0 error code
  */
-bool write_kvmstate_to_list(ARMCPU *cpu);
+int write_kvmstate_to_list(ARMCPU *cpu);
 
 /**
  * kvm_arm_cpu_pre_save:
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 54c5c62433..8a5f3d53ff 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -686,15 +686,16 @@ static const VMStateInfo vmstate_powered_off = {
 static int cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
+    int ret;
 
     if (!kvm_enabled()) {
         pmu_op_start(&cpu->env);
     }
 
     if (kvm_enabled()) {
-        if (!write_kvmstate_to_list(cpu)) {
-            /* This should never fail */
-            g_assert_not_reached();
+        ret = write_kvmstate_to_list(cpu);
+        if (ret) {
+            return ret;
         }
 
         /*
@@ -752,7 +753,7 @@ static int cpu_post_load(void *opaque, int version_id)
 {
     ARMCPU *cpu = opaque;
     CPUARMState *env = &cpu->env;
-    int i, v;
+    int i, v, ret;
 
     /*
      * Handle migration compatibility from old QEMU which didn't
@@ -796,7 +797,9 @@ static int cpu_post_load(void *opaque, int version_id)
     }
 
     if (kvm_enabled()) {
-        if (!write_list_to_kvmstate(cpu, KVM_PUT_FULL_STATE)) {
+        ret = write_list_to_kvmstate(cpu, KVM_PUT_FULL_STATE);
+        if (ret) {
+            error_report("Failed to set KVM register: %s", strerror(-ret));
             return -1;
         }
         /* Note that it's OK for the TCG side not to know about
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] target/arm: Propagate errno when writing list
  2022-12-01 10:33 ` Akihiko Odaki
  (?)
@ 2023-01-24 16:12 ` Peter Maydell
  2023-01-25  4:10   ` Akihiko Odaki
  -1 siblings, 1 reply; 4+ messages in thread
From: Peter Maydell @ 2023-01-24 16:12 UTC (permalink / raw)
  To: Akihiko Odaki; +Cc: qemu-devel, qemu-arm, kvm, Paolo Bonzini

On Thu, 1 Dec 2022 at 10:33, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Before this change, write_kvmstate_to_list() and
> write_list_to_kvmstate() tolerated even if it failed to access some
> register, and returned a bool indicating whether one of the register
> accesses failed. However, it does not make sen not to fail early as the
> the callers check the returned value and fail early anyway.
>
> So let write_kvmstate_to_list() and write_list_to_kvmstate() fail early
> too. This will allow to propagate errno to the callers and log it if
> appropriate.

(Sorry this one didn't get reviewed earlier.)

I agree that all the callers of these functions check for
failure, so there's no major benefit from doing the
don't-fail-early logic. But is there a reason why we should
actively make this change?

In particular, these functions form part of a family with the
similar write_cpustate_to_list() and write_list_to_cpustate(),
and it's inconsistent to have the kvmstate ones return
negative-errno while the cpustate ones still return bool.
For the cpustate ones we *do* rely in some places on
the "don't fail early" behaviour. The kvmstate ones do the
same thing I think mostly for consistency.

So unless there's a specific reason why changing these
functions improves behaviour as seen by users, I think
I favour retaining the consistency.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] target/arm: Propagate errno when writing list
  2023-01-24 16:12 ` Peter Maydell
@ 2023-01-25  4:10   ` Akihiko Odaki
  0 siblings, 0 replies; 4+ messages in thread
From: Akihiko Odaki @ 2023-01-25  4:10 UTC (permalink / raw)
  To: Peter Maydell; +Cc: qemu-devel, qemu-arm, kvm, Paolo Bonzini

On 2023/01/25 1:12, Peter Maydell wrote:
> On Thu, 1 Dec 2022 at 10:33, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> Before this change, write_kvmstate_to_list() and
>> write_list_to_kvmstate() tolerated even if it failed to access some
>> register, and returned a bool indicating whether one of the register
>> accesses failed. However, it does not make sen not to fail early as the
>> the callers check the returned value and fail early anyway.
>>
>> So let write_kvmstate_to_list() and write_list_to_kvmstate() fail early
>> too. This will allow to propagate errno to the callers and log it if
>> appropriate.
> 
> (Sorry this one didn't get reviewed earlier.)
> 
> I agree that all the callers of these functions check for
> failure, so there's no major benefit from doing the
> don't-fail-early logic. But is there a reason why we should
> actively make this change?
> 
> In particular, these functions form part of a family with the
> similar write_cpustate_to_list() and write_list_to_cpustate(),
> and it's inconsistent to have the kvmstate ones return
> negative-errno while the cpustate ones still return bool.
> For the cpustate ones we *do* rely in some places on
> the "don't fail early" behaviour. The kvmstate ones do the
> same thing I think mostly for consistency.
> 
> So unless there's a specific reason why changing these
> functions improves behaviour as seen by users, I think
> I favour retaining the consistency.
> 
> thanks
> -- PMM

I withdraw this patch. The only reason is that it allows to log errno 
when reporting the error, and the benefit is negligible when compared to 
the consistency.

Regards,
Akihiko Odaki

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-01-25  4:10 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-01 10:33 [PATCH] target/arm: Propagate errno when writing list Akihiko Odaki
2022-12-01 10:33 ` Akihiko Odaki
2023-01-24 16:12 ` Peter Maydell
2023-01-25  4:10   ` Akihiko Odaki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.