diff mbox

[v2,4/4] target/arm/kvm: pmu: improve error handling

Message ID 1500471597-2517-5-git-send-email-drjones@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Jones July 19, 2017, 1:39 p.m. UTC 
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 <drjones@redhat.com>
---
 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(-)

Comments

Christoffer Dall July 21, 2017, 11:29 a.m. UTC | #1 
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 <drjones@redhat.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  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
>
Peter Maydell Sept. 4, 2017, 2:17 p.m. UTC | #2 
On 19 July 2017 at 14:39, Andrew Jones <drjones@redhat.com> 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 <drjones@redhat.com>
> ---
>  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(-)

This breaks compile on 32-bit ARM:

/home/peter.maydell/qemu/target/arm/kvm32.c:530:5: error: conflicting
types for 'kvm_arm_pmu_init'
 int kvm_arm_pmu_init(CPUState *cs)
     ^
In file included from /home/peter.maydell/qemu/target/arm/kvm32.c:21:0:
/home/peter.maydell/qemu/target/arm/kvm_arm.h:199:6: note: previous
declaration of 'kvm_arm_pmu_init' w
as here
 void kvm_arm_pmu_init(CPUState *cs);
      ^

Looks like you forgot to update the 32-bit version of the function
to the new API; I'll just fix up the patch in my queue.

thanks
-- PMM
Andrew Jones Sept. 5, 2017, 2:44 p.m. UTC | #3 
On Mon, Sep 04, 2017 at 03:17:21PM +0100, Peter Maydell wrote:
> On 19 July 2017 at 14:39, Andrew Jones <drjones@redhat.com> 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 <drjones@redhat.com>
> > ---
> >  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(-)
> 
> This breaks compile on 32-bit ARM:
> 
> /home/peter.maydell/qemu/target/arm/kvm32.c:530:5: error: conflicting
> types for 'kvm_arm_pmu_init'
>  int kvm_arm_pmu_init(CPUState *cs)
>      ^
> In file included from /home/peter.maydell/qemu/target/arm/kvm32.c:21:0:
> /home/peter.maydell/qemu/target/arm/kvm_arm.h:199:6: note: previous
> declaration of 'kvm_arm_pmu_init' w
> as here
>  void kvm_arm_pmu_init(CPUState *cs);
>       ^

Argh. Sorry about that. I could have sworn I compile-tested for 32-bit
ARM. Maybe I changed the series afterwards and forgot to compile-test
again...

> 
> Looks like you forgot to update the 32-bit version of the function
> to the new API; I'll just fix up the patch in my queue.

Thanks!

drew
diff mbox

Patch

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