Re: [RFC PATCH 3/4] xen/arm: Sanitize cpuinfo ID registers fields

[Julien Grall <julien@xxxxxxx>]

On 12/07/2021 12:03, Bertrand Marquis wrote:
> Hi Julien,
Hi Bertrand,

> > On 12 Jul 2021, at 11:16, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On 29/06/2021 18:08, Bertrand Marquis wrote:
> > > Define a sanitize_cpu function to be called on secondary cores to
> > > sanitize the cpuinfo structure from the boot CPU.
> > > The safest value is taken when possible and the system is marked tainted
> > > if we encounter values which are incompatible with each other.
> > > Call the sanitize_cpu function on all secondary cores and remove the
> > > code disabling secondary cores if they are not the same as the boot core
> > > as we are now able to handle this use case.
> > > This is only supported on arm64 so cpu_sanitize is an empty static
> > > inline on arm32.
> > > This patch also removes the code imported from Linux that will not be
> > > used by Xen.
> > > Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> > > ---
> > >   xen/arch/arm/arm64/cpusanitize.c | 236 ++++++++++++++++++++++++-------
> > >   xen/arch/arm/smpboot.c           |   5 +-
> > >   xen/include/asm-arm/cpufeature.h |   9 ++
> > >   xen/include/xen/lib.h            |   1 +
> > >   4 files changed, 199 insertions(+), 52 deletions(-)
> > > diff --git a/xen/arch/arm/arm64/cpusanitize.c b/xen/arch/arm/arm64/cpusanitize.c
> > > index 4cc8378c14..744006ca7c 100644
> > > --- a/xen/arch/arm/arm64/cpusanitize.c
> > > +++ b/xen/arch/arm/arm64/cpusanitize.c
> > > @@ -97,10 +97,6 @@ struct arm64_ftr_bits {
> > >                 .width = 0,                             \
> > >         }
> > >   -static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap);
> > > -
> > > -static bool __system_matches_cap(unsigned int n);
> > > -
> > >   /*
> > >    * NOTE: Any changes to the visibility of features should be kept in
> > >    * sync with the documentation of the CPU feature register ABI.
> > > @@ -277,31 +273,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
> > >         ARM64_FTR_END,
> > >   };
> > >   -static const struct arm64_ftr_bits ftr_ctr[] = {
> > > -       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */
> > > -       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 
> > > 1, 1),
> > > -       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IDC_SHIFT, 
> > > 1, 1),
> > > -       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, 
> > > CTR_CWG_SHIFT, 4, 0),
> > > -       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, 
> > > CTR_ERG_SHIFT, 4, 0),
> > > -       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, 
> > > CTR_DMINLINE_SHIFT, 4, 1),
> > > -       /*
> > > -        * Linux can handle differing I-cache policies. Userspace JITs will
> > > -        * make use of *minLine.
> > > -        * If we have differing I-cache policies, report it as the weakest - 
> > > VIPT.
> > > -        */
> > > -       ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_EXACT, CTR_L1IP_SHIFT, 
> > > 2, ICACHE_POLICY_VIPT),   /* L1Ip */
> > > -       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, 
> > > CTR_IMINLINE_SHIFT, 4, 0),
> > > -       ARM64_FTR_END,
> > > -};
> > I don't think this is should be dropped. Xen will need to know the safest 
> > cacheline size that can be used for cache maintenance instructions.
> I will surround those entries by #if 0 instead
But, why can't this just be sanitized as the other registers? If this is 
just a matter of missing structure in Xen, then we should add one.
The same goes for the other 2 below.

Cheers,

--
Julien Grall