Re: [PATCH] xen/arm: Add workaround for Cortex-A53 erratum #845719

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 8 Dec 2020, at 09:47, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 08/12/2020 07:23, Michal Orzel wrote:
>> When executing in aarch32 state at EL0, a load at EL0 from a
>> virtual address that matches the bottom 32 bits of the virtual address
>> used by a recent load at (aarch64) EL1 might return incorrect data.
>> The workaround is to insert a write of the contextidr_el1 register
>> on exception return to an aarch32 guest.
> 
> I am a bit confused with this comment. In the previous paragraph, you are 
> suggesting that the problem is an interaction between EL1 AArch64 and EL0 
> AArch32. But here you seem to imply the issue only happen when running a 
> AArch32 guest.
> 
> Can you clarify it?

This can happen when switching from an aarch64 guest to an aarch32 guest so not 
only when there is interaction.

> 
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>>  docs/misc/arm/silicon-errata.txt |  1 +
>>  xen/arch/arm/Kconfig             | 19 +++++++++++++++++++
>>  xen/arch/arm/arm64/entry.S       |  9 +++++++++
>>  xen/arch/arm/cpuerrata.c         |  8 ++++++++
>>  xen/include/asm-arm/cpufeature.h |  3 ++-
>>  5 files changed, 39 insertions(+), 1 deletion(-)
>> diff --git a/docs/misc/arm/silicon-errata.txt 
>> b/docs/misc/arm/silicon-errata.txt
>> index 27bf957ebf..fa3d9af63d 100644
>> --- a/docs/misc/arm/silicon-errata.txt
>> +++ b/docs/misc/arm/silicon-errata.txt
>> @@ -45,6 +45,7 @@ stable hypervisors.
>>  | ARM            | Cortex-A53      | #827319         | ARM64_ERRATUM_827319 
>>    |
>>  | ARM            | Cortex-A53      | #824069         | ARM64_ERRATUM_824069 
>>    |
>>  | ARM            | Cortex-A53      | #819472         | ARM64_ERRATUM_819472 
>>    |
>> +| ARM            | Cortex-A53      | #845719         | ARM64_ERRATUM_845719 
>>    |
>>  | ARM            | Cortex-A55      | #1530923        | N/A                  
>>    |
>>  | ARM            | Cortex-A57      | #852523         | N/A                  
>>    |
>>  | ARM            | Cortex-A57      | #832075         | ARM64_ERRATUM_832075 
>>    |
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index f5b1bcda03..6bea393555 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -244,6 +244,25 @@ config ARM_ERRATUM_858921
>>        If unsure, say Y.
>>  +config ARM64_ERRATUM_845719
>> +    bool "Cortex-A53: 845719: A load might read incorrect data"
>> +    default y
>> +    help
>> +      This option adds an alternative code sequence to work around ARM
>> +      erratum 845719 on Cortex-A53 parts up to r0p4.
>> +
>> +      When executing in aarch32 state at EL0, a load at EL0 from a
>> +      virtual address that matches the bottom 32 bits of the virtual address
>> +      used by a recent load at (aarch64) EL1 might return incorrect data.
>> +
>> +      The workaround is to insert a write of the contextidr_el1 register
>> +      on exception return to an aarch32 guest.
>> +      Please note that this does not necessarily enable the workaround,
>> +      as it depends on the alternative framework, which will only patch
>> +      the kernel if an affected CPU is detected.
>> +
>> +      If unsure, say Y.
>> +
>>  config ARM64_WORKAROUND_REPEAT_TLBI
>>      bool
>>  diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>> index 175ea2981e..ef3336f34a 100644
>> --- a/xen/arch/arm/arm64/entry.S
>> +++ b/xen/arch/arm/arm64/entry.S
>> @@ -96,6 +96,15 @@
>>          msr     SPSR_fiq, x22
>>          msr     SPSR_irq, x23
>>  +#ifdef CONFIG_ARM64_ERRATUM_845719
>> +alternative_if ARM64_WORKAROUND_845719
>> +        /* contextidr_el1 is not accessible from aarch32 guest so we can
>> +         * write xzr to it
>> +         */
> 
> This path is also used when the trapping occurs when running in EL0 aarch32. 
> So wouldn't you clobber it if the EL1 AArch64 use it (Linux may store the PID 
> in it)?

Right we missed that.
In this case i would suggest to do something reading and then writting back in 
contextidr instead so that we do not clobber it.

Regards
Bertrand

> 
> Also the coding style for multi-line comment in Xen is:
> 
> /*
> * Foo
> * Bar
> */
> 
>> +        msr     contextidr_el1, xzr
>> +alternative_else_nop_endif
>> +#endif
> 
> Cheers,
> 
> -- 
> Julien Grall