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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 12 Oct 2021, Julien Grall wrote:
> 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.

I made the change on commit