1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v3] s390x/tcg: add various alignment checks

David Hildenbrand posted 1 patch 7 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180215103822.15179-1-david@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test docker-quick@centos6 passed
Test ppcbe passed
Test ppcle passed
Test s390x passed
target/s390x/insn-data.def | 16 ++++++++--------
target/s390x/mem_helper.c  | 25 +++++++++++++++++++++++++
target/s390x/translate.c   | 43 ++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 75 insertions(+), 9 deletions(-)
[Qemu-devel] [PATCH v3] s390x/tcg: add various alignment checks
Posted by David Hildenbrand 7 years, 8 months ago 
Let's add proper alignment checks for a handful of instructions that
require a SPECIFICATION exception in case alignment is violated.

Introduce new wout/in functions. As we are right now only using them for
privileged instructions, we have to add ugly ifdefs to silence
compilers.

Convert STORE CPU ID right away to make use of the wout function.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/insn-data.def | 16 ++++++++--------
 target/s390x/mem_helper.c  | 25 +++++++++++++++++++++++++
 target/s390x/translate.c   | 43 ++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 621e10d615..157619403d 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -1000,13 +1000,13 @@
     /* ??? Not implemented - is it necessary? */
     C(0xb204, SCK,     S,     Z,   0, 0, 0, 0, 0, 0)
 /* SET CLOCK COMPARATOR */
-    C(0xb206, SCKC,    S,     Z,   0, m2_64, 0, 0, sckc, 0)
+    C(0xb206, SCKC,    S,     Z,   0, m2_64a, 0, 0, sckc, 0)
 /* SET CLOCK PROGRAMMABLE FIELD */
     C(0x0107, SCKPF,   E,     Z,   0, 0, 0, 0, sckpf, 0)
 /* SET CPU TIMER */
-    C(0xb208, SPT,     S,     Z,   0, m2_64, 0, 0, spt, 0)
+    C(0xb208, SPT,     S,     Z,   0, m2_64a, 0, 0, spt, 0)
 /* SET PREFIX */
-    C(0xb210, SPX,     S,     Z,   0, m2_32u, 0, 0, spx, 0)
+    C(0xb210, SPX,     S,     Z,   0, m2_32ua, 0, 0, spx, 0)
 /* SET PSW KEY FROM ADDRESS */
     C(0xb20a, SPKA,    S,     Z,   0, a2, 0, 0, spka, 0)
 /* SET STORAGE KEY EXTENDED */
@@ -1021,20 +1021,20 @@
 /* STORE CLOCK EXTENDED */
     C(0xb278, STCKE,   S,     Z,   0, a2, 0, 0, stcke, 0)
 /* STORE CLOCK COMPARATOR */
-    C(0xb207, STCKC,   S,     Z,   la2, 0, new, m1_64, stckc, 0)
+    C(0xb207, STCKC,   S,     Z,   la2, 0, new, m1_64a, stckc, 0)
 /* STORE CONTROL */
     C(0xb600, STCTL,   RS_a,  Z,   0, a2, 0, 0, stctl, 0)
     C(0xeb25, STCTG,   RSY_a, Z,   0, a2, 0, 0, stctg, 0)
 /* STORE CPU ADDRESS */
-    C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16, stap, 0)
+    C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16a, stap, 0)
 /* STORE CPU ID */
-    C(0xb202, STIDP,   S,     Z,   la2, 0, new, 0, stidp, 0)
+    C(0xb202, STIDP,   S,     Z,   la2, 0, new, m1_64a, stidp, 0)
 /* STORE CPU TIMER */
-    C(0xb209, STPT,    S,     Z,   la2, 0, new, m1_64, stpt, 0)
+    C(0xb209, STPT,    S,     Z,   la2, 0, new, m1_64a, stpt, 0)
 /* STORE FACILITY LIST */
     C(0xb2b1, STFL,    S,     Z,   0, 0, 0, 0, stfl, 0)
 /* STORE PREFIX */
