Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Peter Maydell]

On 26 October 2016 at 07:28, Wei Huang <wei@redhat.com> wrote:
> This patchset adds a pmu=[on/off] option to enable/disable vPMU support
> for guest VM. There are several reasons to justify this option. First,
> vPMU can be problematic for cross-migration between different SoC as perf
> counters are architecture-dependent. It is more flexible to have an option
> to turn it on/off. Secondly this option matches the "pmu" option as
> supported in libvirt. To make sure backward compatible, a PMU-related
> property is added to mach-virt machine types.

(Sorry, sent this as a reply to the v7 patch; resending to
attach it to the right thread...)

So what actually are we defining as "the PMU exists" here?
The PMU exists in ARMv7 and we do actually implement it
in TCG for v7 and v8 cores (though it is a minimal
architecturally-valid implementation which implements no events,
only the cycle counter).

Should the PMU registers in TCG which we currently provide on
any v7-and-better CPU be hung off ARM_FEATURE_PMU instead ?
(this would result in the behaviour of virt-2.6 for TCG
changing in that we stop providing the PMU when we did
before, but I guess that's OK.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Wei Huang]

On 10/27/2016 09:28 AM, Peter Maydell wrote:
> On 26 October 2016 at 07:28, Wei Huang <wei@redhat.com> wrote:
>> This patchset adds a pmu=[on/off] option to enable/disable vPMU support
>> for guest VM. There are several reasons to justify this option. First,
>> vPMU can be problematic for cross-migration between different SoC as perf
>> counters are architecture-dependent. It is more flexible to have an option
>> to turn it on/off. Secondly this option matches the "pmu" option as
>> supported in libvirt. To make sure backward compatible, a PMU-related
>> property is added to mach-virt machine types.
> 
> (Sorry, sent this as a reply to the v7 patch; resending to
> attach it to the right thread...)
> 
> So what actually are we defining as "the PMU exists" here?

"PMU exists" means that PMUv3 presence can be detected and related
driver can be loaded for guest VM:
* For DT: perf_event.c checks the presence of "armv8-pmuv3" in DT and do
armv8pmu_probe_pmu().
* For ACPI: Jeremy Linton submitted an ACPI-based ARM driver for
upstream. This can be loaded when "PMU exists" is detected.

Link:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/415372.html

> The PMU exists in ARMv7 and we do actually implement it
> in TCG for v7 and v8 cores (though it is a minimal
> architecturally-valid implementation which implements no events,
> only the cycle counter).

This option shouldn't affect the existing TCG implementation. It only
touch the recent hardware-assisted vPMU implemented by Shannon. In other
words, it adds an option for users to turn off Shannon's vPMU (for the
reasons of migration compatibility, etc)

> 
> Should the PMU registers in TCG which we currently provide on
> any v7-and-better CPU be hung off ARM_FEATURE_PMU instead ?
> (this would result in the behaviour of virt-2.6 for TCG
> changing in that we stop providing the PMU when we did
> before, but I guess that's OK.)
> 

So you want to disable cycle_count for TCG mode? Reading ARM doc, I
think cycle_count is minimally required for ARM (correct me if I am
wrong). Here is the description for PMCR.N bits:

"Number of event counters. A RO field that indicates the number counters
implemented. A value of 0b00000 in this field indicates that only the
Cycle Count Register PMCCNTR_EL0 is implemented. The value of this field
is the number of event counters implemented. This value is in the range
of 0b00000 , in which case only the PMCCNTR_EL0 is  implemented, to
0b11111 , which indicates that the PMCCNTR_EL0 and 31 event counters are
implemented."

If we want to disable cycle_count, QEMU has to declare PMCR
in-accessible, right?

> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Peter Maydell]

