diff mbox

[v2] target-s390x: fix LOAD MULTIPLE instruction on page boundary

Message ID 1432631384-470-1-git-send-email-aurelien@aurel32.net
State New
Headers show

Commit Message

Aurelien Jarno May 26, 2015, 9:09 a.m. UTC 
When consecutive memory locations are on page boundary a page fault
might occur when using the LOAD MULTIPLE instruction. In that case real
hardware doesn't load any register.

This is an important detail in case the base register is in the list
of registers to be loaded. If a page fault occurs this register might be
overwritten and when the instruction is later restarted the wrong
base register value is useD.

Fix this by first loading the first and last value from memory, hence
triggering all possible page faults, and then the remaining registers.

This fixes random segmentation faults seen in the guest.

Cc: Alexander Graf <agraf@suse.de>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-s390x/translate.c | 128 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 99 insertions(+), 29 deletions(-)

Changes v1->v2:
- Do the load in two steps: first and last registers, and then the remaining ones

Comments

Richard Henderson May 26, 2015, 4:15 p.m. UTC | #1 
On 05/26/2015 02:09 AM, Aurelien Jarno wrote:
> This is an important detail in case the base register is in the list
> of registers to be loaded. If a page fault occurs this register might be
> overwritten and when the instruction is later restarted the wrong
> base register value is useD.
> 
> Fix this by first loading the first and last value from memory, hence
> triggering all possible page faults, and then the remaining registers.
> 
> This fixes random segmentation faults seen in the guest.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  target-s390x/translate.c | 128 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 99 insertions(+), 29 deletions(-)
> 
> Changes v1->v2:
> - Do the load in two steps: first and last registers, and then the remaining ones

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~
Alexander Graf May 26, 2015, 9:43 p.m. UTC | #2 
On 26.05.15 18:15, Richard Henderson wrote:
> On 05/26/2015 02:09 AM, Aurelien Jarno wrote:
>> This is an important detail in case the base register is in the list
>> of registers to be loaded. If a page fault occurs this register might be
>> overwritten and when the instruction is later restarted the wrong
>> base register value is useD.
>>
>> Fix this by first loading the first and last value from memory, hence
>> triggering all possible page faults, and then the remaining registers.
>>
>> This fixes random segmentation faults seen in the guest.
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>>  target-s390x/translate.c | 128 ++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 99 insertions(+), 29 deletions(-)
>>
>> Changes v1->v2:
>> - Do the load in two steps: first and last registers, and then the remaining ones
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Thanks, applied to s390-next.

So what do we do about the other patch set?


Alex
Aurelien Jarno May 26, 2015, 10:10 p.m. UTC | #3 
On 2015-05-26 23:43, Alexander Graf wrote:
> 
> 
> On 26.05.15 18:15, Richard Henderson wrote:
> > On 05/26/2015 02:09 AM, Aurelien Jarno wrote:
> >> This is an important detail in case the base register is in the list
> >> of registers to be loaded. If a page fault occurs this register might be
> >> overwritten and when the instruction is later restarted the wrong
> >> base register value is useD.
> >>
> >> Fix this by first loading the first and last value from memory, hence
> >> triggering all possible page faults, and then the remaining registers.
> >>
> >> This fixes random segmentation faults seen in the guest.
> >>
> >> Cc: Alexander Graf <agraf@suse.de>
> >> Cc: Richard Henderson <rth@twiddle.net>
> >> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> >> ---
> >>  target-s390x/translate.c | 128 ++++++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 99 insertions(+), 29 deletions(-)
> >>
> >> Changes v1->v2:
> >> - Do the load in two steps: first and last registers, and then the remaining ones
> > 
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> 
> Thanks, applied to s390-next.
> 
> So what do we do about the other patch set?

For the other patch set, it seems the best to ignore the STFL/STFLE
part. Patches 01 to 05 and 09 to 10 are still valid, they are just a
collection of bug fixes and improvement not specially linked together.
diff mbox

