* [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32 @ 2020-09-11 1:12 zou xu 2020-09-11 10:41 ` Peter Maydell 0 siblings, 1 reply; 7+ messages in thread From: zou xu @ 2020-09-11 1:12 UTC (permalink / raw) To: qemu-devel [-- 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 --] ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32 2020-09-11 1:12 [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32 zou xu @ 2020-09-11 10:41 ` Peter Maydell 2020-09-11 16:55 ` Richard Henderson 0 siblings, 1 reply; 7+ messages in thread From: Peter Maydell @ 2020-09-11 10:41 UTC (permalink / raw) To: zou xu; +Cc: QEMU Developers 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32 2020-09-11 10:41 ` Peter Maydell @ 2020-09-11 16:55 ` Richard Henderson 2020-09-11 17:23 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 7+ messages in thread From: Richard Henderson @ 2020-09-11 16:55 UTC (permalink / raw) To: Peter Maydell, zou xu; +Cc: QEMU Developers 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~ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32 2020-09-11 16:55 ` Richard Henderson @ 2020-09-11 17:23 ` Philippe Mathieu-Daudé 2020-11-02 10:00 ` target/mips: Deprecate nanoMIPS ISA? Philippe Mathieu-Daudé 0 siblings, 1 reply; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-11 17:23 UTC (permalink / raw) To: Richard Henderson, Peter Maydell, zou xu; +Cc: QEMU Developers 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* target/mips: Deprecate nanoMIPS ISA? 2020-09-11 17:23 ` Philippe Mathieu-Daudé @ 2020-11-02 10:00 ` Philippe Mathieu-Daudé 2020-11-02 11:27 ` Philippe Mathieu-Daudé 2020-11-02 16:05 ` Richard Henderson 0 siblings, 2 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-11-02 10:00 UTC (permalink / raw) To: Richard Henderson, Peter Maydell, Thomas Huth, Markus Armbruster, Matthew Fortune, Paul Burton, Stefan Markovic, Robert Suchanek Cc: James Hogan, Marcin Nowakowski, zou xu, QEMU Developers 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: target/mips: Deprecate nanoMIPS ISA? 2020-11-02 10:00 ` target/mips: Deprecate nanoMIPS ISA? Philippe Mathieu-Daudé @ 2020-11-02 11:27 ` Philippe Mathieu-Daudé 2020-11-02 16:05 ` Richard Henderson 1 sibling, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-11-02 11:27 UTC (permalink / raw) To: Richard Henderson, Peter Maydell, Thomas Huth, Markus Armbruster, Paul Burton Cc: James Hogan, Marcin Nowakowski, zou xu, QEMU Developers 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. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: target/mips: Deprecate nanoMIPS ISA? 2020-11-02 10:00 ` target/mips: Deprecate nanoMIPS ISA? Philippe Mathieu-Daudé 2020-11-02 11:27 ` Philippe Mathieu-Daudé @ 2020-11-02 16:05 ` Richard Henderson 1 sibling, 0 replies; 7+ messages in thread From: Richard Henderson @ 2020-11-02 16:05 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Peter Maydell, Thomas Huth, Markus Armbruster, Matthew Fortune, Paul Burton, Stefan Markovic, Robert Suchanek Cc: James Hogan, Marcin Nowakowski, zou xu, QEMU Developers 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~ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-02 16:06 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-11 1:12 [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32 zou xu 2020-09-11 10:41 ` Peter Maydell 2020-09-11 16:55 ` Richard Henderson 2020-09-11 17:23 ` Philippe Mathieu-Daudé 2020-11-02 10:00 ` target/mips: Deprecate nanoMIPS ISA? Philippe Mathieu-Daudé 2020-11-02 11:27 ` Philippe Mathieu-Daudé 2020-11-02 16:05 ` Richard Henderson
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).