[PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32

[PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32

[zou xu]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

From 533ed02427bdaf0265f62fcb4f961854a41b7037 Mon Sep 17 00:00:00 2001
From: ZouXu <iwatchnima@gmail.com>
Date: Wed, 9 Sep 2020 21:59:13 +0800
Subject: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for
 mips32

Those MIPS32 instructions can cause store operation:
SB/SH/SW/SD/SWL/SWR/SDL/SDR/CACHE
SC/SCD/SWC1/SWC2/SDC1/SDC2
---
 accel/tcg/user-exec.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index bb039eb32d..b5ad721aa5 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -708,10 +708,38 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     siginfo_t *info = pinfo;
     ucontext_t *uc = puc;
     greg_t pc = uc->uc_mcontext.pc;
-    int is_write;
+    int 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 */
+        case 0x7: /* CACHE */
+            is_write = 1;
+        }
+        break;
+    case 0x7:
+        switch((insn>>26) & 0x7) {
+        case 0x0: /* SC */
+        case 0x1: /* SWC1 */
+        case 0x2: /* SWC2 */
+        case 0x4: /* SCD */
+        case 0x5: /* SDC1 */
+        case 0x6: /* SDC2 */
+        case 0x7: /* SD */
+            is_write = 1;
+        }
+        break;
+    }

-    /* XXX: compute is_write */
-    is_write = 0;
     return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
 }

-- 
2.25.1

[-- Attachment #2: Type: text/html, Size: 2181 bytes --]

Re: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32

[Peter Maydell]

On Fri, 11 Sep 2020 at 02:14, zou xu <iwatchnima@gmail.com> wrote:
>
> From 533ed02427bdaf0265f62fcb4f961854a41b7037 Mon Sep 17 00:00:00 2001
> From: ZouXu <iwatchnima@gmail.com>
> Date: Wed, 9 Sep 2020 21:59:13 +0800
> Subject: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for
>  mips32
>
> Those MIPS32 instructions can cause store operation:
> SB/SH/SW/SD/SWL/SWR/SDL/SDR/CACHE
> SC/SCD/SWC1/SWC2/SDC1/SDC2
> ---
>  accel/tcg/user-exec.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index bb039eb32d..b5ad721aa5 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -708,10 +708,38 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      siginfo_t *info = pinfo;
>      ucontext_t *uc = puc;
>      greg_t pc = uc->uc_mcontext.pc;
> -    int is_write;
> +    int 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) {

Here we mask to get a 3-bit field...

> +        case 0x0: /* SB */
> +        case 0x1: /* SH */
> +        case 0x2: /* SWL */
> +        case 0x3: /* SW */
> +        case 0x4: /* SDL */
> +        case 0x5: /* SDR */
> +        case 0x6: /* SWR */
> +        case 0x7: /* CACHE */
> +            is_write = 1;

...but here all 8 cases are handled identically.
Is there a typo/logic error here, or should this
really just be

    case 0x5:
        /* SB, SH, SWL, SW, SDL, SDR, SWR, CACHE */
        is_write = 1;

?

Is CACHE really a write insn ?

> +        }
> +        break;
> +    case 0x7:
> +        switch((insn>>26) & 0x7) {
> +        case 0x0: /* SC */
> +        case 0x1: /* SWC1 */
> +        case 0x2: /* SWC2 */
> +        case 0x4: /* SCD */
> +        case 0x5: /* SDC1 */
> +        case 0x6: /* SDC2 */
> +        case 0x7: /* SD */
> +            is_write = 1;
> +        }
> +        break;
> +    }

thanks
-- PMM

Re: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32

[Richard Henderson]

On 9/11/20 3:41 AM, Peter Maydell wrote:
>> +    /* 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) {
> 
> Here we mask to get a 3-bit field...
> 
>> +        case 0x0: /* SB */
>> +        case 0x1: /* SH */
>> +        case 0x2: /* SWL */
>> +        case 0x3: /* SW */
>> +        case 0x4: /* SDL */
>> +        case 0x5: /* SDR */
>> +        case 0x6: /* SWR */
>> +        case 0x7: /* CACHE */
>> +            is_write = 1;
> 
> ...but here all 8 cases are handled identically.
> Is there a typo/logic error here, or should this
> really just be
> 
>     case 0x5:
>         /* SB, SH, SWL, SW, SDL, SDR, SWR, CACHE */
>         is_write = 1;
> 
> ?
> 
> Is CACHE really a write insn ?

