Drop fprintfs and actually use the return values in the callers.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm_arm.h | 20 --------------------
 target/arm/kvm.c     | 23 ++++++-----------------
 2 files changed, 6 insertions(+), 37 deletions(-)

diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index b4339d49d1..8a44a6b762 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -200,26 +200,6 @@ bool kvm_arm_sve_supported(void);
  */
 int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa);
 
-/**
- * kvm_arm_sync_mpstate_to_kvm:
- * @cpu: ARMCPU
- *
- * If supported set the KVM MP_STATE based on QEMU's model.
- *
- * Returns 0 on success and -1 on failure.
- */
-int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu);
-
-/**
- * kvm_arm_sync_mpstate_to_qemu:
- * @cpu: ARMCPU
- *
- * If supported get the MP_STATE from KVM and store in QEMU's model.
- *
- * Returns 0 on success and aborts on failure.
- */
-int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu);
-
 void kvm_arm_vm_state_change(void *opaque, bool running, RunState state);
 
 int kvm_arm_vgic_probe(void);
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index f9aa55b1a0..19454f432a 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -1006,41 +1006,32 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
 /*
  * Update KVM's MP_STATE based on what QEMU thinks it is
  */
-int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu)
+static int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu)
 {
     if (cap_has_mp_state) {
         struct kvm_mp_state mp_state = {
             .mp_state = (cpu->power_state == PSCI_OFF) ?
             KVM_MP_STATE_STOPPED : KVM_MP_STATE_RUNNABLE
         };
-        int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
-        if (ret) {
-            fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n",
-                    __func__, ret, strerror(-ret));
-            return -1;
-        }
+        return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
     }
-
     return 0;
 }
 
 /*
  * Sync the KVM MP_STATE into QEMU
  */
-int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
+static int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
 {
     if (cap_has_mp_state) {
         struct kvm_mp_state mp_state;
         int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
         if (ret) {
-            fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
-                    __func__, ret, strerror(-ret));
-            abort();
+            return ret;
         }
         cpu->power_state = (mp_state.mp_state == KVM_MP_STATE_STOPPED) ?
             PSCI_OFF : PSCI_ON;
     }
-
     return 0;
 }
 
@@ -2184,9 +2175,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return ret;
     }
 
-    kvm_arm_sync_mpstate_to_kvm(cpu);
-
-    return ret;
+    return kvm_arm_sync_mpstate_to_kvm(cpu);
 }
 
 static int kvm_arch_get_fpsimd(CPUState *cs)
@@ -2367,7 +2356,7 @@ int kvm_arch_get_registers(CPUState *cs)
      */
     write_list_to_cpustate(cpu);
 
-    kvm_arm_sync_mpstate_to_qemu(cpu);
+    ret = kvm_arm_sync_mpstate_to_qemu(cpu);
 
     /* TODO: other registers */
     return ret;
-- 
2.34.1

On 11/23/23 15:42, Richard Henderson wrote:
> Drop fprintfs and actually use the return values in the callers.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/kvm_arm.h | 20 --------------------
>   target/arm/kvm.c     | 23 ++++++-----------------
>   2 files changed, 6 insertions(+), 37 deletions(-)
> 

Reviewed-by: Gavin Shan <gshan@redhat.com>

On 23/11/23 05:42, Richard Henderson wrote:
> Drop fprintfs and actually use the return values in the callers.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/kvm_arm.h | 20 --------------------
>   target/arm/kvm.c     | 23 ++++++-----------------
>   2 files changed, 6 insertions(+), 37 deletions(-)


>   /*
>    * Sync the KVM MP_STATE into QEMU
>    */
> -int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
> +static int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
>   {
>       if (cap_has_mp_state) {
>           struct kvm_mp_state mp_state;
>           int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
>           if (ret) {
> -            fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
> -                    __func__, ret, strerror(-ret));
> -            abort();

I suppose if this abort() had fired, we'd have reworked that code...
Maybe mention its removal? Otherwise,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +            return ret;
>           }
>           cpu->power_state = (mp_state.mp_state == KVM_MP_STATE_STOPPED) ?
>               PSCI_OFF : PSCI_ON;
>       }
> -
>       return 0;
>   }

On Fri, 24 Nov 2023 at 12:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 23/11/23 05:42, Richard Henderson wrote:
> > Drop fprintfs and actually use the return values in the callers.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   target/arm/kvm_arm.h | 20 --------------------
> >   target/arm/kvm.c     | 23 ++++++-----------------
> >   2 files changed, 6 insertions(+), 37 deletions(-)
>
>
> >   /*
> >    * Sync the KVM MP_STATE into QEMU
> >    */
> > -int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
> > +static int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
> >   {
> >       if (cap_has_mp_state) {
> >           struct kvm_mp_state mp_state;
> >           int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
> >           if (ret) {
> > -            fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
> > -                    __func__, ret, strerror(-ret));
> > -            abort();
>
> I suppose if this abort() had fired, we'd have reworked that code...
> Maybe mention its removal? Otherwise,

Well, it's a "KVM has failed in a way that's fatal for the VM"
kind of error. It's OK to drop the abort() here because since
7191f24c7fcf we will catch error returns from these arch-specific
functions in the accel/kvm generic code. When this was written
before that commit then if we didn't detect and print something
here we'd just have silently dropped the error, I think.

I added a brief note to the commit message to that effect.

-- PMM

On 11/12/23 15:34, Peter Maydell wrote:
> On Fri, 24 Nov 2023 at 12:06, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 23/11/23 05:42, Richard Henderson wrote:
>>> Drop fprintfs and actually use the return values in the callers.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>    target/arm/kvm_arm.h | 20 --------------------
>>>    target/arm/kvm.c     | 23 ++++++-----------------
>>>    2 files changed, 6 insertions(+), 37 deletions(-)
>>
>>
>>>    /*
>>>     * Sync the KVM MP_STATE into QEMU
>>>     */
>>> -int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
>>> +static int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
>>>    {
>>>        if (cap_has_mp_state) {
>>>            struct kvm_mp_state mp_state;
>>>            int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
>>>            if (ret) {
>>> -            fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
>>> -                    __func__, ret, strerror(-ret));
>>> -            abort();
>>
>> I suppose if this abort() had fired, we'd have reworked that code...
>> Maybe mention its removal? Otherwise,
> 
> Well, it's a "KVM has failed in a way that's fatal for the VM"
> kind of error. It's OK to drop the abort() here because since
> 7191f24c7fcf we will catch error returns from these arch-specific
> functions in the accel/kvm generic code. When this was written
> before that commit then if we didn't detect and print something
> here we'd just have silently dropped the error, I think.
> 
> I added a brief note to the commit message to that effect.

Thank you.