* [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
* [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 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
* 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).