| Message ID | 20200910074342.20421-2-kele.hwang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg: Fix computing is_write for mips | expand |
On 9/10/20 12:43 AM, Kele Huang wrote: > Detect mips store instructions SWXC1 and SDXC1 for MIPS64 since > MIPS64r1, and MIPS32 since MIPS32r2. > > Signed-off-by: Kele Huang <kele.hwang@gmail.com> > --- > accel/tcg/user-exec.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c > index bb039eb32d..e69b4d8780 100644 > --- a/accel/tcg/user-exec.c > +++ b/accel/tcg/user-exec.c > @@ -712,6 +712,27 @@ int cpu_signal_handler(int host_signum, void *pinfo, > > /* XXX: compute is_write */ > is_write = 0; > + > + /* > + * Detect store instructions. 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; > + default: > + break; > + } > + break; > + default: > + break; You should detect all of the store instructions, not just the coprocessor ones. Compare, for example, the Sparc version around line 485. Once done, you can also remove that /* XXX */ comment just above, which indicates that there is work that needs doing. r~
Sorry for the late reply. We make a new version submit as below. Subject: [PATCH v2 1/1] accel/tcg: Fix computing of is_write for mips 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-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 | 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, -- 2.17.1 On Fri, 11 Sep 2020 at 01:18, Richard Henderson < richard.henderson@linaro.org> wrote: > On 9/10/20 12:43 AM, Kele Huang wrote: > > Detect mips store instructions SWXC1 and SDXC1 for MIPS64 since > > MIPS64r1, and MIPS32 since MIPS32r2. > > > > Signed-off-by: Kele Huang <kele.hwang@gmail.com> > > --- > > accel/tcg/user-exec.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c > > index bb039eb32d..e69b4d8780 100644 > > --- a/accel/tcg/user-exec.c > > +++ b/accel/tcg/user-exec.c > > @@ -712,6 +712,27 @@ int cpu_signal_handler(int host_signum, void *pinfo, > > > > /* XXX: compute is_write */ > > is_write = 0; > > + > > + /* > > + * Detect store instructions. 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; > > + default: > > + break; > > + } > > + break; > > + default: > > + break; > > > You should detect all of the store instructions, not just the coprocessor > ones. > Compare, for example, the Sparc version around line 485. > > Once done, you can also remove that /* XXX */ comment just above, which > indicates that there is work that needs doing. > > > r~ > <div dir="ltr"><div>Sorry for the late reply. We make a new version submit as below.</div><div><br></div>Subject: [PATCH v2 1/1] accel/tcg: Fix computing of is_write for mips<br><br>Detect mips store instructions in cpu_signal_handler for all MIPS<br>versions, and set is_write if encountering such store instructions.<br><br>This fixed the error while dealing with self-modified code for MIPS.<br><br>Signed-off-by: Kele Huang <<a href="mailto:kele.hwang@gmail.com">kele.hwang@gmail.com</a>><br>Signed-off-by: Xu Zou <<a href="mailto:iwatchnima@gmail.com">iwatchnima@gmail.com</a>><br>---<br> accel/tcg/user-exec.c | 51 ++++++++++++++++++++++++++++++++++++++++++-<br> 1 file changed, 50 insertions(+), 1 deletion(-)<br><br>diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c<br>index bb039eb32d..18784516e5 100644<br>--- a/accel/tcg/user-exec.c<br>+++ b/accel/tcg/user-exec.c<br>@@ -710,11 +710,60 @@ int cpu_signal_handler(int host_signum, void *pinfo,<br> greg_t pc = uc->uc_mcontext.pc;<br> int is_write;<br> <br>- /* XXX: compute is_write */<br> is_write = 0;<br>+<br>+ /* Detect store by reading the instruction at the program counter. */<br>+ uint32_t insn = *(uint32_t *)pc;<br>+ switch(insn>>29) {<br>+ case 0x5:<br>+ switch((insn>>26) & 0x7) {<br>+ case 0x0: /* SB */<br>+ case 0x1: /* SH */<br>+ case 0x2: /* SWL */<br>+ case 0x3: /* SW */<br>+ case 0x4: /* SDL */<br>+ case 0x5: /* SDR */<br>+ case 0x6: /* SWR */<br>+ is_write = 1;<br>+ }<br>+ break;<br>+ case 0x7:<br>+ switch((insn>>26) & 0x7) {<br>+ case 0x0: /* SC */<br>+ case 0x1: /* SWC1 */<br>+ case 0x4: /* SCD */<br>+ case 0x5: /* SDC1 */<br>+ case 0x7: /* SD */<br>+#if !defined(__mips_isa_rev) || __mips_isa_rev < 6<br>+ case 0x2: /* SWC2 */<br>+ case 0x6: /* SDC2 */<br>+#endif<br>+ is_write = 1;<br>+ }<br>+ break;<br>+ }<br>+<br>+ /*<br>+ * Required in all versions of MIPS64 since MIPS64r1. Not available<br>+ * in MIPS32r1. Required by MIPS32r2 and subsequent versions of MIPS32.<br>+ */<br>+ switch ((insn >> 3) & 0x7) {<br>+ case 0x1:<br>+ switch (insn & 0x7) {<br>+ case 0x0: /* SWXC1 */<br>+ case 0x1: /* SDXC1 */<br>+ is_write = 1;<br>+ }<br>+ break;<br>+ }<br>+<br> return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);<br> }<br> <br>+#elif defined(__misp16) || defined(__mips_micromips)<br>+<br>+#error "Unsupported encoding"<br>+<br> #elif defined(__riscv)<br> <br> int cpu_signal_handler(int host_signum, void *pinfo,<br>-- <br>2.17.1<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 11 Sep 2020 at 01:18, Richard Henderson <<a href="mailto:richard.henderson@linaro.org">richard.henderson@linaro.org</a>> 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/10/20 12:43 AM, Kele Huang wrote:<br> > Detect mips store instructions SWXC1 and SDXC1 for MIPS64 since<br> > MIPS64r1, and MIPS32 since MIPS32r2.<br> > <br> > Signed-off-by: Kele Huang <<a href="mailto:kele.hwang@gmail.com" target="_blank">kele.hwang@gmail.com</a>><br> > ---<br> > accel/tcg/user-exec.c | 21 +++++++++++++++++++++<br> > 1 file changed, 21 insertions(+)<br> > <br> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c<br> > index bb039eb32d..e69b4d8780 100644<br> > --- a/accel/tcg/user-exec.c<br> > +++ b/accel/tcg/user-exec.c<br> > @@ -712,6 +712,27 @@ int cpu_signal_handler(int host_signum, void *pinfo,<br> > <br> > /* XXX: compute is_write */<br> > is_write = 0;<br> > +<br> > + /*<br> > + * Detect store instructions. Required in all versions of MIPS64<br> > + * since MIPS64r1. Not available in MIPS32r1. Required by MIPS32r2<br> > + * and subsequent versions of MIPS32.<br> > + */<br> > + switch ((insn >> 3) & 0x7) {<br> > + case 0x1:<br> > + switch (insn & 0x7) {<br> > + case 0x0: /* SWXC1 */<br> > + case 0x1: /* SDXC1 */<br> > + is_write = 1;<br> > + break;<br> > + default:<br> > + break;<br> > + }<br> > + break;<br> > + default:<br> > + break;<br> <br> <br> You should detect all of the store instructions, not just the coprocessor ones.<br> Compare, for example, the Sparc version around line 485.<br> <br> Once done, you can also remove that /* XXX */ comment just above, which<br> indicates that there is work that needs doing.<br> <br> <br> r~<br> </blockquote></div>
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index bb039eb32d..e69b4d8780 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -712,6 +712,27 @@ int cpu_signal_handler(int host_signum, void *pinfo, /* XXX: compute is_write */ is_write = 0; + + /* + * Detect store instructions. 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; + default: + break; + } + break; + default: + break; + } + return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); }
Detect mips store instructions SWXC1 and SDXC1 for MIPS64 since MIPS64r1, and MIPS32 since MIPS32r2. Signed-off-by: Kele Huang <kele.hwang@gmail.com> --- accel/tcg/user-exec.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)