| Message ID | CA+Lzsy=o9yshAAAacuTwcs3Hfr4MfWiX-SiUf5A2Fds3vGOrAQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] accel/tcg/user-exec: support computing is_write for mips32 | expand |
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
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~
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.
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.
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. >
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~
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); }
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(-)