| 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 |
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, >
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, > > > > <div dir="ltr">Got it. Thank you very much! <br><div>I will resend the same v2 patch to v1 thread.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 23 Sep 2020 at 19:08, Philippe Mathieu-Daudé <<a href="mailto:f4bug@amsat.org">f4bug@amsat.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">Cc'ing the TCG MIPS maintainers, and also<br> Cc'ing Richard who made a comment in v1.<br> <br> On 9/23/20 11:38 AM, Kele Huang wrote:<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-modifed code for MIPS.<br> <br> Quoting Eric Blake:<br> <br> "It's better to post a v2 as a new top-level thread rather<br> than buried in-reply-to the v1 thread; among other things,<br> burying a reply can cause automated patch tooling to miss<br> the updated series."<br> <br> > <br> > Signed-off-by: Kele Huang <<a href="mailto:kele.hwang@gmail.com" target="_blank">kele.hwang@gmail.com</a>><br> > Signed-off-by: Xu Zou <<a href="mailto:iwatchnima@gmail.com" target="_blank">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> <br> </blockquote></div>
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, > > > > <div dir="ltr">Sorry about my misunderstanding of your guidelines.<div>What is more, I have resend a new v2 patch as a new top-level thread and CC to TCG MIPS maintainers and Richard.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 23 Sep 2020 at 19:08, Philippe Mathieu-Daudé <<a href="mailto:f4bug@amsat.org">f4bug@amsat.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">Cc'ing the TCG MIPS maintainers, and also<br> Cc'ing Richard who made a comment in v1.<br> <br> On 9/23/20 11:38 AM, Kele Huang wrote:<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-modifed code for MIPS.<br> <br> Quoting Eric Blake:<br> <br> "It's better to post a v2 as a new top-level thread rather<br> than buried in-reply-to the v1 thread; among other things,<br> burying a reply can cause automated patch tooling to miss<br> the updated series."<br> <br> > <br> > Signed-off-by: Kele Huang <<a href="mailto:kele.hwang@gmail.com" target="_blank">kele.hwang@gmail.com</a>><br> > Signed-off-by: Xu Zou <<a href="mailto:iwatchnima@gmail.com" target="_blank">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> <br> </blockquote></div>
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~
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~ > <div dir="ltr">Got it. Thank you again!<br><div>I have resend a brand new v3 patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 24 Sep 2020 at 22:05, 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/23/20 2:38 AM, Kele Huang wrote:<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-modifed code for MIPS.<br> > <br> > Signed-off-by: Kele Huang <<a href="mailto:kele.hwang@gmail.com" target="_blank">kele.hwang@gmail.com</a>><br> > Signed-off-by: Xu Zou <<a href="mailto:iwatchnima@gmail.com" target="_blank">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> <br> This would be easier if you simply looked at the entire major opcode field,<br> beginning at bit 26.<br> <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> <br> So this becomes<br> <br> case 050: /* SB */<br> case 051: /* SH */<br> ...<br> <br> I know there are some who don't like octal, but IMO MIPS and its 6 bit fields<br> and 8x8 tables is a natural fit -- one can read the two octal digits right off<br> of the table.<br> <br> Otherwise, perhaps you'd prefer binary constants like 0b101000. But with these<br> tables I find the mental bit-extract from hex to be tiresome.<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> Similarly.<br> <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> This switch is incorrectly placed. It must be within the first switch, under<br> major opcode 023 (COP1X). And again, you should extract the entire 6-bit minor<br> opcode all at once, not one octal digit at a time.<br> <br> > +#elif defined(__misp16) || defined(__mips_micromips)<br> > +<br> > +#error "Unsupported encoding"<br> <br> This is incorrectly placed, because we've already successfully entered the<br> preceeding #elif defined(__mips__). This needs to be<br> <br> #elif defined(__mips__)<br> # if defined(__mips16) || defined(__mips_micromips)<br> # error<br> # endif<br> <br> int cpu_signal_handler(int host_signum, void *pinfo,<br> void *puc)<br> {<br> ...<br> }<br> <br> #elif defined(__riscv)<br> <br> <br> <br> r~<br> </blockquote></div>
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,