[ORLinux] [PATCH 1/1] openrisc: update DTLB-miss handler last

[Jonas Bonn]

The self-modifying code that updates the TLB handler at start-up has
a subtle ordering requirement: the DTLB handler must be the last thing
changed.

What I was seeing was the following:

i) The DTLB handler was updated
ii) The following printk caused a TLB miss and the look-up resulted
in the page containing itlb_vector (0xc0000a00) being bounced from
the TLB.
iii) The subsequent access to itlb_vector caused a TLB miss and reload
of the page containing itlb_vector from the page tables.
iv) But this reload of the page in iii) was being done by the "new"
DTLB-miss handler which resulted (correctly) in the page flags being
set to read-only; the subsequent write-access to itlb_vector thus
resulted in a page (access) fault.

This is easily remedied if we ensure that the boot-time DTLB-miss handler
continues running until the very last bit of self-modifying code has been
executed. This patch should ensure that the very last thing updated is the
DTLB-handler itself.

Signed-off-by: Jonas Bonn <jonas at southpole.se>
---
arch/openrisc/mm/init.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/openrisc/mm/init.c b/arch/openrisc/mm/init.c
index 79dea97..e7fdc50 100644
--- a/arch/openrisc/mm/init.c
+++ b/arch/openrisc/mm/init.c
@@ -167,15 +167,26 @@ void __init paging_init(void)
unsigned long *dtlb_vector = __va(0x900);
unsigned long *itlb_vector = __va(0xa00);

+ printk(KERN_INFO "itlb_miss_handler %p\n", &itlb_miss_handler);
+ *itlb_vector = ((unsigned long)&itlb_miss_handler -
+ (unsigned long)itlb_vector) >> 2;
+
+ /* Soft ordering constraint to ensure that dtlb_vector is
+ * the last thing updated
+ */
+ barrier();
+
printk(KERN_INFO "dtlb_miss_handler %p\n", &dtlb_miss_handler);
*dtlb_vector = ((unsigned long)&dtlb_miss_handler -
(unsigned long)dtlb_vector) >> 2;

- printk(KERN_INFO "itlb_miss_handler %p\n", &itlb_miss_handler);
- *itlb_vector = ((unsigned long)&itlb_miss_handler -
- (unsigned long)itlb_vector) >> 2;
}

+ /* Soft ordering constraint to ensure that cache invalidation and
+ * TLB flush really happen _after_ code has been modified.
+ */
+ barrier();
+
/* Invalidate instruction caches after code modification */
mtspr(SPR_ICBIR, 0x900);
mtspr(SPR_ICBIR, 0xa00);

[Jonas Bonn]

Thanks for the patch of the self modifying code...

Hi Sebastian,

Have you seen this bug, too? Did this patch fix it for you? If yes,
could you reply to the original patch with a line like this:

Tested-by: Your Name <youremail at example.org>

The reason I ask is that this bug is really subtle and quite difficult
to hit. It requires a very specific combination of config options and
compiler so that the code ends up in memory in such a way that this TLB
bounce occurs... I've only hit this bug a very few times and was lucky
to hit it the other day so that I could debug it. Therefore, it would
be good to know that somebody else has verified that this patch fixes a
problem that they have also seen.

Qutoing from: http://www.kernel.org/doc/Documentation/SubmittingPatches

"A Tested-by: tag indicates that the patch has been successfully tested (in
some environment) by the person named. This tag informs maintainers that
some testing has been performed, provides a means to locate testers for
future patches, and ensures credit for the testers."


Thanks,
Jonas

Post by Jonas Bonn
The self-modifying code that updates the TLB handler at start-up has
a subtle ordering requirement: the DTLB handler must be the last thing
changed.
i) The DTLB handler was updated
ii) The following printk caused a TLB miss and the look-up resulted
in the page containing itlb_vector (0xc0000a00) being bounced from
the TLB.
iii) The subsequent access to itlb_vector caused a TLB miss and reload
of the page containing itlb_vector from the page tables.
iv) But this reload of the page in iii) was being done by the "new"
DTLB-miss handler which resulted (correctly) in the page flags being
set to read-only; the subsequent write-access to itlb_vector thus
resulted in a page (access) fault.
This is easily remedied if we ensure that the boot-time DTLB-miss handler
continues running until the very last bit of self-modifying code has been
executed. This patch should ensure that the very last thing updated is the
DTLB-handler itself.
Signed-off-by: Jonas Bonn <jonas at southpole.se>
---
arch/openrisc/mm/init.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/openrisc/mm/init.c b/arch/openrisc/mm/init.c
index 79dea97..e7fdc50 100644
--- a/arch/openrisc/mm/init.c
+++ b/arch/openrisc/mm/init.c
@@ -167,15 +167,26 @@ void __init paging_init(void)
unsigned long *dtlb_vector = __va(0x900);
unsigned long *itlb_vector = __va(0xa00);
+ printk(KERN_INFO "itlb_miss_handler %p\n",
&itlb_miss_handler);
+ *itlb_vector = ((unsigned long)&itlb_miss_handler -
+ (unsigned long)itlb_vector) >> 2;
+
+ /* Soft ordering constraint to ensure that dtlb_vector is
+ * the last thing updated
+ */
+ barrier();
+
printk(KERN_INFO "dtlb_miss_handler %p\n",
&dtlb_miss_handler);
*dtlb_vector = ((unsigned long)&dtlb_miss_handler -
(unsigned long)dtlb_vector) >> 2;
- printk(KERN_INFO "itlb_miss_handler %p\n",
&itlb_miss_handler);
- *itlb_vector = ((unsigned long)&itlb_miss_handler -
- (unsigned long)itlb_vector) >> 2;
}
+ /* Soft ordering constraint to ensure that cache invalidation and
+ * TLB flush really happen _after_ code has been modified.
+ */
+ barrier();
+
/* Invalidate instruction caches after code modification */
mtspr(SPR_ICBIR, 0x900);
mtspr(SPR_ICBIR, 0xa00);

