Re: [Qemu-devel] [PATCH RFC 1/1] arm64: add an option to turn on/off vpmu support

[Andrew Jones]

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 <address@hidden>
> > > ---
> > >  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