Skip to contentSkip to conversation

efi-entry.S: add dsb and isb

PATCH New Joel Schopp 3 participants 5 messages Nov 11, 2014 – Nov 12, 2014 arm64

lore
Share & tools
lore

Joel Schopp

author
#0 lore raw

PATCH 1/1 efi-entry.S: add dsb and isb

Add a dsb and isb after the instruction flush before the data cache and mm offing. Without this patch I am seeing synchronous exceptions occur every few boots.

Signed-off-by Joel Schopp <[email protected]>
Tested-by Tom Lendacky <[email protected]>
---
 arch/arm64/kernel/efi-entry.S |    6 ++++++
 1 file changed, 6 insertions(+)
arch/arm64/kernel/efi-entry.S +6 -0
index 619b1dd..7d95eda 100644
@@ -76,6 +76,12 @@ ENTRY(efi_stub_entry)
 	bl	__flush_dcache_area
 	ic	ialluis
 
+	/* We need to sync again after the instruction cache sync
+	 * and before turning off the dcache and mmu
+	 */
+	dsb	sy
+	isb
+
 	/* Turn off Dcache and MMU */
 	mrs	x0, CurrentEL
 	cmp	x0, #CurrentEL_EL2
reviewer
#1 lore raw

PATCH 1/1 efi-entry.S: add dsb and isb

Hi Joel,

I've Cc'd a few people who were involved in authoring this.

I have an alternative patch [1] that also adds some missing maintenance. Would you be able to give that a go?

Quoted 15 lines · Joel Schopp

Add a dsb and isb after the instruction flush before the data cache and mm offing. Without this patch I am seeing synchronous exceptions occur every few boots.

Signed-off-by Joel Schopp <[email protected]>
Tested-by Tom Lendacky <[email protected]>
---
 arch/arm64/kernel/efi-entry.S |    6 ++++++
 1 file changed, 6 insertions(+)
arch/arm64/kernel/efi-entry.S +0 -0
index 619b1dd..7d95eda 100644

arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 83

Quoted 8 lines
arch/arm64/kernel/efi-entry.S +5 -0
@@ -76,6 +76,12 @@ ENTRY(efi_stub_entry)
 	bl	__flush_dcache_area
 	ic	ialluis
 
+	/* We need to sync again after the instruction cache sync
+	 * and before turning off the dcache and mmu
+	 */
+	dsb	sy
+	isb

In my series I reasoned that it wasn't necessary to have an ISB before we disabled the MMU. The current image must already be visible to the I-cache, so the I-cache can't have stale entries for it. We don't disable the MMU until after the image is visible at the PoC, so we shouldn't break the visbility of the current image to the I-cache.

There is a bug in that We don't flush the current image in case of relocation, but I don't see how the ISB would help there.

Thanks,
Mark.

Quoted 11 lines
arch/arm64/kernel/efi-entry.S +1 -0
+
 	/* Turn off Dcache and MMU */
 	mrs	x0, CurrentEL
 	cmp	x0, #CurrentEL_EL2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/301779.html

#2 lore raw

PATCH 1/1 efi-entry.S: add dsb and isb

Quoted 6 lines · Mark Rutland

Hi Joel,

I've Cc'd a few people who were involved in authoring this.

I have an alternative patch [1] that also adds some missing maintenance. Would you be able to give that a go?

I applied the referenced patch and have booted about two dozen times without seeing a synchronous exception. Prior to either patch I would see a synchronous exception about every three or four (re)boots.

Thanks,
Tom

Quoted 52 lines · Joel Schopp
Quoted 15 lines

Add a dsb and isb after the instruction flush before the data cache and mm offing. Without this patch I am seeing synchronous exceptions occur every few boots.

Signed-off-by Joel Schopp <[email protected]>
Tested-by Tom Lendacky <[email protected]>
---
  arch/arm64/kernel/efi-entry.S |    6 ++++++
  1 file changed, 6 insertions(+)
arch/arm64/kernel/efi-entry.S +0 -0
index 619b1dd..7d95eda 100644

arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 83

Quoted 8 lines
arch/arm64/kernel/efi-entry.S +5 -0
@@ -76,6 +76,12 @@ ENTRY(efi_stub_entry)
  	bl	__flush_dcache_area
  	ic	ialluis
 
+	/* We need to sync again after the instruction cache sync
+	 * and before turning off the dcache and mmu
+	 */
+	dsb	sy
+	isb

In my series I reasoned that it wasn't necessary to have an ISB before we disabled the MMU. The current image must already be visible to the I-cache, so the I-cache can't have stale entries for it. We don't disable the MMU until after the image is visible at the PoC, so we shouldn't break the visbility of the current image to the I-cache.

There is a bug in that We don't flush the current image in case of relocation, but I don't see how the ISB would help there.

Thanks,
Mark.

