Re: [PATCH v4 7/7] xen/arm: Sanitize CTR_EL0

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 16 Sep 2021, Bertrand Marquis wrote:
> Sanitize CTR_EL0 value between cores and taint Xen if incompatible
> values are found.
> 
> In the case of different i-cache types, the sanitize ctr_el0 will have a
> sanitize value but this is currently not used or exposed to guest which
> are seeing the original ctr_el0 value.
> 
> Use the opportunity to rename CTR_L1Ip to use an upper case name like
> Linux does.
> The patch is also defining ICACHE_POLICY_xxx instead of only having
> CTR_L1IP_xxx to sync the definitions with Linux and is updating the code
> using those accordingly (arm32 setup).
> 
> On platforms with only the same type of cores, this patch should not
> modify the current Xen behaviour.
> 
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> ---
> Changes in v4: Remove TID2 support and handling of corresponding
> register emulation in vcpreg/vsysreg.
> Changes in v3: none
> Change in v2: Patch introduced in v2
> ---
>  xen/arch/arm/arm64/cpufeature.c  |  6 ++----
>  xen/arch/arm/cpufeature.c        |  2 ++
>  xen/arch/arm/setup.c             |  2 +-
>  xen/include/asm-arm/cpufeature.h |  9 +++++++++
>  xen/include/asm-arm/processor.h  | 18 +++++++++++++++---
>  5 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
> index d4679f5df3..6e5d30dc7b 100644
> --- a/xen/arch/arm/arm64/cpufeature.c
> +++ b/xen/arch/arm/arm64/cpufeature.c
> @@ -275,9 +275,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
>       ARM64_FTR_END,
>  };
>  
> -#if 0
> -/* TODO: use this to sanitize the cache line size among cores */
> -
>  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),
> @@ -294,7 +291,6 @@ static const struct arm64_ftr_bits ftr_ctr[] = {
>       ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, 
> CTR_IMINLINE_SHIFT, 4, 0),
>       ARM64_FTR_END,
>  };
> -#endif
>  
>  static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>       S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 
> ID_MMFR0_INNERSHR_SHIFT, 4, 0xf),
> @@ -606,6 +602,8 @@ void update_system_features(const struct cpuinfo_arm *new)
>        */
>       SANITIZE_REG(dczid, 0, dczid);
>  
> +     SANITIZE_REG(ctr, 0, ctr);
> +
>       if ( cpu_feature64_has_el0_32(&system_cpuinfo) )
>       {
>               SANITIZE_ID_REG(pfr32, 0, pfr0);
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index 113f20f601..6e51f530a8 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -127,6 +127,8 @@ void identify_cpu(struct cpuinfo_arm *c)
>  
>      c->dczid.bits[0] = READ_SYSREG(DCZID_EL0);
>  
> +    c->ctr.bits[0] = READ_SYSREG(CTR_EL0);
> +
>      aarch32_el0 = cpu_feature64_has_el0_32(c);
>  #endif
>  
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 4ab13d0fbe..49dc90d198 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -650,7 +650,7 @@ static void __init setup_mm(void)
>          panic("No memory bank\n");
>  
>      /* We only supports instruction caches implementing the IVIPT extension. 
> */
> -    if ( ((ctr >> CTR_L1Ip_SHIFT) & CTR_L1Ip_MASK) == CTR_L1Ip_AIVIVT )
> +    if ( ((ctr >> CTR_L1IP_SHIFT) & CTR_L1IP_MASK) == ICACHE_POLICY_AIVIVT )
>          panic("AIVIVT instruction cache not supported\n");
>  
>      init_pdx();
> diff --git a/xen/include/asm-arm/cpufeature.h 
> b/xen/include/asm-arm/cpufeature.h
> index 5219fd3bab..cab89ee142 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -267,6 +267,14 @@ struct cpuinfo_arm {
>          register_t bits[1];
>      } dczid;
>  
> +    /*
> +     * CTR is only used to check for different cache types or policies and
> +     * taint Xen in this case
> +     */
> +    struct {
> +        register_t bits[1];
> +    } ctr;
> +
>  #endif
>  
>      /*
> @@ -339,6 +347,7 @@ extern struct cpuinfo_arm system_cpuinfo;
>  extern void identify_cpu(struct cpuinfo_arm *);
>  
>  #ifdef CONFIG_ARM_64
> +
>  extern void update_system_features(const struct cpuinfo_arm *);
>  #else
>  static inline void update_system_features(const struct cpuinfo_arm *cpuinfo)

Spurious change. I fixed it on commit.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>