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

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

[Michal Orzel]

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.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 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
+         */
+        msr     contextidr_el1, xzr
+alternative_else_nop_endif
+#endif
+
         add     x21, sp, #UREGS_SPSR_und
         ldp     w22, w23, [x21]
         msr     SPSR_und, x22
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index b398d480f1..8959d4d4dc 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -491,6 +491,14 @@ static const struct arm_cpu_capabilities arm_errata[] = {
         .capability = ARM_WORKAROUND_858921,
         MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),
     },
+#endif
+#ifdef CONFIG_ARM64_ERRATUM_845719
+    {
+        /* Cortex-A53 r0p[01234] */
+        .desc = "ARM erratum 845719",
+        .capability = ARM64_WORKAROUND_845719,
+        MIDR_RANGE(MIDR_CORTEX_A53, 0x00, 0x04),
+    },
 #endif
     {
         /* Neoverse r0p0 - r2p0 */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index c7b5052992..1165a1eb62 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -47,8 +47,9 @@
 #define ARM64_WORKAROUND_AT_SPECULATE 9
 #define ARM_WORKAROUND_858921 10
 #define ARM64_WORKAROUND_REPEAT_TLBI 11
+#define ARM64_WORKAROUND_845719 12
 
-#define ARM_NCAPS           12
+#define ARM_NCAPS           13
 
 #ifndef __ASSEMBLY__
 
-- 
2.28.0

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

[Bertrand Marquis]

Hi,

> On 8 Dec 2020, at 07:23, Michal Orzel <Michal.Orzel@arm.com> 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.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks
Bertrand

> ---
> 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
> +         */
> +        msr     contextidr_el1, xzr
> +alternative_else_nop_endif
> +#endif
> +
>         add     x21, sp, #UREGS_SPSR_und
>         ldp     w22, w23, [x21]
>         msr     SPSR_und, x22
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index b398d480f1..8959d4d4dc 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -491,6 +491,14 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>         .capability = ARM_WORKAROUND_858921,
>         MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),
>     },
> +#endif
> +#ifdef CONFIG_ARM64_ERRATUM_845719
> +    {
> +        /* Cortex-A53 r0p[01234] */
> +        .desc = "ARM erratum 845719",
> +        .capability = ARM64_WORKAROUND_845719,
> +        MIDR_RANGE(MIDR_CORTEX_A53, 0x00, 0x04),
> +    },
> #endif
>     {
>         /* Neoverse r0p0 - r2p0 */
> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
> index c7b5052992..1165a1eb62 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -47,8 +47,9 @@
> #define ARM64_WORKAROUND_AT_SPECULATE 9
> #define ARM_WORKAROUND_858921 10
> #define ARM64_WORKAROUND_REPEAT_TLBI 11
> +#define ARM64_WORKAROUND_845719 12
> 
> -#define ARM_NCAPS           12
> +#define ARM_NCAPS           13
> 
> #ifndef __ASSEMBLY__
> 
> -- 
> 2.28.0
> 

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

[Julien Grall]

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@arm.com>
> ---
>   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

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

[Bertrand Marquis]

Hi Julien,

> On 8 Dec 2020, at 09:47, Julien Grall <julien@xen.org> 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@arm.com>
>> ---
>>  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

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

[Julien Grall]

On 08/12/2020 14:38, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 8 Dec 2020, at 09:47, Julien Grall <julien@xen.org> 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.

Right, but the context switch will write to CONTEXTIDR_EL1. So this case 
should already be handled.

Xen will never switch from AArch64 EL1 to AArch32 EL0 without a context 
switch (the inverse can happen if we inject an exception to the guest).

Reading the Cortex-A53 SDEN, it sounds like this is an OS and not 
Hypervisor problem. In fact, Linux only seems to workaround it when 
switching in the OS side rather than the hypervisor.

Therefore, I am not sure to understand why we need to workaroud it in Xen.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Tue, 8 Dec 2020, Julien Grall wrote:
> On 08/12/2020 14:38, Bertrand Marquis wrote:
> > Hi Julien,
> > 
> > > On 8 Dec 2020, at 09:47, Julien Grall <julien@xen.org> 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.

Just to confirm: it cannot happen when switching from aarch64 *EL2* to
aarch32 EL0/1, right?  Because that happens all the time in Xen.


> Right, but the context switch will write to CONTEXTIDR_EL1. So this case
> should already be handled.
> 
> Xen will never switch from AArch64 EL1 to AArch32 EL0 without a context switch
> (the inverse can happen if we inject an exception to the guest).
> 
> Reading the Cortex-A53 SDEN, it sounds like this is an OS and not Hypervisor
> problem. In fact, Linux only seems to workaround it when switching in the OS
> side rather than the hypervisor.
> 
> Therefore, I am not sure to understand why we need to workaroud it in Xen.

It looks like Julien is right in regards to the "aarch64 EL1 to aarch32
EL0" issue.

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

[Michal Orzel]

On 09.12.2020 02:34, Stefano Stabellini wrote:
> On Tue, 8 Dec 2020, Julien Grall wrote:
>> On 08/12/2020 14:38, Bertrand Marquis wrote:
>>> Hi Julien,
>>>
>>>> On 8 Dec 2020, at 09:47, Julien Grall <julien@xen.org> 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.
> 
> Just to confirm: it cannot happen when switching from aarch64 *EL2* to
> aarch32 EL0/1, right?  Because that happens all the time in Xen.
> 
> 
No it cannot. It can only happen when switching from aarch64 EL1 to aarch32 EL0.
>> Right, but the context switch will write to CONTEXTIDR_EL1. So this case
>> should already be handled.
>>
>> Xen will never switch from AArch64 EL1 to AArch32 EL0 without a context switch
>> (the inverse can happen if we inject an exception to the guest).
>>
>> Reading the Cortex-A53 SDEN, it sounds like this is an OS and not Hypervisor
>> problem. In fact, Linux only seems to workaround it when switching in the OS
>> side rather than the hypervisor.
>>
>> Therefore, I am not sure to understand why we need to workaroud it in Xen.
> 
> It looks like Julien is right in regards to the "aarch64 EL1 to aarch32
> EL0" issue.
> 
Yes I agree. I missed the fact that there is a write to CONTEXTIDR_EL1
in 'ctxt_switch_to'. Let's abandon this.

Thanks,
Michal

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

[Stefano Stabellini]

On Wed, 9 Dec 2020, Michal Orzel wrote:
> On 09.12.2020 02:34, Stefano Stabellini wrote:
> > On Tue, 8 Dec 2020, Julien Grall wrote:
> >> On 08/12/2020 14:38, Bertrand Marquis wrote:
> >>> Hi Julien,
> >>>
> >>>> On 8 Dec 2020, at 09:47, Julien Grall <julien@xen.org> 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.
> > 
> > Just to confirm: it cannot happen when switching from aarch64 *EL2* to
> > aarch32 EL0/1, right?  Because that happens all the time in Xen.
> > 
> > 
> No it cannot. It can only happen when switching from aarch64 EL1 to aarch32 EL0.

Excellent, thanks for checking


> >> Right, but the context switch will write to CONTEXTIDR_EL1. So this case
> >> should already be handled.
> >>
> >> Xen will never switch from AArch64 EL1 to AArch32 EL0 without a context switch
> >> (the inverse can happen if we inject an exception to the guest).
> >>
> >> Reading the Cortex-A53 SDEN, it sounds like this is an OS and not Hypervisor
> >> problem. In fact, Linux only seems to workaround it when switching in the OS
> >> side rather than the hypervisor.
> >>
> >> Therefore, I am not sure to understand why we need to workaroud it in Xen.
> > 
> > It looks like Julien is right in regards to the "aarch64 EL1 to aarch32
> > EL0" issue.
> > 
> Yes I agree. I missed the fact that there is a write to CONTEXTIDR_EL1
> in 'ctxt_switch_to'. Let's abandon this.

Job done :-)