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