-    C(0xb211, STPX,    S,     Z,   la2, 0, new, m1_32, stpx, 0)
+    C(0xb211, STPX,    S,     Z,   la2, 0, new, m1_32a, stpx, 0)
 /* STORE SYSTEM INFORMATION */
     C(0xb27d, STSI,    S,     Z,   0, a2, 0, 0, stsi, 0)
 /* STORE THEN AND SYSTEM MASK */
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 427b795a78..d5291b246e 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -693,6 +693,11 @@ void HELPER(lam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
     uintptr_t ra = GETPC();
     int i;
 
+    if (a2 & 0x3) {
+        /* we either came here by lam or lamy, which have different lengths */
+        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
+    }
+
     for (i = r1;; i = (i + 1) % 16) {
         env->aregs[i] = cpu_ldl_data_ra(env, a2, ra);
         a2 += 4;
@@ -709,6 +714,10 @@ void HELPER(stam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
     uintptr_t ra = GETPC();
     int i;
 
+    if (a2 & 0x3) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
+    }
+
     for (i = r1;; i = (i + 1) % 16) {
         cpu_stl_data_ra(env, a2, env->aregs[i], ra);
         a2 += 4;
@@ -1620,6 +1629,10 @@ void HELPER(lctlg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
     uint64_t src = a2;
     uint32_t i;
 
+    if (src & 0x7) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, 6, ra);
+    }
+
     for (i = r1;; i = (i + 1) % 16) {
         uint64_t val = cpu_ldq_data_ra(env, src, ra);
         if (env->cregs[i] != val && i >= 9 && i <= 11) {
@@ -1650,6 +1663,10 @@ void HELPER(lctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
     uint64_t src = a2;
     uint32_t i;
 
+    if (src & 0x3) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
+    }
+
     for (i = r1;; i = (i + 1) % 16) {
         uint32_t val = cpu_ldl_data_ra(env, src, ra);
         if ((uint32_t)env->cregs[i] != val && i >= 9 && i <= 11) {
@@ -1677,6 +1694,10 @@ void HELPER(stctg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
     uint64_t dest = a2;
     uint32_t i;
 
+    if (dest & 0x7) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, 6, ra);
+    }
+
     for (i = r1;; i = (i + 1) % 16) {
         cpu_stq_data_ra(env, dest, env->cregs[i], ra);
         dest += sizeof(uint64_t);
@@ -1693,6 +1714,10 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
     uint64_t dest = a2;
     uint32_t i;
 
+    if (dest & 0x3) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
+    }
+
     for (i = r1;; i = (i + 1) % 16) {
         cpu_stl_data_ra(env, dest, env->cregs[i], ra);
         dest += sizeof(uint32_t);
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 5aea3bbca6..7d39ab350d 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -4062,7 +4062,6 @@ static ExitStatus op_stidp(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
     tcg_gen_ld_i64(o->out, cpu_env, offsetof(CPUS390XState, cpuid));
-    tcg_gen_qemu_st_i64(o->out, o->addr1, get_mem_index(s), MO_TEQ | MO_ALIGN);
     return NO_EXIT;
 }
 
@@ -5220,18 +5219,42 @@ static void wout_m1_16(DisasContext *s, DisasFields *f, DisasOps *o)
 }
 #define SPEC_wout_m1_16 0
 
+#ifndef CONFIG_USER_ONLY
+static void wout_m1_16a(DisasContext *s, DisasFields *f, DisasOps *o)
+{
+    tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), MO_TEUW | MO_ALIGN);
+}
+#define SPEC_wout_m1_16a 0
+#endif
+
 static void wout_m1_32(DisasContext *s, DisasFields *f, DisasOps *o)
 {
     tcg_gen_qemu_st32(o->out, o->addr1, get_mem_index(s));
 }
 #define SPEC_wout_m1_32 0
 
+#ifndef CONFIG_USER_ONLY
+static void wout_m1_32a(DisasContext *s, DisasFields *f, DisasOps *o)
+{
+    tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), MO_TEUL | MO_ALIGN);
+}
+#define SPEC_wout_m1_32a 0
+#endif
+
 static void wout_m1_64(DisasContext *s, DisasFields *f, DisasOps *o)
 {
     tcg_gen_qemu_st64(o->out, o->addr1, get_mem_index(s));
 }
 #define SPEC_wout_m1_64 0
 
+#ifndef CONFIG_USER_ONLY
+static void wout_m1_64a(DisasContext *s, DisasFields *f, DisasOps *o)
+{
+    tcg_gen_qemu_st_i64(o->out, o->addr1, get_mem_index(s), MO_TEQ | MO_ALIGN);
+}
+#define SPEC_wout_m1_64a 0
+#endif
+
 static void wout_m2_32(DisasContext *s, DisasFields *f, DisasOps *o)
 {
     tcg_gen_qemu_st32(o->out, o->in2, get_mem_index(s));
@@ -5657,6 +5680,15 @@ static void in2_m2_32u(DisasContext *s, DisasFields *f, DisasOps *o)
 }
 #define SPEC_in2_m2_32u 0
 
+#ifndef CONFIG_USER_ONLY
+static void in2_m2_32ua(DisasContext *s, DisasFields *f, DisasOps *o)
+{
+    in2_a2(s, f, o);
+    tcg_gen_qemu_ld_tl(o->in2, o->in2, get_mem_index(s), MO_TEUL | MO_ALIGN);
+}
+#define SPEC_in2_m2_32ua 0
+#endif
+
 static void in2_m2_64(DisasContext *s, DisasFields *f, DisasOps *o)
 {
     in2_a2(s, f, o);
@@ -5664,6 +5696,15 @@ static void in2_m2_64(DisasContext *s, DisasFields *f, DisasOps *o)
 }
 #define SPEC_in2_m2_64 0
 
+#ifndef CONFIG_USER_ONLY
+static void in2_m2_64a(DisasContext *s, DisasFields *f, DisasOps *o)
+{
+    in2_a2(s, f, o);
+    tcg_gen_qemu_ld_i64(o->in2, o->in2, get_mem_index(s), MO_TEQ | MO_ALIGN);
+}
+#define SPEC_in2_m2_64a 0
+#endif
+
 static void in2_mri2_16u(DisasContext *s, DisasFields *f, DisasOps *o)
 {
     in2_ri2(s, f, o);
-- 
2.14.3
Re: [Qemu-devel] [PATCH v3] s390x/tcg: add various alignment checks
Posted by David Hildenbrand 7 years, 8 months ago 
On 15.02.2018 11:38, David Hildenbrand wrote:
> Let's add proper alignment checks for a handful of instructions that
> require a SPECIFICATION exception in case alignment is violated.
> 
> Introduce new wout/in functions. As we are right now only using them for
> privileged instructions, we have to add ugly ifdefs to silence
> compilers.
> 
> Convert STORE CPU ID right away to make use of the wout function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/insn-data.def | 16 ++++++++--------
>  target/s390x/mem_helper.c  | 25 +++++++++++++++++++++++++
>  target/s390x/translate.c   | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index 621e10d615..157619403d 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -1000,13 +1000,13 @@
>      /* ??? Not implemented - is it necessary? */
>      C(0xb204, SCK,     S,     Z,   0, 0, 0, 0, 0, 0)
>  /* SET CLOCK COMPARATOR */
> -    C(0xb206, SCKC,    S,     Z,   0, m2_64, 0, 0, sckc, 0)
> +    C(0xb206, SCKC,    S,     Z,   0, m2_64a, 0, 0, sckc, 0)
>  /* SET CLOCK PROGRAMMABLE FIELD */
>      C(0x0107, SCKPF,   E,     Z,   0, 0, 0, 0, sckpf, 0)
>  /* SET CPU TIMER */
> -    C(0xb208, SPT,     S,     Z,   0, m2_64, 0, 0, spt, 0)
> +    C(0xb208, SPT,     S,     Z,   0, m2_64a, 0, 0, spt, 0)
>  /* SET PREFIX */
> -    C(0xb210, SPX,     S,     Z,   0, m2_32u, 0, 0, spx, 0)
> +    C(0xb210, SPX,     S,     Z,   0, m2_32ua, 0, 0, spx, 0)
>  /* SET PSW KEY FROM ADDRESS */
>      C(0xb20a, SPKA,    S,     Z,   0, a2, 0, 0, spka, 0)
>  /* SET STORAGE KEY EXTENDED */
> @@ -1021,20 +1021,20 @@
>  /* STORE CLOCK EXTENDED */
>      C(0xb278, STCKE,   S,     Z,   0, a2, 0, 0, stcke, 0)
>  /* STORE CLOCK COMPARATOR */
> -    C(0xb207, STCKC,   S,     Z,   la2, 0, new, m1_64, stckc, 0)
> +    C(0xb207, STCKC,   S,     Z,   la2, 0, new, m1_64a, stckc, 0)
>  /* STORE CONTROL */
>      C(0xb600, STCTL,   RS_a,  Z,   0, a2, 0, 0, stctl, 0)
>      C(0xeb25, STCTG,   RSY_a, Z,   0, a2, 0, 0, stctg, 0)
>  /* STORE CPU ADDRESS */
> -    C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16, stap, 0)
> +    C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16a, stap, 0)
>  /* STORE CPU ID */
> -    C(0xb202, STIDP,   S,     Z,   la2, 0, new, 0, stidp, 0)
> +    C(0xb202, STIDP,   S,     Z,   la2, 0, new, m1_64a, stidp, 0)
>  /* STORE CPU TIMER */
> -    C(0xb209, STPT,    S,     Z,   la2, 0, new, m1_64, stpt, 0)
> +    C(0xb209, STPT,    S,     Z,   la2, 0, new, m1_64a, stpt, 0)
>  /* STORE FACILITY LIST */
>      C(0xb2b1, STFL,    S,     Z,   0, 0, 0, 0, stfl, 0)
>  /* STORE PREFIX */
> -    C(0xb211, STPX,    S,     Z,   la2, 0, new, m1_32, stpx, 0)
> +    C(0xb211, STPX,    S,     Z,   la2, 0, new, m1_32a, stpx, 0)
>  /* STORE SYSTEM INFORMATION */
>      C(0xb27d, STSI,    S,     Z,   0, a2, 0, 0, stsi, 0)
>  /* STORE THEN AND SYSTEM MASK */
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 427b795a78..d5291b246e 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -693,6 +693,11 @@ void HELPER(lam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>      uintptr_t ra = GETPC();
>      int i;
>  
> +    if (a2 & 0x3) {
> +        /* we either came here by lam or lamy, which have different lengths */
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> +    }
> +
>      for (i = r1;; i = (i + 1) % 16) {
>          env->aregs[i] = cpu_ldl_data_ra(env, a2, ra);
>          a2 += 4;
> @@ -709,6 +714,10 @@ void HELPER(stam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>      uintptr_t ra = GETPC();
>      int i;
>  
> +    if (a2 & 0x3) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
> +    }
> +
>      for (i = r1;; i = (i + 1) % 16) {
>          cpu_stl_data_ra(env, a2, env->aregs[i], ra);
>          a2 += 4;
> @@ -1620,6 +1629,10 @@ void HELPER(lctlg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>      uint64_t src = a2;
>      uint32_t i;
>  
> +    if (src & 0x7) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, 6, ra);
> +    }
> +
>      for (i = r1;; i = (i + 1) % 16) {
>          uint64_t val = cpu_ldq_data_ra(env, src, ra);
>          if (env->cregs[i] != val && i >= 9 && i <= 11) {
> @@ -1650,6 +1663,10 @@ void HELPER(lctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>      uint64_t src = a2;
>      uint32_t i;
>  
> +    if (src & 0x3) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
> +    }
> +
>      for (i = r1;; i = (i + 1) % 16) {
>          uint32_t val = cpu_ldl_data_ra(env, src, ra);
>          if ((uint32_t)env->cregs[i] != val && i >= 9 && i <= 11) {
> @@ -1677,6 +1694,10 @@ void HELPER(stctg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>      uint64_t dest = a2;
>      uint32_t i;
>  
> +    if (dest & 0x7) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, 6, ra);
> +    }
> +
>      for (i = r1;; i = (i + 1) % 16) {
>          cpu_stq_data_ra(env, dest, env->cregs[i], ra);
>          dest += sizeof(uint64_t);
> @@ -1693,6 +1714,10 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>      uint64_t dest = a2;
>      uint32_t i;
>  
> +    if (dest & 0x3) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
> +    }
> +
>      for (i = r1;; i = (i + 1) % 16) {
>          cpu_stl_data_ra(env, dest, env->cregs[i], ra);
>          dest += sizeof(uint32_t);
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 5aea3bbca6..7d39ab350d 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -4062,7 +4062,6 @@ static ExitStatus op_stidp(DisasContext *s, DisasOps *o)
>  {
>      check_privileged(s);
>      tcg_gen_ld_i64(o->out, cpu_env, offsetof(CPUS390XState, cpuid));
> -    tcg_gen_qemu_st_i64(o->out, o->addr1, get_mem_index(s), MO_TEQ | MO_ALIGN);
>      return NO_EXIT;
>  }
>  
> @@ -5220,18 +5219,42 @@ static void wout_m1_16(DisasContext *s, DisasFields *f, DisasOps *o)
>  }
>  #define SPEC_wout_m1_16 0
>  
> +#ifndef CONFIG_USER_ONLY
> +static void wout_m1_16a(DisasContext *s, DisasFields *f, DisasOps *o)
> +{
> +    tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), MO_TEUW | MO_ALIGN);
> +}
> +#define SPEC_wout_m1_16a 0
> +#endif
> +
>  static void wout_m1_32(DisasContext *s, DisasFields *f, DisasOps *o)
>  {
>      tcg_gen_qemu_st32(o->out, o->addr1, get_mem_index(s));
>  }
>  #define SPEC_wout_m1_32 0
>  
> +#ifndef CONFIG_USER_ONLY
> +static void wout_m1_32a(DisasContext *s, DisasFields *f, DisasOps *o)
> +{
> +    tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), MO_TEUL | MO_ALIGN);
> +}
> +#define SPEC_wout_m1_32a 0
> +#endif
> +
>  static void wout_m1_64(DisasContext *s, DisasFields *f, DisasOps *o)
>  {
>      tcg_gen_qemu_st64(o->out, o->addr1, get_mem_index(s));
>  }
>  #define SPEC_wout_m1_64 0
>  
> +#ifndef CONFIG_USER_ONLY
> +static void wout_m1_64a(DisasContext *s, DisasFields *f, DisasOps *o)
> +{
> +    tcg_gen_qemu_st_i64(o->out, o->addr1, get_mem_index(s), MO_TEQ | MO_ALIGN);
> +}
> +#define SPEC_wout_m1_64a 0
> +#endif
> +
>  static void wout_m2_32(DisasContext *s, DisasFields *f, DisasOps *o)
>  {
>      tcg_gen_qemu_st32(o->out, o->in2, get_mem_index(s));
> @@ -5657,6 +5680,15 @@ static void in2_m2_32u(DisasContext *s, DisasFields *f, DisasOps *o)
>  }
>  #define SPEC_in2_m2_32u 0
>  
> +#ifndef CONFIG_USER_ONLY
> +static void in2_m2_32ua(DisasContext *s, DisasFields *f, DisasOps *o)
> +{
> +    in2_a2(s, f, o);
> +    tcg_gen_qemu_ld_tl(o->in2, o->in2, get_mem_index(s), MO_TEUL | MO_ALIGN);
> +}
> +#define SPEC_in2_m2_32ua 0
> +#endif
> +
>  static void in2_m2_64(DisasContext *s, DisasFields *f, DisasOps *o)
>  {
>      in2_a2(s, f, o);
> @@ -5664,6 +5696,15 @@ static void in2_m2_64(DisasContext *s, DisasFields *f, DisasOps *o)
>  }
>  #define SPEC_in2_m2_64 0
>  
> +#ifndef CONFIG_USER_ONLY
> +static void in2_m2_64a(DisasContext *s, DisasFields *f, DisasOps *o)
> +{
> +    in2_a2(s, f, o);
> +    tcg_gen_qemu_ld_i64(o->in2, o->in2, get_mem_index(s), MO_TEQ | MO_ALIGN);
> +}
> +#define SPEC_in2_m2_64a 0
> +#endif
> +
>  static void in2_mri2_16u(DisasContext *s, DisasFields *f, DisasOps *o)
>  {
>      in2_ri2(s, f, o);
> 

Conny, if there are no new comments, can you make sure to add Richard's
r-b when applying? thanks!

-- 

Thanks,

David / dhildenb
Re: [Qemu-devel] [PATCH v3] s390x/tcg: add various alignment checks
Posted by Richard Henderson 7 years, 8 months ago 
On 02/15/2018 02:50 AM, David Hildenbrand wrote:
> On 15.02.2018 11:38, David Hildenbrand wrote:
>> Let's add proper alignment checks for a handful of instructions that
>> require a SPECIFICATION exception in case alignment is violated.
>>
>> Introduce new wout/in functions. As we are right now only using them for
>> privileged instructions, we have to add ugly ifdefs to silence
>> compilers.
>>
>> Convert STORE CPU ID right away to make use of the wout function.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/insn-data.def | 16 ++++++++--------
>>  target/s390x/mem_helper.c  | 25 +++++++++++++++++++++++++
>>  target/s390x/translate.c   | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 75 insertions(+), 9 deletions(-)
>>
>> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
>> index 621e10d615..157619403d 100644
>> --- a/target/s390x/insn-data.def
>> +++ b/target/s390x/insn-data.def
>> @@ -1000,13 +1000,13 @@
>>      /* ??? Not implemented - is it necessary? */
>>      C(0xb204, SCK,     S,     Z,   0, 0, 0, 0, 0, 0)
>>  /* SET CLOCK COMPARATOR */
>> -    C(0xb206, SCKC,    S,     Z,   0, m2_64, 0, 0, sckc, 0)
>> +    C(0xb206, SCKC,    S,     Z,   0, m2_64a, 0, 0, sckc, 0)
>>  /* SET CLOCK PROGRAMMABLE FIELD */
>>      C(0x0107, SCKPF,   E,     Z,   0, 0, 0, 0, sckpf, 0)
>>  /* SET CPU TIMER */
>> -    C(0xb208, SPT,     S,     Z,   0, m2_64, 0, 0, spt, 0)
>> +    C(0xb208, SPT,     S,     Z,   0, m2_64a, 0, 0, spt, 0)
>>  /* SET PREFIX */
>> -    C(0xb210, SPX,     S,     Z,   0, m2_32u, 0, 0, spx, 0)
>> +    C(0xb210, SPX,     S,     Z,   0, m2_32ua, 0, 0, spx, 0)
>>  /* SET PSW KEY FROM ADDRESS */
>>      C(0xb20a, SPKA,    S,     Z,   0, a2, 0, 0, spka, 0)
>>  /* SET STORAGE KEY EXTENDED */
>> @@ -1021,20 +1021,20 @@
>>  /* STORE CLOCK EXTENDED */
>>      C(0xb278, STCKE,   S,     Z,   0, a2, 0, 0, stcke, 0)
>>  /* STORE CLOCK COMPARATOR */
>> -    C(0xb207, STCKC,   S,     Z,   la2, 0, new, m1_64, stckc, 0)
>> +    C(0xb207, STCKC,   S,     Z,   la2, 0, new, m1_64a, stckc, 0)
>>  /* STORE CONTROL */
>>      C(0xb600, STCTL,   RS_a,  Z,   0, a2, 0, 0, stctl, 0)
>>      C(0xeb25, STCTG,   RSY_a, Z,   0, a2, 0, 0, stctg, 0)
>>  /* STORE CPU ADDRESS */
>> -    C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16, stap, 0)
>> +    C(0xb212, STAP,    S,     Z,   la2, 0, new, m1_16a, stap, 0)
>>  /* STORE CPU ID */
>> -    C(0xb202, STIDP,   S,     Z,   la2, 0, new, 0, stidp, 0)
>> +    C(0xb202, STIDP,   S,     Z,   la2, 0, new, m1_64a, stidp, 0)
>>  /* STORE CPU TIMER */
>> -    C(0xb209, STPT,    S,     Z,   la2, 0, new, m1_64, stpt, 0)
>> +    C(0xb209, STPT,    S,     Z,   la2, 0, new, m1_64a, stpt, 0)
>>  /* STORE FACILITY LIST */
>>      C(0xb2b1, STFL,    S,     Z,   0, 0, 0, 0, stfl, 0)
>>  /* STORE PREFIX */
>> -    C(0xb211, STPX,    S,     Z,   la2, 0, new, m1_32, stpx, 0)
>> +    C(0xb211, STPX,    S,     Z,   la2, 0, new, m1_32a, stpx, 0)
>>  /* STORE SYSTEM INFORMATION */
>>      C(0xb27d, STSI,    S,     Z,   0, a2, 0, 0, stsi, 0)
>>  /* STORE THEN AND SYSTEM MASK */
>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
>> index 427b795a78..d5291b246e 100644
>> --- a/target/s390x/mem_helper.c
>> +++ b/target/s390x/mem_helper.c
>> @@ -693,6 +693,11 @@ void HELPER(lam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>      uintptr_t ra = GETPC();
>>      int i;
>>  
>> +    if (a2 & 0x3) {
>> +        /* we either came here by lam or lamy, which have different lengths */
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
>> +    }
>> +
>>      for (i = r1;; i = (i + 1) % 16) {
>>          env->aregs[i] = cpu_ldl_data_ra(env, a2, ra);
>>          a2 += 4;
>> @@ -709,6 +714,10 @@ void HELPER(stam)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>      uintptr_t ra = GETPC();
>>      int i;
>>  
>> +    if (a2 & 0x3) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
>> +    }
>> +
>>      for (i = r1;; i = (i + 1) % 16) {
>>          cpu_stl_data_ra(env, a2, env->aregs[i], ra);
>>          a2 += 4;
>> @@ -1620,6 +1629,10 @@ void HELPER(lctlg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>      uint64_t src = a2;
>>      uint32_t i;
>>  
>> +    if (src & 0x7) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, 6, ra);
>> +    }
>> +
>>      for (i = r1;; i = (i + 1) % 16) {
>>          uint64_t val = cpu_ldq_data_ra(env, src, ra);
>>          if (env->cregs[i] != val && i >= 9 && i <= 11) {
>> @@ -1650,6 +1663,10 @@ void HELPER(lctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>      uint64_t src = a2;
>>      uint32_t i;
>>  
>> +    if (src & 0x3) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
>> +    }
>> +
>>      for (i = r1;; i = (i + 1) % 16) {
>>          uint32_t val = cpu_ldl_data_ra(env, src, ra);
>>          if ((uint32_t)env->cregs[i] != val && i >= 9 && i <= 11) {
>> @@ -1677,6 +1694,10 @@ void HELPER(stctg)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>      uint64_t dest = a2;
>>      uint32_t i;
>>  
>> +    if (dest & 0x7) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, 6, ra);
>> +    }
>> +
>>      for (i = r1;; i = (i + 1) % 16) {
>>          cpu_stq_data_ra(env, dest, env->cregs[i], ra);
>>          dest += sizeof(uint64_t);
>> @@ -1693,6 +1714,10 @@ void HELPER(stctl)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint32_t r3)
>>      uint64_t dest = a2;
>>      uint32_t i;
>>  
>> +    if (dest & 0x3) {
>> +        s390_program_interrupt(env, PGM_SPECIFICATION, 4, ra);
>> +    }
>> +
>>      for (i = r1;; i = (i + 1) % 16) {
>>          cpu_stl_data_ra(env, dest, env->cregs[i], ra);
>>          dest += sizeof(uint32_t);
>> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
>> index 5aea3bbca6..7d39ab350d 100644
>> --- a/target/s390x/translate.c
>> +++ b/target/s390x/translate.c
>> @@ -4062,7 +4062,6 @@ static ExitStatus op_stidp(DisasContext *s, DisasOps *o)
>>  {
>>      check_privileged(s);
>>      tcg_gen_ld_i64(o->out, cpu_env, offsetof(CPUS390XState, cpuid));
>> -    tcg_gen_qemu_st_i64(o->out, o->addr1, get_mem_index(s), MO_TEQ | MO_ALIGN);
>>      return NO_EXIT;
>>  }
>>  
>> @@ -5220,18 +5219,42 @@ static void wout_m1_16(DisasContext *s, DisasFields *f, DisasOps *o)
>>  }
>>  #define SPEC_wout_m1_16 0
>>  
>> +#ifndef CONFIG_USER_ONLY
>> +static void wout_m1_16a(DisasContext *s, DisasFields *f, DisasOps *o)
>> +{
>> +    tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), MO_TEUW | MO_ALIGN);
>> +}
>> +#define SPEC_wout_m1_16a 0
>> +#endif
>> +
>>  static void wout_m1_32(DisasContext *s, DisasFields *f, DisasOps *o)
>>  {
>>      tcg_gen_qemu_st32(o->out, o->addr1, get_mem_index(s));
>>  }
>>  #define SPEC_wout_m1_32 0
>>  
>> +#ifndef CONFIG_USER_ONLY
>> +static void wout_m1_32a(DisasContext *s, DisasFields *f, DisasOps *o)
>> +{
>> +    tcg_gen_qemu_st_tl(o->out, o->addr1, get_mem_index(s), MO_TEUL | MO_ALIGN);
>> +}
>> +#define SPEC_wout_m1_32a 0
>> +#endif
>> +
>>  static void wout_m1_64(DisasContext *s, DisasFields *f, DisasOps *o)
>>  {
>>      tcg_gen_qemu_st64(o->out, o->addr1, get_mem_index(s));
>>  }
>>  #define SPEC_wout_m1_64 0
>>  
>> +#ifndef CONFIG_USER_ONLY
>> +static void wout_m1_64a(DisasContext *s, DisasFields *f, DisasOps *o)
>> +{
>> +    tcg_gen_qemu_st_i64(o->out, o->addr1, get_mem_index(s), MO_TEQ | MO_ALIGN);
>> +}
>> +#define SPEC_wout_m1_64a 0
>> +#endif
>> +
>>  static void wout_m2_32(DisasContext *s, DisasFields *f, DisasOps *o)
>>  {
>>      tcg_gen_qemu_st32(o->out, o->in2, get_mem_index(s));
>> @@ -5657,6 +5680,15 @@ static void in2_m2_32u(DisasContext *s, DisasFields *f, DisasOps *o)
>>  }
>>  #define SPEC_in2_m2_32u 0
>>  
>> +#ifndef CONFIG_USER_ONLY
>> +static void in2_m2_32ua(DisasContext *s, DisasFields *f, DisasOps *o)
>> +{
>> +    in2_a2(s, f, o);
>> +    tcg_gen_qemu_ld_tl(o->in2, o->in2, get_mem_index(s), MO_TEUL | MO_ALIGN);
>> +}
>> +#define SPEC_in2_m2_32ua 0
>> +#endif
>> +
>>  static void in2_m2_64(DisasContext *s, DisasFields *f, DisasOps *o)
>>  {
>>      in2_a2(s, f, o);
>> @@ -5664,6 +5696,15 @@ static void in2_m2_64(DisasContext *s, DisasFields *f, DisasOps *o)
>>  }
>>  #define SPEC_in2_m2_64 0
>>  
>> +#ifndef CONFIG_USER_ONLY
>> +static void in2_m2_64a(DisasContext *s, DisasFields *f, DisasOps *o)
>> +{
>> +    in2_a2(s, f, o);
>> +    tcg_gen_qemu_ld_i64(o->in2, o->in2, get_mem_index(s), MO_TEQ | MO_ALIGN);
>> +}
>> +#define SPEC_in2_m2_64a 0
>> +#endif
>> +
>>  static void in2_mri2_16u(DisasContext *s, DisasFields *f, DisasOps *o)
>>  {
>>      in2_ri2(s, f, o);
>>
> 
> Conny, if there are no new comments, can you make sure to add Richard's
> r-b when applying? thanks!
> 

None from me.  ;-)

(re-)Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Re: [Qemu-devel] [PATCH v3] s390x/tcg: add various alignment checks
Posted by Cornelia Huck 7 years, 8 months ago 
On Thu, 15 Feb 2018 11:38:22 +0100
David Hildenbrand <david@redhat.com> wrote:

> Let's add proper alignment checks for a handful of instructions that
> require a SPECIFICATION exception in case alignment is violated.
> 
> Introduce new wout/in functions. As we are right now only using them for
> privileged instructions, we have to add ugly ifdefs to silence
> compilers.
> 
> Convert STORE CPU ID right away to make use of the wout function.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/insn-data.def | 16 ++++++++--------
>  target/s390x/mem_helper.c  | 25 +++++++++++++++++++++++++
>  target/s390x/translate.c   | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 9 deletions(-)

Thanks, applied.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.