Quoted 11 lines
arch/arm64/kernel/efi-entry.S +1 -0
+
  	/* Turn off Dcache and MMU */
  	mrs	x0, CurrentEL
  	cmp	x0, #CurrentEL_EL2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/301779.html

reviewer
#3 lore raw

PATCH 1/1 efi-entry.S: add dsb and isb

Hi Tom,

Quoted 11 lines · Tom Lendacky
Quoted 6 lines · Mark Rutland

Hi Joel,

I've Cc'd a few people who were involved in authoring this.

I have an alternative patch [1] that also adds some missing maintenance. Would you be able to give that a go?

I applied the referenced patch and have booted about two dozen times without seeing a synchronous exception. Prior to either patch I would see a synchronous exception about every three or four (re)boots.

Thank you for testing.

May I add your Tested-by?

Cheers,
Mark.

Quoted 56 lines

Thanks,
Tom

Quoted 52 lines · Joel Schopp
Quoted 15 lines

Add a dsb and isb after the instruction flush before the data cache and mm offing. Without this patch I am seeing synchronous exceptions occur every few boots.

Signed-off-by Joel Schopp <[email protected]>
Tested-by Tom Lendacky <[email protected]>
---
  arch/arm64/kernel/efi-entry.S |    6 ++++++
  1 file changed, 6 insertions(+)
arch/arm64/kernel/efi-entry.S +0 -0
index 619b1dd..7d95eda 100644

arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 83

Quoted 8 lines
arch/arm64/kernel/efi-entry.S +5 -0
@@ -76,6 +76,12 @@ ENTRY(efi_stub_entry)
  	bl	__flush_dcache_area
  	ic	ialluis
 
+	/* We need to sync again after the instruction cache sync
+	 * and before turning off the dcache and mmu
+	 */
+	dsb	sy
+	isb

In my series I reasoned that it wasn't necessary to have an ISB before we disabled the MMU. The current image must already be visible to the I-cache, so the I-cache can't have stale entries for it. We don't disable the MMU until after the image is visible at the PoC, so we shouldn't break the visbility of the current image to the I-cache.

There is a bug in that We don't flush the current image in case of relocation, but I don't see how the ISB would help there.

Thanks,
Mark.

Quoted 11 lines
arch/arm64/kernel/efi-entry.S +1 -0
+
  	/* Turn off Dcache and MMU */
  	mrs	x0, CurrentEL
  	cmp	x0, #CurrentEL_EL2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/301779.html

#4 lore raw

PATCH 1/1 efi-entry.S: add dsb and isb

Quoted 11 lines · Mark Rutland

Hi Tom,

Quoted 8 lines · Tom Lendacky
Quoted 6 lines · Mark Rutland

Hi Joel,

I've Cc'd a few people who were involved in authoring this.

I have an alternative patch [1] that also adds some missing maintenance. Would you be able to give that a go?

Quoted 7 lines
Quoted 3 lines

I applied the referenced patch and have booted about two dozen times without seeing a synchronous exception. Prior to either patch I would see a synchronous exception about every three or four (re)boots.

Thank you for testing.

May I add your Tested-by?

Hi Mark,

Certainly.

Tested-by Tom Lendacky <[email protected]>

Thanks,
Tom

Quoted 60 lines

Cheers,
Mark.

Quoted 56 lines

Thanks,
Tom

Quoted 52 lines · Joel Schopp
Quoted 15 lines

Add a dsb and isb after the instruction flush before the data cache and mm offing. Without this patch I am seeing synchronous exceptions occur every few boots.

Signed-off-by Joel Schopp <[email protected]>
Tested-by Tom Lendacky <[email protected]>
---
   arch/arm64/kernel/efi-entry.S |    6 ++++++
   1 file changed, 6 insertions(+)
arch/arm64/kernel/efi-entry.S +0 -0
index 619b1dd..7d95eda 100644

arch/arm64/kernel/efi-entry.S ENTRY(efi_stub_entry) · line 83

Quoted 8 lines
arch/arm64/kernel/efi-entry.S +5 -0
@@ -76,6 +76,12 @@ ENTRY(efi_stub_entry)
   	bl	__flush_dcache_area
   	ic	ialluis
 
+	/* We need to sync again after the instruction cache sync
+	 * and before turning off the dcache and mmu
+	 */
+	dsb	sy
+	isb

In my series I reasoned that it wasn't necessary to have an ISB before we disabled the MMU. The current image must already be visible to the I-cache, so the I-cache can't have stale entries for it. We don't disable the MMU until after the image is visible at the PoC, so we shouldn't break the visbility of the current image to the I-cache.

There is a bug in that We don't flush the current image in case of relocation, but I don't see how the ISB would help there.

Thanks,
Mark.

Quoted 11 lines
arch/arm64/kernel/efi-entry.S +1 -0
+
   	/* Turn off Dcache and MMU */
   	mrs	x0, CurrentEL
   	cmp	x0, #CurrentEL_EL2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/301779.html