Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Shanker Donthineni]

Hi James,

On 11/10/2017 04:24 AM, James Morse wrote:
> Hi Shanker,
> 
> On 09/11/17 15:22, Shanker Donthineni wrote:
>> On 11/09/2017 05:08 AM, James Morse wrote:
>>> On 04/11/17 21:43, Shanker Donthineni wrote:
 On 11/03/2017 10:11 AM, Robin Murphy wrote:
> On 03/11/17 03:27, Shanker Donthineni wrote:
>> The ARM architecture defines the memory locations that are permitted
>> to be accessed as the result of a speculative instruction fetch from
>> an exception level for which all stages of translation are disabled.
>> Specifically, the core is permitted to speculatively fetch from the
>> 4KB region containing the current program counter and next 4KB.
>>
>> When translation is changed from enabled to disabled for the running
>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>> Falkor core may errantly speculatively access memory locations outside
>> of the 4KB region permitted by the architecture. The errant memory
>> access may lead to one of the following unexpected behaviors.
>>
>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>to the errant memory access attempting to access a region of memory
>>that is protected by a slave-side memory protection unit.
>> 2) Unpredictable device behavior due to a speculative read from device
>>memory. This behavior may only occur if the instruction cache is
>>disabled prior to or coincident with translation being changed from
>>enabled to disabled.
>>
>> To avoid the errant behavior, software must execute an ISB immediately
>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
>> diff --git a/arch/arm64/include/asm/assembler.h 
>> b/arch/arm64/include/asm/assembler.h
>> index b6dfb4f..4c91efb 100644
>> --- a/arch/arm64/include/asm/assembler.h
>> +++ b/arch/arm64/include/asm/assembler.h
>> @@ -514,6 +515,22 @@
>>   *   reg: the value to be written.
>>   */
>>  .macro  write_sctlr, eln, reg
>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>> +tbnz\reg, #0, 8000f  // enable MMU?
>>>
>>> Won't this match any change that leaves the MMU enabled?
>>
>> Yes. No need to apply workaround if the MMU is going to be enabled.
> 
> (Sorry, looks like I had this upside down)
> 
> My badly-made-point is you can't know if the MMU is being disabled unless you
> have both the old and new values.
> 
> As an example, in el2_setup, (where the MMU is disabled), we set the EE/E0E 
> bits
> to match the kernel's endianness. Won't your macro will insert an unnecessary
> isb? Is this needed for the errata workaround?
> 

Yes, It's not required in this case. I'll post a v2 patch and apply the 
workaround
where it's absolutely required. Seems handling a workaround inside helper macros
causing confusion.

> 
>>> I think the macro is making this more confusing. Disabling the MMU is 
>>> obvious
>>> from the call-site, (and really rare!). Trying to work it out from a macro 
>>> makes
>>> it more complicated than necessary.
> 
>> Not clear, are you suggesting not to use read{write}_sctlr() macros instead 
>> apply 
>> the workaround from the call-site based on the MMU-on status?
> 
> Yes. This is the only way to patch only the locations that turn the MMU off.
> 
> 
>> If yes, It simplifies
>> the code logic but CONFIG_QCOM_FALKOR_ERRATUM_1041 references are scatter 
>> everywhere. 
> 
> Wouldn't they only appear in the places that are affected by the errata?
> This is exactly what we want, anyone touching that code now knows they need to
> double check this behaviour, (and ask you to test it!).
> 
> Otherwise we have a macro second guessing what is happening, if its not quite
> right (because some information has been lost), we're now not sure what we 
> need
> to do if we ever refactor any of this code.
> 
> [...]
> 
 I'll prefer alternatives
 just to avoid the unnecessary overhead on future Qualcomm Datacenter
 server CPUs and regression on other CPUs because of inserting an ISB
>>>
>>> I think hiding errata on other CPUs is a good argument.
>>>
>>> My suggestion would be:
 #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
isb
 #endif
>>>
>>> In head.S and efi-entry.S, as these run before alternatives.
>>> Then use alternatives to add just the isb in the mmu-off path for the other 
>>> callers.
> 
>> Thanks for your opinion on this one, I'll change to an unconditional ISB in 
>> v2 patch.
>> After this change the enable_mmu() issues two ISBs before writing to 
>> SCTLR_EL1.
> 
> Another great reason not to wrap this in a macro, there may already be a
> suitable isb, in which case a comment will suffice.
> 
> 
>> Are you okay with this behavior?
> 
> Back-to-back isb doesn't sound like a good idea.
> 
> 
>>  ENTRY(__enable_mmu)
>> mrs x1, ID_AA64MMFR0_EL1
>> ubfx  

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[James Morse]

