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

[Qemu-devel] [PATCH v2 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.

Changes since v1:
* rebase on master
* Address Andrew's comments, add a macro PPI, fix code style, add
  cpu_to_le32()

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    |  4 ++++
 include/sysemu/kvm.h     |  1 +
 stubs/kvm.c              |  5 +++++
 target-arm/cpu-qom.h     |  2 ++
 target-arm/kvm64.c       | 44 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 90 insertions(+)

-- 
2.0.4

[Qemu-devel] [PATCH v2 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>
---
 target-arm/cpu-qom.h | 2 ++
 target-arm/kvm64.c   | 5 +++++
 2 files changed, 7 insertions(+)

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 v2 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 |  4 ++++
 include/sysemu/kvm.h  |  1 +
 stubs/kvm.c           |  5 +++++
 target-arm/kvm64.c    | 39 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 80 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 56d35c7..d77b314 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -428,6 +428,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, PPI(VIRTUAL_PMU_IRQ));
+    }
+
+    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;
@@ -1246,6 +1275,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..b50f095 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -40,6 +40,10 @@
 #define ARCH_TIMER_NS_EL1_IRQ 14
 #define ARCH_TIMER_NS_EL2_IRQ 10
 
+#define VIRTUAL_PMU_IRQ 7
+
+#define PPI(irq) ((irq) + 16)
+
 enum {
     VIRT_FLASH,
     VIRT_MEM,
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 0e18f15..90c2c54 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -523,4 +523,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..faec4fa 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -382,6 +382,45 @@ 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;
+}
+
+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();
+    }
+
+    attr.group = KVM_ARM_VCPU_PMU_V3_CTRL;
+    attr.attr = KVM_ARM_VCPU_PMU_V3_INIT;
+    attr.addr = 0;
+    attr.flags = 0;
+
+    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();
+    }
+}
 
 static inline void set_feature(uint64_t *features, int feature)
 {
-- 
2.0.4

[Qemu-devel] [PATCH v2 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 f51fe39..5031232 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -491,6 +491,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 = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
     }
 
     if (guest_info->gic_version == 3) {
-- 
2.0.4

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

[Andrew Jones]

On Sat, Apr 23, 2016 at 06:04:13PM +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>
> ---
>  target-arm/cpu-qom.h | 2 ++
>  target-arm/kvm64.c   | 5 +++++
>  2 files changed, 7 insertions(+)

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> 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
> 
> 

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

[Andrew Jones]

On Sat, Apr 23, 2016 at 06:04:14PM +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 |  4 ++++
>  include/sysemu/kvm.h  |  1 +
>  stubs/kvm.c           |  5 +++++
>  target-arm/kvm64.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 80 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 56d35c7..d77b314 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -428,6 +428,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, PPI(VIRTUAL_PMU_IRQ));
> +    }
> +
> +    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> +                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);

The shift will overflow when configuring a machine to have more than
32 cpus. You should confirm smp_cpus is <= GIC_FDT_IRQ_PPI_CPU_WIDTH
before generating a mask this way, otherwise you can just pass in 0xff.

> +
> +    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;
> @@ -1246,6 +1275,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..b50f095 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -40,6 +40,10 @@
>  #define ARCH_TIMER_NS_EL1_IRQ 14
>  #define ARCH_TIMER_NS_EL2_IRQ 10
>  
> +#define VIRTUAL_PMU_IRQ 7
> +
> +#define PPI(irq) ((irq) + 16)
> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 0e18f15..90c2c54 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -523,4 +523,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..faec4fa 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -382,6 +382,45 @@ 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;
> +}
> +
> +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",

You know it's a KVM_SET_DEVICE_ATTR here, so why report SET/GET?

> +                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;
> +
> +    err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, &attr);
> +    if (err < 0) {
> +        fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",

Same SET comment as above.

> +                strerror(-err));
> +        abort();
> +    }
> +}
>  
>  static inline void set_feature(uint64_t *features, int feature)
>  {
> -- 
> 2.0.4
> 
>

Thanks,
drew 

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

[Andrew Jones]

On Sat, Apr 23, 2016 at 06:04:15PM +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(+)

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f51fe39..5031232 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -491,6 +491,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 = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>      }
>  
>      if (guest_info->gic_version == 3) {
> -- 
> 2.0.4
> 
> 
> 

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

[Shannon Zhao]

On 2016/4/25 1:28, Andrew Jones wrote:
> On Sat, Apr 23, 2016 at 06:04:14PM +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 |  4 ++++
>> >  include/sysemu/kvm.h  |  1 +
>> >  stubs/kvm.c           |  5 +++++
>> >  target-arm/kvm64.c    | 39 +++++++++++++++++++++++++++++++++++++++
>> >  5 files changed, 80 insertions(+)
>> > 
>> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> > index 56d35c7..d77b314 100644
>> > --- a/hw/arm/virt.c
>> > +++ b/hw/arm/virt.c
>> > @@ -428,6 +428,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, PPI(VIRTUAL_PMU_IRQ));
>> > +    }
>> > +
>> > +    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
>> > +                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
> The shift will overflow when configuring a machine to have more than
> 32 cpus. You should confirm smp_cpus is <= GIC_FDT_IRQ_PPI_CPU_WIDTH
> before generating a mask this way, otherwise you can just pass in 0xff.
> 
Ah, right. So it could check the gictype like what we do in
fdt_add_timer_nodes.

Thanks,
-- 
Shannon

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

[Andrew Jones]

On Mon, Apr 25, 2016 at 02:52:45PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/4/25 1:28, Andrew Jones wrote:
> > On Sat, Apr 23, 2016 at 06:04:14PM +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 |  4 ++++
> >> >  include/sysemu/kvm.h  |  1 +
> >> >  stubs/kvm.c           |  5 +++++
> >> >  target-arm/kvm64.c    | 39 +++++++++++++++++++++++++++++++++++++++
> >> >  5 files changed, 80 insertions(+)
> >> > 
> >> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> > index 56d35c7..d77b314 100644
> >> > --- a/hw/arm/virt.c
> >> > +++ b/hw/arm/virt.c
> >> > @@ -428,6 +428,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, PPI(VIRTUAL_PMU_IRQ));
> >> > +    }
> >> > +
> >> > +    irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> >> > +                         GIC_FDT_IRQ_PPI_CPU_WIDTH, (1 << vbi->smp_cpus) - 1);
> > The shift will overflow when configuring a machine to have more than
> > 32 cpus. You should confirm smp_cpus is <= GIC_FDT_IRQ_PPI_CPU_WIDTH
> > before generating a mask this way, otherwise you can just pass in 0xff.
> > 
> Ah, right. So it could check the gictype like what we do in
> fdt_add_timer_nodes.

Oh, now that I look closer we definitely need that condition. Otherwise
we're losing the LEVEL_HI flag that irqflags is initialized to for no
reason.

Thanks,
drew