[v4,1/1] accel/tcg: Fix computing of is_write for MIPS

Message ID	20200927082033.8716-1-kele.hwang@gmail.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> From: Kele Huang <kele.hwang@gmail.com> To: qemu-devel@nongnu.org Subject: [PATCH v4 1/1] accel/tcg: Fix computing of is_write for MIPS Date: Sun, 27 Sep 2020 16:20:33 +0800 Message-Id: <20200927082033.8716-1-kele.hwang@gmail.com> Received-SPF: pass client-ip=2607:f8b0:4864:20::42f; envelope-from=kele.hwang@gmail.com; helo=mail-pf1-x42f.google.com Precedence: list Cc: Paolo Bonzini <pbonzini@redhat.com>, Riku Voipio <riku.voipio@iki.fi>, Richard Henderson <richard.henderson@linaro.org>, Kele Huang <kele.hwang@gmail.com>, Xu Zou <iwatchnima@gmail.com> Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>
Series	[v4,1/1] accel/tcg: Fix computing of is_write for MIPS \| expand [v4,1/1] accel/tcg: Fix computing of is_write for MIPS

Kele Huang Sept. 27, 2020, 8:20 a.m. UTC

Detect all MIPS store instructions in cpu_signal_handler for all available
MIPS versions, and set is_write if encountering such store instructions.

This fixed the error while dealing with self-modified code for MIPS.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Kele Huang <kele.hwang@gmail.com>
Signed-off-by: Xu Zou <iwatchnima@gmail.com>
---
 accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

Philippe Mathieu-Daudé Sept. 27, 2020, 8:41 a.m. UTC | #1

On 9/27/20 10:20 AM, Kele Huang wrote:
> Detect all MIPS store instructions in cpu_signal_handler for all available
> MIPS versions, and set is_write if encountering such store instructions.
> 
> This fixed the error while dealing with self-modified code for MIPS.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Kele Huang <kele.hwang@gmail.com>
> Signed-off-by: Xu Zou <iwatchnima@gmail.com>

I already Cc'ed the TCG MIPS maintainers twice for you,
but you don't mind, so this time I won't insist.

> ---
>  accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index bb039eb32d..9ecda6c0d0 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>  
>  #elif defined(__mips__)
>  
> +#if defined(__misp16) || defined(__mips_micromips)
> +#error "Unsupported encoding"
> +#endif
> +
>  int cpu_signal_handler(int host_signum, void *pinfo,
>                         void *puc)
>  {
> @@ -709,9 +713,42 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      ucontext_t *uc = puc;
>      greg_t pc = uc->uc_mcontext.pc;
>      int is_write;
> +    uint32_t insn;
>  
> -    /* XXX: compute is_write */
> +    /* Detect all store instructions at program counter. */
>      is_write = 0;
> +    insn = *(uint32_t *)pc;
> +    switch((insn >> 26) & 077) {
> +    case 050: /* SB */
> +    case 051: /* SH */
> +    case 052: /* SWL */
> +    case 053: /* SW */
> +    case 054: /* SDL */
> +    case 055: /* SDR */
> +    case 056: /* SWR */
> +    case 070: /* SC */
> +    case 071: /* SWC1 */
> +    case 074: /* SCD */
> +    case 075: /* SDC1 */
> +    case 077: /* SD */
> +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
> +    case 072: /* SWC2 */
> +    case 076: /* SDC2 */
> +#endif
> +        is_write = 1;
> +        break;
> +    case 023: /* COP1X */
> +        /* Required in all versions of MIPS64 since 
> +           MIPS64r1 and subsequent versions of MIPS32r2. */
> +        switch (insn & 077) {
> +        case 010: /* SWXC1 */
> +        case 011: /* SDXC1 */
> +        case 015: /* SDXC1 */
> +            is_write = 1;
> +        }
> +        break;
> +    }
> +
>      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
>  }
>  
>

Kele Huang Sept. 27, 2020, 9:49 a.m. UTC | #2