On 27 October 2016 at 18:06, Wei Huang <wei@redhat.com> wrote:
>
>
> On 10/27/2016 09:28 AM, Peter Maydell wrote:
>> On 26 October 2016 at 07:28, Wei Huang <wei@redhat.com> wrote:
>>> This patchset adds a pmu=[on/off] option to enable/disable vPMU support
>>> for guest VM. There are several reasons to justify this option. First,
>>> vPMU can be problematic for cross-migration between different SoC as perf
>>> counters are architecture-dependent. It is more flexible to have an option
>>> to turn it on/off. Secondly this option matches the "pmu" option as
>>> supported in libvirt. To make sure backward compatible, a PMU-related
>>> property is added to mach-virt machine types.
>>
>> (Sorry, sent this as a reply to the v7 patch; resending to
>> attach it to the right thread...)
>>
>> So what actually are we defining as "the PMU exists" here?
>
> "PMU exists" means that PMUv3 presence can be detected and related
> driver can be loaded for guest VM:
> * For DT: perf_event.c checks the presence of "armv8-pmuv3" in DT and do
> armv8pmu_probe_pmu().
> * For ACPI: Jeremy Linton submitted an ACPI-based ARM driver for
> upstream. This can be loaded when "PMU exists" is detected.
>
> Link:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/415372.html
>
>> The PMU exists in ARMv7 and we do actually implement it
>> in TCG for v7 and v8 cores (though it is a minimal
>> architecturally-valid implementation which implements no events,
>> only the cycle counter).
>
> This option shouldn't affect the existing TCG implementation. It only
> touch the recent hardware-assisted vPMU implemented by Shannon. In other
> words, it adds an option for users to turn off Shannon's vPMU (for the
> reasons of migration compatibility, etc)

Right, but at the moment the commit message says there's no
PMU in TCG at all. What's missing that causes there not to be one?
(I think maybe we just don't report it in the feature register,
which might be a bug.)

>> Should the PMU registers in TCG which we currently provide on
>> any v7-and-better CPU be hung off ARM_FEATURE_PMU instead ?
>> (this would result in the behaviour of virt-2.6 for TCG
>> changing in that we stop providing the PMU when we did
>> before, but I guess that's OK.)
>>
>
> So you want to disable cycle_count for TCG mode? Reading ARM doc, I
> think cycle_count is minimally required for ARM (correct me if I am
> wrong). Here is the description for PMCR.N bits:
>
> "Number of event counters. A RO field that indicates the number counters
> implemented. A value of 0b00000 in this field indicates that only the
> Cycle Count Register PMCCNTR_EL0 is implemented. The value of this field
> is the number of event counters implemented. This value is in the range
> of 0b00000 , in which case only the PMCCNTR_EL0 is  implemented, to
> 0b11111 , which indicates that the PMCCNTR_EL0 and 31 event counters are
> implemented."
>
> If we want to disable cycle_count, QEMU has to declare PMCR
> in-accessible, right?

The cycle counter is required if the PMU exists, but the whole PMU
is optional. If the user says 'pmu=off' then shouldn't we be
disabling the implementation in TCG?

thanks
-- PMM

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Wei Huang]

On 10/27/2016 12:15 PM, Peter Maydell wrote:
> On 27 October 2016 at 18:06, Wei Huang <wei@redhat.com> wrote:
>>
>>
>> On 10/27/2016 09:28 AM, Peter Maydell wrote:
>>> On 26 October 2016 at 07:28, Wei Huang <wei@redhat.com> wrote:
>>>> This patchset adds a pmu=[on/off] option to enable/disable vPMU support
>>>> for guest VM. There are several reasons to justify this option. First,
>>>> vPMU can be problematic for cross-migration between different SoC as perf
>>>> counters are architecture-dependent. It is more flexible to have an option
>>>> to turn it on/off. Secondly this option matches the "pmu" option as
>>>> supported in libvirt. To make sure backward compatible, a PMU-related
>>>> property is added to mach-virt machine types.
>>>
>>> (Sorry, sent this as a reply to the v7 patch; resending to
>>> attach it to the right thread...)
>>>
>>> So what actually are we defining as "the PMU exists" here?
>>
>> "PMU exists" means that PMUv3 presence can be detected and related
>> driver can be loaded for guest VM:
>> * For DT: perf_event.c checks the presence of "armv8-pmuv3" in DT and do
>> armv8pmu_probe_pmu().
>> * For ACPI: Jeremy Linton submitted an ACPI-based ARM driver for
>> upstream. This can be loaded when "PMU exists" is detected.
>>
>> Link:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/415372.html
>>
>>> The PMU exists in ARMv7 and we do actually implement it
>>> in TCG for v7 and v8 cores (though it is a minimal
>>> architecturally-valid implementation which implements no events,
>>> only the cycle counter).
>>
>> This option shouldn't affect the existing TCG implementation. It only
>> touch the recent hardware-assisted vPMU implemented by Shannon. In other
>> words, it adds an option for users to turn off Shannon's vPMU (for the
>> reasons of migration compatibility, etc)
> 
> Right, but at the moment the commit message says there's no
> PMU in TCG at all. What's missing that causes there not to be one?
> (I think maybe we just don't report it in the feature register,
> which might be a bug.)

