Introduce new Xen command line parameter called "pmu_access".
The default value is "trap": Xen traps PMU accesses.
In case of setting pmu_access to "native", Xen does not trap
PMU accesses allowing all the guests to access PMU registers.
However, guests cannot make use of PMU overflow interrupts as
PMU uses PPI which Xen cannot route to guests.

This option is only intended for development and testing purposes.
Do not use this in production system.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 docs/misc/xen-command-line.pandoc | 18 +++++++++++++++++
 xen/arch/arm/traps.c              | 33 ++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index b175645fde..03637a9f6d 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1813,6 +1813,24 @@ paging controls access to usermode addresses.
 ### ple_window (Intel)
 > `= <integer>`
 
+### pmu_access (arm)
+> `= trap | native`
+
+> Default: `trap`
+
+Controls for accessing Performance Monitor Unit (PMU).
+
+By default Xen traps Performance Monitor accesses.
+When setting pmu_access to `native`, Xen does not trap PMU accesses allowing
+the guests to access PMU registers. This option is intended to aid monitoring
+and measuring the performance. Setting pmu_access to `native` allows
+all the guests to access PMU, however, there is no mechanism for forwarding
+PMU overflow interrupt requests.
+
+*Warning*
+This option is only intended for development and testing purposes.
+Do not use this in production system.
+
 ### psr (Intel)
 > `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> | cdp:<boolean> )`
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 219ab3c3fb..d30e78b4d6 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -34,6 +34,7 @@
 #include <xen/symbols.h>
 #include <xen/version.h>
 #include <xen/virtual_region.h>
+#include <xen/warning.h>
 
 #include <public/sched.h>
 #include <public/xen.h>
@@ -77,12 +78,19 @@ static int debug_stack_lines = 40;
 #define stack_words_per_line 4
 #endif
 
+static const char __initconst warning_pmu_access[] =
+    "WARNING: PMU ACCESSES ARE NOW ENABLED\n"
+    "This option is intended to aid monitoring and measuring\n"
+    "the performance by allowing the guests to access PMU registers.\n"
+    "It has implications on the security of the system.\n"
+    "Please *DO NOT* use this in production.\n";
+
 integer_param("debug_stack_lines", debug_stack_lines);
 
 static enum {
 	TRAP,
 	NATIVE,
-} vwfi;
+} vwfi, pmu_access;
 
 static int __init parse_vwfi(const char *s)
 {
@@ -95,6 +103,29 @@ static int __init parse_vwfi(const char *s)
 }
 custom_param("vwfi", parse_vwfi);
 
+static int __init parse_pmu_access(const char *s)
+{
+    if ( !strcmp(s, "native") )
+        pmu_access = NATIVE;
+    else
+        pmu_access = TRAP;
+
+    return 0;
+}
+custom_param("pmu_access", parse_pmu_access);
+
+static int __init update_pmu_access(void)
+{
+    if ( pmu_access == NATIVE )
+    {
+        WRITE_SYSREG(READ_SYSREG(MDCR_EL2) &~ (HDCR_TPM|HDCR_TPMCR), MDCR_EL2);
+        warning_add(warning_pmu_access);
+    }
+
+    return 0;
+}
+__initcall(update_pmu_access);
+
 register_t get_default_hcr_flags(void)
 {
     return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
-- 
2.29.0

Hi,

On 01/09/2021 13:43, Michal Orzel wrote:
> Introduce new Xen command line parameter called "pmu_access".
> The default value is "trap": Xen traps PMU accesses.
> In case of setting pmu_access to "native", Xen does not trap
> PMU accesses allowing all the guests to access PMU registers.
> However, guests cannot make use of PMU overflow interrupts as
> PMU uses PPI which Xen cannot route to guests.
> 
> This option is only intended for development and testing purposes.
> Do not use this in production system.
I am afraid your option is not safe even in development system as a vCPU 
may move between pCPUs.

However, even if we restricted the use to pinned vCPU *and* dedicated 
pCPU, I am not convinced that exposing an half backed PMU (the overflow 
interrupt would not work) to the guest is the right solution. This 
likely means the guest OS would need to be modified and therefore the 
usage of this option is fairly limited.

So I think the first steps are:
   1) Make the PPI work. There was some attempt in the past for it on 
xen-devel. You could have a look.
   2) Provide PMU bindings

With that in place, we can discuss how to expose the PMU even if it is 
unsafe in some conditions.

> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com> > ---
>   docs/misc/xen-command-line.pandoc | 18 +++++++++++++++++
>   xen/arch/arm/traps.c              | 33 ++++++++++++++++++++++++++++++-
>   2 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index b175645fde..03637a9f6d 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1813,6 +1813,24 @@ paging controls access to usermode addresses.
>   ### ple_window (Intel)
>   > `= <integer>`
>   
> +### pmu_access (arm)
> +> `= trap | native`
> +
> +> Default: `trap`
> +
> +Controls for accessing Performance Monitor Unit (PMU).
> +
> +By default Xen traps Performance Monitor accesses.
> +When setting pmu_access to `native`, Xen does not trap PMU accesses allowing
> +the guests to access PMU registers. This option is intended to aid monitoring
> +and measuring the performance. Setting pmu_access to `native` allows
> +all the guests to access PMU, however, there is no mechanism for forwarding
> +PMU overflow interrupt requests.
> +
> +*Warning*
> +This option is only intended for development and testing purposes.
> +Do not use this in production system.
> +
>   ### psr (Intel)
>   > `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> | cdp:<boolean> )`
>   
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 219ab3c3fb..d30e78b4d6 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -34,6 +34,7 @@
>   #include <xen/symbols.h>
>   #include <xen/version.h>
>   #include <xen/virtual_region.h>
> +#include <xen/warning.h>
>   
>   #include <public/sched.h>
>   #include <public/xen.h>
> @@ -77,12 +78,19 @@ static int debug_stack_lines = 40;
>   #define stack_words_per_line 4
>   #endif
>   
> +static const char __initconst warning_pmu_access[] =
> +    "WARNING: PMU ACCESSES ARE NOW ENABLED\n"
> +    "This option is intended to aid monitoring and measuring\n"
> +    "the performance by allowing the guests to access PMU registers.\n"
> +    "It has implications on the security of the system.\n"
> +    "Please *DO NOT* use this in production.\n";
> +
>   integer_param("debug_stack_lines", debug_stack_lines);
>   
>   static enum {
>   	TRAP,
>   	NATIVE,
> -} vwfi;
> +} vwfi, pmu_access;
>   
>   static int __init parse_vwfi(const char *s)
>   {
> @@ -95,6 +103,29 @@ static int __init parse_vwfi(const char *s)
>   }
>   custom_param("vwfi", parse_vwfi);
>   
> +static int __init parse_pmu_access(const char *s)
> +{
> +    if ( !strcmp(s, "native") )
> +        pmu_access = NATIVE;
> +    else
> +        pmu_access = TRAP;
> +
> +    return 0;
> +}
> +custom_param("pmu_access", parse_pmu_access);
> +
> +static int __init update_pmu_access(void)
> +{
> +    if ( pmu_access == NATIVE )
> +    {
> +        WRITE_SYSREG(READ_SYSREG(MDCR_EL2) &~ (HDCR_TPM|HDCR_TPMCR), MDCR_EL2);
> +        warning_add(warning_pmu_access);
> +    }
> +
> +    return 0;
> +}
> +__initcall(update_pmu_access);
> +
>   register_t get_default_hcr_flags(void)
>   {
>       return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
> 

Cheers,

-- 
Julien Grall

Hi Julien,

> On 1 Sep 2021, at 13:55, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 01/09/2021 13:43, Michal Orzel wrote:
>> Introduce new Xen command line parameter called "pmu_access".
>> The default value is "trap": Xen traps PMU accesses.
>> In case of setting pmu_access to "native", Xen does not trap
>> PMU accesses allowing all the guests to access PMU registers.
>> However, guests cannot make use of PMU overflow interrupts as
>> PMU uses PPI which Xen cannot route to guests.
>> This option is only intended for development and testing purposes.
>> Do not use this in production system.
> I am afraid your option is not safe even in development system as a vCPU may move between pCPUs.
> 
> However, even if we restricted the use to pinned vCPU *and* dedicated pCPU, I am not convinced that exposing an half backed PMU (the overflow interrupt would not work) to the guest is the right solution. This likely means the guest OS would need to be modified and therefore the usage of this option is fairly limited.
> 
> So I think the first steps are:
>  1) Make the PPI work. There was some attempt in the past for it on xen-devel. You could have a look.
>  2) Provide PMU bindings
> 
> With that in place, we can discuss how to expose the PMU even if it is unsafe in some conditions.

With those limitations, using the PMU to monitor the system performances or on some specific use cases is still really useful.
We are using that to do some benchmarks of Xen or of some applications to compare the behaviour to a native system or
analyse the performances of Xen itself (hypercalls,context switch …etc)

The steps you are mentioning do make sense but using the PMU without those knowing the limitations is also very useful.

Cheers
Bertrand


> 
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com> > ---
>>  docs/misc/xen-command-line.pandoc | 18 +++++++++++++++++
>>  xen/arch/arm/traps.c              | 33 ++++++++++++++++++++++++++++++-
>>  2 files changed, 50 insertions(+), 1 deletion(-)
>> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
>> index b175645fde..03637a9f6d 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1813,6 +1813,24 @@ paging controls access to usermode addresses.
>>  ### ple_window (Intel)
>>  > `= <integer>`
>>  +### pmu_access (arm)
>> +> `= trap | native`
>> +
>> +> Default: `trap`
>> +
>> +Controls for accessing Performance Monitor Unit (PMU).
>> +
>> +By default Xen traps Performance Monitor accesses.
>> +When setting pmu_access to `native`, Xen does not trap PMU accesses allowing
>> +the guests to access PMU registers. This option is intended to aid monitoring
>> +and measuring the performance. Setting pmu_access to `native` allows
>> +all the guests to access PMU, however, there is no mechanism for forwarding
>> +PMU overflow interrupt requests.
>> +
>> +*Warning*
>> +This option is only intended for development and testing purposes.
>> +Do not use this in production system.
>> +
>>  ### psr (Intel)
>>  > `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | cos_max:<integer> | cdp:<boolean> )`
>>  diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 219ab3c3fb..d30e78b4d6 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -34,6 +34,7 @@
>>  #include <xen/symbols.h>
>>  #include <xen/version.h>
>>  #include <xen/virtual_region.h>
>> +#include <xen/warning.h>
>>    #include <public/sched.h>
>>  #include <public/xen.h>
>> @@ -77,12 +78,19 @@ static int debug_stack_lines = 40;
>>  #define stack_words_per_line 4
>>  #endif
>>  +static const char __initconst warning_pmu_access[] =
>> +    "WARNING: PMU ACCESSES ARE NOW ENABLED\n"
>> +    "This option is intended to aid monitoring and measuring\n"
>> +    "the performance by allowing the guests to access PMU registers.\n"
>> +    "It has implications on the security of the system.\n"
>> +    "Please *DO NOT* use this in production.\n";
>> +
>>  integer_param("debug_stack_lines", debug_stack_lines);
>>    static enum {
>>  	TRAP,
>>  	NATIVE,
>> -} vwfi;
>> +} vwfi, pmu_access;
>>    static int __init parse_vwfi(const char *s)
>>  {
>> @@ -95,6 +103,29 @@ static int __init parse_vwfi(const char *s)
>>  }
>>  custom_param("vwfi", parse_vwfi);
>>  +static int __init parse_pmu_access(const char *s)
>> +{
>> +    if ( !strcmp(s, "native") )
>> +        pmu_access = NATIVE;
>> +    else
>> +        pmu_access = TRAP;
>> +
>> +    return 0;
>> +}
>> +custom_param("pmu_access", parse_pmu_access);
>> +
>> +static int __init update_pmu_access(void)
>> +{
>> +    if ( pmu_access == NATIVE )
>> +    {
>> +        WRITE_SYSREG(READ_SYSREG(MDCR_EL2) &~ (HDCR_TPM|HDCR_TPMCR), MDCR_EL2);
>> +        warning_add(warning_pmu_access);
>> +    }
>> +
>> +    return 0;
>> +}
>> +__initcall(update_pmu_access);
>> +
>>  register_t get_default_hcr_flags(void)
>>  {
>>      return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
> 
> Cheers,
> 
> -- 
> Julien Grall

On 01/09/2021 14:10, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 1 Sep 2021, at 13:55, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 01/09/2021 13:43, Michal Orzel wrote:
>>> Introduce new Xen command line parameter called "pmu_access".
>>> The default value is "trap": Xen traps PMU accesses.
>>> In case of setting pmu_access to "native", Xen does not trap
>>> PMU accesses allowing all the guests to access PMU registers.
>>> However, guests cannot make use of PMU overflow interrupts as
>>> PMU uses PPI which Xen cannot route to guests.
>>> This option is only intended for development and testing purposes.
>>> Do not use this in production system.
>> I am afraid your option is not safe even in development system as a vCPU may move between pCPUs.
>>
>> However, even if we restricted the use to pinned vCPU *and* dedicated pCPU, I am not convinced that exposing an half backed PMU (the overflow interrupt would not work) to the guest is the right solution. This likely means the guest OS would need to be modified and therefore the usage of this option is fairly limited.
>>
>> So I think the first steps are:
>>   1) Make the PPI work. There was some attempt in the past for it on xen-devel. You could have a look.
>>   2) Provide PMU bindings
>>
>> With that in place, we can discuss how to expose the PMU even if it is unsafe in some conditions.
> 
> With those limitations, using the PMU to monitor the system performances or on some specific use cases is still really useful.
> We are using that to do some benchmarks of Xen or of some applications to compare the behaviour to a native system or
> analyse the performances of Xen itself (hypercalls,context switch …etc)

I understand this is useful for some setup and I am not trying to say we 
should not have a way to expose the PMU (even unsafely) in upstream. 
However, I think the option as it stands is too wide (this should be a 
per domain knob) and we should properly expose the PMU (interrupts, 
bindings...).

> 
> The steps you are mentioning do make sense but using the PMU without those knowing the limitations is also very useful.

Right, this would require a lot more work than what I wrote before.

Cheers,

-- 
Julien Grall

On Wed, 1 Sep 2021, Julien Grall wrote:
> On 01/09/2021 14:10, Bertrand Marquis wrote:
> > > On 1 Sep 2021, at 13:55, Julien Grall <julien@xen.org> wrote:
> > > 
> > > Hi,
> > > 
> > > On 01/09/2021 13:43, Michal Orzel wrote:
> > > > Introduce new Xen command line parameter called "pmu_access".
> > > > The default value is "trap": Xen traps PMU accesses.
> > > > In case of setting pmu_access to "native", Xen does not trap
> > > > PMU accesses allowing all the guests to access PMU registers.
> > > > However, guests cannot make use of PMU overflow interrupts as
> > > > PMU uses PPI which Xen cannot route to guests.
> > > > This option is only intended for development and testing purposes.
> > > > Do not use this in production system.
> > > I am afraid your option is not safe even in development system as a vCPU
> > > may move between pCPUs.
> > > 
> > > However, even if we restricted the use to pinned vCPU *and* dedicated
> > > pCPU, I am not convinced that exposing an half backed PMU (the overflow
> > > interrupt would not work) to the guest is the right solution. This likely
> > > means the guest OS would need to be modified and therefore the usage of
> > > this option is fairly limited.
> > > 
> > > So I think the first steps are:
> > >   1) Make the PPI work. There was some attempt in the past for it on
> > > xen-devel. You could have a look.
> > >   2) Provide PMU bindings
> > > 
> > > With that in place, we can discuss how to expose the PMU even if it is
> > > unsafe in some conditions.
> > 
> > With those limitations, using the PMU to monitor the system performances or
> > on some specific use cases is still really useful.
> > We are using that to do some benchmarks of Xen or of some applications to
> > compare the behaviour to a native system or
> > analyse the performances of Xen itself (hypercalls,context switch …etc)