Indeed not.  However, it's also illegal for user-mode, so we cannot arrive here
with SIGSEGV by executing it.  So we could ignore that case and not decode this
field.

>> +    case 0x7:
>> +        switch((insn>>26) & 0x7) {
>> +        case 0x0: /* SC */
>> +        case 0x1: /* SWC1 */
>> +        case 0x2: /* SWC2 */
>> +        case 0x4: /* SCD */
>> +        case 0x5: /* SDC1 */
>> +        case 0x6: /* SDC2 */
>> +        case 0x7: /* SD */
>> +            is_write = 1;

Well, the unconditional check of SWC2/SDC2 is not quite right. MIPS64R6 removes
them and replaces them with some compact branches.  That's easy enough to
include here, using

#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
    case 2: /* SWC2 */
    case 6: /* SDC2 */
#endif

We should also add

#if defined(__mips16) || defined(__mips_micromips)
# error "Unsupported encoding"
#endif

I see no upstream compiler support for nanomips at all, so there's no point in
checking for that encoding.  (Indeed, I wonder at the code in target/mips...
how could it be tested?)


r~

Re: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32

[Philippe Mathieu-Daudé]

On 9/11/20 6:55 PM, Richard Henderson wrote:
> On 9/11/20 3:41 AM, Peter Maydell wrote:
>>> +    /* 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) {
>>
>> Here we mask to get a 3-bit field...
>>
>>> +        case 0x0: /* SB */
>>> +        case 0x1: /* SH */
>>> +        case 0x2: /* SWL */
>>> +        case 0x3: /* SW */
>>> +        case 0x4: /* SDL */
>>> +        case 0x5: /* SDR */
>>> +        case 0x6: /* SWR */
>>> +        case 0x7: /* CACHE */
>>> +            is_write = 1;
>>
>> ...but here all 8 cases are handled identically.
>> Is there a typo/logic error here, or should this
>> really just be
>>
>>     case 0x5:
>>         /* SB, SH, SWL, SW, SDL, SDR, SWR, CACHE */
>>         is_write = 1;
>>
>> ?
>>
>> Is CACHE really a write insn ?
> 
> Indeed not.  However, it's also illegal for user-mode, so we cannot arrive here
> with SIGSEGV by executing it.  So we could ignore that case and not decode this
> field.
> 
>>> +    case 0x7:
>>> +        switch((insn>>26) & 0x7) {
>>> +        case 0x0: /* SC */
>>> +        case 0x1: /* SWC1 */
>>> +        case 0x2: /* SWC2 */
>>> +        case 0x4: /* SCD */
>>> +        case 0x5: /* SDC1 */
>>> +        case 0x6: /* SDC2 */
>>> +        case 0x7: /* SD */
>>> +            is_write = 1;
> 
> Well, the unconditional check of SWC2/SDC2 is not quite right. MIPS64R6 removes
> them and replaces them with some compact branches.  That's easy enough to
> include here, using
> 
> #if !defined(__mips_isa_rev) || __mips_isa_rev < 6
>     case 2: /* SWC2 */
>     case 6: /* SDC2 */
> #endif
> 
> We should also add
> 
> #if defined(__mips16) || defined(__mips_micromips)
> # error "Unsupported encoding"
> #endif
> 
> I see no upstream compiler support for nanomips at all, so there's no point in
> checking for that encoding.  (Indeed, I wonder at the code in target/mips...
> how could it be tested?)

I took the information from commit f7d257cb4a1
("qemu-doc: Add nanoMIPS ISA information") to add
the tests in  f375ad6a0d6 ("BootLinuxConsoleTest:
Test nanoMIPS kernels on the I7200 CPU"), but I
haven't tried to recompile these files myself.

Regards,

Phil.

target/mips: Deprecate nanoMIPS ISA?

[Philippe Mathieu-Daudé]

Hi Richard,

Cc'ing developers who introduced nanoMIPS.

On 9/11/20 7:23 PM, Philippe Mathieu-Daudé wrote:
> On 9/11/20 6:55 PM, Richard Henderson wrote:
>> On 9/11/20 3:41 AM, Peter Maydell wrote:
>>>> +    /* 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) {
>>>
>>> Here we mask to get a 3-bit field...
>>>
>>>> +        case 0x0: /* SB */
>>>> +        case 0x1: /* SH */
>>>> +        case 0x2: /* SWL */
>>>> +        case 0x3: /* SW */
>>>> +        case 0x4: /* SDL */
>>>> +        case 0x5: /* SDR */
>>>> +        case 0x6: /* SWR */
>>>> +        case 0x7: /* CACHE */
>>>> +            is_write = 1;
>>>
>>> ...but here all 8 cases are handled identically.
>>> Is there a typo/logic error here, or should this
>>> really just be
>>>
>>>     case 0x5:
>>>         /* SB, SH, SWL, SW, SDL, SDR, SWR, CACHE */
>>>         is_write = 1;
>>>
>>> ?
>>>
>>> Is CACHE really a write insn ?
>>
>> Indeed not.  However, it's also illegal for user-mode, so we cannot arrive here
>> with SIGSEGV by executing it.  So we could ignore that case and not decode this
>> field.
>>
>>>> +    case 0x7:
>>>> +        switch((insn>>26) & 0x7) {
>>>> +        case 0x0: /* SC */
>>>> +        case 0x1: /* SWC1 */
>>>> +        case 0x2: /* SWC2 */
>>>> +        case 0x4: /* SCD */
>>>> +        case 0x5: /* SDC1 */
>>>> +        case 0x6: /* SDC2 */
>>>> +        case 0x7: /* SD */
>>>> +            is_write = 1;
>>
>> Well, the unconditional check of SWC2/SDC2 is not quite right. MIPS64R6 removes
>> them and replaces them with some compact branches.  That's easy enough to
>> include here, using
>>
>> #if !defined(__mips_isa_rev) || __mips_isa_rev < 6
>>     case 2: /* SWC2 */
>>     case 6: /* SDC2 */
>> #endif
>>
>> We should also add
>>
>> #if defined(__mips16) || defined(__mips_micromips)
>> # error "Unsupported encoding"
>> #endif
>>
>> I see no upstream compiler support for nanomips at all, so there's no point in
>> checking for that encoding.  (Indeed, I wonder at the code in target/mips...
>> how could it be tested?)
> 
> I took the information from commit f7d257cb4a1
> ("qemu-doc: Add nanoMIPS ISA information") to add
> the tests in  f375ad6a0d6 ("BootLinuxConsoleTest:
> Test nanoMIPS kernels on the I7200 CPU"), but I
> haven't tried to recompile these files myself.

I checked the various nanoMIPS announcements:
GCC:   https://gcc.gnu.org/legacy-ml/gcc/2018-05/msg00012.html
Linux: https://lwn.net/Articles/753605/
QEMU:  https://www.mail-archive.com/qemu-devel@nongnu.org/msg530721.html

Unfortunately www.mips.com doesn't work anymore.

From this Wayback machine link:
https://web.archive.org/web/20180904044530/https://www.mips.com/develop/tools/compilers/

I found this working web (the toolchain is a later release than the
one referenced in the announcement mails):
http://codescape.mips.com/components/toolchain/nanomips/2018.04-02/downloads.html

The toolchain page mention LLVM but simply links http://llvm.org/
where I couldn't find any reference on nanoMIPS.

The only reference in the GCC mailing list, is the nanoMIPS
announcement: https://gcc.gnu.org/pipermail/gcc/2018-May.txt


It looks safe for QEMU to declare nanoMIPS deprecated (it has no
maintainer), to give time to interested parties to finish upstreaming
process and step in to maintain it.
Thoughts?

Regards,

Phil.

Re: target/mips: Deprecate nanoMIPS ISA?

[Philippe Mathieu-Daudé]

On 11/2/20 11:00 AM, Philippe Mathieu-Daudé wrote:
> Hi Richard,
> 
> Cc'ing developers who introduced nanoMIPS.

Who are now unreachable, as I got:

Your message to Stefan.Markovic@mips.com couldn't be delivered.

Your message to smarkovic@wavecomp.com couldn't be delivered.

Couldn't deliver the message to the following recipients:
Robert.Suchanek@mips.com, matthew.fortune@mips.com,
marcin.nowakowski@mips.com

> 
> On 9/11/20 7:23 PM, Philippe Mathieu-Daudé wrote:
>> On 9/11/20 6:55 PM, Richard Henderson wrote:
>>> On 9/11/20 3:41 AM, Peter Maydell wrote:
>>>>> +    /* 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) {
>>>>
>>>> Here we mask to get a 3-bit field...
>>>>
>>>>> +        case 0x0: /* SB */
>>>>> +        case 0x1: /* SH */
>>>>> +        case 0x2: /* SWL */
>>>>> +        case 0x3: /* SW */
>>>>> +        case 0x4: /* SDL */
>>>>> +        case 0x5: /* SDR */
>>>>> +        case 0x6: /* SWR */
>>>>> +        case 0x7: /* CACHE */
>>>>> +            is_write = 1;
>>>>
>>>> ...but here all 8 cases are handled identically.
>>>> Is there a typo/logic error here, or should this
>>>> really just be
>>>>
>>>>     case 0x5:
>>>>         /* SB, SH, SWL, SW, SDL, SDR, SWR, CACHE */
>>>>         is_write = 1;
>>>>
>>>> ?
>>>>
>>>> Is CACHE really a write insn ?
>>>
>>> Indeed not.  However, it's also illegal for user-mode, so we cannot arrive here
>>> with SIGSEGV by executing it.  So we could ignore that case and not decode this
>>> field.
>>>
>>>>> +    case 0x7:
>>>>> +        switch((insn>>26) & 0x7) {
>>>>> +        case 0x0: /* SC */
>>>>> +        case 0x1: /* SWC1 */
>>>>> +        case 0x2: /* SWC2 */
>>>>> +        case 0x4: /* SCD */
>>>>> +        case 0x5: /* SDC1 */
>>>>> +        case 0x6: /* SDC2 */
>>>>> +        case 0x7: /* SD */
>>>>> +            is_write = 1;
>>>
>>> Well, the unconditional check of SWC2/SDC2 is not quite right. MIPS64R6 removes
>>> them and replaces them with some compact branches.  That's easy enough to
>>> include here, using
>>>
>>> #if !defined(__mips_isa_rev) || __mips_isa_rev < 6
>>>     case 2: /* SWC2 */
>>>     case 6: /* SDC2 */
>>> #endif
>>>
>>> We should also add
>>>
>>> #if defined(__mips16) || defined(__mips_micromips)
>>> # error "Unsupported encoding"
>>> #endif
>>>
>>> I see no upstream compiler support for nanomips at all, so there's no point in
>>> checking for that encoding.  (Indeed, I wonder at the code in target/mips...
>>> how could it be tested?)
>>
>> I took the information from commit f7d257cb4a1
>> ("qemu-doc: Add nanoMIPS ISA information") to add
>> the tests in  f375ad6a0d6 ("BootLinuxConsoleTest:
>> Test nanoMIPS kernels on the I7200 CPU"), but I
>> haven't tried to recompile these files myself.
> 
> I checked the various nanoMIPS announcements:
> GCC:   https://gcc.gnu.org/legacy-ml/gcc/2018-05/msg00012.html
> Linux: https://lwn.net/Articles/753605/
> QEMU:  https://www.mail-archive.com/qemu-devel@nongnu.org/msg530721.html
> 
> Unfortunately www.mips.com doesn't work anymore.
> 
> From this Wayback machine link:
> https://web.archive.org/web/20180904044530/https://www.mips.com/develop/tools/compilers/
> 
> I found this working web (the toolchain is a later release than the
> one referenced in the announcement mails):
> http://codescape.mips.com/components/toolchain/nanomips/2018.04-02/downloads.html
> 
> The toolchain page mention LLVM but simply links http://llvm.org/
> where I couldn't find any reference on nanoMIPS.
> 
> The only reference in the GCC mailing list, is the nanoMIPS
> announcement: https://gcc.gnu.org/pipermail/gcc/2018-May.txt
> 
> 
> It looks safe for QEMU to declare nanoMIPS deprecated (it has no
> maintainer), to give time to interested parties to finish upstreaming
> process and step in to maintain it.
> Thoughts?
> 
> Regards,
> 
> Phil.
> 

Re: target/mips: Deprecate nanoMIPS ISA?

[Richard Henderson]

On 11/2/20 2:00 AM, Philippe Mathieu-Daudé wrote:
> The toolchain page mention LLVM but simply links http://llvm.org/
> where I couldn't find any reference on nanoMIPS.
> 
> The only reference in the GCC mailing list, is the nanoMIPS
> announcement: https://gcc.gnu.org/pipermail/gcc/2018-May.txt
> 
> 
> It looks safe for QEMU to declare nanoMIPS deprecated (it has no
> maintainer), to give time to interested parties to finish upstreaming
> process and step in to maintain it.
> Thoughts?

I think that's reasonable.


r~