diff mbox series

[v3,1/1] accel/tcg: Fix computing of is_write for MIPS

Message ID 20200925083307.13761-1-kele.hwang@gmail.com
State Superseded
Headers show
Series [v3,1/1] accel/tcg: Fix computing of is_write for MIPS | expand

Commit Message

Kele Huang Sept. 25, 2020, 8:33 a.m. UTC 
Detect all MIPS store instructions in cpu_signal_handler for all available
MIPS versions, and set is_write if encountering such store instructions.

This fixed the error while dealing with self-modified 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 | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Sept. 25, 2020, 9:05 a.m. UTC | #1 
Cc'ing TCG MIPS maintainers *again*.

On 9/25/20 10:33 AM, Kele Huang wrote:
> Detect all MIPS store instructions in cpu_signal_handler for all available

> MIPS versions, and set is_write if encountering such store instructions.

> 

> This fixed the error while dealing with self-modified 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 | 38 +++++++++++++++++++++++++++++++++++++-

>  1 file changed, 37 insertions(+), 1 deletion(-)

> 

> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c

> index bb039eb32d..c4494c93e7 100644

> --- a/accel/tcg/user-exec.c

> +++ b/accel/tcg/user-exec.c

> @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void *pinfo,

>  

>  #elif defined(__mips__)

>  

> +#if defined(__misp16) || defined(__mips_micromips)

> +#error "Unsupported encoding"

> +#endif

> +

>  int cpu_signal_handler(int host_signum, void *pinfo,

>                         void *puc)

>  {

> @@ -709,9 +713,41 @@ int cpu_signal_handler(int host_signum, void *pinfo,

>      ucontext_t *uc = puc;

>      greg_t pc = uc->uc_mcontext.pc;

>      int is_write;

> +    uint32_t insn;

>  

> -    /* XXX: compute is_write */

> +    /* Detect all store instructions at program counter. */

>      is_write = 0;

> +    insn = *(uint32_t *)pc;

> +    switch((insn >> 26) & 077) {

> +    case 050: /* SB */

> +    case 051: /* SH */

> +    case 052: /* SWL */

> +    case 053: /* SW */

> +    case 054: /* SDL */

> +    case 055: /* SDR */

> +    case 056: /* SWR */

> +    case 070: /* SC */

> +    case 071: /* SWC1 */

> +    case 074: /* SCD */

> +    case 075: /* SDC1 */

> +    case 077: /* SD */

> +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6

> +    case 072: /* SWC2 */

> +    case 076: /* SDC2 */

> +#endif

> +        is_write = 1;

> +        break;

> +    case 023: /* COP1X */

> +        /* Required in all versions of MIPS64 since 

> +           MIPS64r1 and subsequent versions of MIPS32. */

> +        switch (insn & 077) {

> +        case 010: /* SWXC1 */

> +        case 011: /* SDXC1 */

> +            is_write = 1;

> +        }

> +        break;

> +    }

> +

>      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);

>  }

>  