Hi Shanker,

On 09/11/17 15:22, Shanker Donthineni wrote:
> On 11/09/2017 05:08 AM, James Morse wrote:
>> On 04/11/17 21:43, Shanker Donthineni wrote:
>>> On 11/03/2017 10:11 AM, Robin Murphy wrote:
 On 03/11/17 03:27, Shanker Donthineni wrote:
> The ARM architecture defines the memory locations that are permitted
> to be accessed as the result of a speculative instruction fetch from
> an exception level for which all stages of translation are disabled.
> Specifically, the core is permitted to speculatively fetch from the
> 4KB region containing the current program counter and next 4KB.
>
> When translation is changed from enabled to disabled for the running
> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
> Falkor core may errantly speculatively access memory locations outside
> of the 4KB region permitted by the architecture. The errant memory
> access may lead to one of the following unexpected behaviors.
>
> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>to the errant memory access attempting to access a region of memory
>that is protected by a slave-side memory protection unit.
> 2) Unpredictable device behavior due to a speculative read from device
>memory. This behavior may only occur if the instruction cache is
>disabled prior to or coincident with translation being changed from
>enabled to disabled.
>
> To avoid the errant behavior, software must execute an ISB immediately
> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.

> diff --git a/arch/arm64/include/asm/assembler.h 
> b/arch/arm64/include/asm/assembler.h
> index b6dfb4f..4c91efb 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -514,6 +515,22 @@
>   *   reg: the value to be written.
>   */
>   .macro  write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
> + tbnz\reg, #0, 8000f  // enable MMU?
>>
>> Won't this match any change that leaves the MMU enabled?
> 
> Yes. No need to apply workaround if the MMU is going to be enabled.

(Sorry, looks like I had this upside down)

My badly-made-point is you can't know if the MMU is being disabled unless you
have both the old and new values.

As an example, in el2_setup, (where the MMU is disabled), we set the EE/E0E bits
to match the kernel's endianness. Won't your macro will insert an unnecessary
isb? Is this needed for the errata workaround?


>> I think the macro is making this more confusing. Disabling the MMU is obvious
>> from the call-site, (and really rare!). Trying to work it out from a macro 
>> makes
>> it more complicated than necessary.

> Not clear, are you suggesting not to use read{write}_sctlr() macros instead 
> apply 
> the workaround from the call-site based on the MMU-on status?

Yes. This is the only way to patch only the locations that turn the MMU off.


> If yes, It simplifies
> the code logic but CONFIG_QCOM_FALKOR_ERRATUM_1041 references are scatter 
> everywhere. 

Wouldn't they only appear in the places that are affected by the errata?
This is exactly what we want, anyone touching that code now knows they need to
double check this behaviour, (and ask you to test it!).

Otherwise we have a macro second guessing what is happening, if its not quite
right (because some information has been lost), we're now not sure what we need
to do if we ever refactor any of this code.

[...]

>>> I'll prefer alternatives
>>> just to avoid the unnecessary overhead on future Qualcomm Datacenter
>>> server CPUs and regression on other CPUs because of inserting an ISB
>>
>> I think hiding errata on other CPUs is a good argument.
>>
>> My suggestion would be:
>>> #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>> isb
>>> #endif
>>
>> In head.S and efi-entry.S, as these run before alternatives.
>> Then use alternatives to add just the isb in the mmu-off path for the other 
>> callers.

> Thanks for your opinion on this one, I'll change to an unconditional ISB in 
> v2 patch.
> After this change the enable_mmu() issues two ISBs before writing to 
> SCTLR_EL1.

Another great reason not to wrap this in a macro, there may already be a
suitable isb, in which case a comment will suffice.


> Are you okay with this behavior?

Back-to-back isb doesn't sound like a good idea.


>  ENTRY(__enable_mmu)
> mrs x1, ID_AA64MMFR0_EL1
> ubfxx2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
> cmp x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
> b.ne__no_granule_support
> update_early_cpu_boot_status 0, x1, x2
> adrpx1, idmap_pg_dir
> adrpx2, swapper_pg_dir
> msr ttbr0_el1, x1   // load TTBR0
> msr ttbr1_el1, x2   // load TTBR1
> isb
> write_sctlr 

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Shanker Donthineni]

