[Qemu-devel] [PATCH 0/3] ARM: KVM: Enable in-kernel PMU with user space gic

[Qemu-devel] [PATCH 0/3] ARM: KVM: Enable in-kernel PMU with user space gic

[Andrew Jones]

Andrew Jones (3):
  hw/arm/virt: add pmu interrupt state
  target/arm/kvm: split pmu init from creation
  hw/arm/virt: allow pmu instantiation with userspace irqchip

 hw/arm/virt.c        | 13 +++++++++++--
 target/arm/cpu.c     |  2 ++
 target/arm/cpu.h     |  2 ++
 target/arm/kvm.c     |  6 +++++-
 target/arm/kvm32.c   |  6 ++++++
 target/arm/kvm64.c   | 55 +++++++++++++++++++++++++---------------------------
 target/arm/kvm_arm.h |  6 ++++++
 7 files changed, 58 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] hw/arm/virt: add pmu interrupt state

[Andrew Jones]

Mimicking gicv3-maintenance-interrupt, add the PMU's interrupt to
CPU state.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c    | 3 +++
 target/arm/cpu.c | 2 ++
 target/arm/cpu.h | 2 ++
 3 files changed, 7 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 010f7244bf7c..9781e1cc5ed7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -610,6 +610,9 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
         qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", 0,
                                     qdev_get_gpio_in(gicdev, ppibase
                                                      + ARCH_GICV3_MAINT_IRQ));
+        qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
+                                    qdev_get_gpio_in(gicdev, ppibase
+                                                     + VIRTUAL_PMU_IRQ));
 
         sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
         sysbus_connect_irq(gicbusdev, i + smp_cpus,
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 28a914129857..70cc8f4474af 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -499,6 +499,8 @@ static void arm_cpu_initfn(Object *obj)
 
     qdev_init_gpio_out_named(DEVICE(cpu), &cpu->gicv3_maintenance_interrupt,
                              "gicv3-maintenance-interrupt", 1);
+    qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt,
+                             "pmu-interrupt", 1);
 #endif
 
     /* DTB consumers generally don't in fact care what the 'compatible'
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 102c58afac52..8d91166eb97b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -584,6 +584,8 @@ struct ARMCPU {
     qemu_irq gt_timer_outputs[NUM_GTIMERS];
     /* GPIO output for GICv3 maintenance interrupt signal */
     qemu_irq gicv3_maintenance_interrupt;
+    /* GPIO output for the PMU interrupt */
+    qemu_irq pmu_interrupt;
 
     /* MemoryRegion to use for secure physical accesses */
     MemoryRegion *secure_memory;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/3] hw/arm/virt: add pmu interrupt state

[Peter Maydell]

On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote:
> Mimicking gicv3-maintenance-interrupt, add the PMU's interrupt to
> CPU state.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH 2/3] target/arm/kvm: split pmu init from creation

[Andrew Jones]

When adding a PMU with a userspace irqchip we only do the INIT
stage of the device creation.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c        | 10 ++++++++--
 target/arm/kvm32.c   |  6 ++++++
 target/arm/kvm64.c   | 52 +++++++++++++++++++++++++---------------------------
 target/arm/kvm_arm.h |  6 ++++++
 4 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9781e1cc5ed7..0cb8b479232d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -492,8 +492,14 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
 
     CPU_FOREACH(cpu) {
         armcpu = ARM_CPU(cpu);
-        if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU) ||
-            (kvm_enabled() && !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ)))) {
+        if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
+            return;
+        }
+        if (kvm_enabled() &&
+            !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) {
+            return;
+        }
+        if (kvm_enabled() && !kvm_arm_pmu_init(cpu)) {
             return;
         }
     }
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 069da0c5fd10..a51695f25911 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -527,3 +527,9 @@ int kvm_arm_pmu_create(CPUState *cs, int irq)
     qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
     return 0;
 }
+
+int kvm_arm_pmu_init(CPUState *cs)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
+    return 0;
+}
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index a16abc8d129e..d94e0a04f015 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -381,46 +381,44 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
     return NULL;
 }
 
-static bool kvm_arm_pmu_support_ctrl(CPUState *cs, struct kvm_device_attr *attr)
-{
-    return kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr) == 0;
-}
-
-int kvm_arm_pmu_create(CPUState *cs, int irq)
+static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
 {
     int err;
 
-    struct kvm_device_attr attr = {
-        .group = KVM_ARM_VCPU_PMU_V3_CTRL,
-        .addr = (intptr_t)&irq,
-        .attr = KVM_ARM_VCPU_PMU_V3_IRQ,
-        .flags = 0,
-    };
-
-    if (!kvm_arm_pmu_support_ctrl(cs, &attr)) {
-        return 0;
+    err = kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr);
+    if (err != 0) {
+        return false;
     }
 
-    err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, &attr);
+    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();
     }
 