This sounds like a bug (or incomplete). In current version of QEMU, if I
forced vPMU declaration in the feature register under TCG mode, PMU
driver will be loaded in VM. But as soon as "perf stat" is executed,
guest VM panic. I think some features, like PMU IRQ, is still missing
for TCG to turn vPMU on. I will debug it further.

> 
>>> Should the PMU registers in TCG which we currently provide on
>>> any v7-and-better CPU be hung off ARM_FEATURE_PMU instead ?
>>> (this would result in the behaviour of virt-2.6 for TCG
>>> changing in that we stop providing the PMU when we did
>>> before, but I guess that's OK.)
>>>
>>
>> So you want to disable cycle_count for TCG mode? Reading ARM doc, I
>> think cycle_count is minimally required for ARM (correct me if I am
>> wrong). Here is the description for PMCR.N bits:
>>
>> "Number of event counters. A RO field that indicates the number counters
>> implemented. A value of 0b00000 in this field indicates that only the
>> Cycle Count Register PMCCNTR_EL0 is implemented. The value of this field
>> is the number of event counters implemented. This value is in the range
>> of 0b00000 , in which case only the PMCCNTR_EL0 is  implemented, to
>> 0b11111 , which indicates that the PMCCNTR_EL0 and 31 event counters are
>> implemented."
>>
>> If we want to disable cycle_count, QEMU has to declare PMCR
>> in-accessible, right?
> 
> The cycle counter is required if the PMU exists, but the whole PMU
> is optional. If the user says 'pmu=off' then shouldn't we be
> disabling the implementation in TCG?
> 

Let me clarify. Assuming we can get TCG mode to support VM's PMU driver
(see discussion above, only cycle counter supported for TCG), will the
following be sufficient?
(1) ,pmu=on
perf driver can be loaded in VM, and "perf" command can use HW counters
to profile.
(2) ,pmu=off
perf feature can't be detected, so the perf driver can't be loaded
inside VM. "perf" can't use HW perf counters. But PMU registers (PMCR,
PMCCNTR, ...) can still be accessed inside VM.

OR

You prefer to disable access permission for PMU registers in (2). Note:
this is very strict. I am not sure if current KVM already follows this rule.


> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Peter Maydell]

On 27 October 2016 at 21:53, Wei Huang <wei@redhat.com> wrote:
> On 10/27/2016 12:15 PM, Peter Maydell wrote:
>> Right, but at the moment the commit message says there's no
>> PMU in TCG at all. What's missing that causes there not to be one?
>> (I think maybe we just don't report it in the feature register,
>> which might be a bug.)
>
> This sounds like a bug (or incomplete). In current version of QEMU, if I
> forced vPMU declaration in the feature register under TCG mode, PMU
> driver will be loaded in VM. But as soon as "perf stat" is executed,
> guest VM panic. I think some features, like PMU IRQ, is still missing
> for TCG to turn vPMU on. I will debug it further.

You're right, we don't have the PMU IRQ implemented.
I wouldn't expect that to cause the guest to panic, though...

> Let me clarify. Assuming we can get TCG mode to support VM's PMU driver
> (see discussion above, only cycle counter supported for TCG), will the
> following be sufficient?
> (1) ,pmu=on
> perf driver can be loaded in VM, and "perf" command can use HW counters
> to profile.
> (2) ,pmu=off
> perf feature can't be detected, so the perf driver can't be loaded
> inside VM. "perf" can't use HW perf counters. But PMU registers (PMCR,
> PMCCNTR, ...) can still be accessed inside VM.

I think that's what we get today (with this patch set). But it
would seem more consistent if pmu=off didn't provide the PMCR,
PMCCNTR, etc to the guest under TCG in the same way that you
don't get them under KVM if you say pmu=off.

Anyway, I think that these patches are the right thing for KVM,
so I'm going to apply them to target-arm.next. We should probably
look at straightening out the TCG situation, but we can do that
with a separate patchset (and perhaps not for 2.8).

Thanks for your patience in working through all the revisions
of this series.

-- PMM

[Qemu-devel] [PATCH V8 0/2] Add option to configure guest vPMU

[Wei Huang]

This patchset adds a pmu=[on/off] option to enable/disable vPMU support 
for guest VM. There are several reasons to justify this option. First,
vPMU can be problematic for cross-migration between different SoC as perf
counters are architecture-dependent. It is more flexible to have an option
to turn it on/off. Secondly this option matches the "pmu" option as
supported in libvirt. To make sure backward compatible, a PMU-related
property is added to mach-virt machine types.

