* [PATCH 0/1] accel/tcg: Fix computing is_write for mips [not found] <503406> @ 2020-09-10 7:43 ` Kele Huang 2020-09-10 7:43 ` [PATCH 1/1] " Kele Huang 2020-09-23 9:38 ` [PATCH v2 1/1] accel/tcg: Fix computing of " Kele Huang 1 sibling, 1 reply; 10+ messages in thread From: Kele Huang @ 2020-09-10 7:43 UTC (permalink / raw) To: qemu-devel; +Cc: Kele Huang Detect mips store instructions SWXC1 and SDXC1 for MIPS64 since MIPS64r1, and MIPS32 since MIPS32r2. Can work better with self-modifying codes (SMC) now. Kele Huang (1): accel/tcg: Fix computing is_write for mips accel/tcg/user-exec.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] accel/tcg: Fix computing is_write for mips 2020-09-10 7:43 ` [PATCH 0/1] accel/tcg: Fix computing is_write for mips Kele Huang @ 2020-09-10 7:43 ` Kele Huang 2020-09-10 17:18 ` Richard Henderson 0 siblings, 1 reply; 10+ messages in thread From: Kele Huang @ 2020-09-10 7:43 UTC (permalink / raw) To: qemu-devel; +Cc: Kele Huang 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; + } + return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] accel/tcg: Fix computing is_write for mips 2020-09-10 7:43 ` [PATCH 1/1] " Kele Huang @ 2020-09-10 17:18 ` Richard Henderson 2020-09-24 8:59 ` Kele Huang 0 siblings, 1 reply; 10+ messages in thread From: Richard Henderson @ 2020-09-10 17:18 UTC (permalink / raw) To: Kele Huang, qemu-devel 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~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] accel/tcg: Fix computing is_write for mips 2020-09-10 17:18 ` Richard Henderson @ 2020-09-24 8:59 ` Kele Huang 0 siblings, 0 replies; 10+ messages in thread From: Kele Huang @ 2020-09-24 8:59 UTC (permalink / raw) To: Richard Henderson, peter.maydell, qemu-devel, Philippe Mathieu-Daudé, Xu Zou [-- Attachment #1: Type: text/plain, Size: 3948 bytes --] 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~ > [-- Attachment #2: Type: text/html, Size: 5257 bytes --] ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 1/1] accel/tcg: Fix computing of is_write for mips [not found] <503406> 2020-09-10 7:43 ` [PATCH 0/1] accel/tcg: Fix computing is_write for mips Kele Huang @ 2020-09-23 9:38 ` Kele Huang 2020-09-23 11:08 ` Philippe Mathieu-Daudé 2020-09-24 14:05 ` Richard Henderson 1 sibling, 2 replies; 10+ messages in thread From: Kele Huang @ 2020-09-23 9:38 UTC (permalink / raw) To: qemu-devel; +Cc: Xu Zou, Kele Huang 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) { + 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] accel/tcg: Fix computing of is_write for mips 2020-09-23 9:38 ` [PATCH v2 1/1] accel/tcg: Fix computing of " Kele Huang @ 2020-09-23 11:08 ` Philippe Mathieu-Daudé 2020-09-24 8:52 ` Kele Huang 2020-09-24 10:01 ` Kele Huang 2020-09-24 14:05 ` Richard Henderson 1 sibling, 2 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-23 11:08 UTC (permalink / raw) To: Kele Huang, qemu-devel Cc: Richard Henderson, Aleksandar Markovic, Xu Zou, Aleksandar Rikalo 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, > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] accel/tcg: Fix computing of is_write for mips 2020-09-23 11:08 ` Philippe Mathieu-Daudé @ 2020-09-24 8:52 ` Kele Huang 2020-09-24 10:01 ` Kele Huang 1 sibling, 0 replies; 10+ messages in thread From: Kele Huang @ 2020-09-24 8:52 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Aleksandar Rikalo, Xu Zou, Richard Henderson, qemu-devel, Aleksandar Markovic [-- Attachment #1: Type: text/plain, Size: 3130 bytes --] 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, > > > > [-- Attachment #2: Type: text/html, Size: 4216 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] accel/tcg: Fix computing of is_write for mips 2020-09-23 11:08 ` Philippe Mathieu-Daudé 2020-09-24 8:52 ` Kele Huang @ 2020-09-24 10:01 ` Kele Huang 1 sibling, 0 replies; 10+ messages in thread From: Kele Huang @ 2020-09-24 10:01 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3221 bytes --] 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, > > > > [-- Attachment #2: Type: text/html, Size: 4300 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] accel/tcg: Fix computing of is_write for mips 2020-09-23 9:38 ` [PATCH v2 1/1] accel/tcg: Fix computing of " Kele Huang 2020-09-23 11:08 ` Philippe Mathieu-Daudé @ 2020-09-24 14:05 ` Richard Henderson 2020-09-25 8:34 ` Kele Huang 1 sibling, 1 reply; 10+ messages in thread From: Richard Henderson @ 2020-09-24 14:05 UTC (permalink / raw) To: Kele Huang, qemu-devel; +Cc: Xu Zou 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~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] accel/tcg: Fix computing of is_write for mips 2020-09-24 14:05 ` Richard Henderson @ 2020-09-25 8:34 ` Kele Huang 0 siblings, 0 replies; 10+ messages in thread From: Kele Huang @ 2020-09-25 8:34 UTC (permalink / raw) To: Richard Henderson; +Cc: Xu Zou, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3659 bytes --] 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~ > [-- Attachment #2: Type: text/html, Size: 4903 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-25 8:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <503406>
2020-09-10 7:43 ` [PATCH 0/1] accel/tcg: Fix computing is_write for mips Kele Huang
2020-09-10 7:43 ` [PATCH 1/1] " Kele Huang
2020-09-10 17:18 ` Richard Henderson
2020-09-24 8:59 ` Kele Huang
2020-09-23 9:38 ` [PATCH v2 1/1] accel/tcg: Fix computing of " Kele Huang
2020-09-23 11:08 ` Philippe Mathieu-Daudé
2020-09-24 8:52 ` Kele Huang
2020-09-24 10:01 ` Kele Huang
2020-09-24 14:05 ` Richard Henderson
2020-09-25 8:34 ` Kele Huang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).