Re: [PATCH] xen/arm: Introduce pmu_access parameter

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 1 Sep 2021, at 13:55, Julien Grall <julien@xxxxxxx> 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@xxxxxxx> > ---
>>  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