-    attr.group = KVM_ARM_VCPU_PMU_V3_CTRL;
-    attr.attr = KVM_ARM_VCPU_PMU_V3_INIT;
-    attr.addr = 0;
-    attr.flags = 0;
+    return true;
+}
 
-    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();
-    }
+int 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);
+}
+
+int kvm_arm_pmu_create(CPUState *cs, int irq)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_ARM_VCPU_PMU_V3_CTRL,
+        .addr = (intptr_t)&irq,
+        .attr = KVM_ARM_VCPU_PMU_V3_IRQ,
+    };
 
-    return 1;
+    return kvm_arm_pmu_set_attr(cs, &attr);
 }
 
 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 633d08828a5d..3382762aa023 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -196,6 +196,7 @@ int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu);
 int kvm_arm_vgic_probe(void);
 
 int kvm_arm_pmu_create(CPUState *cs, int irq);
+int kvm_arm_pmu_init(CPUState *cs);
 
 #else
 
@@ -209,6 +210,11 @@ static inline int kvm_arm_pmu_create(CPUState *cs, int irq)
     return 0;
 }
 
+static inline int kvm_arm_pmu_init(CPUState *cs)
+{
+    return 0;
+}
+
 #endif
 
 static inline const char *gic_class_name(void)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/3] target/arm/kvm: split pmu init from creation

[Peter Maydell]

On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote:
> When adding a PMU with a userspace irqchip we only do the INIT
> stage of the device creation.

Can you explain why? My assumption would be that either
(a) we don't need the kernel's PMU, in which case don't
create it at all, or
(b) we do need the kernel's PMU, so we should both create
and INIT it.

If we do one but not the other then we've left a half
created and unusable PMU device in the kernel, haven't we?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] target/arm/kvm: split pmu init from creation

[Andrew Jones]

On Mon, Jul 10, 2017 at 04:27:35PM +0100, Peter Maydell wrote:
> On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote:
> > When adding a PMU with a userspace irqchip we only do the INIT
> > stage of the device creation.
> 
> Can you explain why? My assumption would be that either
> (a) we don't need the kernel's PMU, in which case don't
> create it at all, or
> (b) we do need the kernel's PMU, so we should both create
> and INIT it.
> 
> If we do one but not the other then we've left a half
> created and unusable PMU device in the kernel, haven't we?
>

I should have renamed the 'create' function to 'set_irq', then
it would make sense, because after the split done by this patch
'create' only sets the irq, which can only be done with an
in-kernel irqchip. Both irqchip types still require INIT though,
see kernel doc Documentation/virtual/kvm/devices/vcpu.txt.

I'll send a v2 that renames the create function. But, before I do,
assuming the rename, do you have another comments on this or the
next patch? We might as well batch all the changes :-)

Thanks,
drew

Re: [Qemu-devel] [PATCH 2/3] target/arm/kvm: split pmu init from creation

[Peter Maydell]

On 18 July 2017 at 13:38, Andrew Jones <drjones@redhat.com> wrote:
> On Mon, Jul 10, 2017 at 04:27:35PM +0100, Peter Maydell wrote:
>> On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote:
>> > When adding a PMU with a userspace irqchip we only do the INIT
>> > stage of the device creation.
>>
>> Can you explain why? My assumption would be that either
>> (a) we don't need the kernel's PMU, in which case don't
>> create it at all, or
>> (b) we do need the kernel's PMU, so we should both create
>> and INIT it.
>>
>> If we do one but not the other then we've left a half
>> created and unusable PMU device in the kernel, haven't we?
>>
>
> I should have renamed the 'create' function to 'set_irq', then
> it would make sense, because after the split done by this patch
> 'create' only sets the irq, which can only be done with an
> in-kernel irqchip. Both irqchip types still require INIT though,
> see kernel doc Documentation/virtual/kvm/devices/vcpu.txt.

Oh, I see, I read the commit message as "we only do the
INIT stage when adding a PMU with a userspace irqchip",
which isn't the same meaning as what you actually wrote.
(perhaps "When adding a PMU with a userspace irqchip we
skip the set-irq stage of device creation" would be clearer?)

> I'll send a v2 that renames the create function. But, before I do,
> assuming the rename, do you have another comments on this or the
> next patch? We might as well batch all the changes :-)

