| Message ID | 1458899181-17896-3-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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 > >
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) {