Sorry about that, I only got maintainers by './scripts/get_maintainer.pl -f
accel/tcg/user-exec.c' and missed your advice about maintainers.
In another words, I thought I had Cc'ed the TCG MIPS maintainers. 😅
And sorry to maintainers. 😅

On Sun, 27 Sep 2020 at 16:41, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 9/27/20 10:20 AM, Kele Huang wrote:
> > Detect all MIPS store instructions in cpu_signal_handler for all
> available
> > MIPS versions, and set is_write if encountering such store instructions.
> >
> > This fixed the error while dealing with self-modified code for MIPS.
> >
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Kele Huang <kele.hwang@gmail.com>
> > Signed-off-by: Xu Zou <iwatchnima@gmail.com>
>
> I already Cc'ed the TCG MIPS maintainers twice for you,
> but you don't mind, so this time I won't insist.
>
> > ---
> >  accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> > index bb039eb32d..9ecda6c0d0 100644
> > --- a/accel/tcg/user-exec.c
> > +++ b/accel/tcg/user-exec.c
> > @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void *pinfo,
> >
> >  #elif defined(__mips__)
> >
> > +#if defined(__misp16) || defined(__mips_micromips)
> > +#error "Unsupported encoding"
> > +#endif
> > +
> >  int cpu_signal_handler(int host_signum, void *pinfo,
> >                         void *puc)
> >  {
> > @@ -709,9 +713,42 @@ int cpu_signal_handler(int host_signum, void *pinfo,
> >      ucontext_t *uc = puc;
> >      greg_t pc = uc->uc_mcontext.pc;
> >      int is_write;
> > +    uint32_t insn;
> >
> > -    /* XXX: compute is_write */
> > +    /* Detect all store instructions at program counter. */
> >      is_write = 0;
> > +    insn = *(uint32_t *)pc;
> > +    switch((insn >> 26) & 077) {
> > +    case 050: /* SB */
> > +    case 051: /* SH */
> > +    case 052: /* SWL */
> > +    case 053: /* SW */
> > +    case 054: /* SDL */
> > +    case 055: /* SDR */
> > +    case 056: /* SWR */
> > +    case 070: /* SC */
> > +    case 071: /* SWC1 */
> > +    case 074: /* SCD */
> > +    case 075: /* SDC1 */
> > +    case 077: /* SD */
> > +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
> > +    case 072: /* SWC2 */
> > +    case 076: /* SDC2 */
> > +#endif
> > +        is_write = 1;
> > +        break;
> > +    case 023: /* COP1X */
> > +        /* Required in all versions of MIPS64 since
> > +           MIPS64r1 and subsequent versions of MIPS32r2. */
> > +        switch (insn & 077) {
> > +        case 010: /* SWXC1 */
> > +        case 011: /* SDXC1 */
> > +        case 015: /* SDXC1 */
> > +            is_write = 1;
> > +        }
> > +        break;
> > +    }
> > +
> >      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
> >  }
> >
> >
>
>

Aleksandar Markovic Sept. 28, 2020, 8:14 a.m. UTC | #3

On Sunday, September 27, 2020, Kele Huang <kele.hwang@gmail.com> wrote:

> Sorry about that, I only got maintainers by './scripts/get_maintainer.pl
> -f accel/tcg/user-exec.c' and missed your advice about maintainers.
> In another words, I thought I had Cc'ed the TCG MIPS maintainers. 😅
> And sorry to maintainers. 😅
>
>>
>>
This is fine, Kele. :)

The granularity of get_maintainer.py is at file level, so this is one of
the cases where you can use your own judgement and include more email
addresses, even though get_maintainer.py doesn't list them.
get_maintainer.py is good most of the time, but not always. But not a big
deal.

Thanks for the patch! :)

I expect Richard is going to include it in his next tcg queue.

Yours,
Aleksandar