I also already had to write a patch almost exactly like this one to
provide to customers a few months back.


> I understand this is useful for some setup and I am not trying to say we
> should not have a way to expose the PMU (even unsafely) in upstream. However,
> I think the option as it stands is too wide (this should be a per domain knob)
> and we should properly expose the PMU (interrupts, bindings...).

I have never used PMU directly myself, only provided a patch similar to
this one.  But as far as I could tell the users were fully satisfied
with it and it had no interrupts support either. Could it be that
interrupts are not actually needed to read the perf counters, which is
probably what users care about?

In regards to "this should be a per domain knob", in reality if you are
doing PMU experiments you don't care if only one or all domains have
access: you are working in a controlled environment trying to figure out
if your setup meets the timing requirements. There are no security or
safety concerns (those are different experiments) and there is no
interference problems in the sense of multiple domains trying to access
PMU at the same time -- you control the domains you decide which one is
accessing them.

So I am in favor of a patch like this one because it actually satisfy
our requirements. Even if we had per-domain support and interrupts
support, I don't think they would end up being used.

Hi Stefano,

On 01/09/2021 18:54, Stefano Stabellini wrote:
> On Wed, 1 Sep 2021, Julien Grall wrote:
>> On 01/09/2021 14:10, Bertrand Marquis wrote:
>>>> On 1 Sep 2021, at 13:55, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 01/09/2021 13:43, Michal Orzel wrote:
>>>>> Introduce new Xen command line parameter called "pmu_access".
>>>>> The default value is "trap": Xen traps PMU accesses.
>>>>> In case of setting pmu_access to "native", Xen does not trap
>>>>> PMU accesses allowing all the guests to access PMU registers.
>>>>> However, guests cannot make use of PMU overflow interrupts as
>>>>> PMU uses PPI which Xen cannot route to guests.
>>>>> This option is only intended for development and testing purposes.
>>>>> Do not use this in production system.
>>>> I am afraid your option is not safe even in development system as a vCPU
>>>> may move between pCPUs.
>>>>
>>>> However, even if we restricted the use to pinned vCPU *and* dedicated
>>>> pCPU, I am not convinced that exposing an half backed PMU (the overflow
>>>> interrupt would not work) to the guest is the right solution. This likely
>>>> means the guest OS would need to be modified and therefore the usage of
>>>> this option is fairly limited.
>>>>
>>>> So I think the first steps are:
>>>>    1) Make the PPI work. There was some attempt in the past for it on
>>>> xen-devel. You could have a look.
>>>>    2) Provide PMU bindings
>>>>
>>>> With that in place, we can discuss how to expose the PMU even if it is
>>>> unsafe in some conditions.
>>>
>>> With those limitations, using the PMU to monitor the system performances or
>>> on some specific use cases is still really useful.
>>> We are using that to do some benchmarks of Xen or of some applications to
>>> compare the behaviour to a native system or
>>> analyse the performances of Xen itself (hypercalls,context switch …etc)
> 
> I also already had to write a patch almost exactly like this one to
> provide to customers a few months back.
> 
> 
>> I understand this is useful for some setup and I am not trying to say we
>> should not have a way to expose the PMU (even unsafely) in upstream. However,
>> I think the option as it stands is too wide (this should be a per domain knob)
>> and we should properly expose the PMU (interrupts, bindings...).
> 
> I have never used PMU directly myself, only provided a patch similar to
> this one.  But as far as I could tell the users were fully satisfied
> with it and it had no interrupts support either. Could it be that
> interrupts are not actually needed to read the perf counters, which is
> probably what users care about?

You don't need the interrupts to read the perf counters. But AFAIU, you 
would have to poll at a regular interval yourself. There is also the 
question on how to catch the overflow?

> 
> In regards to "this should be a per domain knob", in reality if you are
> doing PMU experiments you don't care if only one or all domains have
> access: you are working in a controlled environment trying to figure out
> if your setup meets the timing requirements. There are no security or
> safety concerns (those are different experiments) and there is no
> interference problems in the sense of multiple domains trying to access
> PMU at the same time -- you control the domains you decide which one is
> accessing them. >
> So I am in favor of a patch like this one because it actually satisfy
> our requirements. Even if we had per-domain support and interrupts
> support, I don't think they would end up being used.

I have to disagree with that. There are valid use-cases where you may 
want to expose the PMU for using perf to a single domain because you 
know it is safe to do so. I appreciate this is may not be the use case 
for your users (yet), but I have seen (and used) it on x86. So to me 
this approach is short-sighed.

