[Qemu-devel] [PATCH 0/3] Add guest PMU in machine virt

[Qemu-devel] [PATCH 0/3] Add guest PMU in machine virt

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

KVM-ARM64 supports guest PMU now. This series add the support in machine
virt so that guest could use PMU.

Shannon Zhao (3):
  target-arm: kvm64: set guest PMUv3 feature bit if supported
  hw/arm/virt: Add PMU node for virt machine
  hw/arm/virt-acpi-build: Add PMU IRQ number in ACPI table

 hw/arm/virt-acpi-build.c      |  3 +++
 hw/arm/virt.c                 | 31 ++++++++++++++++++++++++
 include/hw/arm/virt.h         |  2 ++
 include/sysemu/kvm.h          |  1 +
 linux-headers/asm-arm64/kvm.h |  6 +++++
 linux-headers/linux/kvm.h     |  2 ++
 stubs/kvm.c                   |  5 ++++
 target-arm/cpu-qom.h          |  2 ++
 target-arm/kvm64.c            | 56 +++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 108 insertions(+)

-- 
2.0.4

[Qemu-devel] [PATCH 1/3] target-arm: kvm64: set guest PMUv3 feature bit if supported

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Check if kvm supports guest PMUv3. If so, set the corresponding feature
bit for vcpu.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 linux-headers/asm-arm64/kvm.h | 6 ++++++
 linux-headers/linux/kvm.h     | 2 ++
 target-arm/cpu-qom.h          | 2 ++
 target-arm/kvm64.c            | 5 +++++
 4 files changed, 15 insertions(+)

diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index a2fd4d9..3097f2f 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -94,6 +94,7 @@ struct kvm_regs {
 #define KVM_ARM_VCPU_POWER_OFF		0 /* CPU is started in OFF state */
 #define KVM_ARM_VCPU_EL1_32BIT		1 /* CPU running a 32bit VM */
 #define KVM_ARM_VCPU_PSCI_0_2		2 /* CPU uses PSCI v0.2 */
+#define KVM_ARM_VCPU_PMU_V3		3
 
 struct kvm_vcpu_init {
 	__u32 target;
@@ -204,6 +205,11 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
 
+/* Device Control API on vcpu fd */
+#define KVM_ARM_VCPU_PMU_V3_CTRL	0
+#define   KVM_ARM_VCPU_PMU_V3_IRQ	0
+#define   KVM_ARM_VCPU_PMU_V3_INIT	1
+
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
 #define KVM_ARM_IRQ_TYPE_MASK		0xff
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 4a56b9e..104210a 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -856,6 +856,8 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_IOEVENTFD_ANY_LENGTH 122
 #define KVM_CAP_HYPERV_SYNIC 123
 #define KVM_CAP_S390_RI 124
+#define KVM_CAP_ARM_PMU_V3 125
+#define KVM_CAP_VCPU_ATTRIBUTES 126
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 1061c08..93aa6a4 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -105,6 +105,8 @@ typedef struct ARMCPU {
     bool powered_off;
     /* CPU has security extension */
     bool has_el3;
+    /* CPU has PMU (Performance Monitor Unit) */
+    bool has_pmu;
 
     /* CPU has memory protection unit */
     bool has_mpu;
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index e8527bf..b364789 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -461,6 +461,11 @@ 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)) {
+        cpu->has_pmu = true;
+        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
+    }
 
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
-- 
2.0.4

[Qemu-devel] [PATCH 2/3] hw/arm/virt: Add PMU node for virt machine

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add a virtual PMU device for virt machine while use PPI 7 for PMU
overflow interrupt number.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/arm/virt.c         | 31 +++++++++++++++++++++++++++++++
 include/hw/arm/virt.h |  2 ++
 include/sysemu/kvm.h  |  1 +
 stubs/kvm.c           |  5 +++++
 target-arm/kvm64.c    | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 90 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 95331a5..94c2beb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -427,6 +427,35 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi, int type)
     qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
 }
 
