| Message ID | 1498927637-14496-3-git-send-email-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote: > When adding a PMU with a userspace irqchip we only do the INIT > stage of the device creation. Can you explain why? My assumption would be that either (a) we don't need the kernel's PMU, in which case don't create it at all, or (b) we do need the kernel's PMU, so we should both create and INIT it. If we do one but not the other then we've left a half created and unusable PMU device in the kernel, haven't we? thanks -- PMM
On Mon, Jul 10, 2017 at 04:27:35PM +0100, Peter Maydell wrote: > On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote: > > When adding a PMU with a userspace irqchip we only do the INIT > > stage of the device creation. > > Can you explain why? My assumption would be that either > (a) we don't need the kernel's PMU, in which case don't > create it at all, or > (b) we do need the kernel's PMU, so we should both create > and INIT it. > > If we do one but not the other then we've left a half > created and unusable PMU device in the kernel, haven't we? > I should have renamed the 'create' function to 'set_irq', then it would make sense, because after the split done by this patch 'create' only sets the irq, which can only be done with an in-kernel irqchip. Both irqchip types still require INIT though, see kernel doc Documentation/virtual/kvm/devices/vcpu.txt. I'll send a v2 that renames the create function. But, before I do, assuming the rename, do you have another comments on this or the next patch? We might as well batch all the changes :-) Thanks, drew
On 18 July 2017 at 13:38, Andrew Jones <drjones@redhat.com> wrote: > On Mon, Jul 10, 2017 at 04:27:35PM +0100, Peter Maydell wrote: >> On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote: >> > When adding a PMU with a userspace irqchip we only do the INIT >> > stage of the device creation. >> >> Can you explain why? My assumption would be that either >> (a) we don't need the kernel's PMU, in which case don't >> create it at all, or >> (b) we do need the kernel's PMU, so we should both create >> and INIT it. >> >> If we do one but not the other then we've left a half >> created and unusable PMU device in the kernel, haven't we? >> > > I should have renamed the 'create' function to 'set_irq', then > it would make sense, because after the split done by this patch > 'create' only sets the irq, which can only be done with an > in-kernel irqchip. Both irqchip types still require INIT though, > see kernel doc Documentation/virtual/kvm/devices/vcpu.txt. Oh, I see, I read the commit message as "we only do the INIT stage when adding a PMU with a userspace irqchip", which isn't the same meaning as what you actually wrote. (perhaps "When adding a PMU with a userspace irqchip we skip the set-irq stage of device creation" would be clearer?) > I'll send a v2 that renames the create function. But, before I do, > assuming the rename, do you have another comments on this or the > next patch? We might as well batch all the changes :-) I think they looked mostly OK. A minor nit: + if (kvm_enabled() && + !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { + return; + } + if (kvm_enabled() && !kvm_arm_pmu_init(cpu)) { return; } might be better as if (kvm_enabled()) { if (!kvm_arm_pmu_create(...)) { /* error handling */ } if (!kvm_arm_pmu_init(...)) { /* error handling */ } } (and I'm not sure "silently return" is really the right error handling, but that is what we do currently, so whatever.) thanks -- PMM
On Tue, Jul 18, 2017 at 01:44:34PM +0100, Peter Maydell wrote: > On 18 July 2017 at 13:38, Andrew Jones <drjones@redhat.com> wrote: > > On Mon, Jul 10, 2017 at 04:27:35PM +0100, Peter Maydell wrote: > >> On 1 July 2017 at 17:47, Andrew Jones <drjones@redhat.com> wrote: > >> > When adding a PMU with a userspace irqchip we only do the INIT > >> > stage of the device creation. > >> > >> Can you explain why? My assumption would be that either > >> (a) we don't need the kernel's PMU, in which case don't > >> create it at all, or > >> (b) we do need the kernel's PMU, so we should both create > >> and INIT it. > >> > >> If we do one but not the other then we've left a half > >> created and unusable PMU device in the kernel, haven't we? > >> > > > > I should have renamed the 'create' function to 'set_irq', then > > it would make sense, because after the split done by this patch > > 'create' only sets the irq, which can only be done with an > > in-kernel irqchip. Both irqchip types still require INIT though, > > see kernel doc Documentation/virtual/kvm/devices/vcpu.txt. > > Oh, I see, I read the commit message as "we only do the > INIT stage when adding a PMU with a userspace irqchip", > which isn't the same meaning as what you actually wrote. > (perhaps "When adding a PMU with a userspace irqchip we > skip the set-irq stage of device creation" would be clearer?) > > > I'll send a v2 that renames the create function. But, before I do, > > assuming the rename, do you have another comments on this or the > > next patch? We might as well batch all the changes :-) > > I think they looked mostly OK. A minor nit: > > + if (kvm_enabled() && > + !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { > + return; > + } > + if (kvm_enabled() && !kvm_arm_pmu_init(cpu)) { > return; > } > > might be better as > if (kvm_enabled()) { > if (!kvm_arm_pmu_create(...)) { > /* error handling */ > } > if (!kvm_arm_pmu_init(...)) { > /* error handling */ > } > } With the top version I was setting things up for a one-liner change in the next patch, as only the first condition needs to have s/kvm_enabled/kvm_irqchip_in_kernel/ done to it. > > (and I'm not sure "silently return" is really the right error > handling, but that is what we do currently, so whatever.) I'll give the error handling a bit of thought before respinning. Thanks, drew
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 9781e1cc5ed7..0cb8b479232d 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -492,8 +492,14 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms) CPU_FOREACH(cpu) { armcpu = ARM_CPU(cpu); - if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU) || - (kvm_enabled() && !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ)))) { + if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { + return; + } + if (kvm_enabled() && + !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { + return; + } + if (kvm_enabled() && !kvm_arm_pmu_init(cpu)) { return; } } diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c index 069da0c5fd10..a51695f25911 100644 --- a/target/arm/kvm32.c +++ b/target/arm/kvm32.c @@ -527,3 +527,9 @@ int kvm_arm_pmu_create(CPUState *cs, int irq) qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__); return 0; } + +int kvm_arm_pmu_init(CPUState *cs) +{ + qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__); + return 0; +} diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index a16abc8d129e..d94e0a04f015 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -381,46 +381,44 @@ 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; -} - -int kvm_arm_pmu_create(CPUState *cs, int irq) +static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr) { 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 0; + err = kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr); + if (err != 0) { + return false; } - err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, &attr); + err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, attr); if (err < 0) { fprintf(stderr, "KVM_SET_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; + return true; +} - err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, &attr); - if (err < 0) { - fprintf(stderr, "KVM_SET_DEVICE_ATTR failed: %s\n", - strerror(-err)); - abort(); - } +int kvm_arm_pmu_init(CPUState *cs) +{ + struct kvm_device_attr attr = { + .group = KVM_ARM_VCPU_PMU_V3_CTRL, + .attr = KVM_ARM_VCPU_PMU_V3_INIT, + }; + + return kvm_arm_pmu_set_attr(cs, &attr); +} + +int kvm_arm_pmu_create(CPUState *cs, int irq) +{ + struct kvm_device_attr attr = { + .group = KVM_ARM_VCPU_PMU_V3_CTRL, + .addr = (intptr_t)&irq, + .attr = KVM_ARM_VCPU_PMU_V3_IRQ, + }; - return 1; + return kvm_arm_pmu_set_attr(cs, &attr); } static inline void set_feature(uint64_t *features, int feature) diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h index 633d08828a5d..3382762aa023 100644 --- a/target/arm/kvm_arm.h +++ b/target/arm/kvm_arm.h @@ -196,6 +196,7 @@ int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu); int kvm_arm_vgic_probe(void); int kvm_arm_pmu_create(CPUState *cs, int irq); +int kvm_arm_pmu_init(CPUState *cs); #else @@ -209,6 +210,11 @@ static inline int kvm_arm_pmu_create(CPUState *cs, int irq) return 0; } +static inline int kvm_arm_pmu_init(CPUState *cs) +{ + return 0; +} + #endif static inline const char *gic_class_name(void)
When adding a PMU with a userspace irqchip we only do the INIT stage of the device creation. Signed-off-by: Andrew Jones <drjones@redhat.com> --- hw/arm/virt.c | 10 ++++++++-- target/arm/kvm32.c | 6 ++++++ target/arm/kvm64.c | 52 +++++++++++++++++++++++++--------------------------- target/arm/kvm_arm.h | 6 ++++++ 4 files changed, 45 insertions(+), 29 deletions(-)