I think they looked mostly OK. A minor nit:

+        if (kvm_enabled() &&
+            !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) {
+            return;
+        }
+        if (kvm_enabled() && !kvm_arm_pmu_init(cpu)) {
             return;
         }

might be better as
       if (kvm_enabled()) {
           if (!kvm_arm_pmu_create(...)) {
               /* error handling */
           }
           if (!kvm_arm_pmu_init(...)) {
               /* error handling */
           }
       }

(and I'm not sure "silently return" is really the right error
handling, but that is what we do currently, so whatever.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/3] target/arm/kvm: split pmu init from creation

[Andrew Jones]

On Tue, Jul 18, 2017 at 01:44:34PM +0100, Peter Maydell wrote:
> On 18 July 2017 at 13:38, Andrew Jones <drjones@redhat.com> wrote:
> > On Mon, Jul 10, 2017 at 04:27:35PM +0100, Peter Maydell wrote:
> >> On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote:
> >> > When adding a PMU with a userspace irqchip we only do the INIT
> >> > stage of the device creation.
> >>
> >> Can you explain why? My assumption would be that either
> >> (a) we don't need the kernel's PMU, in which case don't
> >> create it at all, or
> >> (b) we do need the kernel's PMU, so we should both create
> >> and INIT it.
> >>
> >> If we do one but not the other then we've left a half
> >> created and unusable PMU device in the kernel, haven't we?
> >>
> >
> > I should have renamed the 'create' function to 'set_irq', then
> > it would make sense, because after the split done by this patch
> > 'create' only sets the irq, which can only be done with an
> > in-kernel irqchip. Both irqchip types still require INIT though,
> > see kernel doc Documentation/virtual/kvm/devices/vcpu.txt.
> 
> Oh, I see, I read the commit message as "we only do the
> INIT stage when adding a PMU with a userspace irqchip",
> which isn't the same meaning as what you actually wrote.
> (perhaps "When adding a PMU with a userspace irqchip we
> skip the set-irq stage of device creation" would be clearer?)
> 
> > I'll send a v2 that renames the create function. But, before I do,
> > assuming the rename, do you have another comments on this or the
> > next patch? We might as well batch all the changes :-)
> 
> I think they looked mostly OK. A minor nit:
> 
> +        if (kvm_enabled() &&
> +            !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) {
> +            return;
> +        }
> +        if (kvm_enabled() && !kvm_arm_pmu_init(cpu)) {
>              return;
>          }
> 
> might be better as
>        if (kvm_enabled()) {
>            if (!kvm_arm_pmu_create(...)) {
>                /* error handling */
>            }
>            if (!kvm_arm_pmu_init(...)) {
>                /* error handling */
>            }
>        }

With the top version I was setting things up for a one-liner
change in the next patch, as only the first condition needs
to have s/kvm_enabled/kvm_irqchip_in_kernel/ done to it.

> 
> (and I'm not sure "silently return" is really the right error
> handling, but that is what we do currently, so whatever.)

I'll give the error handling a bit of thought before respinning.

Thanks,
drew

[Qemu-devel] [PATCH 3/3] hw/arm/virt: allow pmu instantiation with userspace irqchip

[Andrew Jones]

Move the in-kernel-irqchip test to only guard the creation,
not the init'ing of the PMU.  Also add the PMU to the KVM
device irq line synchronization to enable its use.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c      | 2 +-
 target/arm/kvm.c   | 6 +++++-
 target/arm/kvm64.c | 3 +--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0cb8b479232d..53592fd0f30c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -495,7 +495,7 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
         if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
             return;
         }
-        if (kvm_enabled() &&
+        if (kvm_irqchip_in_kernel() &&
             !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) {
             return;
         }
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 7c17f0d629d7..211a7bf7befd 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -567,7 +567,11 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
             switched_level &= ~KVM_ARM_DEV_EL1_PTIMER;
         }
 
-        /* XXX PMU IRQ is missing */
+        if (switched_level & KVM_ARM_DEV_PMU) {
+            qemu_set_irq(cpu->pmu_interrupt,
+                         !!(run->s.regs.device_irq_level & KVM_ARM_DEV_PMU));
+            switched_level &= ~KVM_ARM_DEV_PMU;
+        }
 
         if (switched_level) {
             qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n",
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index d94e0a04f015..54e58e407812 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -506,8 +506,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
     if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
     }
-    if (!kvm_irqchip_in_kernel() ||
-        !kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
+    if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
             cpu->has_pmu = false;
     }
     if (cpu->has_pmu) {
-- 
1.8.3.1