Hi James,

On 11/09/2017 05:08 AM, James Morse wrote:
> Hi Shanker, Robin,
> 
> On 04/11/17 21:43, Shanker Donthineni wrote:
>> On 11/03/2017 10:11 AM, Robin Murphy wrote:
>>> On 03/11/17 03:27, Shanker Donthineni wrote:
 The ARM architecture defines the memory locations that are permitted
 to be accessed as the result of a speculative instruction fetch from
 an exception level for which all stages of translation are disabled.
 Specifically, the core is permitted to speculatively fetch from the
 4KB region containing the current program counter and next 4KB.

 When translation is changed from enabled to disabled for the running
 exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
 Falkor core may errantly speculatively access memory locations outside
 of the 4KB region permitted by the architecture. The errant memory
 access may lead to one of the following unexpected behaviors.

 1) A System Error Interrupt (SEI) being raised by the Falkor core due
to the errant memory access attempting to access a region of memory
that is protected by a slave-side memory protection unit.
 2) Unpredictable device behavior due to a speculative read from device
memory. This behavior may only occur if the instruction cache is
disabled prior to or coincident with translation being changed from
enabled to disabled.

 To avoid the errant behavior, software must execute an ISB immediately
 prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
> 
 diff --git a/arch/arm64/include/asm/assembler.h 
 b/arch/arm64/include/asm/assembler.h
 index b6dfb4f..4c91efb 100644
 --- a/arch/arm64/include/asm/assembler.h
 +++ b/arch/arm64/include/asm/assembler.h
 @@ -30,6 +30,7 @@
  #include 
  #include 
  #include 
 +#include 
  
  /*
   * Enable and disable interrupts.
 @@ -514,6 +515,22 @@
   *   reg: the value to be written.
   */
.macro  write_sctlr, eln, reg
 +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
 +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
 +  tbnz\reg, #0, 8000f  // enable MMU?
> 
> Won't this match any change that leaves the MMU enabled?
> 

Yes. No need to apply workaround if the MMU is going to be enabled.

> I think the macro is making this more confusing. Disabling the MMU is obvious
> from the call-site, (and really rare!). Trying to work it out from a macro 
> makes
> it more complicated than necessary.
>

Not clear, are you suggesting not to use read{write}_sctlr() macros instead 
apply 
the workaround from the call-site based on the MMU-on status? If yes, It 
simplifies
the code logic but CONFIG_QCOM_FALKOR_ERRATUM_1041 references are scatter 
everywhere. 
 
> 
>>> Do we really need the branch here? It's not like enabling the MMU is
>>> something we do on the syscall fastpath, and I can't imagine an extra
>>> ISB hurts much (and is probably comparable to a mispredicted branch
> 
>> I don't have any strong opinion on whether to use an ISB conditionally
>> or unconditionally. Yes, the current kernel code is not touching
>> SCTLR_ELn register on the system call fast path. I would like to keep
>> it as a conditional ISB in case if the future kernel accesses the
>> SCTLR_ELn on the fast path. An extra ISB should not hurt a lot but I
>> believe it has more overhead than the TBZ+branch mis-prediction on Falkor
>> CPU. This patch has been tested on the real hardware to fix the problem.
> 
>> I'm open to change to an unconditional ISB if it's the better fix.
>>
>>> anyway). In fact, is there any noticeable hit on other
>>> microarchitectures if we save the alternative bother and just do it
>>> unconditionally always?
>>>
>>
>> I can't comment on the performance impacts of other CPUs since I don't
>> have access to their development platforms. I'll prefer alternatives
>> just to avoid the unnecessary overhead on future Qualcomm Datacenter
>> server CPUs and regression on other CPUs because of inserting an ISB
> 
> I think hiding errata on other CPUs is a good argument.
> 
> My suggestion would be:
>> #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>  isb
>> #endif
> 
> In head.S and efi-entry.S, as these run before alternatives.
> Then use alternatives to add just the isb in the mmu-off path for the other 
> callers.
> 
> 

Thanks for your opinion on this one, I'll change to an unconditional ISB in v2 
patch.
After this change the enable_mmu() issues two ISBs before writing to SCTLR_EL1. 
Are
you okay with this behavior?

 ENTRY(__enable_mmu)