TBH, this is not the first time I have seen patch for "let's expose the 
same feature to everyone because this is easy to do" and really dislike 
it. Exposing a new feature from the toolstack is easier than you think 
(baring the lack of reviews), this is a matter of creating a new flag a 
new option. This would make Xen a lot more flexible and enable more users...

So as it stands:

NAcked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Hi Julien,

> On 1 Sep 2021, at 19:33, Julien Grall <julien@xen.org> wrote:
> 
> Hi Stefano,
> 
> On 01/09/2021 18:54, Stefano Stabellini wrote:
>> On Wed, 1 Sep 2021, Julien Grall wrote:
>>> On 01/09/2021 14:10, Bertrand Marquis wrote:
>>>>> On 1 Sep 2021, at 13:55, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> On 01/09/2021 13:43, Michal Orzel wrote:
>>>>>> Introduce new Xen command line parameter called "pmu_access".
>>>>>> The default value is "trap": Xen traps PMU accesses.
>>>>>> In case of setting pmu_access to "native", Xen does not trap
>>>>>> PMU accesses allowing all the guests to access PMU registers.
>>>>>> However, guests cannot make use of PMU overflow interrupts as
>>>>>> PMU uses PPI which Xen cannot route to guests.
>>>>>> This option is only intended for development and testing purposes.
>>>>>> Do not use this in production system.
>>>>> I am afraid your option is not safe even in development system as a vCPU
>>>>> may move between pCPUs.
>>>>> 
>>>>> However, even if we restricted the use to pinned vCPU *and* dedicated
>>>>> pCPU, I am not convinced that exposing an half backed PMU (the overflow
>>>>> interrupt would not work) to the guest is the right solution. This likely
>>>>> means the guest OS would need to be modified and therefore the usage of
>>>>> this option is fairly limited.
>>>>> 
>>>>> So I think the first steps are:
>>>>>   1) Make the PPI work. There was some attempt in the past for it on
>>>>> xen-devel. You could have a look.
>>>>>   2) Provide PMU bindings
>>>>> 
>>>>> With that in place, we can discuss how to expose the PMU even if it is
>>>>> unsafe in some conditions.
>>>> 
>>>> With those limitations, using the PMU to monitor the system performances or
>>>> on some specific use cases is still really useful.
>>>> We are using that to do some benchmarks of Xen or of some applications to
>>>> compare the behaviour to a native system or
>>>> analyse the performances of Xen itself (hypercalls,context switch …etc)
>> I also already had to write a patch almost exactly like this one to
>> provide to customers a few months back.
>>> I understand this is useful for some setup and I am not trying to say we
>>> should not have a way to expose the PMU (even unsafely) in upstream. However,
>>> I think the option as it stands is too wide (this should be a per domain knob)
>>> and we should properly expose the PMU (interrupts, bindings...).
>> I have never used PMU directly myself, only provided a patch similar to
>> this one.  But as far as I could tell the users were fully satisfied
>> with it and it had no interrupts support either. Could it be that
>> interrupts are not actually needed to read the perf counters, which is
>> probably what users care about?
> 
> You don't need the interrupts to read the perf counters. But AFAIU, you would have to poll at a regular interval yourself. There is also the question on how to catch the overflow?
> 
>> In regards to "this should be a per domain knob", in reality if you are
>> doing PMU experiments you don't care if only one or all domains have
>> access: you are working in a controlled environment trying to figure out
>> if your setup meets the timing requirements. There are no security or
>> safety concerns (those are different experiments) and there is no
>> interference problems in the sense of multiple domains trying to access
>> PMU at the same time -- you control the domains you decide which one is
>> accessing them. >
>> So I am in favor of a patch like this one because it actually satisfy
>> our requirements. Even if we had per-domain support and interrupts
>> support, I don't think they would end up being used.
> 
> I have to disagree with that. There are valid use-cases where you may want to expose the PMU for using perf to a single domain because you know it is safe to do so. I appreciate this is may not be the use case for your users (yet), but I have seen (and used) it on x86. So to me this approach is short-sighed.
> 
> TBH, this is not the first time I have seen patch for "let's expose the same feature to everyone because this is easy to do" and really dislike it. Exposing a new feature from the toolstack is easier than you think (baring the lack of reviews), this is a matter of creating a new flag a new option. This would make Xen a lot more flexible and enable more users...

If I understand it right, you want a per guest parameter to be able to allow PMU accesses.
This would require:
- to save/restore MDCR on context switch
- introduce a new config parameter for guests (might or might not need a tool change)
- have a xen command line parameter to have a solution to Allow PMU for dom0 (or maybe a DTB one)

But this would NOT include:
- interrupt support (only needed to get informed of overflow)
- provide PMU virtualization (not even sure something like that could make much sense)

I am not saying that we will do that now but as I need to unblock this I could evaluate the effort and see if it could be possible to do this in the future.
In the meantime we will start maintaining an internal branch with patches refused upstream as this is blocking us.

Kind regards
Bertrand

> 
> So as it stands:
> 
> NAcked-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 
> -- 
> Julien Grall

