Re: [Qemu-arm] [PATCH v2 4/4] target/arm/kvm: pmu: improve error handling

[Christoffer Dall]

On Wed, Jul 19, 2017 at 09:39:57AM -0400, Andrew Jones wrote:
> If a KVM PMU init or set-irq attr call fails we just silently stop
> the PMU DT node generation. The only way they could fail, though,
> is if the attr's respective KVM has-attr call fails. But that should
> never happen if KVM advertises the PMU capability, because both
> attrs have been available since the capability was introduced. Let's
> just abort if this should-never-happen stuff does happen, because,
> if it does, then something is obviously horribly wrong.
> 
> Signed-off-by: Andrew Jones <address@hidden>

Reviewed-by: Christoffer Dall <address@hidden>

> ---
>  hw/arm/virt.c        |  9 +++------
>  target/arm/kvm32.c   |  3 +--
>  target/arm/kvm64.c   | 28 ++++++++++++++++++++--------
>  target/arm/kvm_arm.h | 15 ++++-----------
>  4 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a215330444da..4bc50964d52b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -496,13 +496,10 @@ static void fdt_add_pmu_nodes(const VirtMachineState 
> *vms)
>              return;
>          }
>          if (kvm_enabled()) {
> -            if (kvm_irqchip_in_kernel() &&
> -                !kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ))) {
> -                return;
> -            }
> -            if (!kvm_arm_pmu_init(cpu)) {
> -                return;
> +            if (kvm_irqchip_in_kernel()) {
> +                kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
>              }
> +            kvm_arm_pmu_init(cpu);
>          }
>      }
>  
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index e3aab89a1a94..717a2562670b 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -522,10 +522,9 @@ bool kvm_arm_hw_debug_active(CPUState *cs)
>      return false;
>  }
>  
> -int kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> +void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
>  {
>      qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> -    return 0;
>  }
>  
>  int kvm_arm_pmu_init(CPUState *cs)
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index ec7d85314acc..6554c30007a4 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -387,30 +387,36 @@ static bool kvm_arm_pmu_set_attr(CPUState *cs, struct 
> kvm_device_attr *attr)
>  
>      err = kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr);
>      if (err != 0) {
> +        error_report("PMU: KVM_HAS_DEVICE_ATTR: %s", strerror(-err));
>          return false;
>      }
>  
>      err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, attr);
> -    if (err < 0) {
> -        fprintf(stderr, "KVM_SET_DEVICE_ATTR failed: %s\n",
> -                strerror(-err));
> -        abort();
> +    if (err != 0) {
> +        error_report("PMU: KVM_SET_DEVICE_ATTR: %s", strerror(-err));
> +        return false;
>      }
>  
>      return true;
>  }
>  
> -int kvm_arm_pmu_init(CPUState *cs)
> +void kvm_arm_pmu_init(CPUState *cs)
>  {
>      struct kvm_device_attr attr = {
>          .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>          .attr = KVM_ARM_VCPU_PMU_V3_INIT,
>      };
>  
> -    return kvm_arm_pmu_set_attr(cs, &attr);
> +    if (!ARM_CPU(cs)->has_pmu) {
> +        return;
> +    }
> +    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> +        error_report("failed to init PMU");
> +        abort();
> +    }
>  }
>  
> -int kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> +void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
>  {
>      struct kvm_device_attr attr = {
>          .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> @@ -418,7 +424,13 @@ int kvm_arm_pmu_set_irq(CPUState *cs, int irq)
>          .attr = KVM_ARM_VCPU_PMU_V3_IRQ,
>      };
>  
> -    return kvm_arm_pmu_set_attr(cs, &attr);
> +    if (!ARM_CPU(cs)->has_pmu) {
> +        return;
> +    }
> +    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> +        error_report("failed to set irq for PMU");
> +        abort();
> +    }
>  }
>  
>  static inline void set_feature(uint64_t *features, int feature)
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index cab5ea9be55c..ff53e9fafb7a 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -195,8 +195,8 @@ int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu);
>  
>  int kvm_arm_vgic_probe(void);
>  
> -int kvm_arm_pmu_set_irq(CPUState *cs, int irq);
> -int kvm_arm_pmu_init(CPUState *cs);
> +void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
> +void kvm_arm_pmu_init(CPUState *cs);
>  
>  #else
>  
> @@ -205,15 +205,8 @@ static inline int kvm_arm_vgic_probe(void)
>      return 0;
>  }
>  
> -static inline int kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> -{
> -    return 0;
> -}
> -
> -static inline int kvm_arm_pmu_init(CPUState *cs)
> -{
> -    return 0;
> -}
> +static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {}
> +static inline void kvm_arm_pmu_init(CPUState *cs) {}
>  
>  #endif
>  
> -- 
> 1.8.3.1
>