diff mbox

target-arm: Break the TB after IC invalidation to execute self-modified code correctly

Message ID 1440589001-15997-1-git-send-email-afarallax@yandex.ru
State New
Headers show

Commit Message

Sergey Sorokin Aug. 26, 2015, 11:36 a.m. UTC 
If any store instruction writes the code inside the same TB
after this store insn, the execution of the TB must be stopped
to execute new code correctly.
As described in ARMv8 manual D3.4.6 a self-modified code need to do
IC invalidation to be valid. So it's enough to end the TB
after IC invalidation instruction on the code translation.

Signed-off-by: Sergey Sorokin <afarallax@yandex.ru>
---
 target-arm/helper.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

Comments

Peter Maydell Aug. 27, 2015, 4:54 p.m. UTC | #1 
On 26 August 2015 at 12:36, Sergey Sorokin <afarallax@yandex.ru> wrote:
> If any store instruction writes the code inside the same TB
> after this store insn, the execution of the TB must be stopped
> to execute new code correctly.
> As described in ARMv8 manual D3.4.6 a self-modified code need to do
> IC invalidation to be valid. So it's enough to end the TB
> after IC invalidation instruction on the code translation.

I think it would be better to fix this problem by requiring
that we end the TB on every ISB instruction. We need to do
that anyway, because the v8 ARM ARM D1.14.4 says that we
must take interrupts immediately after an ISB. And if you have
self-modifying code then you'll need to put an ISB between
the store and the execution, so it will deal with your bug too.

thanks
-- PMM
Sergey Sorokin Aug. 28, 2015, 6:55 a.m. UTC | #2 
27.08.2015, 19:54, "Peter Maydell" <peter.maydell@linaro.org>:
> On 26 August 2015 at 12:36, Sergey Sorokin <afarallax@yandex.ru> wrote:
>>  If any store instruction writes the code inside the same TB
>>  after this store insn, the execution of the TB must be stopped
>>  to execute new code correctly.
>>  As described in ARMv8 manual D3.4.6 a self-modified code need to do
>>  IC invalidation to be valid. So it's enough to end the TB
>>  after IC invalidation instruction on the code translation.
>
> I think it would be better to fix this problem by requiring
> that we end the TB on every ISB instruction. We need to do
> that anyway, because the v8 ARM ARM D1.14.4 says that we
> must take interrupts immediately after an ISB. And if you have
> self-modifying code then you'll need to put an ISB between
> the store and the execution, so it will deal with your bug too.
>
> thanks
> -- PMM

Such was the first internal version of the patch, but I altered it before the sending :) Ok, I'll bring back this solution.
diff mbox

Patch

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 7df1f06..5289325 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2844,16 +2844,23 @@  static const ARMCPRegInfo v8_cp_reginfo[] = {
     { .name = "CURRENTEL", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 0, .opc2 = 2, .crn = 4, .crm = 2,
       .access = PL1_R, .type = ARM_CP_CURRENTEL },
-    /* Cache ops: all NOPs since we don't emulate caches */
+    /* Cache ops: all NOPs since we don't emulate caches.
+     * We need to break the TB after IC invalidation to
+     * execute a self-modified code correctly.
+     * So use arm_cp_write_ignore() function instead of ARM_CP_NOP flag.
+     */
     { .name = "IC_IALLUIS", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 1, .opc2 = 0,
-      .access = PL1_W, .type = ARM_CP_NOP },
+      .access = PL1_W, .type = ARM_CP_NO_RAW,
+      .writefn = arm_cp_write_ignore },
     { .name = "IC_IALLU", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 5, .opc2 = 0,
-      .access = PL1_W, .type = ARM_CP_NOP },
+      .access = PL1_W, .type = ARM_CP_NO_RAW,
+      .writefn = arm_cp_write_ignore },
     { .name = "IC_IVAU", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 3, .crn = 7, .crm = 5, .opc2 = 1,
-      .access = PL0_W, .type = ARM_CP_NOP,
+      .access = PL0_W, .type = ARM_CP_NO_RAW,
+      .writefn = arm_cp_write_ignore,
       .accessfn = aa64_cacheop_access },
     { .name = "DC_IVAC", .state = ARM_CP_STATE_AA64,
       .opc0 = 1, .opc1 = 0, .crn = 7, .crm = 6, .opc2 = 1,
@@ -3005,14 +3012,18 @@  static const ARMCPRegInfo v8_cp_reginfo[] = {
     { .name = "TLBIMVAAL", .cp = 15, .opc1 = 0, .crn = 8, .crm = 7, .opc2 = 7,
       .type = ARM_CP_NO_RAW, .access = PL1_W, .writefn = tlbimvaa_write },
     /* 32 bit cache operations */
+    /* We need to break the TB after IC invalidation to
+     * execute a self-modified code correctly.
+     * So use arm_cp_write_ignore() function instead of ARM_CP_NOP flag.
+     */
     { .name = "ICIALLUIS", .cp = 15, .opc1 = 0, .crn = 7, .crm = 1, .opc2 = 0,
-      .type = ARM_CP_NOP, .access = PL1_W },
+      .type = ARM_CP_NO_RAW, .access = PL1_W, .writefn = arm_cp_write_ignore },
     { .name = "BPIALLUIS", .cp = 15, .opc1 = 0, .crn = 7, .crm = 1, .opc2 = 6,
       .type = ARM_CP_NOP, .access = PL1_W },
     { .name = "ICIALLU", .cp = 15, .opc1 = 0, .crn = 7, .crm = 5, .opc2 = 0,
-      .type = ARM_CP_NOP, .access = PL1_W },
+      .type = ARM_CP_NO_RAW, .access = PL1_W, .writefn = arm_cp_write_ignore },
     { .name = "ICIMVAU", .cp = 15, .opc1 = 0, .crn = 7, .crm = 5, .opc2 = 1,
-      .type = ARM_CP_NOP, .access = PL1_W },
+      .type = ARM_CP_NO_RAW, .access = PL1_W, .writefn = arm_cp_write_ignore },
     { .name = "BPIALL", .cp = 15, .opc1 = 0, .crn = 7, .crm = 5, .opc2 = 6,
       .type = ARM_CP_NOP, .access = PL1_W },
     { .name = "BPIMVA", .cp = 15, .opc1 = 0, .crn = 7, .crm = 5, .opc2 = 7,