Got it. Thank you again!
I have resend a brand new v3 patch.
On Thu, 24 Sep 2020 at 22:05, Richard Henderson <
richard.hender...@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.hw...@gmail.com>
> > Signed-off-by: Xu Zou <iwatchn...@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~
>