The following are testing results with this patchset:
 CONFIG (qemu-system-aarch64)                       vPMU   WARNING
  -M virt-2.8/virt,accel=kvm -cpu host               YES    NO
  -M virt-2.8/virt,accel=kvm -cpu host,pmu=off       NO     NO
  -M virt-2.8/virt,accel=kvm -cpu host,pmu=on        YES    NO
  -M virt-2.7,accel=kvm -cpu host                    YES    NO
  -M virt-2.7,accel=kvm -cpu host,pmu=off            NO     NO
  -M virt-2.7,accel=kvm -cpu host,pmu=on             YES    NO
  -M virt-2.6,accel=kvm -cpu host                    NO     NO
  -M virt-2.6,accel=kvm -cpu host,pmu=off            NO     NO
  -M virt-2.6,accel=kvm -cpu host,pmu=on             NO     NO

  -M virt-2.8/virt,accel=tcg -cpu cortex-a57         NO     NO
  -M virt-2.8/virt,accel=tcg -cpu cortex-a57,pmu=off NO     NO
  -M virt-2.8/virt,accel=tcg -cpu cortex-a57,pmu=on  NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a57              NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=off      NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a57,pmu=on       NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a57              NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=off      NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a57,pmu=on       NO     NO

  -M virt-2.8/virt,accel=tcg -cpu cortex-a15         NO     NO
  -M virt-2.8/virt,accel=tcg -cpu cortex-a15,pmu=off NO     "No PMU property"
  -M virt-2.8/virt,accel=tcg -cpu cortex-a15,pmu=on  NO     "No PMU property"
  -M virt-2.7,accel=tcg -cpu cortex-a15              NO     NO
  -M virt-2.7,accel=tcg -cpu cortex-a15,pmu=off      NO     "No PMU property"
  -M virt-2.7,accel=tcg -cpu cortex-a15,pmu=on       NO     "No PMU property"
  -M virt-2.6,accel=tcg -cpu cortex-a15              NO     NO
  -M virt-2.6,accel=tcg -cpu cortex-a15,pmu=off      NO     "No PMU property"
  -M virt-2.6,accel=tcg -cpu cortex-a15,pmu=on       NO     "No PMU property"

  * "No PMU property" msg
    can't apply global cortex-a15-arm-cpu.pmu=off: Property '.pmu' not found

V7->V8:
  * add back the "pmu" property for TCG mode

V6->V7:
  * change has_pmu variable type from OnOffAuto to Boolean
  * only add "pmu" property to CPU under kvm mode, default ON
  * set no_pmu=true for machvirt-2.6

V5->V6:
  * adapt patches for new machine type 2.8

V4->V5:
  * remove comment change for has_pmu
  * remove warning msg when pmu_default_on=TRUE && has_pmu=AUTO && tcg=TRUE

V3->V4:
  * change has_pmu from Boolean to OnOffAuto to handle different cases
  * "pmu" property is re-defined as DEFINE_PROP_ON_OFF_AUTO

V2->V3:
  * revise patch 1 commit msg and if-else statement (Drew) 
  * move property field into VirtMachineClass (Drew)

V1->V2:
  * keep the original field name as "has_pmu"
  * add a warning message when PMU is turned on without KVM
  * use the feature bit to check PMU availability, instead of using has_pmu
  * add PMU compat support to mach-virt machine type

RFC->V1:
  * set default pmu=off
  * change struct ARMCPU field name "has_pmu" ==> "has_host_pmu"
  * like el3, add a new feature ARM_FEATURE_HOST_PMU
  * "pmu" property becomes CPU dependent. Only cortex-a53/cortex-a57/host
    running on kvm supports this option.

Thanks,
-Wei

Wei Huang (2):
  arm: Add an option to turn on/off vPMU support
  arm: virt: add PMU property to mach-virt machine type

 hw/arm/virt-acpi-build.c |  2 +-
 hw/arm/virt.c            |  9 ++++++++-
 target-arm/cpu.c         | 15 +++++++++++++++
 target-arm/cpu.h         |  1 +
 target-arm/cpu64.c       |  2 ++
 target-arm/kvm64.c       | 17 ++++++++++++++---
 6 files changed, 41 insertions(+), 5 deletions(-)

-- 
1.8.3.1