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.z...@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.z...@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
>
>
