diff mbox series

[v2,1/1] accel/tcg: Fix computing of is_write for mips

Message ID 20200923093800.9845-1-kele.hwang@gmail.com
State New
Headers show
Series [v2,1/1] accel/tcg: Fix computing of is_write for mips | expand

Commit Message

Kele Huang Sept. 23, 2020, 9:38 a.m. UTC 
Detect mips store instructions in cpu_signal_handler for all MIPS
versions, and set is_write if encountering such store instructions.

This fixed the error while dealing with self-modifed code for MIPS.

Signed-off-by: Kele Huang <kele.hwang@gmail.com>
Signed-off-by: Xu Zou <iwatchnima@gmail.com>
---
 accel/tcg/user-exec.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Sept. 23, 2020, 11:08 a.m. UTC | #1 
Cc'ing the TCG MIPS maintainers, and also
Cc'ing Richard who made a comment in v1.

On 9/23/20 11:38 AM, Kele Huang wrote:
> Detect mips store instructions in cpu_signal_handler for all MIPS
> versions, and set is_write if encountering such store instructions.
> 
> This fixed the error while dealing with self-modifed code for MIPS.

Quoting Eric Blake:

"It's better to post a v2 as a new top-level thread rather
than buried in-reply-to the v1 thread; among other things,
burying a reply can cause automated patch tooling to miss
the updated series."