+static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi)
+{
+    CPUState *cpu;
+    ARMCPU *armcpu;
+    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
+
+    CPU_FOREACH(cpu) {
+        armcpu = ARM_CPU(cpu);
+	if (!armcpu->has_pmu) {
+	    return;
+	}
+
+        kvm_arm_pmu_create(cpu, VIRTUAL_PMU_IRQ + 16);
+    }
+
+    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
+                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
+
+    armcpu = ARM_CPU(qemu_get_cpu(0));
+    qemu_fdt_add_subnode(vbi->fdt, "/pmu");
+    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
+        const char compat[] = "arm,armv8-pmuv3";
+        qemu_fdt_setprop(vbi->fdt, "/pmu", "compatible",
+                         compat, sizeof(compat));
+        qemu_fdt_setprop_cells(vbi->fdt, "/pmu", "interrupts",
+                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_PMU_IRQ, irqflags);
+    }
+}
+
 static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
 {
     int i;
@@ -1242,6 +1271,8 @@ static void machvirt_init(MachineState *machine)
 
     create_gic(vbi, pic, gic_version, vms->secure);
 
+    fdt_add_pmu_nodes(vbi);
+
     create_uart(vbi, pic, VIRT_UART, sysmem);
 
     if (vms->secure) {
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ecd8589..864eb49 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -40,6 +40,8 @@
 #define ARCH_TIMER_NS_EL1_IRQ 14
 #define ARCH_TIMER_NS_EL2_IRQ 10
 
+#define VIRTUAL_PMU_IRQ 7
+
 enum {
     VIRT_FLASH,
     VIRT_MEM,
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 6695fa7..80b6cb3 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -514,4 +514,5 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source);
  * Returns: 0 on success, or a negative errno on failure.
  */
 int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target);
+void kvm_arm_pmu_create(CPUState *cs, int irq);
 #endif
diff --git a/stubs/kvm.c b/stubs/kvm.c
index ddd6204..58a348a 100644
--- a/stubs/kvm.c
+++ b/stubs/kvm.c
@@ -6,3 +6,8 @@ int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
 {
     return 0;
 }
+
+void kvm_arm_pmu_create(CPUState *cs, int irq)
+{
+    return;
+}
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index b364789..b97b9ef 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -382,6 +382,57 @@ 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;
+}
+
+static void kvm_arm_pmu_init(CPUState *cs)
+{
+    int err;
+
+    struct kvm_device_attr attr = {
+        .group = KVM_ARM_VCPU_PMU_V3_CTRL,
+        .attr = KVM_ARM_VCPU_PMU_V3_INIT,
+        .flags = 0,
+    };
+
+    if (!kvm_arm_pmu_support_ctrl(cs, &attr)) {
+        return;
+    }
+
+    err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, &attr);
+    if (err < 0) {
+        fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
+                strerror(-err));
+        abort();
+    }
+}
+
+void kvm_arm_pmu_create(CPUState *cs, int irq)
+{
+    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;
+    }
+
+    err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, &attr);
+    if (err < 0) {
+        fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
+                strerror(-err));
+        abort();
+    }
+
+    kvm_arm_pmu_init(cs);
+}
 
 static inline void set_feature(uint64_t *features, int feature)
 {
-- 
2.0.4

[Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add PMU IRQ number in ACPI table

[Shannon Zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add PMU IRQ number in ACPI table, then we can use PMU in guest through
ACPI.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/arm/virt-acpi-build.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6a86b2c..7a377e5 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -490,6 +490,9 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
         gicc->arm_mpidr = armcpu->mp_affinity;
         gicc->uid = i;
         gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
+
+        if (armcpu->has_pmu)
+            gicc->performance_interrupt = VIRTUAL_PMU_IRQ + 16;
     }
 
     if (guest_info->gic_version == 3) {
-- 
2.0.4

Re: [Qemu-devel] [PATCH 0/3] Add guest PMU in machine virt

[Peter Maydell]

On 25 March 2016 at 09:46, Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> KVM-ARM64 supports guest PMU now. This series add the support in machine
> virt so that guest could use PMU.
>
> Shannon Zhao (3):
>   target-arm: kvm64: set guest PMUv3 feature bit if supported
>   hw/arm/virt: Add PMU node for virt machine
>   hw/arm/virt-acpi-build: Add PMU IRQ number in ACPI table

Given the timing, I'm not planning to put this into 2.6.
(It is still on my review queue for 2.7.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] target-arm: kvm64: set guest PMUv3 feature bit if supported

[Andrew Jones]

On Fri, Mar 25, 2016 at 05:46:19PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Check if kvm supports guest PMUv3. If so, set the corresponding feature
> bit for vcpu.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  linux-headers/asm-arm64/kvm.h | 6 ++++++
>  linux-headers/linux/kvm.h     | 2 ++

The above two files should be updated with their own patch and not
manually. Update with scripts/update-linux-headers.sh

>  target-arm/cpu-qom.h          | 2 ++
>  target-arm/kvm64.c            | 5 +++++
>  4 files changed, 15 insertions(+)
> 
> diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
> index a2fd4d9..3097f2f 100644
> --- a/linux-headers/asm-arm64/kvm.h
> +++ b/linux-headers/asm-arm64/kvm.h
> @@ -94,6 +94,7 @@ struct kvm_regs {
>  #define KVM_ARM_VCPU_POWER_OFF		0 /* CPU is started in OFF state */
>  #define KVM_ARM_VCPU_EL1_32BIT		1 /* CPU running a 32bit VM */
>  #define KVM_ARM_VCPU_PSCI_0_2		2 /* CPU uses PSCI v0.2 */
> +#define KVM_ARM_VCPU_PMU_V3		3
>  
>  struct kvm_vcpu_init {
>  	__u32 target;
> @@ -204,6 +205,11 @@ struct kvm_arch_memory_slot {
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
>  
> +/* Device Control API on vcpu fd */
> +#define KVM_ARM_VCPU_PMU_V3_CTRL	0
> +#define   KVM_ARM_VCPU_PMU_V3_IRQ	0
> +#define   KVM_ARM_VCPU_PMU_V3_INIT	1
> +
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
>  #define KVM_ARM_IRQ_TYPE_MASK		0xff
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 4a56b9e..104210a 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -856,6 +856,8 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_IOEVENTFD_ANY_LENGTH 122
>  #define KVM_CAP_HYPERV_SYNIC 123
>  #define KVM_CAP_S390_RI 124
> +#define KVM_CAP_ARM_PMU_V3 125
> +#define KVM_CAP_VCPU_ATTRIBUTES 126

These defines are wrong, but will be fixed with update-linux-headers

>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 1061c08..93aa6a4 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -105,6 +105,8 @@ typedef struct ARMCPU {
>      bool powered_off;
>      /* CPU has security extension */
>      bool has_el3;
> +    /* CPU has PMU (Performance Monitor Unit) */
> +    bool has_pmu;
>  
>      /* CPU has memory protection unit */
>      bool has_mpu;
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index e8527bf..b364789 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -461,6 +461,11 @@ 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)) {
> +        cpu->has_pmu = true;
> +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> +    }
>  
>      /* Do KVM_ARM_VCPU_INIT ioctl */
>      ret = kvm_arm_vcpu_init(cs);
> -- 
> 2.0.4
>

Otherwise looks good.

drew 

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add PMU node for virt machine

[Andrew Jones]

On Fri, Mar 25, 2016 at 05:46:20PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add a virtual PMU device for virt machine while use PPI 7 for PMU
> overflow interrupt number.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/arm/virt.c         | 31 +++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  2 ++
>  include/sysemu/kvm.h  |  1 +
>  stubs/kvm.c           |  5 +++++
>  target-arm/kvm64.c    | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 90 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 95331a5..94c2beb 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -427,6 +427,35 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi, int type)
>      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
>  }
>  
> +static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi)
> +{
> +    CPUState *cpu;
> +    ARMCPU *armcpu;
> +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> +
> +    CPU_FOREACH(cpu) {
> +        armcpu = ARM_CPU(cpu);
> +	if (!armcpu->has_pmu) {
> +	    return;

funny indentation here

> +	}
> +
> +        kvm_arm_pmu_create(cpu, VIRTUAL_PMU_IRQ + 16);

I think we should have a PPI(irq) ((irq) + 16) type of macro.

> +    }
> +
> +    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> +                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
> +
> +    armcpu = ARM_CPU(qemu_get_cpu(0));
> +    qemu_fdt_add_subnode(vbi->fdt, "/pmu");
> +    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> +        const char compat[] = "arm,armv8-pmuv3";
> +        qemu_fdt_setprop(vbi->fdt, "/pmu", "compatible",
> +                         compat, sizeof(compat));
> +        qemu_fdt_setprop_cells(vbi->fdt, "/pmu", "interrupts",
> +                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_PMU_IRQ, irqflags);
> +    }

else what? I guess it's not possible to have has_pmu and !ARM_FEATURE_V8
at the same time right now, but it seems strange to create a /pmu node,
but then only conditionally populate it.

> +}
> +
>  static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
>  {
>      int i;
> @@ -1242,6 +1271,8 @@ static void machvirt_init(MachineState *machine)
>  
>      create_gic(vbi, pic, gic_version, vms->secure);
>  
> +    fdt_add_pmu_nodes(vbi);
> +
>      create_uart(vbi, pic, VIRT_UART, sysmem);
>  
>      if (vms->secure) {
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ecd8589..864eb49 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -40,6 +40,8 @@
>  #define ARCH_TIMER_NS_EL1_IRQ 14
>  #define ARCH_TIMER_NS_EL2_IRQ 10
>  
> +#define VIRTUAL_PMU_IRQ 7

Can we find a way to make this configurable? a cpu property?

> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 6695fa7..80b6cb3 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -514,4 +514,5 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source);
>   * Returns: 0 on success, or a negative errno on failure.
>   */
>  int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target);
> +void kvm_arm_pmu_create(CPUState *cs, int irq);
>  #endif
> diff --git a/stubs/kvm.c b/stubs/kvm.c
> index ddd6204..58a348a 100644
> --- a/stubs/kvm.c
> +++ b/stubs/kvm.c
> @@ -6,3 +6,8 @@ int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
>  {
>      return 0;
>  }
> +
> +void kvm_arm_pmu_create(CPUState *cs, int irq)
> +{
> +    return;
> +}
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index b364789..b97b9ef 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -382,6 +382,57 @@ 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;
> +}
> +
> +static void kvm_arm_pmu_init(CPUState *cs)
> +{
> +    int err;
> +
> +    struct kvm_device_attr attr = {
> +        .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> +        .attr = KVM_ARM_VCPU_PMU_V3_INIT,
> +        .flags = 0,
> +    };
> +
> +    if (!kvm_arm_pmu_support_ctrl(cs, &attr)) {
> +        return;
> +    }

I don't think we need to do this check again here. kvm_arm_pmu_init is
only called from a function that already checked for the IRQ attribute,
and both IRQ and INIT were added to the kernel at the same time.

Actually I think we could just opencode kvm_arm_pmu_init in
kvm_arm_pmu_create.

> +
> +    err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, &attr);
> +    if (err < 0) {
> +        fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
> +                strerror(-err));
> +        abort();
> +    }
> +}
> +
> +void kvm_arm_pmu_create(CPUState *cs, int irq)
> +{
> +    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;
> +    }
> +
> +    err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, &attr);
> +    if (err < 0) {
> +        fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
> +                strerror(-err));
> +        abort();
> +    }
> +
> +    kvm_arm_pmu_init(cs);
> +}
>  
>  static inline void set_feature(uint64_t *features, int feature)
>  {
> -- 
> 2.0.4
> 
> 
> 

Thanks,
drew

Re: [Qemu-devel] [PATCH 3/3] hw/arm/virt-acpi-build: Add PMU IRQ number in ACPI table

[Andrew Jones]

On Fri, Mar 25, 2016 at 05:46:21PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add PMU IRQ number in ACPI table, then we can use PMU in guest through
> ACPI.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/arm/virt-acpi-build.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 6a86b2c..7a377e5 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -490,6 +490,9 @@ build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>          gicc->arm_mpidr = armcpu->mp_affinity;
>          gicc->uid = i;
>          gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
> +
> +        if (armcpu->has_pmu)
> +            gicc->performance_interrupt = VIRTUAL_PMU_IRQ + 16;

missing cpu_to_le32(). Actually this function is missing lots of those.
Here's another place we could use a PPI() macro to avoid sprinkling
16's around everywhere.

>      }
>  
>      if (guest_info->gic_version == 3) {
> -- 
> 2.0.4

Otherwise looks good.

drew

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add PMU node for virt machine

[Shannon Zhao]

On 2016/4/22 22:32, Andrew Jones wrote:
> On Fri, Mar 25, 2016 at 05:46:20PM +0800, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Add a virtual PMU device for virt machine while use PPI 7 for PMU
>> overflow interrupt number.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  hw/arm/virt.c         | 31 +++++++++++++++++++++++++++++++
>>  include/hw/arm/virt.h |  2 ++
>>  include/sysemu/kvm.h  |  1 +
>>  stubs/kvm.c           |  5 +++++
>>  target-arm/kvm64.c    | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 90 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 95331a5..94c2beb 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -427,6 +427,35 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi, int type)
>>      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
>>  }
>>  
>> +static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi)
>> +{
>> +    CPUState *cpu;
>> +    ARMCPU *armcpu;
>> +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
>> +
>> +    CPU_FOREACH(cpu) {
>> +        armcpu = ARM_CPU(cpu);
>> +	if (!armcpu->has_pmu) {
>> +	    return;
> 
> funny indentation here
> 
>> +	}
>> +
>> +        kvm_arm_pmu_create(cpu, VIRTUAL_PMU_IRQ + 16);
> 
> I think we should have a PPI(irq) ((irq) + 16) type of macro.
> 
>> +    }
>> +
>> +    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
>> +                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
>> +
>> +    armcpu = ARM_CPU(qemu_get_cpu(0));
>> +    qemu_fdt_add_subnode(vbi->fdt, "/pmu");
>> +    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
>> +        const char compat[] = "arm,armv8-pmuv3";
>> +        qemu_fdt_setprop(vbi->fdt, "/pmu", "compatible",
>> +                         compat, sizeof(compat));
>> +        qemu_fdt_setprop_cells(vbi->fdt, "/pmu", "interrupts",
>> +                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_PMU_IRQ, irqflags);
>> +    }
> 
> else what? I guess it's not possible to have has_pmu and !ARM_FEATURE_V8
> at the same time right now, but it seems strange to create a /pmu node,
> but then only conditionally populate it.
> 
Yeah, currently kvm only supports guest PMU for ARMv8, but maybe in the
future it will support ARMv7.