mrs x1, ID_AA64MMFR0_EL1
ubfxx2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
cmp x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
b.ne__no_granule_support
update_early_cpu_boot_status 0, x1, x2
adrpx1, idmap_pg_dir
adrpx2, swapper_pg_dir
msr ttbr0_el1, x1   // load TTBR0

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[James Morse]

Hi Shanker, Robin,

On 04/11/17 21:43, Shanker Donthineni wrote:
> On 11/03/2017 10:11 AM, Robin Murphy wrote:
>> On 03/11/17 03:27, Shanker Donthineni wrote:
>>> The ARM architecture defines the memory locations that are permitted
>>> to be accessed as the result of a speculative instruction fetch from
>>> an exception level for which all stages of translation are disabled.
>>> Specifically, the core is permitted to speculatively fetch from the
>>> 4KB region containing the current program counter and next 4KB.
>>>
>>> When translation is changed from enabled to disabled for the running
>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>> Falkor core may errantly speculatively access memory locations outside
>>> of the 4KB region permitted by the architecture. The errant memory
>>> access may lead to one of the following unexpected behaviors.
>>>
>>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>>to the errant memory access attempting to access a region of memory
>>>that is protected by a slave-side memory protection unit.
>>> 2) Unpredictable device behavior due to a speculative read from device
>>>memory. This behavior may only occur if the instruction cache is
>>>disabled prior to or coincident with translation being changed from
>>>enabled to disabled.
>>>
>>> To avoid the errant behavior, software must execute an ISB immediately
>>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.


>>> diff --git a/arch/arm64/include/asm/assembler.h 
>>> b/arch/arm64/include/asm/assembler.h
>>> index b6dfb4f..4c91efb 100644
>>> --- a/arch/arm64/include/asm/assembler.h
>>> +++ b/arch/arm64/include/asm/assembler.h
>>> @@ -30,6 +30,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  
>>>  /*
>>>   * Enable and disable interrupts.
>>> @@ -514,6 +515,22 @@
>>>   *   reg: the value to be written.
>>>   */
>>> .macro  write_sctlr, eln, reg
>>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>>> +   tbnz\reg, #0, 8000f  // enable MMU?

Won't this match any change that leaves the MMU enabled?

I think the macro is making this more confusing. Disabling the MMU is obvious
from the call-site, (and really rare!). Trying to work it out from a macro makes
it more complicated than necessary.


>> Do we really need the branch here? It's not like enabling the MMU is
>> something we do on the syscall fastpath, and I can't imagine an extra
>> ISB hurts much (and is probably comparable to a mispredicted branch

> I don't have any strong opinion on whether to use an ISB conditionally
> or unconditionally. Yes, the current kernel code is not touching
> SCTLR_ELn register on the system call fast path. I would like to keep
> it as a conditional ISB in case if the future kernel accesses the
> SCTLR_ELn on the fast path. An extra ISB should not hurt a lot but I
> believe it has more overhead than the TBZ+branch mis-prediction on Falkor
> CPU. This patch has been tested on the real hardware to fix the problem.

> I'm open to change to an unconditional ISB if it's the better fix.
> 
>> anyway). In fact, is there any noticeable hit on other
>> microarchitectures if we save the alternative bother and just do it
>> unconditionally always?
>>
> 
> I can't comment on the performance impacts of other CPUs since I don't
> have access to their development platforms. I'll prefer alternatives
> just to avoid the unnecessary overhead on future Qualcomm Datacenter
> server CPUs and regression on other CPUs because of inserting an ISB

I think hiding errata on other CPUs is a good argument.

My suggestion would be:
> #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>   isb
> #endif

In head.S and efi-entry.S, as these run before alternatives.
Then use alternatives to add just the isb in the mmu-off path for the other 
callers.


> prior to SCTLR_ELn register update. Let's see what rest of the ARM 
> maintainers think about always using an ISB instead of alternatives.


Thanks,

James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Shanker Donthineni]

Hi Robin, Thanks for your review comments. 