[Julius Baxter]

Post by Jonas Bonn
The self-modifying code that updates the TLB handler at start-up has
a subtle ordering requirement: the DTLB handler must be the last thing
changed.
i) The DTLB handler was updated
ii) The following printk caused a TLB miss and the look-up resulted
in the page containing itlb_vector (0xc0000a00) being bounced from
the TLB.
iii) The subsequent access to itlb_vector caused a TLB miss and reload
of the page containing itlb_vector from the page tables.
iv) But this reload of the page in iii) was being done by the "new"
DTLB-miss handler which resulted (correctly) in the page flags being
set to read-only; the subsequent write-access to itlb_vector thus
resulted in a page (access) fault.
This is easily remedied if we ensure that the boot-time DTLB-miss handler
continues running until the very last bit of self-modifying code has been
executed. This patch should ensure that the very last thing updated is the
DTLB-handler itself.
Signed-off-by: Jonas Bonn <jonas at southpole.se>
---
arch/openrisc/mm/init.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/openrisc/mm/init.c b/arch/openrisc/mm/init.c
index 79dea97..e7fdc50 100644
--- a/arch/openrisc/mm/init.c
+++ b/arch/openrisc/mm/init.c
@@ -167,15 +167,26 @@ void __init paging_init(void)
unsigned long *dtlb_vector = __va(0x900);
unsigned long *itlb_vector = __va(0xa00);
+ printk(KERN_INFO "itlb_miss_handler %p\n", &itlb_miss_handler);
+ *itlb_vector = ((unsigned long)&itlb_miss_handler -
+ (unsigned long)itlb_vector) >> 2;
+
+ /* Soft ordering constraint to ensure that dtlb_vector is
+ * the last thing updated
+ */
+ barrier();
+
printk(KERN_INFO "dtlb_miss_handler %p\n", &dtlb_miss_handler);
*dtlb_vector = ((unsigned long)&dtlb_miss_handler -
(unsigned long)dtlb_vector) >> 2;
- printk(KERN_INFO "itlb_miss_handler %p\n", &itlb_miss_handler);
- *itlb_vector = ((unsigned long)&itlb_miss_handler -
- (unsigned long)itlb_vector) >> 2;
}
+ /* Soft ordering constraint to ensure that cache invalidation and
+ * TLB flush really happen _after_ code has been modified.
+ */
+ barrier();
+
/* Invalidate instruction caches after code modification */
mtspr(SPR_ICBIR, 0x900);
mtspr(SPR_ICBIR, 0xa00);
--
1.8.1.2
_______________________________________________
Linux mailing list
Linux at lists.openrisc.net
http://lists.openrisc.net/listinfo/linux

Acked-by: Julius Baxter <juliusbaxter at gmail.com>

[Sebastian Macke]

Tested-by: Sebastian Macke <sebastian at macke.de>

By flushing the tlb before and after the printk commands I could force
the bug in the old version. Testing it with the new patch no kernel
panic occurs.

Post by Jonas Bonn
The self-modifying code that updates the TLB handler at start-up has
a subtle ordering requirement: the DTLB handler must be the last thing
changed.
i) The DTLB handler was updated
ii) The following printk caused a TLB miss and the look-up resulted
in the page containing itlb_vector (0xc0000a00) being bounced from
the TLB.
iii) The subsequent access to itlb_vector caused a TLB miss and reload
of the page containing itlb_vector from the page tables.
iv) But this reload of the page in iii) was being done by the "new"
DTLB-miss handler which resulted (correctly) in the page flags being
set to read-only; the subsequent write-access to itlb_vector thus
resulted in a page (access) fault.
This is easily remedied if we ensure that the boot-time DTLB-miss handler
continues running until the very last bit of self-modifying code has been
executed. This patch should ensure that the very last thing updated is the
DTLB-handler itself.
Signed-off-by: Jonas Bonn <jonas at southpole.se>
---
arch/openrisc/mm/init.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/openrisc/mm/init.c b/arch/openrisc/mm/init.c
index 79dea97..e7fdc50 100644
--- a/arch/openrisc/mm/init.c
+++ b/arch/openrisc/mm/init.c
@@ -167,15 +167,26 @@ void __init paging_init(void)
unsigned long *dtlb_vector = __va(0x900);
unsigned long *itlb_vector = __va(0xa00);
+ printk(KERN_INFO "itlb_miss_handler %p\n", &itlb_miss_handler);
+ *itlb_vector = ((unsigned long)&itlb_miss_handler -
+ (unsigned long)itlb_vector) >> 2;
+
+ /* Soft ordering constraint to ensure that dtlb_vector is
+ * the last thing updated
+ */
+ barrier();
+
printk(KERN_INFO "dtlb_miss_handler %p\n", &dtlb_miss_handler);
*dtlb_vector = ((unsigned long)&dtlb_miss_handler -
(unsigned long)dtlb_vector) >> 2;
- printk(KERN_INFO "itlb_miss_handler %p\n", &itlb_miss_handler);
- *itlb_vector = ((unsigned long)&itlb_miss_handler -
- (unsigned long)itlb_vector) >> 2;
}
+ /* Soft ordering constraint to ensure that cache invalidation and
+ * TLB flush really happen _after_ code has been modified.
+ */
+ barrier();
+
/* Invalidate instruction caches after code modification */
mtspr(SPR_ICBIR, 0x900);
mtspr(SPR_ICBIR, 0xa00);