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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

> On 16 Sep 2021, at 22:20, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> 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.

Sorry for that and thanks.

Bertrand

> 
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>