>> +}
>> +
>>  static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
>>  {
>>      int i;
>> @@ -1242,6 +1271,8 @@ static void machvirt_init(MachineState *machine)
>>  
>>      create_gic(vbi, pic, gic_version, vms->secure);
>>  
>> +    fdt_add_pmu_nodes(vbi);
>> +
>>      create_uart(vbi, pic, VIRT_UART, sysmem);
>>  
>>      if (vms->secure) {
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index ecd8589..864eb49 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -40,6 +40,8 @@
>>  #define ARCH_TIMER_NS_EL1_IRQ 14
>>  #define ARCH_TIMER_NS_EL2_IRQ 10
>>  
>> +#define VIRTUAL_PMU_IRQ 7
> 
> Can we find a way to make this configurable? a cpu property?
> 
Of course we can. But as we are the maker of the virt machine board, we
can decide the design of the hardware. In addition, what's the purpose
for making it configurable?

>> +
>>  enum {
>>      VIRT_FLASH,
>>      VIRT_MEM,
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index 6695fa7..80b6cb3 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -514,4 +514,5 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source);
>>   * Returns: 0 on success, or a negative errno on failure.
>>   */
>>  int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target);
>> +void kvm_arm_pmu_create(CPUState *cs, int irq);
>>  #endif
>> diff --git a/stubs/kvm.c b/stubs/kvm.c
>> index ddd6204..58a348a 100644
>> --- a/stubs/kvm.c
>> +++ b/stubs/kvm.c
>> @@ -6,3 +6,8 @@ int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
>>  {
>>      return 0;
>>  }
>> +
>> +void kvm_arm_pmu_create(CPUState *cs, int irq)
>> +{
>> +    return;
>> +}
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index b364789..b97b9ef 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -382,6 +382,57 @@ 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;
>> +}
>> +
>> +static void kvm_arm_pmu_init(CPUState *cs)
>> +{
>> +    int err;
>> +
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_ARM_VCPU_PMU_V3_CTRL,
>> +        .attr = KVM_ARM_VCPU_PMU_V3_INIT,
>> +        .flags = 0,
>> +    };
>> +
>> +    if (!kvm_arm_pmu_support_ctrl(cs, &attr)) {
>> +        return;
>> +    }
> 
> I don't think we need to do this check again here. kvm_arm_pmu_init is
> only called from a function that already checked for the IRQ attribute,
> and both IRQ and INIT were added to the kernel at the same time.
> 
> Actually I think we could just opencode kvm_arm_pmu_init in
> kvm_arm_pmu_create.
> 
Sure. Thanks.

