| Message ID | 20231123044219.896776-21-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: kvm cleanups | expand |
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 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 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.
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;
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(-)