> 
> Signed-off-by: Kele Huang <kele.hwang@gmail.com>
> Signed-off-by: Xu Zou <iwatchnima@gmail.com>
> ---
>  accel/tcg/user-exec.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index bb039eb32d..18784516e5 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -710,11 +710,60 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      greg_t pc = uc->uc_mcontext.pc;
>      int is_write;
>  
> -    /* XXX: compute is_write */
>      is_write = 0;
> +
> +    /* Detect store by reading the instruction at the program counter. */
> +    uint32_t insn = *(uint32_t *)pc;
> +    switch(insn>>29) {
> +    case 0x5:
> +        switch((insn>>26) & 0x7) {
> +        case 0x0: /* SB */
> +        case 0x1: /* SH */
> +        case 0x2: /* SWL */
> +        case 0x3: /* SW */
> +        case 0x4: /* SDL */
> +        case 0x5: /* SDR */
> +        case 0x6: /* SWR */
> +            is_write = 1;
> +        }
> +        break;
> +    case 0x7:
> +        switch((insn>>26) & 0x7) {
> +        case 0x0: /* SC */
> +        case 0x1: /* SWC1 */
> +        case 0x4: /* SCD */
> +        case 0x5: /* SDC1 */
> +        case 0x7: /* SD */
> +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
> +        case 0x2: /* SWC2 */
> +        case 0x6: /* SDC2 */
> +#endif
> +            is_write = 1;
> +        }
> +        break;
> +    }
> +
> +    /*
> +     * Required in all versions of MIPS64 since MIPS64r1. Not available
> +     * in MIPS32r1. Required by MIPS32r2 and subsequent versions of MIPS32.
> +     */
> +    switch ((insn >> 3) & 0x7) {
> +    case 0x1:
> +        switch (insn & 0x7) {
> +        case 0x0: /* SWXC1 */
> +        case 0x1: /* SDXC1 */
> +            is_write = 1;
> +        }
> +        break;
> +    }
> +
>      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
>  }
>  
> +#elif defined(__misp16) || defined(__mips_micromips)
> +
> +#error "Unsupported encoding"
> +
>  #elif defined(__riscv)
>  
>  int cpu_signal_handler(int host_signum, void *pinfo,
>
Kele Huang Sept. 24, 2020, 8:52 a.m. UTC | #2 
Got it. Thank you very much!
I will resend the same v2 patch to v1 thread.

On Wed, 23 Sep 2020 at 19:08, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Cc'ing the TCG MIPS maintainers, and also
> Cc'ing Richard who made a comment in v1.
>
> On 9/23/20 11:38 AM, Kele Huang wrote:
> > Detect mips store instructions in cpu_signal_handler for all MIPS
> > versions, and set is_write if encountering such store instructions.
> >
> > This fixed the error while dealing with self-modifed code for MIPS.
>
> Quoting Eric Blake:
>
> "It's better to post a v2 as a new top-level thread rather
> than buried in-reply-to the v1 thread; among other things,
> burying a reply can cause automated patch tooling to miss
> the updated series."
>
> >
> > Signed-off-by: Kele Huang <kele.hwang@gmail.com>
> > Signed-off-by: Xu Zou <iwatchnima@gmail.com>
> > ---
> >  accel/tcg/user-exec.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> > index bb039eb32d..18784516e5 100644
> > --- a/accel/tcg/user-exec.c
> > +++ b/accel/tcg/user-exec.c
> > @@ -710,11 +710,60 @@ int cpu_signal_handler(int host_signum, void
> *pinfo,
> >      greg_t pc = uc->uc_mcontext.pc;
> >      int is_write;
> >
> > -    /* XXX: compute is_write */
> >      is_write = 0;
> > +
> > +    /* Detect store by reading the instruction at the program counter.
> */
> > +    uint32_t insn = *(uint32_t *)pc;
> > +    switch(insn>>29) {
> > +    case 0x5:
> > +        switch((insn>>26) & 0x7) {
> > +        case 0x0: /* SB */
> > +        case 0x1: /* SH */
> > +        case 0x2: /* SWL */
> > +        case 0x3: /* SW */
> > +        case 0x4: /* SDL */
> > +        case 0x5: /* SDR */
> > +        case 0x6: /* SWR */
> > +            is_write = 1;
> > +        }
> > +        break;
> > +    case 0x7:
> > +        switch((insn>>26) & 0x7) {
> > +        case 0x0: /* SC */
> > +        case 0x1: /* SWC1 */
> > +        case 0x4: /* SCD */
> > +        case 0x5: /* SDC1 */
> > +        case 0x7: /* SD */
> > +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
> > +        case 0x2: /* SWC2 */
> > +        case 0x6: /* SDC2 */
> > +#endif
> > +            is_write = 1;
> > +        }
> > +        break;
> > +    }
> > +
> > +    /*
> > +     * Required in all versions of MIPS64 since MIPS64r1. Not available
> > +     * in MIPS32r1. Required by MIPS32r2 and subsequent versions of
> MIPS32.
> > +     */
> > +    switch ((insn >> 3) & 0x7) {
> > +    case 0x1:
> > +        switch (insn & 0x7) {
> > +        case 0x0: /* SWXC1 */
> > +        case 0x1: /* SDXC1 */
> > +            is_write = 1;
> > +        }
> > +        break;
> > +    }
> > +
> >      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
> >  }
> >
> > +#elif defined(__misp16) || defined(__mips_micromips)
> > +
> > +#error "Unsupported encoding"
> > +
> >  #elif defined(__riscv)
> >
> >  int cpu_signal_handler(int host_signum, void *pinfo,
> >
>
>
Kele Huang Sept. 24, 2020, 10:01 a.m. UTC | #3 
Sorry about my misunderstanding of your guidelines.
What is more, I have resend a new v2 patch as a new top-level thread and CC
to TCG MIPS maintainers and Richard.

On Wed, 23 Sep 2020 at 19:08, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Cc'ing the TCG MIPS maintainers, and also
> Cc'ing Richard who made a comment in v1.
>
> On 9/23/20 11:38 AM, Kele Huang wrote:
> > Detect mips store instructions in cpu_signal_handler for all MIPS
> > versions, and set is_write if encountering such store instructions.
> >
> > This fixed the error while dealing with self-modifed code for MIPS.
>
> Quoting Eric Blake:
>
> "It's better to post a v2 as a new top-level thread rather
> than buried in-reply-to the v1 thread; among other things,
> burying a reply can cause automated patch tooling to miss
> the updated series."
>
> >
> > Signed-off-by: Kele Huang <kele.hwang@gmail.com>
> > Signed-off-by: Xu Zou <iwatchnima@gmail.com>
> > ---
> >  accel/tcg/user-exec.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> > index bb039eb32d..18784516e5 100644
> > --- a/accel/tcg/user-exec.c
> > +++ b/accel/tcg/user-exec.c
> > @@ -710,11 +710,60 @@ int cpu_signal_handler(int host_signum, void
> *pinfo,
> >      greg_t pc = uc->uc_mcontext.pc;
> >      int is_write;
> >
> > -    /* XXX: compute is_write */
> >      is_write = 0;
> > +
> > +    /* Detect store by reading the instruction at the program counter.
> */
> > +    uint32_t insn = *(uint32_t *)pc;
> > +    switch(insn>>29) {
> > +    case 0x5:
> > +        switch((insn>>26) & 0x7) {
> > +        case 0x0: /* SB */
> > +        case 0x1: /* SH */
> > +        case 0x2: /* SWL */
> > +        case 0x3: /* SW */
> > +        case 0x4: /* SDL */
> > +        case 0x5: /* SDR */
> > +        case 0x6: /* SWR */
> > +            is_write = 1;
> > +        }
> > +        break;
> > +    case 0x7:
> > +        switch((insn>>26) & 0x7) {
> > +        case 0x0: /* SC */
> > +        case 0x1: /* SWC1 */
> > +        case 0x4: /* SCD */
> > +        case 0x5: /* SDC1 */
> > +        case 0x7: /* SD */
> > +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
> > +        case 0x2: /* SWC2 */
> > +        case 0x6: /* SDC2 */
> > +#endif
> > +            is_write = 1;
> > +        }
> > +        break;
> > +    }
> > +
> > +    /*
> > +     * Required in all versions of MIPS64 since MIPS64r1. Not available
> > +     * in MIPS32r1. Required by MIPS32r2 and subsequent versions of
> MIPS32.
> > +     */
> > +    switch ((insn >> 3) & 0x7) {
> > +    case 0x1:
> > +        switch (insn & 0x7) {
> > +        case 0x0: /* SWXC1 */
> > +        case 0x1: /* SDXC1 */
> > +            is_write = 1;
> > +        }
> > +        break;
> > +    }
> > +
> >      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
> >  }
> >
> > +#elif defined(__misp16) || defined(__mips_micromips)
> > +
> > +#error "Unsupported encoding"
> > +
> >  #elif defined(__riscv)
> >
> >  int cpu_signal_handler(int host_signum, void *pinfo,
> >
>
>
Richard Henderson Sept. 24, 2020, 2:05 p.m. UTC | #4 
On 9/23/20 2:38 AM, Kele Huang wrote:
> Detect mips store instructions in cpu_signal_handler for all MIPS
> versions, and set is_write if encountering such store instructions.
> 
> This fixed the error while dealing with self-modifed code for MIPS.
> 
> Signed-off-by: Kele Huang <kele.hwang@gmail.com>
> Signed-off-by: Xu Zou <iwatchnima@gmail.com>
> ---
>  accel/tcg/user-exec.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index bb039eb32d..18784516e5 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -710,11 +710,60 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      greg_t pc = uc->uc_mcontext.pc;
>      int is_write;
>  
> -    /* XXX: compute is_write */
>      is_write = 0;
> +
> +    /* Detect store by reading the instruction at the program counter. */
> +    uint32_t insn = *(uint32_t *)pc;
> +    switch(insn>>29) {

This would be easier if you simply looked at the entire major opcode field,
beginning at bit 26.

> +    case 0x5:
> +        switch((insn>>26) & 0x7) {
> +        case 0x0: /* SB */
> +        case 0x1: /* SH */
> +        case 0x2: /* SWL */
> +        case 0x3: /* SW */
> +        case 0x4: /* SDL */
> +        case 0x5: /* SDR */
> +        case 0x6: /* SWR */
> +            is_write = 1;
> +        }

So this becomes

    case 050: /* SB */
    case 051: /* SH */
      ...

I know there are some who don't like octal, but IMO MIPS and its 6 bit fields
and 8x8 tables is a natural fit -- one can read the two octal digits right off
of the table.

Otherwise, perhaps you'd prefer binary constants like 0b101000.  But with these
tables I find the mental bit-extract from hex to be tiresome.

> +        break;
> +    case 0x7:
> +        switch((insn>>26) & 0x7) {
> +        case 0x0: /* SC */
> +        case 0x1: /* SWC1 */
> +        case 0x4: /* SCD */
> +        case 0x5: /* SDC1 */
> +        case 0x7: /* SD */
> +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
> +        case 0x2: /* SWC2 */
> +        case 0x6: /* SDC2 */
> +#endif
> +            is_write = 1;

Similarly.

> +        }
> +        break;
> +    }
> +
> +    /*
> +     * Required in all versions of MIPS64 since MIPS64r1. Not available
> +     * in MIPS32r1. Required by MIPS32r2 and subsequent versions of MIPS32.
> +     */
> +    switch ((insn >> 3) & 0x7) {
> +    case 0x1:
> +        switch (insn & 0x7) {
> +        case 0x0: /* SWXC1 */
> +        case 0x1: /* SDXC1 */
> +            is_write = 1;
> +        }
> +        break;
> +    }

This switch is incorrectly placed.  It must be within the first switch, under
major opcode 023 (COP1X).  And again, you should extract the entire 6-bit minor
opcode all at once, not one octal digit at a time.

> +#elif defined(__misp16) || defined(__mips_micromips)
> +
> +#error "Unsupported encoding"

This is incorrectly placed, because we've already successfully entered the
preceeding #elif defined(__mips__).  This needs to be

#elif defined(__mips__)
# if defined(__mips16) || defined(__mips_micromips)
#  error
# endif

int cpu_signal_handler(int host_signum, void *pinfo,
                       void *puc)
{
   ...
}

#elif defined(__riscv)



r~
Kele Huang Sept. 25, 2020, 8:34 a.m. UTC | #5 
Got it. Thank you again!
I have resend a brand new v3 patch.

On Thu, 24 Sep 2020 at 22:05, Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/23/20 2:38 AM, Kele Huang wrote:
> > Detect mips store instructions in cpu_signal_handler for all MIPS
> > versions, and set is_write if encountering such store instructions.
> >
> > This fixed the error while dealing with self-modifed code for MIPS.
> >
> > Signed-off-by: Kele Huang <kele.hwang@gmail.com>
> > Signed-off-by: Xu Zou <iwatchnima@gmail.com>
> > ---
> >  accel/tcg/user-exec.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> > index bb039eb32d..18784516e5 100644
> > --- a/accel/tcg/user-exec.c
> > +++ b/accel/tcg/user-exec.c
> > @@ -710,11 +710,60 @@ int cpu_signal_handler(int host_signum, void
> *pinfo,
> >      greg_t pc = uc->uc_mcontext.pc;
> >      int is_write;
> >
> > -    /* XXX: compute is_write */
> >      is_write = 0;
> > +
> > +    /* Detect store by reading the instruction at the program counter.
> */
> > +    uint32_t insn = *(uint32_t *)pc;
> > +    switch(insn>>29) {
>
> This would be easier if you simply looked at the entire major opcode field,
> beginning at bit 26.
>
> > +    case 0x5:
> > +        switch((insn>>26) & 0x7) {
> > +        case 0x0: /* SB */
> > +        case 0x1: /* SH */
> > +        case 0x2: /* SWL */
> > +        case 0x3: /* SW */
> > +        case 0x4: /* SDL */
> > +        case 0x5: /* SDR */
> > +        case 0x6: /* SWR */
> > +            is_write = 1;
> > +        }
>
> So this becomes
>
>     case 050: /* SB */
>     case 051: /* SH */
>       ...
>
> I know there are some who don't like octal, but IMO MIPS and its 6 bit
> fields
> and 8x8 tables is a natural fit -- one can read the two octal digits right
> off
> of the table.
>
> Otherwise, perhaps you'd prefer binary constants like 0b101000.  But with
> these
> tables I find the mental bit-extract from hex to be tiresome.
>
> > +        break;
> > +    case 0x7:
> > +        switch((insn>>26) & 0x7) {
> > +        case 0x0: /* SC */
> > +        case 0x1: /* SWC1 */
> > +        case 0x4: /* SCD */
> > +        case 0x5: /* SDC1 */
> > +        case 0x7: /* SD */
> > +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
> > +        case 0x2: /* SWC2 */
> > +        case 0x6: /* SDC2 */
> > +#endif
> > +            is_write = 1;
>
> Similarly.
>
> > +        }
> > +        break;
> > +    }
> > +
> > +    /*
> > +     * Required in all versions of MIPS64 since MIPS64r1. Not available
> > +     * in MIPS32r1. Required by MIPS32r2 and subsequent versions of
> MIPS32.
> > +     */
> > +    switch ((insn >> 3) & 0x7) {
> > +    case 0x1:
> > +        switch (insn & 0x7) {
> > +        case 0x0: /* SWXC1 */
> > +        case 0x1: /* SDXC1 */
> > +            is_write = 1;
> > +        }
> > +        break;
> > +    }
>
> This switch is incorrectly placed.  It must be within the first switch,
> under
> major opcode 023 (COP1X).  And again, you should extract the entire 6-bit
> minor
> opcode all at once, not one octal digit at a time.
>
> > +#elif defined(__misp16) || defined(__mips_micromips)
> > +
> > +#error "Unsupported encoding"
>
> This is incorrectly placed, because we've already successfully entered the
> preceeding #elif defined(__mips__).  This needs to be
>
> #elif defined(__mips__)
> # if defined(__mips16) || defined(__mips_micromips)
> #  error
> # endif
>
> int cpu_signal_handler(int host_signum, void *pinfo,
>                        void *puc)
> {
>    ...
> }
>
> #elif defined(__riscv)
>
>
>
> r~
>
diff mbox series

Patch

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index bb039eb32d..18784516e5 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -710,11 +710,60 @@  int cpu_signal_handler(int host_signum, void *pinfo,
     greg_t pc = uc->uc_mcontext.pc;
     int is_write;
 
-    /* XXX: compute is_write */
     is_write = 0;
+
+    /* Detect store by reading the instruction at the program counter. */
+    uint32_t insn = *(uint32_t *)pc;
+    switch(insn>>29) {
+    case 0x5:
+        switch((insn>>26) & 0x7) {
+        case 0x0: /* SB */
+        case 0x1: /* SH */
+        case 0x2: /* SWL */
+        case 0x3: /* SW */
+        case 0x4: /* SDL */
+        case 0x5: /* SDR */
+        case 0x6: /* SWR */
+            is_write = 1;
+        }
+        break;
+    case 0x7:
+        switch((insn>>26) & 0x7) {
+        case 0x0: /* SC */
+        case 0x1: /* SWC1 */
+        case 0x4: /* SCD */
+        case 0x5: /* SDC1 */
+        case 0x7: /* SD */
+#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
+        case 0x2: /* SWC2 */
+        case 0x6: /* SDC2 */
+#endif
+            is_write = 1;
+        }
+        break;
+    }
+
+    /*
+     * Required in all versions of MIPS64 since MIPS64r1. Not available
+     * in MIPS32r1. Required by MIPS32r2 and subsequent versions of MIPS32.
+     */
+    switch ((insn >> 3) & 0x7) {
+    case 0x1:
+        switch (insn & 0x7) {
+        case 0x0: /* SWXC1 */
+        case 0x1: /* SDXC1 */
+            is_write = 1;
+        }
+        break;
+    }
+
     return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }
 
+#elif defined(__misp16) || defined(__mips_micromips)
+
+#error "Unsupported encoding"
+
 #elif defined(__riscv)
 
 int cpu_signal_handler(int host_signum, void *pinfo,