-- 
Shannon

Re: [Qemu-devel] [PATCH 1/3] target-arm: kvm64: set guest PMUv3 feature bit if supported

[Shannon Zhao]

On 2016/4/22 21:41, Andrew Jones wrote:
> On Fri, Mar 25, 2016 at 05:46:19PM +0800, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > Check if kvm supports guest PMUv3. If so, set the corresponding feature
>> > bit for vcpu.
>> > 
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> >  linux-headers/asm-arm64/kvm.h | 6 ++++++
>> >  linux-headers/linux/kvm.h     | 2 ++
> The above two files should be updated with their own patch and not
> manually. Update with scripts/update-linux-headers.sh
> 
Ok, will use that.

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH 2/3] hw/arm/virt: Add PMU node for virt machine

[Andrew Jones]

On Sat, Apr 23, 2016 at 09:01:01AM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/4/22 22:32, Andrew Jones wrote:
> > On Fri, Mar 25, 2016 at 05:46:20PM +0800, Shannon Zhao wrote:
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> Add a virtual PMU device for virt machine while use PPI 7 for PMU
> >> overflow interrupt number.
> >>
> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> ---
> >>  hw/arm/virt.c         | 31 +++++++++++++++++++++++++++++++
> >>  include/hw/arm/virt.h |  2 ++
> >>  include/sysemu/kvm.h  |  1 +
> >>  stubs/kvm.c           |  5 +++++
> >>  target-arm/kvm64.c    | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  5 files changed, 90 insertions(+)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index 95331a5..94c2beb 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -427,6 +427,35 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi, int type)
> >>      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", vbi->gic_phandle);
> >>  }
> >>  
> >> +static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi)
> >> +{
> >> +    CPUState *cpu;
> >> +    ARMCPU *armcpu;
> >> +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> >> +
> >> +    CPU_FOREACH(cpu) {
> >> +        armcpu = ARM_CPU(cpu);
> >> +	if (!armcpu->has_pmu) {
> >> +	    return;
> > 
> > funny indentation here
> > 
> >> +	}
> >> +
> >> +        kvm_arm_pmu_create(cpu, VIRTUAL_PMU_IRQ + 16);
> > 
> > I think we should have a PPI(irq) ((irq) + 16) type of macro.
> > 
> >> +    }
> >> +
> >> +    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> >> +                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
> >> +
> >> +    armcpu = ARM_CPU(qemu_get_cpu(0));
> >> +    qemu_fdt_add_subnode(vbi->fdt, "/pmu");
> >> +    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> >> +        const char compat[] = "arm,armv8-pmuv3";
> >> +        qemu_fdt_setprop(vbi->fdt, "/pmu", "compatible",
> >> +                         compat, sizeof(compat));
> >> +        qemu_fdt_setprop_cells(vbi->fdt, "/pmu", "interrupts",
> >> +                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_PMU_IRQ, irqflags);
> >> +    }
> > 
> > else what? I guess it's not possible to have has_pmu and !ARM_FEATURE_V8
> > at the same time right now, but it seems strange to create a /pmu node,
> > but then only conditionally populate it.
> > 
> Yeah, currently kvm only supports guest PMU for ARMv8, but maybe in the
> future it will support ARMv7.
> 
> >> +}
> >> +
> >>  static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
> >>  {
> >>      int i;
> >> @@ -1242,6 +1271,8 @@ static void machvirt_init(MachineState *machine)
> >>  
> >>      create_gic(vbi, pic, gic_version, vms->secure);
> >>  
> >> +    fdt_add_pmu_nodes(vbi);
> >> +
> >>      create_uart(vbi, pic, VIRT_UART, sysmem);
> >>  
> >>      if (vms->secure) {
> >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> >> index ecd8589..864eb49 100644
> >> --- a/include/hw/arm/virt.h
> >> +++ b/include/hw/arm/virt.h
> >> @@ -40,6 +40,8 @@
> >>  #define ARCH_TIMER_NS_EL1_IRQ 14
> >>  #define ARCH_TIMER_NS_EL2_IRQ 10
> >>  
> >> +#define VIRTUAL_PMU_IRQ 7
> > 
> > Can we find a way to make this configurable? a cpu property?
> > 
> Of course we can. But as we are the maker of the virt machine board, we
> can decide the design of the hardware. In addition, what's the purpose
> for making it configurable?

Yeah, nevermind. I can't think of any good reason right now. I was only
thinking about it because your KVM interface allows for either SPI or
PPI. But, even considering that, I guess we still don't need to allow
the number(s) to be configurable, just the type. For SPI we'd need to
reserve a range of numbers though, since each cpu needs their own.

> 
> >> +
> >>  enum {
> >>      VIRT_FLASH,
> >>      VIRT_MEM,
> >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >> index 6695fa7..80b6cb3 100644
> >> --- a/include/sysemu/kvm.h
> >> +++ b/include/sysemu/kvm.h
> >> @@ -514,4 +514,5 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source);
> >>   * Returns: 0 on success, or a negative errno on failure.
> >>   */
> >>  int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target);
> >> +void kvm_arm_pmu_create(CPUState *cs, int irq);
> >>  #endif
> >> diff --git a/stubs/kvm.c b/stubs/kvm.c
> >> index ddd6204..58a348a 100644
> >> --- a/stubs/kvm.c
> >> +++ b/stubs/kvm.c
> >> @@ -6,3 +6,8 @@ int kvm_arch_irqchip_create(MachineState *ms, KVMState *s)
> >>  {
> >>      return 0;
> >>  }
> >> +
> >> +void kvm_arm_pmu_create(CPUState *cs, int irq)
> >> +{
> >> +    return;
> >> +}
> >> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> >> index b364789..b97b9ef 100644
> >> --- a/target-arm/kvm64.c
> >> +++ b/target-arm/kvm64.c
> >> @@ -382,6 +382,57 @@ 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;
> >> +}
> >> +
> >> +static void kvm_arm_pmu_init(CPUState *cs)
> >> +{
> >> +    int err;
> >> +
> >> +    struct kvm_device_attr attr = {
> >> +        .group = KVM_ARM_VCPU_PMU_V3_CTRL,
> >> +        .attr = KVM_ARM_VCPU_PMU_V3_INIT,
> >> +        .flags = 0,
> >> +    };
> >> +
> >> +    if (!kvm_arm_pmu_support_ctrl(cs, &attr)) {
> >> +        return;
> >> +    }
> > 
> > I don't think we need to do this check again here. kvm_arm_pmu_init is
> > only called from a function that already checked for the IRQ attribute,
> > and both IRQ and INIT were added to the kernel at the same time.
> > 
> > Actually I think we could just opencode kvm_arm_pmu_init in
> > kvm_arm_pmu_create.
> > 
> Sure. Thanks.
> 
> -- 
> Shannon
> 
>