Re: [PATCH v6] xen: Expose the PMU to the guests

[Michal Orzel <michal.orzel@xxxxxxx>]

On 13.10.2021 10:29, Julien Grall wrote:
> Hi Michal,
> 
> On 13/10/2021 08:45, Michal Orzel wrote:
>> Add parameter vpmu to xl domain configuration syntax
>> to enable the access to PMU registers by disabling
>> the PMU traps(currently only for ARM).
>>
>> The current status is that the PMU registers are not
>> virtualized and the physical registers are directly
>> accessible when this parameter is enabled. There is no
>> interrupt support and Xen will not save/restore the
>> register values on context switches.
>>
>> According to Arm Arm, section D7.1:
>> "The Performance Monitors Extension is common
>> to AArch64 operation and AArch32 operation."
>> That means we have an ensurance that if PMU is
>> present in one exception state, it must also be
>> present in the other.
>>
>> Please note that this feature is experimental.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> Signed-off-by: Julien Grall <julien@xxxxxxx>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> Acked-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> ---
>> Changes since v5:
>> -extracted from the series as other patches have been merged
>> -fold ARM vPMU feature status in SUPPORT.md into existing section for x86
>> -fix coding style in libxl
>> Changes since v4:
>> -update SUPPORT.md
>> -move vpmu_is_available assignment after the secondary
>> CPUs have been brought up
>> -introduce masks for flags that need to be set and
>> for flags that are optional
>> Changes since v3:
>> -fail if vpmu is set but not supported
>> Changes since v2:
>> -remove redundant check from x86 code
>> -do not define bit position and mask separately
>> Changes since v1:
>> -modify vpmu parameter to be common rather than arch specific
>> ---
>>   SUPPORT.md                           | 12 ++++++++++--
>>   docs/man/xl.cfg.5.pod.in             | 17 +++++++++++++++++
>>   tools/golang/xenlight/helpers.gen.go |  6 ++++++
>>   tools/golang/xenlight/types.gen.go   |  1 +
>>   tools/include/libxl.h                |  6 ++++++
>>   tools/libs/light/libxl_create.c      | 10 ++++++++++
>>   tools/libs/light/libxl_types.idl     |  2 ++
>>   tools/ocaml/libs/xc/xenctrl.ml       |  1 +
>>   tools/ocaml/libs/xc/xenctrl.mli      |  1 +
>>   tools/xl/xl_parse.c                  |  2 ++
>>   xen/arch/arm/domain.c                | 13 ++++++++++---
>>   xen/arch/arm/setup.c                 |  3 +++
>>   xen/common/domain.c                  | 10 +++++++++-
>>   xen/include/asm-arm/domain.h         |  1 +
>>   xen/include/public/domctl.h          |  4 +++-
>>   15 files changed, 82 insertions(+), 7 deletions(-)
>>
>> diff --git a/SUPPORT.md b/SUPPORT.md
>> index 317392d8f3..d0305abc0b 100644
>> --- a/SUPPORT.md
>> +++ b/SUPPORT.md
>> @@ -671,11 +671,19 @@ such as KVM, Hyper-V, Bromium, and so on as guests.
>>     ### vPMU
>>   -Virtual Performance Management Unit for HVM guests
>> +Virtual Performance Management Unit
> While I agree that "HVM guests" doesn't make any sense on Arm, you are not 
> adding back for x86. So this is changing the statement support to both HVM 
> and PV guest.
> 
> I don't know why only HVM guests was previously mentionned. Jan, Andrew? 
> Anyway, for this patch, I think we should add...
> 
>>         Status, x86: Supported, Not security supported
> 
> ... add HVM here. Looking at other section, we seem to use:
> 
> Status, x86 HVM: ...
> 
> One can send a follow-up patch if we want to mention the support status for 
> PV guests.
> 
> Cheers,
> 
Ok so it is the matter of adding "HVM" word into status for x86.
Is this something that can be done while committing?

Cheers,
Michal