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

[Andrea Bolognani <abologna@redhat.com>]

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