| Message ID | 20210224093738.3629662-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Assorted MMU-on fixes | expand |
Hi Marc, On Wed, Feb 24, 2021 at 09:37:37AM +0000, Marc Zyngier wrote: > Although there has been a bit of back and forth on the subject, > it appears that invalidating TLBs requires an ISB instruction > after the TLBI/DSB sequence, as documented in d0b7a302d58a > ("Revert "arm64: Remove unnecessary ISBs from set_{pte,pmd,pud}""). That commit describes a different scenario (going faulting->valid without TLB maintenance), and I don't think that implies anything about the behaviour in the presence of a TLBI, which is quite different. Howerver, I do see that commits: 7f0b1bf045113489 ("arm64: Fix barriers used for page table modifications") 51696d346c49c6cf ("arm64: tlb: Ensure we execute an ISB following walk cache invalidation") ... assume that we need an ISB after a TLBI+DSB, so I think it would be better to refer to those, to avoid conflating the distinct cases. > Add the missing ISB in __primary_switch, just in case. > > Fixes: 3c5e9f238bc4 ("arm64: head.S: move KASLR processing out of __enable_mmu()") > Suggested-by: Will Deacon <will@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> For consistency with the other kernel TLBI paths, I'm fine with this (assuming we update the commit message accordingly): Acked-by: Mark Rutland <mark.rutland@arm.com> My understanding is that we don't need an ISB after invalidation, and if we align on that understanding we can follow up and update all of the TLBI paths in one go. Thanks, Mark. > --- > arch/arm64/kernel/head.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > index 1e30b5550d2a..66b0e0b66e31 100644 > --- a/arch/arm64/kernel/head.S > +++ b/arch/arm64/kernel/head.S > @@ -837,6 +837,7 @@ SYM_FUNC_START_LOCAL(__primary_switch) > > tlbi vmalle1 // Remove any stale TLB entries > dsb nsh > + isb
On Wed, Feb 24, 2021 at 11:06:43AM +0000, Mark Rutland wrote: > On Wed, Feb 24, 2021 at 09:37:37AM +0000, Marc Zyngier wrote: > > Although there has been a bit of back and forth on the subject, > > it appears that invalidating TLBs requires an ISB instruction > > after the TLBI/DSB sequence, as documented in d0b7a302d58a > > ("Revert "arm64: Remove unnecessary ISBs from set_{pte,pmd,pud}""). > > That commit describes a different scenario (going faulting->valid > without TLB maintenance), and I don't think that implies anything about > the behaviour in the presence of a TLBI, which is quite different. Sorry, that was my fault. I remember this all happened around the same time. > Howerver, I do see that commits: > > 7f0b1bf045113489 ("arm64: Fix barriers used for page table modifications") > 51696d346c49c6cf ("arm64: tlb: Ensure we execute an ISB following walk cache invalidation") > > ... assume that we need an ISB after a TLBI+DSB, so I think it would be > better to refer to those, to avoid conflating the distinct cases. > > > Add the missing ISB in __primary_switch, just in case. > > > > Fixes: 3c5e9f238bc4 ("arm64: head.S: move KASLR processing out of __enable_mmu()") > > Suggested-by: Will Deacon <will@kernel.org> > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > For consistency with the other kernel TLBI paths, I'm fine with this > (assuming we update the commit message accordingly): > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > My understanding is that we don't need an ISB after invalidation, and if > we align on that understanding we can follow up and update all of the > TLBI paths in one go. Without FEAT_ETS, I think the ISB is required to complete TLBI: | In an implementation that does not implement FEAT_ETS, a TLB maintenance | instruction executed by a PE, PEx, can complete at any time after it is | issued, but is only guaranteed to be finished for a PE, PEx, after the | execution of DSB by the PEx followed by a Context synchronization event. Will
On Wed, Feb 24, 2021 at 11:16:50AM +0000, Will Deacon wrote: > On Wed, Feb 24, 2021 at 11:06:43AM +0000, Mark Rutland wrote: > > On Wed, Feb 24, 2021 at 09:37:37AM +0000, Marc Zyngier wrote: > > > Although there has been a bit of back and forth on the subject, > > > it appears that invalidating TLBs requires an ISB instruction > > > after the TLBI/DSB sequence, as documented in d0b7a302d58a > > > ("Revert "arm64: Remove unnecessary ISBs from set_{pte,pmd,pud}""). > > > > That commit describes a different scenario (going faulting->valid > > without TLB maintenance), and I don't think that implies anything about > > the behaviour in the presence of a TLBI, which is quite different. > > Sorry, that was my fault. I remember this all happened around the same > time. > > > Howerver, I do see that commits: > > > > 7f0b1bf045113489 ("arm64: Fix barriers used for page table modifications") > > 51696d346c49c6cf ("arm64: tlb: Ensure we execute an ISB following walk cache invalidation") > > > > ... assume that we need an ISB after a TLBI+DSB, so I think it would be > > better to refer to those, to avoid conflating the distinct cases. > > > > > Add the missing ISB in __primary_switch, just in case. > > > > > > Fixes: 3c5e9f238bc4 ("arm64: head.S: move KASLR processing out of __enable_mmu()") > > > Suggested-by: Will Deacon <will@kernel.org> > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > > For consistency with the other kernel TLBI paths, I'm fine with this > > (assuming we update the commit message accordingly): > > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > > > My understanding is that we don't need an ISB after invalidation, and if > > we align on that understanding we can follow up and update all of the > > TLBI paths in one go. > > Without FEAT_ETS, I think the ISB is required to complete TLBI: > > | In an implementation that does not implement FEAT_ETS, a TLB maintenance > | instruction executed by a PE, PEx, can complete at any time after it is > | issued, but is only guaranteed to be finished for a PE, PEx, after the > | execution of DSB by the PEx followed by a Context synchronization event. Ah; given that quote, I agree. Can we put that in the commit message? I think that's clearer than referring to any commit, and with that my ack definitely stands. I /hope/ that quote means the same PEx in both cases (i.e. only the issuing PE needs the context synchronization event), or we have much greater problems! Thanks, Mark.
On Wed, Feb 24, 2021 at 11:45:40AM +0000, Mark Rutland wrote: > On Wed, Feb 24, 2021 at 11:16:50AM +0000, Will Deacon wrote: > > On Wed, Feb 24, 2021 at 11:06:43AM +0000, Mark Rutland wrote: > > > On Wed, Feb 24, 2021 at 09:37:37AM +0000, Marc Zyngier wrote: > > > > Although there has been a bit of back and forth on the subject, > > > > it appears that invalidating TLBs requires an ISB instruction > > > > after the TLBI/DSB sequence, as documented in d0b7a302d58a > > > > ("Revert "arm64: Remove unnecessary ISBs from set_{pte,pmd,pud}""). > > > > > > That commit describes a different scenario (going faulting->valid > > > without TLB maintenance), and I don't think that implies anything about > > > the behaviour in the presence of a TLBI, which is quite different. > > > > Sorry, that was my fault. I remember this all happened around the same > > time. > > > > > Howerver, I do see that commits: > > > > > > 7f0b1bf045113489 ("arm64: Fix barriers used for page table modifications") > > > 51696d346c49c6cf ("arm64: tlb: Ensure we execute an ISB following walk cache invalidation") > > > > > > ... assume that we need an ISB after a TLBI+DSB, so I think it would be > > > better to refer to those, to avoid conflating the distinct cases. > > > > > > > Add the missing ISB in __primary_switch, just in case. > > > > > > > > Fixes: 3c5e9f238bc4 ("arm64: head.S: move KASLR processing out of __enable_mmu()") > > > > Suggested-by: Will Deacon <will@kernel.org> > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > > > > For consistency with the other kernel TLBI paths, I'm fine with this > > > (assuming we update the commit message accordingly): > > > > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > > > > > My understanding is that we don't need an ISB after invalidation, and if > > > we align on that understanding we can follow up and update all of the > > > TLBI paths in one go. > > > > Without FEAT_ETS, I think the ISB is required to complete TLBI: > > > > | In an implementation that does not implement FEAT_ETS, a TLB maintenance > > | instruction executed by a PE, PEx, can complete at any time after it is > > | issued, but is only guaranteed to be finished for a PE, PEx, after the > > | execution of DSB by the PEx followed by a Context synchronization event. > > Ah; given that quote, I agree. > > Can we put that in the commit message? I think that's clearer than > referring to any commit, and with that my ack definitely stands. Done! > I /hope/ that quote means the same PEx in both cases (i.e. only the > issuing PE needs the context synchronization event), or we have much > greater problems! Yeah, that's definitely what it's supposed to mean (I remember working on this), but the wording ain't great! Other PEs are covered in a separate statement: | A TLB maintenance instruction executed by a PE, PEx, can complete at any time | after it is issued, but is only guaranteed to be finished for a PE other than | PEx after the execution of DSB by the PEx. which would make for an interesting tutorial exercise for somebody studying temporal logic. Will
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 1e30b5550d2a..66b0e0b66e31 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -837,6 +837,7 @@ SYM_FUNC_START_LOCAL(__primary_switch) tlbi vmalle1 // Remove any stale TLB entries dsb nsh + isb set_sctlr_el1 x19 // re-enable the MMU
Although there has been a bit of back and forth on the subject, it appears that invalidating TLBs requires an ISB instruction after the TLBI/DSB sequence, as documented in d0b7a302d58a ("Revert "arm64: Remove unnecessary ISBs from set_{pte,pmd,pud}""). Add the missing ISB in __primary_switch, just in case. Fixes: 3c5e9f238bc4 ("arm64: head.S: move KASLR processing out of __enable_mmu()") Suggested-by: Will Deacon <will@kernel.org> Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kernel/head.S | 1 + 1 file changed, 1 insertion(+)