On 11/03/2017 10:11 AM, Robin Murphy wrote:
> On 03/11/17 03:27, Shanker Donthineni wrote:
>> The ARM architecture defines the memory locations that are permitted
>> to be accessed as the result of a speculative instruction fetch from
>> an exception level for which all stages of translation are disabled.
>> Specifically, the core is permitted to speculatively fetch from the
>> 4KB region containing the current program counter and next 4KB.
>>
>> When translation is changed from enabled to disabled for the running
>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>> Falkor core may errantly speculatively access memory locations outside
>> of the 4KB region permitted by the architecture. The errant memory
>> access may lead to one of the following unexpected behaviors.
>>
>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>to the errant memory access attempting to access a region of memory
>>that is protected by a slave-side memory protection unit.
>> 2) Unpredictable device behavior due to a speculative read from device
>>memory. This behavior may only occur if the instruction cache is
>>disabled prior to or coincident with translation being changed from
>>enabled to disabled.
>>
>> To avoid the errant behavior, software must execute an ISB immediately
>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
>>
>> Signed-off-by: Shanker Donthineni 
>> ---
>>  Documentation/arm64/silicon-errata.txt |  1 +
>>  arch/arm64/Kconfig | 10 ++
>>  arch/arm64/include/asm/assembler.h | 17 +
>>  arch/arm64/include/asm/cpucaps.h   |  3 ++-
>>  arch/arm64/kernel/cpu_errata.c | 16 
>>  arch/arm64/kernel/efi-entry.S  |  4 ++--
>>  arch/arm64/kernel/head.S   |  4 ++--
>>  7 files changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/arm64/silicon-errata.txt 
>> b/Documentation/arm64/silicon-errata.txt
>> index 66e8ce1..704770c0 100644
>> --- a/Documentation/arm64/silicon-errata.txt
>> +++ b/Documentation/arm64/silicon-errata.txt
>> @@ -74,3 +74,4 @@ stable kernels.
>>  | Qualcomm Tech. | Falkor v1   | E1003   | 
>> QCOM_FALKOR_ERRATUM_1003|
>>  | Qualcomm Tech. | Falkor v1   | E1009   | 
>> QCOM_FALKOR_ERRATUM_1009|
>>  | Qualcomm Tech. | QDF2400 ITS | E0065   | 
>> QCOM_QDF2400_ERRATUM_0065   |
>> +| Qualcomm Tech. | Falkor v{1,2}   | E1041   | 
>> QCOM_FALKOR_ERRATUM_1041|
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 0df64a6..7e933fb 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -539,6 +539,16 @@ config QCOM_QDF2400_ERRATUM_0065
>>  
>>If unsure, say Y.
>>  
>> +config QCOM_FALKOR_ERRATUM_1041
>> +bool "Falkor E1041: Speculative instruction fetches might cause errant 
>> memory access"
>> +default y
>> +help
>> +  Falkor CPU may speculatively fetch instructions from an improper
>> +  memory location when MMU translation is changed from SCTLR_ELn[M]=1
>> +  to SCTLR_ELn[M]=0. Prefix an ISB instruction to fix the problem.
>> +
>> +  If unsure, say Y.
>> +
>>  endmenu
>>  
>>  
>> diff --git a/arch/arm64/include/asm/assembler.h 
>> b/arch/arm64/include/asm/assembler.h
>> index b6dfb4f..4c91efb 100644
>> --- a/arch/arm64/include/asm/assembler.h
>> +++ b/arch/arm64/include/asm/assembler.h
>> @@ -30,6 +30,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  /*
>>   * Enable and disable interrupts.
>> @@ -514,6 +515,22 @@
>>   *   reg: the value to be written.
>>   */
>>  .macro  write_sctlr, eln, reg
>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>> +tbnz\reg, #0, 8000f  // enable MMU?
> 
> Do we really need the branch here? It's not like enabling the MMU is
> something we do on the syscall fastpath, and I can't imagine an extra
> ISB hurts much (and is probably comparable to a mispredicted branch

I don't have any strong opinion on whether to use an ISB conditionally
or unconditionally. Yes, the current kernel code is not touching
SCTLR_ELn register on the system call fast path. I would like to keep
it as a conditional ISB in case if the future kernel accesses the
SCTLR_ELn on the fast path. An extra ISB should not hurt a lot but I
believe it has more overhead than the TBZ+branch mis-prediction on Falkor
CPU. This patch has been tested on the real hardware to fix the problem.

I'm open to change to an unconditional ISB if it's the better fix.

> anyway). In fact, is there any noticeable hit on other
> microarchitectures if we save the alternative bother and just do it
> unconditionally always?
> 

I can't comment on the performance impacts of other CPUs since I don't
have access to their development platforms. I'll prefer alternatives
just to 

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Robin Murphy]

