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, > > > >
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, > > > >
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~ >
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,