Re: [PATCH] xen/arm: Expose the PMU to the guests

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Andrew,

On 30.09.2021 12:40, Andrew Cooper wrote:
> On 30/09/2021 10:26, Michal Orzel wrote:
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index 4b1e3028d2..4a75203b9f 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -2843,6 +2843,18 @@ Currently, only the "sbsa_uart" model is supported 
>> for ARM.
>>  
>>  =back
>>  
>> +=item B<vpmu=BOOLEAN>
>> +
>> +Specifies whether to enable the access to PMU registers by disabling
>> +the PMU traps.
>> +
>> +This change does not expose the full PMU to the guest.
>> +Currently there is no support for virtualization, interrupts, etc.
>> +
>> +Enabling this option may result in potential security holes.
>> +
>> +If this option is not specified then it will default to B<false>.
> 
> There are rather better ways of phrasing this...
> 
> It isn't "maybe security holes".  There are two salient points.
> 
> 1) vPMU, by design and purpose, exposes system level performance
> information to the guest.  Only to be used by sufficiently privileged
> domains (however the system admin cares to draw this particular line).
> 
> 2) Feature is experimental, and thus might explode on you.  Bugfixes
> welcome.
> 
Ok I will provide more description.
>> +
>>  =head3 x86
>>  
>>  =over 4
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index b9ba16d698..c6694e977d 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -502,6 +502,13 @@
>>   */
>>  #define LIBXL_HAVE_X86_MSR_RELAXED 1
>>  
>> +/*
>> + * LIBXL_HAVE_ARM_VPMU indicates the toolstack has support for enabling
>> + * the access to PMU registers by disabling the PMU traps. This is done
>> + * by setting the libxl_domain_build_info arch_arm.vpmu field.
>> + */
>> +#define LIBXL_HAVE_ARM_VPMU 1
> 
> Please make this generic, not ARM-specific.
> 
Ok, I will.
> The domctl flag is (correctly) common, and x86 could do with this too,
> as well as other architectures.
> 
> Don't worry about plumbing the x86 side to work - that's a little more
> involved, and can be done at a later date.
> 
> 
> However, you do need Xen to report the availability of vPMU on the
> hardware as a prerequisite.  The toolstack needs to be able to know
> whether XEN_DOMCTL_CDF_pmu will be accepted so it can error out in a
> useful way on hardware lacking the capabilities.
> 
> You probably want to follow the example in
> a48066d25c652aeecafba5a3f33e77ad9a9c07f6
> 
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 96696e3842..a55ceb81db 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -70,9 +70,12 @@ struct xen_domctl_createdomain {
>>  #define XEN_DOMCTL_CDF_iommu          (1U<<_XEN_DOMCTL_CDF_iommu)
>>  #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           7
>> +#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
>> +#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_pmu
> 
> Without an adjustment in the Ocaml bindings, the ABI check will fail.
> 
You're right. I forgot about that. 
> ~Andrew
> 

Cheers,
Michal