Re: [PATCH v5 2/3] xen/arm: Check for PMU platform support

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 12/10/2021 09:13, Michal Orzel wrote:
> ID_AA64DFR0_EL1/ID_DFR0_EL1 registers provide
> information about PMU support. Replace structure
> dbg64/dbg32 with a union and fill in all the
> register fields according to document:
> ARM Architecture Registers(DDI 0595, 2021-06).
>
> Add macros boot_dbg_feature64/boot_dbg_feature32
> to check for a debug feature. Add macro
> cpu_has_pmu to check for PMU support.
> Any value higher than 0 and less than 15 means
> that PMU is supported (we do not care about its
> version for now).
>
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> Changes since v4:
> -improve checks for PMU
> Changes since v3:
> -none
> Changes since v2:
> -none
> Changes since v1:
> -new in v2
> ---
>   xen/include/asm-arm/cpufeature.h | 51 ++++++++++++++++++++++++++++++--
>   1 file changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index 5ca09b0bff..0ddf38858a 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -4,6 +4,7 @@
>   #ifdef CONFIG_ARM_64
>   #define cpu_feature64(c, feat)         ((c)->pfr64.feat)
>   #define boot_cpu_feature64(feat)       (system_cpuinfo.pfr64.feat)
> +#define boot_dbg_feature64(feat)       (system_cpuinfo.dbg64.feat)
>   
>   #define cpu_feature64_has_el0_32(c)    (cpu_feature64(c, el0) == 2)
>   
> @@ -22,6 +23,7 @@
>   
>   #define cpu_feature32(c, feat)         ((c)->pfr32.feat)
>   #define boot_cpu_feature32(feat)       (system_cpuinfo.pfr32.feat)
> +#define boot_dbg_feature32(feat)       (system_cpuinfo.dbg32.feat)
>   
>   #define cpu_has_arm       (boot_cpu_feature32(arm) == 1)
>   #define cpu_has_thumb     (boot_cpu_feature32(thumb) >= 1)
> @@ -32,8 +34,12 @@
>   
>   #ifdef CONFIG_ARM_32
>   #define cpu_has_gentimer  (boot_cpu_feature32(gentimer) == 1)
> +#define cpu_has_pmu       ((boot_dbg_feature32(perfmon) >= 1) && \
> +                           (boot_dbg_feature32(perfmon) < 15))
So I am happy with this check for arm32. But I would still like to have 
a comment explaining the fact that on Armv7 perfmon == 0 may mean PMUv1 
is may be used. Something like:
"On Armv7, the value 0 is used to indicate that PMUv2 is not supported. 
IOW this doesn't tell us whether the PMU is not supported (a processor 
may implement PMUv1).
For convenience, we treat 0 as not supported which this match the 
meaning on Armv8".
The rest of the code looks fine to me.

Cheers,

--
Julien Grall