>
Richard Henderson Sept. 25, 2020, 2:58 p.m. UTC | #2 
On 9/25/20 1:33 AM, Kele Huang wrote:
> Detect all MIPS store instructions in cpu_signal_handler for all available
> MIPS versions, and set is_write if encountering such store instructions.
> 
> This fixed the error while dealing with self-modified 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 | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index bb039eb32d..c4494c93e7 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>  
>  #elif defined(__mips__)
>  
> +#if defined(__misp16) || defined(__mips_micromips)
> +#error "Unsupported encoding"
> +#endif
> +
>  int cpu_signal_handler(int host_signum, void *pinfo,
>                         void *puc)
>  {
> @@ -709,9 +713,41 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      ucontext_t *uc = puc;
>      greg_t pc = uc->uc_mcontext.pc;
>      int is_write;
> +    uint32_t insn;
>  
> -    /* XXX: compute is_write */
> +    /* Detect all store instructions at program counter. */
>      is_write = 0;
> +    insn = *(uint32_t *)pc;
> +    switch((insn >> 26) & 077) {
> +    case 050: /* SB */
> +    case 051: /* SH */
> +    case 052: /* SWL */
> +    case 053: /* SW */
> +    case 054: /* SDL */
> +    case 055: /* SDR */
> +    case 056: /* SWR */
> +    case 070: /* SC */
> +    case 071: /* SWC1 */
> +    case 074: /* SCD */
> +    case 075: /* SDC1 */
> +    case 077: /* SD */
> +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
> +    case 072: /* SWC2 */
> +    case 076: /* SDC2 */
> +#endif
> +        is_write = 1;
> +        break;
> +    case 023: /* COP1X */
> +        /* Required in all versions of MIPS64 since 
> +           MIPS64r1 and subsequent versions of MIPS32. */
> +        switch (insn & 077) {
> +        case 010: /* SWXC1 */
> +        case 011: /* SDXC1 */
> +            is_write = 1;

Much better.  I just noticed you're missing SUXC1 (COP1X minor 015).  With that
fixed,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Kele Huang Sept. 27, 2020, 8:22 a.m. UTC | #3 
Fixed! I have resent a v4 patch which contains SUXC1.
Thank you!

On Fri, 25 Sep 2020 at 22:58, Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/25/20 1:33 AM, Kele Huang wrote:

> > Detect all MIPS store instructions in cpu_signal_handler for all

> available

> > MIPS versions, and set is_write if encountering such store instructions.

> >

> > This fixed the error while dealing with self-modified 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 | 38 +++++++++++++++++++++++++++++++++++++-

> >  1 file changed, 37 insertions(+), 1 deletion(-)

> >

> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c

> > index bb039eb32d..c4494c93e7 100644

> > --- a/accel/tcg/user-exec.c

> > +++ b/accel/tcg/user-exec.c

> > @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void *pinfo,

> >

> >  #elif defined(__mips__)

> >

> > +#if defined(__misp16) || defined(__mips_micromips)

> > +#error "Unsupported encoding"

> > +#endif

> > +

> >  int cpu_signal_handler(int host_signum, void *pinfo,

> >                         void *puc)

> >  {

> > @@ -709,9 +713,41 @@ int cpu_signal_handler(int host_signum, void *pinfo,

> >      ucontext_t *uc = puc;

> >      greg_t pc = uc->uc_mcontext.pc;

> >      int is_write;

> > +    uint32_t insn;

> >

> > -    /* XXX: compute is_write */

> > +    /* Detect all store instructions at program counter. */

> >      is_write = 0;

> > +    insn = *(uint32_t *)pc;

> > +    switch((insn >> 26) & 077) {

> > +    case 050: /* SB */

> > +    case 051: /* SH */

> > +    case 052: /* SWL */

> > +    case 053: /* SW */

> > +    case 054: /* SDL */

> > +    case 055: /* SDR */

> > +    case 056: /* SWR */

> > +    case 070: /* SC */

> > +    case 071: /* SWC1 */

> > +    case 074: /* SCD */

> > +    case 075: /* SDC1 */

> > +    case 077: /* SD */

> > +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6

> > +    case 072: /* SWC2 */

> > +    case 076: /* SDC2 */

> > +#endif

> > +        is_write = 1;

> > +        break;

> > +    case 023: /* COP1X */

> > +        /* Required in all versions of MIPS64 since

> > +           MIPS64r1 and subsequent versions of MIPS32. */

> > +        switch (insn & 077) {

> > +        case 010: /* SWXC1 */

> > +        case 011: /* SDXC1 */

> > +            is_write = 1;

>

> Much better.  I just noticed you're missing SUXC1 (COP1X minor 015).  With

> that

> fixed,

>

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

>

> r~

>
<div dir="ltr">Fixed! I have resent a v4 patch which contains SUXC1.<div>Thank you!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 25 Sep 2020 at 22:58, Richard Henderson &lt;<a href="mailto:richard.henderson@linaro.org">richard.henderson@linaro.org</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 9/25/20 1:33 AM, Kele Huang wrote:<br>
&gt; Detect all MIPS store instructions in cpu_signal_handler for all available<br>
&gt; MIPS versions, and set is_write if encountering such store instructions.<br>
&gt; <br>
&gt; This fixed the error while dealing with self-modified code for MIPS.<br>
&gt; <br>
&gt; Signed-off-by: Kele Huang &lt;<a href="mailto:kele.hwang@gmail.com" target="_blank">kele.hwang@gmail.com</a>&gt;<br>
&gt; Signed-off-by: Xu Zou &lt;<a href="mailto:iwatchnima@gmail.com" target="_blank">iwatchnima@gmail.com</a>&gt;<br>
&gt; ---<br>
&gt;  accel/tcg/user-exec.c | 38 +++++++++++++++++++++++++++++++++++++-<br>
&gt;  1 file changed, 37 insertions(+), 1 deletion(-)<br>
&gt; <br>
&gt; diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c<br>
&gt; index bb039eb32d..c4494c93e7 100644<br>
&gt; --- a/accel/tcg/user-exec.c<br>
&gt; +++ b/accel/tcg/user-exec.c<br>
&gt; @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void *pinfo,<br>
&gt;  <br>
&gt;  #elif defined(__mips__)<br>
&gt;  <br>
&gt; +#if defined(__misp16) || defined(__mips_micromips)<br>
&gt; +#error &quot;Unsupported encoding&quot;<br>
&gt; +#endif<br>
&gt; +<br>
&gt;  int cpu_signal_handler(int host_signum, void *pinfo,<br>
&gt;                         void *puc)<br>
&gt;  {<br>
&gt; @@ -709,9 +713,41 @@ int cpu_signal_handler(int host_signum, void *pinfo,<br>
&gt;      ucontext_t *uc = puc;<br>
&gt;      greg_t pc = uc-&gt;uc_mcontext.pc;<br>
&gt;      int is_write;<br>
&gt; +    uint32_t insn;<br>
&gt;  <br>
&gt; -    /* XXX: compute is_write */<br>
&gt; +    /* Detect all store instructions at program counter. */<br>
&gt;      is_write = 0;<br>
&gt; +    insn = *(uint32_t *)pc;<br>
&gt; +    switch((insn &gt;&gt; 26) &amp; 077) {<br>
&gt; +    case 050: /* SB */<br>
&gt; +    case 051: /* SH */<br>
&gt; +    case 052: /* SWL */<br>
&gt; +    case 053: /* SW */<br>
&gt; +    case 054: /* SDL */<br>
&gt; +    case 055: /* SDR */<br>
&gt; +    case 056: /* SWR */<br>
&gt; +    case 070: /* SC */<br>
&gt; +    case 071: /* SWC1 */<br>
&gt; +    case 074: /* SCD */<br>
&gt; +    case 075: /* SDC1 */<br>
&gt; +    case 077: /* SD */<br>
&gt; +#if !defined(__mips_isa_rev) || __mips_isa_rev &lt; 6<br>
&gt; +    case 072: /* SWC2 */<br>
&gt; +    case 076: /* SDC2 */<br>
&gt; +#endif<br>
&gt; +        is_write = 1;<br>
&gt; +        break;<br>
&gt; +    case 023: /* COP1X */<br>
&gt; +        /* Required in all versions of MIPS64 since <br>
&gt; +           MIPS64r1 and subsequent versions of MIPS32. */<br>
&gt; +        switch (insn &amp; 077) {<br>
&gt; +        case 010: /* SWXC1 */<br>
&gt; +        case 011: /* SDXC1 */<br>
&gt; +            is_write = 1;<br>
<br>
Much better.  I just noticed you&#39;re missing SUXC1 (COP1X minor 015).  With that<br>
fixed,<br>
<br>
Reviewed-by: Richard Henderson &lt;<a href="mailto:richard.henderson@linaro.org" target="_blank">richard.henderson@linaro.org</a>&gt;<br>

<br>
r~<br>
</blockquote></div>
diff mbox series

Patch

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index bb039eb32d..c4494c93e7 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -702,6 +702,10 @@  int cpu_signal_handler(int host_signum, void *pinfo,
 
 #elif defined(__mips__)
 
+#if defined(__misp16) || defined(__mips_micromips)
+#error "Unsupported encoding"
+#endif
+
 int cpu_signal_handler(int host_signum, void *pinfo,
                        void *puc)
 {
@@ -709,9 +713,41 @@  int cpu_signal_handler(int host_signum, void *pinfo,
     ucontext_t *uc = puc;
     greg_t pc = uc->uc_mcontext.pc;
     int is_write;
+    uint32_t insn;
 
-    /* XXX: compute is_write */
+    /* Detect all store instructions at program counter. */
     is_write = 0;
+    insn = *(uint32_t *)pc;
+    switch((insn >> 26) & 077) {
+    case 050: /* SB */
+    case 051: /* SH */
+    case 052: /* SWL */
+    case 053: /* SW */
+    case 054: /* SDL */
+    case 055: /* SDR */
+    case 056: /* SWR */
+    case 070: /* SC */
+    case 071: /* SWC1 */
+    case 074: /* SCD */
+    case 075: /* SDC1 */
+    case 077: /* SD */
+#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
+    case 072: /* SWC2 */
+    case 076: /* SDC2 */
+#endif
+        is_write = 1;
+        break;
+    case 023: /* COP1X */
+        /* Required in all versions of MIPS64 since 
+           MIPS64r1 and subsequent versions of MIPS32. */
+        switch (insn & 077) {
+        case 010: /* SWXC1 */
+        case 011: /* SDXC1 */
+            is_write = 1;
+        }
+        break;
+    }
+
     return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }