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

[Julien Grall <julien@xxxxxxx>]

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?

> 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)?
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