On 02/09/2021 08:57, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> If I understand it right, you want a per guest parameter to be able to allow PMU accesses.
> This would require:
> - to save/restore MDCR on context switch
> - introduce a new config parameter for guests (might or might not need a tool change)
> - have a xen command line parameter to have a solution to Allow PMU for dom0 (or maybe a DTB one)
Yes.

> 
> But this would NOT include:
> - interrupt support (only needed to get informed of overflow)
> - provide PMU virtualization (not even sure something like that could make much sense)

I am guessing the following is also included here:

- provide a PMU node in the DTB for the domain.

Without those 3, I feel we are exposing an half backed PMU to the guest. 
But this would still be a good first step, so I would be OK if they are 
not implemented in the first shot.

> 
> I am not saying that we will do that now but as I need to unblock this I could evaluate the effort and see if it could be possible to do this in the future.

Well... Below the patch I wrote during my breakfast this morning. This 
has not been tested and miss some documentation.

 From 690a92cffed82451dcbd8b966e8dee31c1dce5fc Mon Sep 17 00:00:00 2001
From: Julien Grall <jgrall@amazon.com>
Date: Thu, 2 Sep 2021 08:46:12 +0000
Subject: [PATCH] xen/arm: Expose the PMU to the guest

There are requests to expose the PMU (even in a hackish/non-secure way)
to the guests. This is taking the first steps by adding a per-domain
flag to disable the PMU traps.

Long term, we will want to at least expose the PMU interrupts, device-tree
binding. For more use cases, we will also need to save/restore the
PMU context.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
  tools/include/libxl.h            |  2 ++
  tools/libs/light/libxl_arm.c     |  3 +++
  tools/libs/light/libxl_types.idl |  2 ++
  tools/xl/xl_parse.c              |  3 +++
  xen/arch/arm/domain.c            | 10 ++++++++--
  xen/include/asm-arm/domain.h     |  1 +
  xen/include/public/domctl.h      |  4 ++++
  7 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d69869..d3e795a38670 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -502,6 +502,8 @@
   */
  #define LIBXL_HAVE_X86_MSR_RELAXED 1

+#define LIBXL_HAVE_ARM_VPMU 1
+
  /*
   * libxl ABI compatibility
   *
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6e0039..89865a90dd3e 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -29,6 +29,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
      uint32_t vuart_irq;
      bool vuart_enabled = false;

+    if (d_config->b_info.arch.vpmu)
+        config->flags |= XEN_DOMCTL_CDF_PMU;
+
      /*
       * If pl011 vuart is enabled then increment the nr_spis to allow 
allocation
       * of SPI VIRQ for pl011.
diff --git a/tools/libs/light/libxl_types.idl 
b/tools/libs/light/libxl_types.idl
index 3f9fff653a4a..daf768cba568 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -644,6 +644,8 @@ libxl_domain_build_info = Struct("domain_build_info",[

      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
                                 ("vuart", libxl_vuart_type),
+                              # XXX: Can this be common?
+                               ("vpmu", boolean)
                                ])),
      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
                                ])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 17dddb4cd534..6e497cc0b67e 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2729,6 +2729,9 @@ skip_usbdev:
          }
      }

+    /* XXX: This probably want to be common or #ifdef-ed */
+    xlu_cfg_get_defbool(config, "vpmu", &b_info->arch_arm.vpmu, 0);
+
      if (!xlu_cfg_get_string (config, "tee", &buf, 1)) {
          e = libxl_tee_type_from_string(buf, &b_info->tee);
          if (e) {
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 19c756ac3d46..a0e2321008ab 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -276,6 +276,9 @@ static void ctxt_switch_to(struct vcpu *n)
       * timer. The interrupt needs to be injected into the guest. */
      WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
      virt_timer_restore(n);
+
+    /* XXX: Check the position and synchronization requirement */
+    WRITE_SYSREG(n->arch.mdcr_el2, MDCR_EL2);
  }

  /* Update per-VCPU guest runstate shared memory area (if registered). */
@@ -585,6 +588,9 @@ int arch_vcpu_create(struct vcpu *v)
      v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);

      v->arch.hcr_el2 = get_default_hcr_flags();
+    v->arch.mdcr_el2 = HDCR_TDRA|HDCR_TDOSA|HDCR_TDA;
+    if ( !(v->domain->options & XEN_DOMCTL_CDF_PMU) )
+        v->arch.mdcr_el2 |= HDCR_TPM|HDCR_TPMCR;

      if ( (rc = vcpu_vgic_init(v)) != 0 )
          goto fail;
