Thanks Andrew for raising the issue and Marc and Raghu for your inputs.
The effect of stage 2 when Stage 1 is disabled is something that was overlooked in the workaround implementation. After some discussions this is what we currently understand regarding the problem.
1. EL3 does not modify the EL2 stage 1 translations and hence any speculative execution of AT execution in EL3 and resulting PTW and TLB caching should not be a problem as we always return to the same context in EL2. TF-A in EL3 need not worry about this. Also, we expect that future CPUs having v8.4 extensions for S-EL2 will have already corrected this errata.
2. Whenever the EL1/0 Stage 1 translations are switched in EL3 as part of switching the worlds, it should take Stage 2 into account. Hence disabling the Stage 1 MMU is not effective, as Marc pointed out, because speculative PTW can still take place using Stage 2 identity mapping. After discussion with James, we also realized that flipping the SCR_EL3.NS bit could take Stage 2 out of context (Stage 2 is not valid in secure world) leaving Stage 1 pointing to invalid translations.
As discussed in previous emails, on Entry to EL3, if TCR_EL1.EPDx = 1 and SCTLR_EL3.M = 1, any speculative AT PTWs for EL1 context can be prevented (including Stage 2).
So the fix as we currently understand would involve the following sequence :
a. On Entry to EL3, save the incoming SCTLR_EL1.M and TCR_EL1.EPDx bits and set them (ensure TCR_EL1.EPDx =1 prior to SCTLR_EL1.M =1 using isb())
b. Prior to Exit from EL3, after the target context is restored, restore the SCTLR_EL1.M and TCR_EL1.EPDx bits.
The above sequence now means that any use of AT instruction targeted at lower EL from EL3 that require PTW will fault. So prior to use of AT, ensure the PTW are re-enabled and disabled back again after the AT instructions.
If the above sequence is agreed upon to resolve the errata, then we can work on a patch for the same. I suspect current el1 register save and restore sequence in TF-A is a bit unwieldy and we may need to analyze all the entry points to EL3 to ensure we cover everything.
Best Regards
Soby Mathew
> -----Original Message-----
> From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Andrew
> Scull via TF-A
> Sent: 02 July 2020 09:20
> To: Raghu K <raghu.ncstate(a)icloud.com>
> Cc: android-kvm(a)google.com; willdeacon(a)google.com; Marc Zyngier
> <mzyngier(a)google.com>; tf-a(a)lists.trustedfirmware.org
> Subject: Re: [TF-A] Erroneous speculative AT workaround
>
> On Wed, Jul 01, 2020 at 05:47:00PM -0700, Raghu K wrote:
> > This is interesting. It appears that there is no way on entry to EL3
> > to guarantee that the out-of-context(el2 and el1) translation regimes
> > are in a consistent state and on every entry into EL3, we have to
> > conservatively assume that it is in an inconsistent state. This is
> > because of the situation Andrew mentioned(interrupts to EL3 can occur at any
> time).
> >
> > If this is the case, on EL3 entry:
> > 1) For EL1, we will need to save SCTLR_EL1, set SCTLR_EL1.M = 1,.EPDx
> > = 0
>
> This would still be racing against any potential speculative execution of an AT
> instruction upon the switch to EL3, IIUC. The window would be much smaller
> but not entirely eliminated.
>
> For KVM, this would be enough as KVM will have already applied this
> workaround (with Marc's corrections) whenever it is going to enter an
> inconsistent state. However, other EL2 software may choose to handle the
> errata differently, possibly going to the lengths of ensuring that no AT
> instruction is ever mapped executable.
> --
> TF-A mailing list
> TF-A(a)lists.trustedfirmware.org
> https://lists.trustedfirmware.org/mailman/listinfo/tf-a
This is interesting. It appears that there is no way on entry to EL3 to
guarantee that the out-of-context(el2 and el1) translation regimes are
in a consistent state and on every entry into EL3, we have to
conservatively assume that it is in an inconsistent state. This is
because of the situation Andrew mentioned(interrupts to EL3 can occur at
any time).
If this is the case, on EL3 entry:
1) For EL1, we will need to save SCTLR_EL1, set SCTLR_EL1.M = 1,.EPDx = 0
2) Set whatever bits we need to for EL2 and S2 translations to not
succeed(What are these?)
3) DSB, to ensure no speculative AT can be issued until completion of
DSB, so any AT that occurs will not fill the TLB with bad translations.
On exit(right before ERET), we need to restore the registers saved on
entry, and have the ERET followed by a DSB so that there can be no
speculative execution of AT instructions.
Thanks
Raghu
On 7/1/20 5:54 AM, Marc Zyngier via TF-A wrote:
> Hi Manish,
>
> On Wed, 1 Jul 2020 at 13:14, Manish Badarkhe <Manish.Badarkhe(a)arm.com> wrote:
>> Hi Andrew,
>>
>> As per current implementation, in “el1_sysregs_context_restore” routine do below things:
>>
>> 1. TCR_EL1.EPD0 = 1
>> 2. TCR_EL1.EPD1 = 1
>> 3. SCTR_EL1.M = 0
>> 4. Isb
>> Code snippet:
>> mrs x9, tcr_el1
>> orr x9, x9, #TCR_EPD0_BIT
>> orr x9, x9, #TCR_EPD1_BIT
>> msr tcr_el1, x9
>> mrs x9, sctlr_el1
>> bic x9, x9, #SCTLR_M_BIT
>> msr sctlr_el1, x9
>> isb
>> This is to avoid PTW through while updating system registers at step 5
> Unfortunately, this doesn't prevent anything.
>
> If SCTLR_EL1.M is clear, TCR_EL1.EPDx don't mean much (S1 MMU is
> disabled, no S1 page table walk), and you can still have S2 PTWs
> (using an idmap for S1) and creating TLB corruption if these entry
> alias with any S1 mapping that exists at EL1.
>
> Which is why KVM does *set* SCTLR_EL1.M, which prevents the use of a
> 1:1 mapping at S1, and at which point the TCR_EL1.EPDx bits are
> actually useful in preventing a PTW.
>
>> 5. Restore all system registers for El1 except SCTLR_EL1 and TCR_EL1
>> 6. isb()
>> 7. restore SCTLR_EL1 and TCR_EL1
>> Code Snippet:
>> ldr x9, [x0, #CTX_SCTLR_EL1] -> saved value from "el2_sysregs_context_save"
>> msr sctlr_el1, x9
>> ldr x9, [x0, #CTX_TCR_EL1]
>> msr tcr_el1, x9
>>
>> As per above steps. SCTLR_EL1 get restored back with actual settings at step 7.
>> Similar flow is present for “el2_sysregs_context_restore” to restore SCTLR_EL1 register.
>>
>> In conclusion, this routine temporarily clear M bit of SCTLR_EL1 to avoid speculation but restored it back
>> to its original setting while leaving back to its caller. Please let us know whether this align with KVM
>> workaround for speculative AT erratum.
> It doesn't, unfortunately. I believe this code actively creates
> problems on a system that is affected by speculative AT execution.
>
> I don't understand your rationale for touching SCTLR_EL2.M either if
> you are not context-switching the EL2 S1 state: as far as I understand
> no affected cores have S-EL2, so no switch should happen at this
> stage.
>
> Thanks,
>
> M.
Hi Andrew,
As per current implementation, in “el1_sysregs_context_restore” routine do below things:
1. TCR_EL1.EPD0 = 1
2. TCR_EL1.EPD1 = 1
3. SCTR_EL1.M = 0
4. Isb
Code snippet:
mrs x9, tcr_el1
orr x9, x9, #TCR_EPD0_BIT
orr x9, x9, #TCR_EPD1_BIT
msr tcr_el1, x9
mrs x9, sctlr_el1
bic x9, x9, #SCTLR_M_BIT
msr sctlr_el1, x9
isb
This is to avoid PTW through while updating system registers at step 5
5. Restore all system registers for El1 except SCTLR_EL1 and TCR_EL1
6. isb()
7. restore SCTLR_EL1 and TCR_EL1
Code Snippet:
ldr x9, [x0, #CTX_SCTLR_EL1] -> saved value from "el2_sysregs_context_save"
msr sctlr_el1, x9
ldr x9, [x0, #CTX_TCR_EL1]
msr tcr_el1, x9
As per above steps. SCTLR_EL1 get restored back with actual settings at step 7.
Similar flow is present for “el2_sysregs_context_restore” to restore SCTLR_EL1 register.
In conclusion, this routine temporarily clear M bit of SCTLR_EL1 to avoid speculation but restored it back
to its original setting while leaving back to its caller. Please let us know whether this align with KVM
workaround for speculative AT erratum.
Thanks
Manish Badarkhe
On 01/07/2020, 17:02, "TF-A on behalf of Joanna Farley via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote:
Thanks Andrew for the notification. We will look into it and get back to this thread.
Thanks
Joanna
On 01/07/2020, 10:16, "TF-A on behalf of Andrew Scull via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote:
Whilst looking at the speculative AT workaround in KVM, I compared it
against the workaround in TF-A and noticed an inconsistency whereby TF-A
**breaks** KVM's workaround.
In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared
however Linux requires this to be set for its workaround to be correct.
If an exception is taken to EL3 partway through a VM context switch,
e.g. a secure interrupt, causing a switch to the secure world, TF-A will
reintroduce the possibility of TLB corruption.
The above explains how it is broken for Linux's chosen workaround
however TF-A will also have to be compatible with whatever workaround
the EL2 software is using.
Starting this thread with the issue identified and we can add more
details as needed.
Thanks
Andrew
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
+Updated comment for SCTLR_EL2 register restoration.
On 01/07/2020, 17:45, "TF-A on behalf of Manish Badarkhe via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote:
Hi Andrew,
As per current implementation, in “el1_sysregs_context_restore” routine do below things:
1. TCR_EL1.EPD0 = 1
2. TCR_EL1.EPD1 = 1
3. SCTR_EL1.M = 0
4. Isb
Code snippet:
mrs x9, tcr_el1
orr x9, x9, #TCR_EPD0_BIT
orr x9, x9, #TCR_EPD1_BIT
msr tcr_el1, x9
mrs x9, sctlr_el1
bic x9, x9, #SCTLR_M_BIT
msr sctlr_el1, x9
isb
This is to avoid PTW through while updating system registers at step 5
5. Restore all system registers for El1 except SCTLR_EL1 and TCR_EL1
6. isb()
7. restore SCTLR_EL1 and TCR_EL1
Code Snippet:
ldr x9, [x0, #CTX_SCTLR_EL1] -> saved value from "el2_sysregs_context_save"
msr sctlr_el1, x9
ldr x9, [x0, #CTX_TCR_EL1]
msr tcr_el1, x9
As per above steps. SCTLR_EL1 get restored back with actual settings at step 7.
Similar flow is present for “el2_sysregs_context_restore” to restore SCTLR_EL2 register.
In conclusion, this routine temporarily clear M bit of SCTLR_EL1 to avoid speculation but restored it back
to its original setting while leaving back to its caller. Please let us know whether this align with KVM
workaround for speculative AT erratum.
Thanks
Manish Badarkhe
On 01/07/2020, 17:02, "TF-A on behalf of Joanna Farley via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote:
Thanks Andrew for the notification. We will look into it and get back to this thread.
Thanks
Joanna
On 01/07/2020, 10:16, "TF-A on behalf of Andrew Scull via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote:
Whilst looking at the speculative AT workaround in KVM, I compared it
against the workaround in TF-A and noticed an inconsistency whereby TF-A
**breaks** KVM's workaround.
In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared
however Linux requires this to be set for its workaround to be correct.
If an exception is taken to EL3 partway through a VM context switch,
e.g. a secure interrupt, causing a switch to the secure world, TF-A will
reintroduce the possibility of TLB corruption.
The above explains how it is broken for Linux's chosen workaround
however TF-A will also have to be compatible with whatever workaround
the EL2 software is using.
Starting this thread with the issue identified and we can add more
details as needed.
Thanks
Andrew
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
Thanks Andrew for the notification. We will look into it and get back to this thread.
Thanks
Joanna
On 01/07/2020, 10:16, "TF-A on behalf of Andrew Scull via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote:
Whilst looking at the speculative AT workaround in KVM, I compared it
against the workaround in TF-A and noticed an inconsistency whereby TF-A
**breaks** KVM's workaround.
In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared
however Linux requires this to be set for its workaround to be correct.
If an exception is taken to EL3 partway through a VM context switch,
e.g. a secure interrupt, causing a switch to the secure world, TF-A will
reintroduce the possibility of TLB corruption.
The above explains how it is broken for Linux's chosen workaround
however TF-A will also have to be compatible with whatever workaround
the EL2 software is using.
Starting this thread with the issue identified and we can add more
details as needed.
Thanks
Andrew
--
TF-A mailing list
TF-A(a)lists.trustedfirmware.org
https://lists.trustedfirmware.org/mailman/listinfo/tf-a
Whilst looking at the speculative AT workaround in KVM, I compared it
against the workaround in TF-A and noticed an inconsistency whereby TF-A
**breaks** KVM's workaround.
In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared
however Linux requires this to be set for its workaround to be correct.
If an exception is taken to EL3 partway through a VM context switch,
e.g. a secure interrupt, causing a switch to the secure world, TF-A will
reintroduce the possibility of TLB corruption.
The above explains how it is broken for Linux's chosen workaround
however TF-A will also have to be compatible with whatever workaround
the EL2 software is using.
Starting this thread with the issue identified and we can add more
details as needed.
Thanks
Andrew
Hi Sandrine,
Sounds like good changes in general! I'm curious what the ACLs are for
Code-Owner-Review? Is it tied to docs/about/maintainers.rst or just
based on the honor system? (I notice that I seem to be able to give a
+1 for code I'm not owning, but maybe that's because I am a
maintainer?) Also, are code owners allowed to +1 themselves (I think
we said we didn't want maintainers to do that, but for code owners I
could see how we might want to allow it since there are usually not
that many)? What do we do when someone uploads the first patch for a
new platform, do they just COR+1 themselves (since there is no code
owner yet)?
I think it might still be useful to retain the existing Code-Review as
a +1/-1 label next to the two new ones, just to allow other interested
parties to record their opinion (without it having any enforcing
effect on the merge). In other Gerrit instances I have used people
often like to give CR+1 as a "I'm not the one who needs to approve
this but I have looked at it and think it's fine" or a CR-1 as a "I
can't stop you from doing this but I still want to be clear that I
don't think it's a good idea". It allows people outside the official
approval process a clearer way to participate and can aid the official
approvers in their decisions (e.g. when I am reviewing a patch as a
maintainer that already has a CR-1 from someone else I know to pay
extra attention to their concerns, and it's more visible than just
some random comment further up in the list). What do you think?
Best regards,
Julius