Patch

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 1819217..23c78e7 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -2434,21 +2434,45 @@  static ExitStatus op_lm32(DisasContext *s, DisasOps *o)
 {
     int r1 = get_field(s->fields, r1);
     int r3 = get_field(s->fields, r3);
-    TCGv_i64 t = tcg_temp_new_i64();
-    TCGv_i64 t4 = tcg_const_i64(4);
+    TCGv_i64 t1, t2;
 
-    while (1) {
-        tcg_gen_qemu_ld32u(t, o->in2, get_mem_index(s));
-        store_reg32_i64(r1, t);
-        if (r1 == r3) {
-            break;
-        }
-        tcg_gen_add_i64(o->in2, o->in2, t4);
+    /* Only one register to read. */
+    t1 = tcg_temp_new_i64();
+    if (unlikely(r1 == r3)) {
+        tcg_gen_qemu_ld32u(t1, o->in2, get_mem_index(s));
+        store_reg32_i64(r1, t1);
+        tcg_temp_free(t1);
+        return NO_EXIT;
+    }
+
+    /* First load the values of the first and last registers to trigger
+       possible page faults. */
+    t2 = tcg_temp_new_i64();
+    tcg_gen_qemu_ld32u(t1, o->in2, get_mem_index(s));
+    tcg_gen_addi_i64(t2, o->in2, 4 * ((r3 - r1) & 15));
+    tcg_gen_qemu_ld32u(t2, t2, get_mem_index(s));
+    store_reg32_i64(r1, t1);
+    store_reg32_i64(r3, t2);
+
+    /* Only two registers to read. */
+    if (((r1 + 1) & 15) == r3) {
+        tcg_temp_free(t2);
+        tcg_temp_free(t1);
+        return NO_EXIT;
+    }
+
+    /* Then load the remaining registers. Page fault can't occur. */
+    r3 = (r3 - 1) & 15;
+    tcg_gen_movi_i64(t2, 4);
+    while (r1 != r3) {
         r1 = (r1 + 1) & 15;
+        tcg_gen_add_i64(o->in2, o->in2, t2);
+        tcg_gen_qemu_ld32u(t1, o->in2, get_mem_index(s));
+        store_reg32_i64(r1, t1);
     }
+    tcg_temp_free(t2);
+    tcg_temp_free(t1);
 
-    tcg_temp_free_i64(t);
-    tcg_temp_free_i64(t4);
     return NO_EXIT;
 }
 
@@ -2456,21 +2480,45 @@  static ExitStatus op_lmh(DisasContext *s, DisasOps *o)
 {
     int r1 = get_field(s->fields, r1);
     int r3 = get_field(s->fields, r3);
-    TCGv_i64 t = tcg_temp_new_i64();
-    TCGv_i64 t4 = tcg_const_i64(4);
+    TCGv_i64 t1, t2;
 
-    while (1) {
-        tcg_gen_qemu_ld32u(t, o->in2, get_mem_index(s));
-        store_reg32h_i64(r1, t);
-        if (r1 == r3) {
-            break;
-        }
-        tcg_gen_add_i64(o->in2, o->in2, t4);
+    /* Only one register to read. */
+    t1 = tcg_temp_new_i64();
+    if (unlikely(r1 == r3)) {
+        tcg_gen_qemu_ld32u(t1, o->in2, get_mem_index(s));
+        store_reg32h_i64(r1, t1);
+        tcg_temp_free(t1);
+        return NO_EXIT;
+    }
+
+    /* First load the values of the first and last registers to trigger
+       possible page faults. */
+    t2 = tcg_temp_new_i64();
+    tcg_gen_qemu_ld32u(t1, o->in2, get_mem_index(s));
+    tcg_gen_addi_i64(t2, o->in2, 4 * ((r3 - r1) & 15));
+    tcg_gen_qemu_ld32u(t2, t2, get_mem_index(s));
+    store_reg32h_i64(r1, t1);
+    store_reg32h_i64(r3, t2);
+
+    /* Only two registers to read. */
+    if (((r1 + 1) & 15) == r3) {
+        tcg_temp_free(t2);
+        tcg_temp_free(t1);
+        return NO_EXIT;
+    }
+
+    /* Then load the remaining registers. Page fault can't occur. */
+    r3 = (r3 - 1) & 15;
+    tcg_gen_movi_i64(t2, 4);
+    while (r1 != r3) {
         r1 = (r1 + 1) & 15;
+        tcg_gen_add_i64(o->in2, o->in2, t2);
+        tcg_gen_qemu_ld32u(t1, o->in2, get_mem_index(s));
+        store_reg32h_i64(r1, t1);
     }
+    tcg_temp_free(t2);
+    tcg_temp_free(t1);
 
-    tcg_temp_free_i64(t);
-    tcg_temp_free_i64(t4);
     return NO_EXIT;
 }
 
@@ -2478,18 +2526,40 @@  static ExitStatus op_lm64(DisasContext *s, DisasOps *o)
 {
     int r1 = get_field(s->fields, r1);
     int r3 = get_field(s->fields, r3);
-    TCGv_i64 t8 = tcg_const_i64(8);
+    TCGv_i64 t1, t2;
 
-    while (1) {
+    /* Only one register to read. */
+    if (unlikely(r1 == r3)) {
         tcg_gen_qemu_ld64(regs[r1], o->in2, get_mem_index(s));
-        if (r1 == r3) {
-            break;
-        }
-        tcg_gen_add_i64(o->in2, o->in2, t8);
+        return NO_EXIT;
+    }
+
+    /* First load the values of the first and last registers to trigger
+       possible page faults. */
+    t1 = tcg_temp_new_i64();
+    t2 = tcg_temp_new_i64();
+    tcg_gen_qemu_ld64(t1, o->in2, get_mem_index(s));
+    tcg_gen_addi_i64(t2, o->in2, 8 * ((r3 - r1) & 15));
+    tcg_gen_qemu_ld64(regs[r3], t2, get_mem_index(s));
+    tcg_gen_mov_i64(regs[r1], t1);
+    tcg_temp_free(t2);
+
+    /* Only two registers to read. */
+    if (((r1 + 1) & 15) == r3) {
+        tcg_temp_free(t1);
+        return NO_EXIT;
+    }
+
+    /* Then load the remaining registers. Page fault can't occur. */
+    r3 = (r3 - 1) & 15;
+    tcg_gen_movi_i64(t1, 8);
+    while (r1 != r3) {
         r1 = (r1 + 1) & 15;
+        tcg_gen_add_i64(o->in2, o->in2, t1);
+        tcg_gen_qemu_ld64(regs[r1], o->in2, get_mem_index(s));
     }
+    tcg_temp_free(t1);
 
-    tcg_temp_free_i64(t8);
     return NO_EXIT;
 }