@@ -622,8 +628,8 @@ int arch_sanitise_domain_config(struct 
xen_domctl_createdomain *config)
  {
      unsigned int max_vcpus;

-    /* HVM and HAP must be set. IOMMU may or may not be */
-    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) !=
+    /* HVM and HAP must be set. IOMMU and PMU may or may not be */
+    if ( (config->flags & ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_pmu)) !=
           (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
      {
          dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c9277b5c6d94..14e575288f77 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -166,6 +166,7 @@ struct arch_vcpu

      /* HYP configuration */
      register_t hcr_el2;
+    register_t mdcr_el2;

      uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
  #ifdef CONFIG_ARM_32
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 96696e3842da..db9539ddd579 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -71,6 +71,10 @@ struct xen_domctl_createdomain {
  #define _XEN_DOMCTL_CDF_nested_virt   6
  #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)

+/* Should we expose the vPMU to the guest? */
+#define _XEN_DOMCTL_CDF_pmu           6
+#define XEN_DOMCTL_CDF_pmu            (1U<<_XEN_DOMCTL_CDF_pmu)
+
  /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
  #define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt

> In the meantime we will start maintaining an internal branch with patches refused upstream as this is blocking us.

For the future, please consider a per-domain option from the beginning. 
This is not much extra effort (see the patch above) and would make the 
acceptance of a patch more likely.

Cheers,

-- 
Julien Grall

Hi,

> On 2 Sep 2021, at 09:59, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 02/09/2021 08:57, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>> If I understand it right, you want a per guest parameter to be able to allow PMU accesses.
>> This would require:
>> - to save/restore MDCR on context switch
>> - introduce a new config parameter for guests (might or might not need a tool change)
>> - have a xen command line parameter to have a solution to Allow PMU for dom0 (or maybe a DTB one)
> Yes.
> 
>> But this would NOT include:
>> - interrupt support (only needed to get informed of overflow)
>> - provide PMU virtualization (not even sure something like that could make much sense)
> 
> I am guessing the following is also included here:
> 
> - provide a PMU node in the DTB for the domain.
> 
> Without those 3, I feel we are exposing an half backed PMU to the guest. But this would still be a good first step, so I would be OK if they are not implemented in the first shot.
> 
>> I am not saying that we will do that now but as I need to unblock this I could evaluate the effort and see if it could be possible to do this in the future.
> 
> Well... Below the patch I wrote during my breakfast this morning. This has not been tested and miss some documentation.

Impressive but be careful not to put jam on your keyboard :-)

We are clearly not at your level of expertise and this would have taken us a lot more time, even if we tried without eating in parallel.

> 
> From 690a92cffed82451dcbd8b966e8dee31c1dce5fc Mon Sep 17 00:00:00 2001
> From: Julien Grall <jgrall@amazon.com>
> Date: Thu, 2 Sep 2021 08:46:12 +0000
> Subject: [PATCH] xen/arm: Expose the PMU to the guest
> 
> There are requests to expose the PMU (even in a hackish/non-secure way)
> to the guests. This is taking the first steps by adding a per-domain
> flag to disable the PMU traps.
> 
> Long term, we will want to at least expose the PMU interrupts, device-tree
> binding. For more use cases, we will also need to save/restore the
> PMU context.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> tools/include/libxl.h            |  2 ++
> tools/libs/light/libxl_arm.c     |  3 +++
> tools/libs/light/libxl_types.idl |  2 ++
> tools/xl/xl_parse.c              |  3 +++
> xen/arch/arm/domain.c            | 10 ++++++++--
> xen/include/asm-arm/domain.h     |  1 +
> xen/include/public/domctl.h      |  4 ++++
> 7 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index b9ba16d69869..d3e795a38670 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -502,6 +502,8 @@
>  */
> #define LIBXL_HAVE_X86_MSR_RELAXED 1
> 
> +#define LIBXL_HAVE_ARM_VPMU 1
> +
> /*
>  * libxl ABI compatibility
>  *
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6e0039..89865a90dd3e 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -29,6 +29,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>     uint32_t vuart_irq;
>     bool vuart_enabled = false;
> 
> +    if (d_config->b_info.arch.vpmu)
> +        config->flags |= XEN_DOMCTL_CDF_PMU;
> +
>     /*
>      * If pl011 vuart is enabled then increment the nr_spis to allow allocation
>      * of SPI VIRQ for pl011.
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 3f9fff653a4a..daf768cba568 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -644,6 +644,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
> 
>     ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>                                ("vuart", libxl_vuart_type),
> +                              # XXX: Can this be common?
> +                               ("vpmu", boolean)
>                               ])),
>     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>                               ])),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 17dddb4cd534..6e497cc0b67e 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2729,6 +2729,9 @@ skip_usbdev:
>         }
>     }
> 
> +    /* XXX: This probably want to be common or #ifdef-ed */
> +    xlu_cfg_get_defbool(config, "vpmu", &b_info->arch_arm.vpmu, 0);
> +
>     if (!xlu_cfg_get_string (config, "tee", &buf, 1)) {
>         e = libxl_tee_type_from_string(buf, &b_info->tee);
>         if (e) {
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 19c756ac3d46..a0e2321008ab 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -276,6 +276,9 @@ static void ctxt_switch_to(struct vcpu *n)
>      * timer. The interrupt needs to be injected into the guest. */
>     WRITE_SYSREG(n->arch.cntkctl, CNTKCTL_EL1);
>     virt_timer_restore(n);
> +
> +    /* XXX: Check the position and synchronization requirement */
> +    WRITE_SYSREG(n->arch.mdcr_el2, MDCR_EL2);
> }
> 
> /* Update per-VCPU guest runstate shared memory area (if registered). */
> @@ -585,6 +588,9 @@ int arch_vcpu_create(struct vcpu *v)
>     v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);
> 
>     v->arch.hcr_el2 = get_default_hcr_flags();
> +    v->arch.mdcr_el2 = HDCR_TDRA|HDCR_TDOSA|HDCR_TDA;
> +    if ( !(v->domain->options & XEN_DOMCTL_CDF_PMU) )
> +        v->arch.mdcr_el2 |= HDCR_TPM|HDCR_TPMCR;
> 
>     if ( (rc = vcpu_vgic_init(v)) != 0 )
>         goto fail;
> @@ -622,8 +628,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> {
>     unsigned int max_vcpus;
> 
> -    /* HVM and HAP must be set. IOMMU may or may not be */
> -    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) !=
> +    /* HVM and HAP must be set. IOMMU and PMU may or may not be */
> +    if ( (config->flags & ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_pmu)) !=
>          (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
>     {
>         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index c9277b5c6d94..14e575288f77 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -166,6 +166,7 @@ struct arch_vcpu
> 
>     /* HYP configuration */
>     register_t hcr_el2;
> +    register_t mdcr_el2;
> 
>     uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
> #ifdef CONFIG_ARM_32
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 96696e3842da..db9539ddd579 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -71,6 +71,10 @@ struct xen_domctl_createdomain {
> #define _XEN_DOMCTL_CDF_nested_virt   6
> #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
> 
> +/* Should we expose the vPMU to the guest? */
> +#define _XEN_DOMCTL_CDF_pmu           6
> +#define XEN_DOMCTL_CDF_pmu            (1U<<_XEN_DOMCTL_CDF_pmu)
> +
> /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
> #define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt
> 
>> In the meantime we will start maintaining an internal branch with patches refused upstream as this is blocking us.
> 
> For the future, please consider a per-domain option from the beginning. This is not much extra effort (see the patch above) and would make the acceptance of a patch more likely.

We wanted to share something we did internally which we thought could be useful for others.
We will be more careful in the future.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Hi Stefano,

On 01.09.2021 19:54, Stefano Stabellini wrote:
> On Wed, 1 Sep 2021, Julien Grall wrote:
>> On 01/09/2021 14:10, Bertrand Marquis wrote:
>>>> On 1 Sep 2021, at 13:55, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 01/09/2021 13:43, Michal Orzel wrote:
>>>>> Introduce new Xen command line parameter called "pmu_access".
>>>>> The default value is "trap": Xen traps PMU accesses.
>>>>> In case of setting pmu_access to "native", Xen does not trap
>>>>> PMU accesses allowing all the guests to access PMU registers.
>>>>> However, guests cannot make use of PMU overflow interrupts as
>>>>> PMU uses PPI which Xen cannot route to guests.
>>>>> This option is only intended for development and testing purposes.
>>>>> Do not use this in production system.
>>>> I am afraid your option is not safe even in development system as a vCPU
>>>> may move between pCPUs.
>>>>
>>>> However, even if we restricted the use to pinned vCPU *and* dedicated
>>>> pCPU, I am not convinced that exposing an half backed PMU (the overflow
>>>> interrupt would not work) to the guest is the right solution. This likely
>>>> means the guest OS would need to be modified and therefore the usage of
>>>> this option is fairly limited.
>>>>
>>>> So I think the first steps are:
>>>>   1) Make the PPI work. There was some attempt in the past for it on
>>>> xen-devel. You could have a look.
>>>>   2) Provide PMU bindings
>>>>
>>>> With that in place, we can discuss how to expose the PMU even if it is
>>>> unsafe in some conditions.
>>>
>>> With those limitations, using the PMU to monitor the system performances or
>>> on some specific use cases is still really useful.
>>> We are using that to do some benchmarks of Xen or of some applications to
>>> compare the behaviour to a native system or
>>> analyse the performances of Xen itself (hypercalls,context switch …etc)
> 
> I also already had to write a patch almost exactly like this one to
> provide to customers a few months back.
> 
> 
>> I understand this is useful for some setup and I am not trying to say we
>> should not have a way to expose the PMU (even unsafely) in upstream. However,
>> I think the option as it stands is too wide (this should be a per domain knob)
>> and we should properly expose the PMU (interrupts, bindings...).
> 
> I have never used PMU directly myself, only provided a patch similar to
> this one.  But as far as I could tell the users were fully satisfied
> with it and it had no interrupts support either. Could it be that
> interrupts are not actually needed to read the perf counters, which is
> probably what users care about?
> 
Most of the people using PMU do not need interrupt as PMU can only be programmed
to generate overflow interrupt request when a counter overflows.
If we want to catch counter overflow without using interrupt, we can check
if overflow status bit is set to 1 in PMOVSCLR register.

> In regards to "this should be a per domain knob", in reality if you are
> doing PMU experiments you don't care if only one or all domains have
> access: you are working in a controlled environment trying to figure out
> if your setup meets the timing requirements. There are no security or
> safety concerns (those are different experiments) and there is no
> interference problems in the sense of multiple domains trying to access
> PMU at the same time -- you control the domains you decide which one is
> accessing them.
> 
> So I am in favor of a patch like this one because it actually satisfy
> our requirements. Even if we had per-domain support and interrupts
> support, I don't think they would end up being used.
>