> On Sun, 27 Sep 2020 at 16:41, Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
>
>> On 9/27/20 10:20 AM, Kele Huang wrote:
>> > Detect all MIPS store instructions in cpu_signal_handler for all
>> available
>> > MIPS versions, and set is_write if encountering such store instructions.
>> >
>> > This fixed the error while dealing with self-modified code for MIPS.
>> >
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Signed-off-by: Kele Huang <kele.hwang@gmail.com>
>> > Signed-off-by: Xu Zou <iwatchnima@gmail.com>
>>
>> I already Cc'ed the TCG MIPS maintainers twice for you,
>> but you don't mind, so this time I won't insist.
>>
>> > ---
>> >  accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++++++++-
>> >  1 file changed, 38 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
>> > index bb039eb32d..9ecda6c0d0 100644
>> > --- a/accel/tcg/user-exec.c
>> > +++ b/accel/tcg/user-exec.c
>> > @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void
>> *pinfo,
>> >
>> >  #elif defined(__mips__)
>> >
>> > +#if defined(__misp16) || defined(__mips_micromips)
>> > +#error "Unsupported encoding"
>> > +#endif
>> > +
>> >  int cpu_signal_handler(int host_signum, void *pinfo,
>> >                         void *puc)
>> >  {
>> > @@ -709,9 +713,42 @@ int cpu_signal_handler(int host_signum, void
>> *pinfo,
>> >      ucontext_t *uc = puc;
>> >      greg_t pc = uc->uc_mcontext.pc;
>> >      int is_write;
>> > +    uint32_t insn;
>> >
>> > -    /* XXX: compute is_write */
>> > +    /* Detect all store instructions at program counter. */
>> >      is_write = 0;
>> > +    insn = *(uint32_t *)pc;
>> > +    switch((insn >> 26) & 077) {
>> > +    case 050: /* SB */
>> > +    case 051: /* SH */
>> > +    case 052: /* SWL */
>> > +    case 053: /* SW */
>> > +    case 054: /* SDL */
>> > +    case 055: /* SDR */
>> > +    case 056: /* SWR */
>> > +    case 070: /* SC */
>> > +    case 071: /* SWC1 */
>> > +    case 074: /* SCD */
>> > +    case 075: /* SDC1 */
>> > +    case 077: /* SD */
>> > +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
>> > +    case 072: /* SWC2 */
>> > +    case 076: /* SDC2 */
>> > +#endif
>> > +        is_write = 1;
>> > +        break;
>> > +    case 023: /* COP1X */
>> > +        /* Required in all versions of MIPS64 since
>> > +           MIPS64r1 and subsequent versions of MIPS32r2. */
>> > +        switch (insn & 077) {
>> > +        case 010: /* SWXC1 */
>> > +        case 011: /* SDXC1 */
>> > +        case 015: /* SDXC1 */
>> > +            is_write = 1;
>> > +        }
>> > +        break;
>> > +    }
>> > +
>> >      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
>> >  }
>> >
>> >
>>
>>

Kele Huang Sept. 29, 2020, 1:59 a.m. UTC | #4

Thank you so much!


On Mon, 28 Sep 2020 at 16:14, Aleksandar Markovic <
aleksandar.qemu.devel@gmail.com> wrote:

>
>
> On Sunday, September 27, 2020, Kele Huang <kele.hwang@gmail.com> wrote:
>
>> Sorry about that, I only got maintainers by './scripts/get_maintainer.pl
>> -f accel/tcg/user-exec.c' and missed your advice about maintainers.
>> In another words, I thought I had Cc'ed the TCG MIPS maintainers. 😅
>> And sorry to maintainers. 😅
>>
>>>
>>>
> This is fine, Kele. :)
>
> The granularity of get_maintainer.py is at file level, so this is one of
> the cases where you can use your own judgement and include more email
> addresses, even though get_maintainer.py doesn't list them.
> get_maintainer.py is good most of the time, but not always. But not a big
> deal.
>
> Thanks for the patch! :)
>
> I expect Richard is going to include it in his next tcg queue.
>
> Yours,
> Aleksandar
>
>
>> On Sun, 27 Sep 2020 at 16:41, Philippe Mathieu-Daudé <f4bug@amsat.org>
>> wrote:
>>
>>> On 9/27/20 10:20 AM, Kele Huang wrote:
>>> > Detect all MIPS store instructions in cpu_signal_handler for all
>>> available
>>> > MIPS versions, and set is_write if encountering such store
>>> instructions.
>>> >
>>> > This fixed the error while dealing with self-modified code for MIPS.
>>> >
>>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> > Signed-off-by: Kele Huang <kele.hwang@gmail.com>
>>> > Signed-off-by: Xu Zou <iwatchnima@gmail.com>
>>>
>>> I already Cc'ed the TCG MIPS maintainers twice for you,
>>> but you don't mind, so this time I won't insist.
>>>
>>> > ---
>>> >  accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++++++++-
>>> >  1 file changed, 38 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
>>> > index bb039eb32d..9ecda6c0d0 100644
>>> > --- a/accel/tcg/user-exec.c
>>> > +++ b/accel/tcg/user-exec.c
>>> > @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void
>>> *pinfo,
>>> >
>>> >  #elif defined(__mips__)
>>> >
>>> > +#if defined(__misp16) || defined(__mips_micromips)
>>> > +#error "Unsupported encoding"
>>> > +#endif
>>> > +
>>> >  int cpu_signal_handler(int host_signum, void *pinfo,
>>> >                         void *puc)
>>> >  {
>>> > @@ -709,9 +713,42 @@ int cpu_signal_handler(int host_signum, void
>>> *pinfo,
>>> >      ucontext_t *uc = puc;
>>> >      greg_t pc = uc->uc_mcontext.pc;
>>> >      int is_write;
>>> > +    uint32_t insn;
>>> >
>>> > -    /* XXX: compute is_write */
>>> > +    /* Detect all store instructions at program counter. */
>>> >      is_write = 0;
>>> > +    insn = *(uint32_t *)pc;
>>> > +    switch((insn >> 26) & 077) {
>>> > +    case 050: /* SB */
>>> > +    case 051: /* SH */
>>> > +    case 052: /* SWL */
>>> > +    case 053: /* SW */
>>> > +    case 054: /* SDL */
>>> > +    case 055: /* SDR */
>>> > +    case 056: /* SWR */
>>> > +    case 070: /* SC */
>>> > +    case 071: /* SWC1 */
>>> > +    case 074: /* SCD */
>>> > +    case 075: /* SDC1 */
>>> > +    case 077: /* SD */
>>> > +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
>>> > +    case 072: /* SWC2 */
>>> > +    case 076: /* SDC2 */
>>> > +#endif
>>> > +        is_write = 1;
>>> > +        break;
>>> > +    case 023: /* COP1X */
>>> > +        /* Required in all versions of MIPS64 since
>>> > +           MIPS64r1 and subsequent versions of MIPS32r2. */
>>> > +        switch (insn & 077) {
>>> > +        case 010: /* SWXC1 */
>>> > +        case 011: /* SDXC1 */
>>> > +        case 015: /* SDXC1 */
>>> > +            is_write = 1;
>>> > +        }
>>> > +        break;
>>> > +    }
>>> > +
>>> >      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
>>> >  }
>>> >
>>> >
>>>
>>>

Kele Huang Oct. 2, 2020, 8:22 a.m. UTC | #5

> +        case 015: /* SDXC1 */

I just found a comment mistake about SUXC1,  and I have rectified it and
resent a new patch.

On Tue, 29 Sep 2020 at 09:59, Kele Huang <kele.hwang@gmail.com> wrote:

> Thank you so much!
>
>
> On Mon, 28 Sep 2020 at 16:14, Aleksandar Markovic <
> aleksandar.qemu.devel@gmail.com> wrote:
>
>>
>>
>> On Sunday, September 27, 2020, Kele Huang <kele.hwang@gmail.com> wrote:
>>
>>> Sorry about that, I only got maintainers by './scripts/get_maintainer.pl
>>> -f accel/tcg/user-exec.c' and missed your advice about maintainers.
>>> In another words, I thought I had Cc'ed the TCG MIPS maintainers. 😅
>>> And sorry to maintainers. 😅
>>>
>>>>
>>>>
>> This is fine, Kele. :)
>>
>> The granularity of get_maintainer.py is at file level, so this is one of
>> the cases where you can use your own judgement and include more email
>> addresses, even though get_maintainer.py doesn't list them.
>> get_maintainer.py is good most of the time, but not always. But not a big
>> deal.
>>
>> Thanks for the patch! :)
>>
>> I expect Richard is going to include it in his next tcg queue.
>>
>> Yours,
>> Aleksandar
>>
>>
>>> On Sun, 27 Sep 2020 at 16:41, Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> wrote:
>>>
>>>> On 9/27/20 10:20 AM, Kele Huang wrote:
>>>> > Detect all MIPS store instructions in cpu_signal_handler for all
>>>> available
>>>> > MIPS versions, and set is_write if encountering such store
>>>> instructions.
>>>> >
>>>> > This fixed the error while dealing with self-modified code for MIPS.
>>>> >
>>>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>> > Signed-off-by: Kele Huang <kele.hwang@gmail.com>
>>>> > Signed-off-by: Xu Zou <iwatchnima@gmail.com>
>>>>
>>>> I already Cc'ed the TCG MIPS maintainers twice for you,
>>>> but you don't mind, so this time I won't insist.
>>>>
>>>> > ---
>>>> >  accel/tcg/user-exec.c | 39 ++++++++++++++++++++++++++++++++++++++-
>>>> >  1 file changed, 38 insertions(+), 1 deletion(-)
>>>> >
>>>> > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
>>>> > index bb039eb32d..9ecda6c0d0 100644
>>>> > --- a/accel/tcg/user-exec.c
>>>> > +++ b/accel/tcg/user-exec.c
>>>> > @@ -702,6 +702,10 @@ int cpu_signal_handler(int host_signum, void
>>>> *pinfo,
>>>> >
>>>> >  #elif defined(__mips__)
>>>> >
>>>> > +#if defined(__misp16) || defined(__mips_micromips)
>>>> > +#error "Unsupported encoding"
>>>> > +#endif
>>>> > +
>>>> >  int cpu_signal_handler(int host_signum, void *pinfo,
>>>> >                         void *puc)
>>>> >  {
>>>> > @@ -709,9 +713,42 @@ int cpu_signal_handler(int host_signum, void
>>>> *pinfo,
>>>> >      ucontext_t *uc = puc;
>>>> >      greg_t pc = uc->uc_mcontext.pc;
>>>> >      int is_write;
>>>> > +    uint32_t insn;
>>>> >
>>>> > -    /* XXX: compute is_write */
>>>> > +    /* Detect all store instructions at program counter. */
>>>> >      is_write = 0;
>>>> > +    insn = *(uint32_t *)pc;
>>>> > +    switch((insn >> 26) & 077) {
>>>> > +    case 050: /* SB */
>>>> > +    case 051: /* SH */
>>>> > +    case 052: /* SWL */
>>>> > +    case 053: /* SW */
>>>> > +    case 054: /* SDL */
>>>> > +    case 055: /* SDR */
>>>> > +    case 056: /* SWR */
>>>> > +    case 070: /* SC */
>>>> > +    case 071: /* SWC1 */
>>>> > +    case 074: /* SCD */
>>>> > +    case 075: /* SDC1 */
>>>> > +    case 077: /* SD */
>>>> > +#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
>>>> > +    case 072: /* SWC2 */
>>>> > +    case 076: /* SDC2 */
>>>> > +#endif
>>>> > +        is_write = 1;
>>>> > +        break;
>>>> > +    case 023: /* COP1X */
>>>> > +        /* Required in all versions of MIPS64 since
>>>> > +           MIPS64r1 and subsequent versions of MIPS32r2. */
>>>> > +        switch (insn & 077) {
>>>> > +        case 010: /* SWXC1 */
>>>> > +        case 011: /* SDXC1 */
>>>> > +        case 015: /* SDXC1 */
>>>> > +            is_write = 1;
>>>> > +        }
>>>> > +        break;
>>>> > +    }
>>>> > +
>>>> >      return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
>>>> >  }
>>>> >
>>>> >
>>>>
>>>>

[v4,1/1] accel/tcg: Fix computing of is_write for MIPS

Commit Message

Comments

Patch