| Message ID | 1469723896-28049-1-git-send-email-wei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2016/7/29 0:38, Wei Huang wrote: > This patch adds a pmu=[on/off] option to enable/disable vpmu support > in guest vm. There are several reasons to justify this option. First > vpmu can be problematic for cross-migration between different SoC as > perf counters is architecture-dependent. It is more flexible to > have an option to turn it on/off. Secondly it matches the -cpu pmu > option in libivrt. This patch has been tested on both DT/ACPI modes. > > Signed-off-by: Wei Huang <wei@redhat.com> Reviewed-by：Shannon Zhao <shannon.zhao@linaro.org> > --- > hw/arm/virt-acpi-build.c | 2 +- > hw/arm/virt.c | 2 +- > target-arm/cpu.c | 1 + > target-arm/cpu.h | 5 +++-- > target-arm/kvm64.c | 10 +++++----- > 5 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 28fc59c..dc5f66d 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info) > gicc->uid = i; > gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED); > > - if (armcpu->has_pmu) { > + if (armcpu->enable_pmu) { > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > } > } > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a193b5a..6aea901 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype) > > CPU_FOREACH(cpu) { > armcpu = ARM_CPU(cpu); > - if (!armcpu->has_pmu || > + if (!armcpu->enable_pmu || > !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { > return; > } > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index ce8b8f4..f7daf81 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = { > }; > > static Property arm_cpu_properties[] = { > + DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true), > DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false), > DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0), > DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0), > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 76d824d..f2341c0 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -579,8 +579,9 @@ struct ARMCPU { > bool powered_off; > /* CPU has security extension */ > bool has_el3; > - /* CPU has PMU (Performance Monitor Unit) */ > - bool has_pmu; > + > + /* CPU has vPMU (Performance Monitor Unit) support */ > + bool enable_pmu; > > /* CPU has memory protection unit */ > bool has_mpu; > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index 5faa76c..ca21670 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -501,11 +501,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; > - } > + > + /* enable vPMU based on KVM mode and hardware capability */ > + cpu->enable_pmu &= (kvm_irqchip_in_kernel() && > + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)); > + cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3; > > /* Do KVM_ARM_VCPU_INIT ioctl */ > ret = kvm_arm_vcpu_init(cs); >
On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote: > This patch adds a pmu=[on/off] option to enable/disable vpmu support > in guest vm. There are several reasons to justify this option. First > vpmu can be problematic for cross-migration between different SoC as > perf counters is architecture-dependent. It is more flexible to > have an option to turn it on/off. Secondly it matches the -cpu pmu > option in libivrt. This patch has been tested on both DT/ACPI modes. > > Signed-off-by: Wei Huang <wei@redhat.com> > --- > hw/arm/virt-acpi-build.c | 2 +- > hw/arm/virt.c | 2 +- > target-arm/cpu.c | 1 + > target-arm/cpu.h | 5 +++-- > target-arm/kvm64.c | 10 +++++----- > 5 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 28fc59c..dc5f66d 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info) > gicc->uid = i; > gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED); > > - if (armcpu->has_pmu) { > + if (armcpu->enable_pmu) { > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > } > } > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a193b5a..6aea901 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype) > > CPU_FOREACH(cpu) { > armcpu = ARM_CPU(cpu); > - if (!armcpu->has_pmu || > + if (!armcpu->enable_pmu || > !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { > return; > } > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index ce8b8f4..f7daf81 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = { > }; > > static Property arm_cpu_properties[] = { > + DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true), x86's pmu property defaults to off. I'm not sure if it's necessary to have a consistent default between x86 and arm in order for libvirt to be able to use it in the same way. We should confirm with libvirt people. Anyway, I think I'd prefer we default off here, and then we can default on in machine code for configurations that we want it by default (only AArch64 KVM). Or, maybe we don't want it by default at all? Possibly we should only set it on by default for virt-2.6, and then, from 2.7 on, require users to opt-in to the feature. It makes sense to require opting-in to features that can cause problems with migration. > DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false), > DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0), > DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0), > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 76d824d..f2341c0 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -579,8 +579,9 @@ struct ARMCPU { > bool powered_off; > /* CPU has security extension */ > bool has_el3; > - /* CPU has PMU (Performance Monitor Unit) */ > - bool has_pmu; > + > + /* CPU has vPMU (Performance Monitor Unit) support */ > + bool enable_pmu; > > /* CPU has memory protection unit */ > bool has_mpu; > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index 5faa76c..ca21670 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -501,11 +501,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; > - } > + > + /* enable vPMU based on KVM mode and hardware capability */ > + cpu->enable_pmu &= (kvm_irqchip_in_kernel() && > + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)); nit: the () aren't necessary > + cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3; > > /* Do KVM_ARM_VCPU_INIT ioctl */ > ret = kvm_arm_vcpu_init(cs); > -- > 2.4.11 > > OK, so this property will be exposed to all ARM cpu types, and if a user turns it on, then it will stay on for all types, except when using KVM with an aarch64 cpu type, and KVM doesn't support it. This could mislead users to believe they'll get a pmu, by simply adding pmu=on, even when they can't. I think we'd ideally keep has_pmu, and the current code that sets it, and then add code like if (enable_pmu && !has_pmu) { error_report("Warning: ...") } somewhere. Unfortunately I don't think there's any one place we could add that. We'd need to add it to every ARM machine type that cares about not misleading users. Too bad cpu properties aren't whitelisted by machines to avoid this issue. Anyway, all that said, I see this is just how cpu properties currently work, so we probably don't need to worry about it for every machine. I do still suggest we add the above warning to mach-virt though. Thanks, drew
On 28 July 2016 at 17:38, Wei Huang <wei@redhat.com> wrote: > This patch adds a pmu=[on/off] option to enable/disable vpmu support > in guest vm. There are several reasons to justify this option. First > vpmu can be problematic for cross-migration between different SoC as > perf counters is architecture-dependent. It is more flexible to > have an option to turn it on/off. Secondly it matches the -cpu pmu > option in libivrt. This patch has been tested on both DT/ACPI modes. What particular two systems are you trying to migrate between? In general we don't support migrating between different CPU types at the moment, so the PMU sholud be the same on both ends. (If we ever do get to supporting cross-cpu-type migration then it will probably involve a very long and detailed command line to specify exactly a whole lot of things like pmu yes/no, number of hw breakpoints/watchpoints, and everything else that can differ between implementations.) That said, I don't have any objection to making the PMU presence controllable (especially if we have similar control on x86). > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -579,8 +579,9 @@ struct ARMCPU { > bool powered_off; > /* CPU has security extension */ > bool has_el3; > - /* CPU has PMU (Performance Monitor Unit) */ > - bool has_pmu; > + > + /* CPU has vPMU (Performance Monitor Unit) support */ > + bool enable_pmu; Why rename the flag? has_foo is what we use for other features, as you can see in the context of this bit of the patch. > > /* CPU has memory protection unit */ > bool has_mpu; thanks -- PMM
On 07/29/2016 01:54 AM, Andrew Jones wrote: > On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote: >> This patch adds a pmu=[on/off] option to enable/disable vpmu support >> in guest vm. There are several reasons to justify this option. First >> vpmu can be problematic for cross-migration between different SoC as >> perf counters is architecture-dependent. It is more flexible to >> have an option to turn it on/off. Secondly it matches the -cpu pmu >> option in libivrt. This patch has been tested on both DT/ACPI modes. >> >> Signed-off-by: Wei Huang <wei@redhat.com> >> --- >> hw/arm/virt-acpi-build.c | 2 +- >> hw/arm/virt.c | 2 +- >> target-arm/cpu.c | 1 + >> target-arm/cpu.h | 5 +++-- >> target-arm/kvm64.c | 10 +++++----- >> 5 files changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 28fc59c..dc5f66d 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info) >> gicc->uid = i; >> gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED); >> >> - if (armcpu->has_pmu) { >> + if (armcpu->enable_pmu) { >> gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); >> } >> } >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index a193b5a..6aea901 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype) >> >> CPU_FOREACH(cpu) { >> armcpu = ARM_CPU(cpu); >> - if (!armcpu->has_pmu || >> + if (!armcpu->enable_pmu || >> !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { >> return; >> } >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >> index ce8b8f4..f7daf81 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = { >> }; >> >> static Property arm_cpu_properties[] = { >> + DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true), > > x86's pmu property defaults to off. I'm not sure if it's necessary to > have a consistent default between x86 and arm in order for libvirt to > be able to use it in the same way. We should confirm with libvirt > people. Anyway, I think I'd prefer we default off here, and then we > can default on in machine code for configurations that we want it by > default (only AArch64 KVM). Or, maybe we don't want it by default at > all? Possibly we should only set it on by default for virt-2.6, and > then, from 2.7 on, require users to opt-in to the feature. It makes > sense to require opting-in to features that can cause problems with > migration. This option is default=off on x86. I agree that it is a compatibility related issue which can break migration. So it is reasonable to turn it off on ARM as well. Let us wait for libvirt people to voice out. > >> DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false), >> DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0), >> DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0), >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 76d824d..f2341c0 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -579,8 +579,9 @@ struct ARMCPU { >> bool powered_off; >> /* CPU has security extension */ >> bool has_el3; >> - /* CPU has PMU (Performance Monitor Unit) */ >> - bool has_pmu; >> + >> + /* CPU has vPMU (Performance Monitor Unit) support */ >> + bool enable_pmu; >> >> /* CPU has memory protection unit */ >> bool has_mpu; >> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c >> index 5faa76c..ca21670 100644 >> --- a/target-arm/kvm64.c >> +++ b/target-arm/kvm64.c >> @@ -501,11 +501,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; >> - } >> + >> + /* enable vPMU based on KVM mode and hardware capability */ >> + cpu->enable_pmu &= (kvm_irqchip_in_kernel() && >> + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)); > > nit: the () aren't necessary > Will fix >> + cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3; >> >> /* Do KVM_ARM_VCPU_INIT ioctl */ >> ret = kvm_arm_vcpu_init(cs); >> -- >> 2.4.11 >> >> > > OK, so this property will be exposed to all ARM cpu types, and if a user > turns it on, then it will stay on for all types, except when using KVM > with an aarch64 cpu type, and KVM doesn't support it. This could mislead > users to believe they'll get a pmu, by simply adding pmu=on, even when > they can't. I think we'd ideally keep has_pmu, and the current code that > sets it, and then add code like > > if (enable_pmu && !has_pmu) { > error_report("Warning: ...") > } > > somewhere. Unfortunately I don't think there's any one place we could > add that. We'd need to add it to every ARM machine type that cares about > not misleading users. Too bad cpu properties aren't whitelisted by > machines to avoid this issue. > > Anyway, all that said, I see this is just how cpu properties currently > work, so we probably don't need to worry about it for every machine. I > do still suggest we add the above warning to mach-virt though. Will try to keep both has_pmu & enable_pmu in the next re-spin... > > Thanks, > drew >
On 07/29/2016 02:57 AM, Peter Maydell wrote: > On 28 July 2016 at 17:38, Wei Huang <wei@redhat.com> wrote: >> This patch adds a pmu=[on/off] option to enable/disable vpmu support >> in guest vm. There are several reasons to justify this option. First >> vpmu can be problematic for cross-migration between different SoC as >> perf counters is architecture-dependent. It is more flexible to >> have an option to turn it on/off. Secondly it matches the -cpu pmu >> option in libivrt. This patch has been tested on both DT/ACPI modes. > > > What particular two systems are you trying to migrate between? One example: APM's Mustang has 5 perf counters while AMD's Seattle has 7 counters. > In general we don't support migrating between different CPU > types at the moment, so the PMU sholud be the same on both ends. > > (If we ever do get to supporting cross-cpu-type migration > then it will probably involve a very long and detailed command > line to specify exactly a whole lot of things like pmu yes/no, > number of hw breakpoints/watchpoints, and everything else that > can differ between implementations.) > > That said, I don't have any objection to making the PMU > presence controllable (especially if we have similar > control on x86). > >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -579,8 +579,9 @@ struct ARMCPU { >> bool powered_off; >> /* CPU has security extension */ >> bool has_el3; >> - /* CPU has PMU (Performance Monitor Unit) */ >> - bool has_pmu; >> + >> + /* CPU has vPMU (Performance Monitor Unit) support */ >> + bool enable_pmu; > > Why rename the flag? has_foo is what we use for other features, > as you can see in the context of this bit of the patch. I will fix it. Maybe follow the suggestion Drew's suggestion, keeping has_pmu and add another option for turning it on/off. > >> >> /* CPU has memory protection unit */ >> bool has_mpu; > > thanks > -- PMM >
On 29 July 2016 at 16:08, Wei Huang <wei@redhat.com> wrote: > > > On 07/29/2016 02:57 AM, Peter Maydell wrote: >> On 28 July 2016 at 17:38, Wei Huang <wei@redhat.com> wrote: >>> This patch adds a pmu=[on/off] option to enable/disable vpmu support >>> in guest vm. There are several reasons to justify this option. First >>> vpmu can be problematic for cross-migration between different SoC as >>> perf counters is architecture-dependent. It is more flexible to >>> have an option to turn it on/off. Secondly it matches the -cpu pmu >>> option in libivrt. This patch has been tested on both DT/ACPI modes. >> >> >> What particular two systems are you trying to migrate between? > > One example: APM's Mustang has 5 perf counters while AMD's Seattle has 7 > counters. Right; this is a cross-implementation migration, which isn't supposed to work at the moment, and more will be required for it to work than just pmu on/off. thanks -- PMM
On 29 July 2016 at 07:54, Andrew Jones <drjones@redhat.com> wrote: > OK, so this property will be exposed to all ARM cpu types, and if a user > turns it on, then it will stay on for all types, except when using KVM > with an aarch64 cpu type, and KVM doesn't support it. This could mislead > users to believe they'll get a pmu, by simply adding pmu=on, even when > they can't. I think we'd ideally keep has_pmu, and the current code that > sets it, and then add code like > > if (enable_pmu && !has_pmu) { > error_report("Warning: ...") > } > > somewhere. I think we should probably follow the existing model used by has_el3, where the property only exists if it's valid to set it. thanks -- PMM
On Fri, 2016-07-29 at 08:54 +0200, Andrew Jones wrote: > On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote: > > > > This patch adds a pmu=[on/off] option to enable/disable vpmu support > > in guest vm. There are several reasons to justify this option. First > > vpmu can be problematic for cross-migration between different SoC as > > perf counters is architecture-dependent. It is more flexible to > > have an option to turn it on/off. Secondly it matches the -cpu pmu > > option in libivrt. This patch has been tested on both DT/ACPI modes. > > > > Signed-off-by: Wei Huang <wei@redhat.com> > > --- > > hw/arm/virt-acpi-build.c | 2 +- > > hw/arm/virt.c | 2 +- > > target-arm/cpu.c | 1 + > > target-arm/cpu.h | 5 +++-- > > target-arm/kvm64.c | 10 +++++----- > > 5 files changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index 28fc59c..dc5f66d 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info) > > gicc->uid = i; > > gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED); > > > > - if (armcpu->has_pmu) { > > + if (armcpu->enable_pmu) { > > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > > } > > } > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index a193b5a..6aea901 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype) > > > > CPU_FOREACH(cpu) { > > armcpu = ARM_CPU(cpu); > > - if (!armcpu->has_pmu || > > + if (!armcpu->enable_pmu || > > !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { > > return; > > } > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > index ce8b8f4..f7daf81 100644 > > --- a/target-arm/cpu.c > > +++ b/target-arm/cpu.c > > @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = { > > }; > > > > static Property arm_cpu_properties[] = { > > + DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true), > > x86's pmu property defaults to off. I'm not sure if it's necessary to > have a consistent default between x86 and arm in order for libvirt to > be able to use it in the same way. We should confirm with libvirt > people. Anyway, I think I'd prefer we default off here, and then we > can default on in machine code for configurations that we want it by > default (only AArch64 KVM). Or, maybe we don't want it by default at > all? Possibly we should only set it on by default for virt-2.6, and > then, from 2.7 on, require users to opt-in to the feature. It makes > sense to require opting-in to features that can cause problems with > migration. After thinking about this a bit, I don't think it matters that much (from libvirt's point of view) whether the default is on or off - there are a bunch of other situations where the user is required to specify explicitly whether he wants the feature or not, and if he doesn't choose either side he will get whatever QEMU uses as a default. What's important is that the user can pick one or the other when it matters to him, and having a pmu property like the one x86 already has fits the bill. That said, defaulting to off looks like it would be the least confusing behaviour. > > + cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3; > > > > /* Do KVM_ARM_VCPU_INIT ioctl */ > > ret = kvm_arm_vcpu_init(cs); > > OK, so this property will be exposed to all ARM cpu types, and if a user > turns it on, then it will stay on for all types, except when using KVM > with an aarch64 cpu type, and KVM doesn't support it. This could mislead > users to believe they'll get a pmu, by simply adding pmu=on, even when > they can't. I think we'd ideally keep has_pmu, and the current code that > sets it, and then add code like > > if (enable_pmu && !has_pmu) { > error_report("Warning: ...") > } > > somewhere. Unfortunately I don't think there's any one place we could > add that. We'd need to add it to every ARM machine type that cares about > not misleading users. Too bad cpu properties aren't whitelisted by > machines to avoid this issue. > > Anyway, all that said, I see this is just how cpu properties currently > work, so we probably don't need to worry about it for every machine. I > do still suggest we add the above warning to mach-virt though. I'm not sure a warning is enough: if I start a guest and explicitly ask for a PMU, I expect it to be there, or for the guest not to start at all. How does x86 behave in this regard? -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Aug 01, 2016 at 02:04:59PM +0200, Andrea Bolognani wrote: > On Fri, 2016-07-29 at 08:54 +0200, Andrew Jones wrote: > > On Thu, Jul 28, 2016 at 11:38:16AM -0500, Wei Huang wrote: > > > > > > This patch adds a pmu=[on/off] option to enable/disable vpmu support > > > in guest vm. There are several reasons to justify this option. First > > > vpmu can be problematic for cross-migration between different SoC as > > > perf counters is architecture-dependent. It is more flexible to > > > have an option to turn it on/off. Secondly it matches the -cpu pmu > > > option in libivrt. This patch has been tested on both DT/ACPI modes. > > > > > > Signed-off-by: Wei Huang <wei@redhat.com> > > > --- > > > hw/arm/virt-acpi-build.c | 2 +- > > > hw/arm/virt.c | 2 +- > > > target-arm/cpu.c | 1 + > > > target-arm/cpu.h | 5 +++-- > > > target-arm/kvm64.c | 10 +++++----- > > > 5 files changed, 11 insertions(+), 9 deletions(-) > > > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > > index 28fc59c..dc5f66d 100644 > > > --- a/hw/arm/virt-acpi-build.c > > > +++ b/hw/arm/virt-acpi-build.c > > > @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info) > > > gicc->uid = i; > > > gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED); > > > > > > - if (armcpu->has_pmu) { > > > + if (armcpu->enable_pmu) { > > > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > > > } > > > } > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > > index a193b5a..6aea901 100644 > > > --- a/hw/arm/virt.c > > > +++ b/hw/arm/virt.c > > > @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype) > > > > > > CPU_FOREACH(cpu) { > > > armcpu = ARM_CPU(cpu); > > > - if (!armcpu->has_pmu || > > > + if (!armcpu->enable_pmu || > > > !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { > > > return; > > > } > > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > > > index ce8b8f4..f7daf81 100644 > > > --- a/target-arm/cpu.c > > > +++ b/target-arm/cpu.c > > > @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = { > > > }; > > > > > > static Property arm_cpu_properties[] = { > > > + DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true), > > > > x86's pmu property defaults to off. I'm not sure if it's necessary to > > have a consistent default between x86 and arm in order for libvirt to > > be able to use it in the same way. We should confirm with libvirt > > people. Anyway, I think I'd prefer we default off here, and then we > > can default on in machine code for configurations that we want it by > > default (only AArch64 KVM). Or, maybe we don't want it by default at > > all? Possibly we should only set it on by default for virt-2.6, and > > then, from 2.7 on, require users to opt-in to the feature. It makes > > sense to require opting-in to features that can cause problems with > > migration. > > After thinking about this a bit, I don't think it matters that > much (from libvirt's point of view) whether the default is on > or off - there are a bunch of other situations where the user > is required to specify explicitly whether he wants the feature > or not, and if he doesn't choose either side he will get > whatever QEMU uses as a default. > > What's important is that the user can pick one or the other > when it matters to him, and having a pmu property like the one > x86 already has fits the bill. > > That said, defaulting to off looks like it would be the least > confusing behaviour. OK, so the default is still up for debate. Pros of ON Cons of ON ---------- ---------- We already do it The default instance is less migratable Less typing on cmdline (libvirt covers typing for us anyway...) Pros of OFF Cons of OFF ----------- ----------- See 'Cons of ON' See 'Pros on ON' (virt-2.6 needs compat code) > > > > + cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3; > > > > > > /* Do KVM_ARM_VCPU_INIT ioctl */ > > > ret = kvm_arm_vcpu_init(cs); > > > > OK, so this property will be exposed to all ARM cpu types, and if a user > > turns it on, then it will stay on for all types, except when using KVM > > with an aarch64 cpu type, and KVM doesn't support it. This could mislead > > users to believe they'll get a pmu, by simply adding pmu=on, even when > > they can't. I think we'd ideally keep has_pmu, and the current code that > > sets it, and then add code like > > > > if (enable_pmu && !has_pmu) { > > error_report("Warning: ...") > > } > > > > somewhere. Unfortunately I don't think there's any one place we could > > add that. We'd need to add it to every ARM machine type that cares about > > not misleading users. Too bad cpu properties aren't whitelisted by > > machines to avoid this issue. > > > > Anyway, all that said, I see this is just how cpu properties currently > > work, so we probably don't need to worry about it for every machine. I > > do still suggest we add the above warning to mach-virt though. > > I'm not sure a warning is enough: if I start a guest and > explicitly ask for a PMU, I expect it to be there, or for > the guest not to start at all. How does x86 behave in this > regard? Peter had a good suggestion for this. We need to wrap the property addition in an arm_feature check like the has_el3 property. That will remove it from all cpu types that don't support it. Then there's no need for the enable_pmu && !has_pmu check as the has_pmu part is covered very early with the feature flag in arm_cpu_post_init(). Peter also suggested we keep the 'has_pmu' name, rather than change it to 'enable_pmu'. On that one I would disagree. 'has_pmu' indicates that the feature is available at all, which it is to all cpu types that have the arm feature, but not all users will want it enabled. 'enable_pmu', which matches x86's naming, seems more appropriate for that. Thanks, drew > > -- > Andrea Bolognani / Red Hat / Virtualization
On 1 August 2016 at 14:08, Andrew Jones <drjones@redhat.com> wrote: > Peter had a good suggestion for this. We need to wrap the property > addition in an arm_feature check like the has_el3 property. That will > remove it from all cpu types that don't support it. Then there's no > need for the enable_pmu && !has_pmu check as the has_pmu part is covered > very early with the feature flag in arm_cpu_post_init(). Peter also > suggested we keep the 'has_pmu' name, rather than change it to > 'enable_pmu'. On that one I would disagree. 'has_pmu' indicates that the > feature is available at all, which it is to all cpu types that have the > arm feature, but not all users will want it enabled. 'enable_pmu', which > matches x86's naming, seems more appropriate for that. If you create the CPU with pmu=off then it does not have a PMU, and so has_pmu is false. I don't see any reason for the naming convention for the PMU to diverge from what we have for EL3 and for the MPU, where the property names and the struct fields both use 'has'. thanks -- PMM
On Mon, 2016-08-01 at 15:08 +0200, Andrew Jones wrote: > > I'm not sure a warning is enough: if I start a guest and > > explicitly ask for a PMU, I expect it to be there, or for > > the guest not to start at all. How does x86 behave in this > > regard? > > Peter had a good suggestion for this. We need to wrap the property > addition in an arm_feature check like the has_el3 property. That will > remove it from all cpu types that don't support it. Wouldn't that mean that you'd be unable to use -cpu foo,pmu=off if CPU model 'foo' doesn't support a PMU? I'd expect that to work. I've played around with this a bit on x86 and it doesn't look like it necessarily behaves the way I'd expect it to, either, so maybe this is just a case of my expectations being unreasonable? :) -- Andrea Bolognani / Red Hat / Virtualization
On 1 August 2016 at 14:26, Andrea Bolognani <abologna@redhat.com> wrote: > On Mon, 2016-08-01 at 15:08 +0200, Andrew Jones wrote: >> > I'm not sure a warning is enough: if I start a guest and >> > explicitly ask for a PMU, I expect it to be there, or for >> > the guest not to start at all. How does x86 behave in this >> > regard? >> >> Peter had a good suggestion for this. We need to wrap the property >> addition in an arm_feature check like the has_el3 property. That will >> remove it from all cpu types that don't support it. > > Wouldn't that mean that you'd be unable to use > > -cpu foo,pmu=off > > if CPU model 'foo' doesn't support a PMU? I'd expect that > to work. The current precedent (has_el3) doesn't work like that: if foo isn't a CPU which can support EL3 then the property doesn't exist, and it's an error to try to set it. thanks -- PMM
On Mon, 2016-08-01 at 14:32 +0100, Peter Maydell wrote: > > Wouldn't that mean that you'd be unable to use > > > > -cpu foo,pmu=off > > > > if CPU model 'foo' doesn't support a PMU? I'd expect that > > to work. > > The current precedent (has_el3) doesn't work like that: if > foo isn't a CPU which can support EL3 then the property doesn't > exist, and it's an error to try to set it. Doesn't look like the pmu option works like that on x86, though, unless I'm missing something. I have a guest running with -cpu pentium,pmu=on and I can't see hardware perf events from inside the guest, eg. dmesg reports Performance Events: no PMU driver, software events only. and perf tells me <not supported> instructions <not supported> branches ... I'm not sure whether that's because the PMU is being emulated but the kernel doesn't have a driver for it, or whether it's not being emulated at all. Any way to find out? -- Andrea Bolognani / Red Hat / Virtualization
On 08/01/2016 08:32 AM, Peter Maydell wrote: > On 1 August 2016 at 14:26, Andrea Bolognani <abologna@redhat.com> wrote: >> On Mon, 2016-08-01 at 15:08 +0200, Andrew Jones wrote: >>>> I'm not sure a warning is enough: if I start a guest and >>>> explicitly ask for a PMU, I expect it to be there, or for >>>> the guest not to start at all. How does x86 behave in this >>>> regard? >>> >>> Peter had a good suggestion for this. We need to wrap the property >>> addition in an arm_feature check like the has_el3 property. That will >>> remove it from all cpu types that don't support it. >> >> Wouldn't that mean that you'd be unable to use >> >> -cpu foo,pmu=off >> >> if CPU model 'foo' doesn't support a PMU? I'd expect that >> to work. > > The current precedent (has_el3) doesn't work like that: if > foo isn't a CPU which can support EL3 then the property doesn't > exist, and it's an error to try to set it. V1 sent. I tried to follow everyone's advice. See the following: * set default pmu=off * like el3, add a new feature ARM_FEATURE_HOST_PMU * "pmu" property becomes CPU dependent. Only cortex-a53/cortex-a57/host under certain mode support this option * change struct ARMCPU field name "has_pmu" ==> "has_host_pmu" because IMO "has_pmu" is misleading BTW answering Andrea's question above: "-cpu foo,pmu=off" won't be allowed in this patch if CPU "foo" doesn't support host-backed PMU. QEMU will fail to run in this case. Maybe this is what we want? Thanks, -Wei > > thanks > -- PMM >
On Sat, 2016-08-13 at 01:06 -0500, Wei Huang wrote: > > > Wouldn't that mean that you'd be unable to use > > > > > > -cpu foo,pmu=off > > > > > > if CPU model 'foo' doesn't support a PMU? I'd expect that > > > to work. > > > > The current precedent (has_el3) doesn't work like that: if > > foo isn't a CPU which can support EL3 then the property doesn't > > exist, and it's an error to try to set it. > > V1 sent. I tried to follow everyone's advice. See the following: > > * set default pmu=off > * like el3, add a new feature ARM_FEATURE_HOST_PMU > * "pmu" property becomes CPU dependent. Only cortex-a53/cortex-a57/host > under certain mode support this option > * change struct ARMCPU field name "has_pmu" ==> "has_host_pmu" because > IMO "has_pmu" is misleading > > BTW answering Andrea's question above: "-cpu foo,pmu=off" won't be > allowed in this patch if CPU "foo" doesn't support host-backed PMU. QEMU > will fail to run in this case. Maybe this is what we want? After discussing this a bit offline, I came to the conclusion that there isn't a Single Right Way™ to handle this - both my proposal and what you implemented are reasonable behaviors one could expect. On the other hand, what you implemented: * matches x86 * is more strict than what I proposed, so there's room to change it later without breaking any existing guest so I'm happy with it :) -- Andrea Bolognani / Red Hat / Virtualization
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 28fc59c..dc5f66d 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -540,7 +540,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtGuestInfo *guest_info) gicc->uid = i; gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED); - if (armcpu->has_pmu) { + if (armcpu->enable_pmu) { gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); } } diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a193b5a..6aea901 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -477,7 +477,7 @@ static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype) CPU_FOREACH(cpu) { armcpu = ARM_CPU(cpu); - if (!armcpu->has_pmu || + if (!armcpu->enable_pmu || !kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ))) { return; } diff --git a/target-arm/cpu.c b/target-arm/cpu.c index ce8b8f4..f7daf81 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -1412,6 +1412,7 @@ static const ARMCPUInfo arm_cpus[] = { }; static Property arm_cpu_properties[] = { + DEFINE_PROP_BOOL("pmu", ARMCPU, enable_pmu, true), DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false), DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0), DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0), diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 76d824d..f2341c0 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -579,8 +579,9 @@ struct ARMCPU { bool powered_off; /* CPU has security extension */ bool has_el3; - /* CPU has PMU (Performance Monitor Unit) */ - bool has_pmu; + + /* CPU has vPMU (Performance Monitor Unit) support */ + bool enable_pmu; /* CPU has memory protection unit */ bool has_mpu; diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c index 5faa76c..ca21670 100644 --- a/target-arm/kvm64.c +++ b/target-arm/kvm64.c @@ -501,11 +501,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; - } + + /* enable vPMU based on KVM mode and hardware capability */ + cpu->enable_pmu &= (kvm_irqchip_in_kernel() && + kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)); + cpu->kvm_init_features[0] |= cpu->enable_pmu << KVM_ARM_VCPU_PMU_V3; /* Do KVM_ARM_VCPU_INIT ioctl */ ret = kvm_arm_vcpu_init(cs);
This patch adds a pmu=[on/off] option to enable/disable vpmu support in guest vm. There are several reasons to justify this option. First vpmu can be problematic for cross-migration between different SoC as perf counters is architecture-dependent. It is more flexible to have an option to turn it on/off. Secondly it matches the -cpu pmu option in libivrt. This patch has been tested on both DT/ACPI modes. Signed-off-by: Wei Huang <wei@redhat.com> --- hw/arm/virt-acpi-build.c | 2 +- hw/arm/virt.c | 2 +- target-arm/cpu.c | 1 + target-arm/cpu.h | 5 +++-- target-arm/kvm64.c | 10 +++++----- 5 files changed, 11 insertions(+), 9 deletions(-)