On 03/11/17 03:27, Shanker Donthineni wrote:
> The ARM architecture defines the memory locations that are permitted
> to be accessed as the result of a speculative instruction fetch from
> an exception level for which all stages of translation are disabled.
> Specifically, the core is permitted to speculatively fetch from the
> 4KB region containing the current program counter and next 4KB.
> 
> When translation is changed from enabled to disabled for the running
> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
> Falkor core may errantly speculatively access memory locations outside
> of the 4KB region permitted by the architecture. The errant memory
> access may lead to one of the following unexpected behaviors.
> 
> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>to the errant memory access attempting to access a region of memory
>that is protected by a slave-side memory protection unit.
> 2) Unpredictable device behavior due to a speculative read from device
>memory. This behavior may only occur if the instruction cache is
>disabled prior to or coincident with translation being changed from
>enabled to disabled.
> 
> To avoid the errant behavior, software must execute an ISB immediately
> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
> Signed-off-by: Shanker Donthineni 
> ---
>  Documentation/arm64/silicon-errata.txt |  1 +
>  arch/arm64/Kconfig | 10 ++
>  arch/arm64/include/asm/assembler.h | 17 +
>  arch/arm64/include/asm/cpucaps.h   |  3 ++-
>  arch/arm64/kernel/cpu_errata.c | 16 
>  arch/arm64/kernel/efi-entry.S  |  4 ++--
>  arch/arm64/kernel/head.S   |  4 ++--
>  7 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/arm64/silicon-errata.txt 
> b/Documentation/arm64/silicon-errata.txt
> index 66e8ce1..704770c0 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -74,3 +74,4 @@ stable kernels.
>  | Qualcomm Tech. | Falkor v1   | E1003   | 
> QCOM_FALKOR_ERRATUM_1003|
>  | Qualcomm Tech. | Falkor v1   | E1009   | 
> QCOM_FALKOR_ERRATUM_1009|
>  | Qualcomm Tech. | QDF2400 ITS | E0065   | 
> QCOM_QDF2400_ERRATUM_0065   |
> +| Qualcomm Tech. | Falkor v{1,2}   | E1041   | 
> QCOM_FALKOR_ERRATUM_1041|
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 0df64a6..7e933fb 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -539,6 +539,16 @@ config QCOM_QDF2400_ERRATUM_0065
>  
> If unsure, say Y.
>  
> +config QCOM_FALKOR_ERRATUM_1041
> + bool "Falkor E1041: Speculative instruction fetches might cause errant 
> memory access"
> + default y
> + help
> +   Falkor CPU may speculatively fetch instructions from an improper
> +   memory location when MMU translation is changed from SCTLR_ELn[M]=1
> +   to SCTLR_ELn[M]=0. Prefix an ISB instruction to fix the problem.
> +
> +   If unsure, say Y.
> +
>  endmenu
>  
>  
> diff --git a/arch/arm64/include/asm/assembler.h 
> b/arch/arm64/include/asm/assembler.h
> index b6dfb4f..4c91efb 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * Enable and disable interrupts.
> @@ -514,6 +515,22 @@
>   *   reg: the value to be written.
>   */
>   .macro  write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
> + tbnz\reg, #0, 8000f  // enable MMU?

Do we really need the branch here? It's not like enabling the MMU is
something we do on the syscall fastpath, and I can't imagine an extra
ISB hurts much (and is probably comparable to a mispredicted branch
anyway). In fact, is there any noticeable hit on other
microarchitectures if we save the alternative bother and just do it
unconditionally always?

Robin.

> + isb
> +8000:
> +alternative_else_nop_endif
> +#endif
> + msr sctlr_\eln, \reg
> + .endm
> +
> + .macro  early_write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> + tbnz\reg, #0, 8000f  // enable MMU?
> + isb
> +8000:
> +#endif
>   msr sctlr_\eln, \reg
>   .endm
>  
> diff --git a/arch/arm64/include/asm/cpucaps.h 
> b/arch/arm64/include/asm/cpucaps.h
> index 8da6216..7f7a59d 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -40,7 +40,8 @@
>  #define ARM64_WORKAROUND_858921  19
>  #define ARM64_WORKAROUND_CAVIUM_3011520
>  #define ARM64_HAS_DCPOP  21
> +#define ARM64_WORKAROUND_QCOM_FALKOR_E1041   22
>  
> -#define ARM64_NCAPS  22
> +#define ARM64_NCAPS

[PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Shanker Donthineni]

The ARM architecture defines the memory locations that are permitted
to be accessed as the result of a speculative instruction fetch from
an exception level for which all stages of translation are disabled.
Specifically, the core is permitted to speculatively fetch from the
4KB region containing the current program counter and next 4KB.

When translation is changed from enabled to disabled for the running
exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
Falkor core may errantly speculatively access memory locations outside
of the 4KB region permitted by the architecture. The errant memory
access may lead to one of the following unexpected behaviors.

1) A System Error Interrupt (SEI) being raised by the Falkor core due
   to the errant memory access attempting to access a region of memory
   that is protected by a slave-side memory protection unit.
2) Unpredictable device behavior due to a speculative read from device
   memory. This behavior may only occur if the instruction cache is
   disabled prior to or coincident with translation being changed from
   enabled to disabled.

To avoid the errant behavior, software must execute an ISB immediately
prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.

Signed-off-by: Shanker Donthineni 
---
 Documentation/arm64/silicon-errata.txt |  1 +
 arch/arm64/Kconfig | 10 ++
 arch/arm64/include/asm/assembler.h | 17 +
 arch/arm64/include/asm/cpucaps.h   |  3 ++-
 arch/arm64/kernel/cpu_errata.c | 16 
 arch/arm64/kernel/efi-entry.S  |  4 ++--
 arch/arm64/kernel/head.S   |  4 ++--
 7 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/Documentation/arm64/silicon-errata.txt 
b/Documentation/arm64/silicon-errata.txt
index 66e8ce1..704770c0 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -74,3 +74,4 @@ stable kernels.
 | Qualcomm Tech. | Falkor v1   | E1003   | 
QCOM_FALKOR_ERRATUM_1003|
 | Qualcomm Tech. | Falkor v1   | E1009   | 
QCOM_FALKOR_ERRATUM_1009|
 | Qualcomm Tech. | QDF2400 ITS | E0065   | 
QCOM_QDF2400_ERRATUM_0065   |
+| Qualcomm Tech. | Falkor v{1,2}   | E1041   | 
QCOM_FALKOR_ERRATUM_1041|
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 0df64a6..7e933fb 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -539,6 +539,16 @@ config QCOM_QDF2400_ERRATUM_0065
 
  If unsure, say Y.
 
+config QCOM_FALKOR_ERRATUM_1041
+   bool "Falkor E1041: Speculative instruction fetches might cause errant 
memory access"
+   default y
+   help
+ Falkor CPU may speculatively fetch instructions from an improper
+ memory location when MMU translation is changed from SCTLR_ELn[M]=1
+ to SCTLR_ELn[M]=0. Prefix an ISB instruction to fix the problem.
+
+ If unsure, say Y.
+
 endmenu
 
 
diff --git a/arch/arm64/include/asm/assembler.h 
b/arch/arm64/include/asm/assembler.h
index b6dfb4f..4c91efb 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * Enable and disable interrupts.
@@ -514,6 +515,22 @@
  *   reg: the value to be written.
  */
.macro  write_sctlr, eln, reg
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
+   tbnz\reg, #0, 8000f  // enable MMU?
+   isb
+8000:
+alternative_else_nop_endif
+#endif
+   msr sctlr_\eln, \reg
+   .endm
+
+   .macro  early_write_sctlr, eln, reg
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
+   tbnz\reg, #0, 8000f  // enable MMU?
+   isb
+8000:
+#endif
msr sctlr_\eln, \reg
.endm
 
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index 8da6216..7f7a59d 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -40,7 +40,8 @@
 #define ARM64_WORKAROUND_85892119
 #define ARM64_WORKAROUND_CAVIUM_30115  20
 #define ARM64_HAS_DCPOP21
+#define ARM64_WORKAROUND_QCOM_FALKOR_E1041 22
 
-#define ARM64_NCAPS22
+#define ARM64_NCAPS23
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 0e27f86..27f9a45 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -179,6 +179,22 @@ static int cpu_enable_trap_ctr_access(void *__unused)
   MIDR_CPU_VAR_REV(0, 0)),
},
 #endif
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
+   {
+   .desc = "Qualcomm Technologies Falkor erratum 1041",
+   .capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
+   MIDR_RANGE(MIDR_QCOM_FALKOR_V1,
+