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

[Andrew Jones <drjones@redhat.com>]

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