| Message ID | 1405091884-29955-5-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | New |
| Headers | show |
On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> wrote: > ppc logs every type of Invalid instruction. This generates a lot > of garbage on console when sshd/ssh_keygen executes as > they try various insn to optimize its performance. > The invalid operation log is still there so an unknown insn > will still be logged. > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> > --- > linux-user/main.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/linux-user/main.c b/linux-user/main.c > index b453a39..71a33c7 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -1698,7 +1698,6 @@ void cpu_loop(CPUPPCState *env) > } > break; > case POWERPC_EXCP_INVAL: > - EXCP_DUMP(env, "Invalid instruction\n"); > info.si_signo = TARGET_SIGILL; > info.si_errno = 0; > switch (env->error_code & 0xF) { Rather than just deleting this EXCP_DUMP, I would suggest changing the EXCP_DUMP macro so it only does anything if the user has passed the "-d int" debug logging flag: #define EXCP_DUMP(env, fmt, ...) \ do { \ CPUState *cs = ENV_GET_CPU(env); \ qemu_log_mask(CPU_LOG_INT, fmt, ## __VA_ARGS__); \ log_cpu_state_mask(CPU_LOG_INT, cs, 0); \ } while (0) (untested code!). Some applications (like QEMU itself!) use SIGSEGV as well as SIGILL intentionally, for instance. (cc'd qemu-ppc to see if they have an opinion.) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:14:25: > > On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> wrote: > > ppc logs every type of Invalid instruction. This generates a lot > > of garbage on console when sshd/ssh_keygen executes as > > they try various insn to optimize its performance. > > The invalid operation log is still there so an unknown insn > > will still be logged. > > > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> > > --- > > linux-user/main.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/linux-user/main.c b/linux-user/main.c > > index b453a39..71a33c7 100644 > > --- a/linux-user/main.c > > +++ b/linux-user/main.c > > @@ -1698,7 +1698,6 @@ void cpu_loop(CPUPPCState *env) > > } > > break; > > case POWERPC_EXCP_INVAL: > > - EXCP_DUMP(env, "Invalid instruction\n"); > > info.si_signo = TARGET_SIGILL; > > info.si_errno = 0; > > switch (env->error_code & 0xF) { > > Rather than just deleting this EXCP_DUMP, I would suggest > changing the EXCP_DUMP macro so it only does anything > if the user has passed the "-d int" debug logging flag: I don't think ppc wants that. They want unconditionally debug on to get relevant bug reports. This one is getting in the way of normal operations so I think it should be deleted. Jocke
On 11 July 2014 19:15, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:14:25: >> On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> > wrote: >> > ppc logs every type of Invalid instruction. This generates a lot >> Rather than just deleting this EXCP_DUMP, I would suggest >> changing the EXCP_DUMP macro so it only does anything >> if the user has passed the "-d int" debug logging flag: > > I don't think ppc wants that. They want unconditionally > debug on to get relevant bug reports. This one is getting in the > way of normal operations so I think it should be deleted. If the PPC maintainers want that behaviour then they need to defend it. No other architecture's linux-user code spews junk to stderr for exceptions, and PPC shouldn't either. The debug log switches are exactly for allowing us to say "please turn on debug logging" when bugs are reported, and those are what we should use. thanks -- PMM
On 11.07.14 20:22, Peter Maydell wrote: > On 11 July 2014 19:15, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: >> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:14:25: >>> On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> >> wrote: >>>> ppc logs every type of Invalid instruction. This generates a lot >>> Rather than just deleting this EXCP_DUMP, I would suggest >>> changing the EXCP_DUMP macro so it only does anything >>> if the user has passed the "-d int" debug logging flag: >> I don't think ppc wants that. They want unconditionally >> debug on to get relevant bug reports. This one is getting in the >> way of normal operations so I think it should be deleted. > If the PPC maintainers want that behaviour then they need > to defend it. No other architecture's linux-user code spews > junk to stderr for exceptions, and PPC shouldn't either. > The debug log switches are exactly for allowing us to > say "please turn on debug logging" when bugs are reported, > and those are what we should use. I agree - and it's how we behave in system emulation mode already :). What do the other platforms do on illegal instructions during user mode? Any way we can get consistency across the board? Alex
Alexander Graf <agraf@suse.de> wrote on 2014/07/12 02:39:21: > > > On 11.07.14 20:22, Peter Maydell wrote: > > On 11 July 2014 19:15, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote: > >> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 19:14:25: > >>> On 11 July 2014 16:18, Joakim Tjernlund <Joakim.Tjernlund@transmode.se> > >> wrote: > >>>> ppc logs every type of Invalid instruction. This generates a lot > >>> Rather than just deleting this EXCP_DUMP, I would suggest > >>> changing the EXCP_DUMP macro so it only does anything > >>> if the user has passed the "-d int" debug logging flag: > >> I don't think ppc wants that. They want unconditionally > >> debug on to get relevant bug reports. This one is getting in the > >> way of normal operations so I think it should be deleted. > > If the PPC maintainers want that behaviour then they need > > to defend it. No other architecture's linux-user code spews > > junk to stderr for exceptions, and PPC shouldn't either. > > The debug log switches are exactly for allowing us to > > say "please turn on debug logging" when bugs are reported, > > and those are what we should use. > > I agree - and it's how we behave in system emulation mode already :). > > What do the other platforms do on illegal instructions during user mode? > Any way we can get consistency across the board? In this case it is not an illegal insn, it is a valid insn but not just for the QEMU emulated CPU.
On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote: > What do the other platforms do on illegal instructions during user mode? > Any way we can get consistency across the board? Mostly it looks like they just silently generate the SIGILL. Consistency has never been our strong point :-) thanks -- PMM
On 12.07.14 10:58, Peter Maydell wrote: > On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote: >> What do the other platforms do on illegal instructions during user mode? >> Any way we can get consistency across the board? > Mostly it looks like they just silently generate the SIGILL. > Consistency has never been our strong point :-) That means this patch brings things towards consistency? It's certainly good for me then :) Alex
On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote: > > On 12.07.14 10:58, Peter Maydell wrote: >> >> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote: >>> >>> What do the other platforms do on illegal instructions during user mode? >>> Any way we can get consistency across the board? >> >> Mostly it looks like they just silently generate the SIGILL. >> Consistency has never been our strong point :-) > > > That means this patch brings things towards consistency? It's certainly good > for me then :) No, this just removes one use of this logging. If you wanted consistency we should remove all of them... -- PMM
On 12.07.14 12:40, Peter Maydell wrote: > On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote: >> On 12.07.14 10:58, Peter Maydell wrote: >>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote: >>>> What do the other platforms do on illegal instructions during user mode? >>>> Any way we can get consistency across the board? >>> Mostly it looks like they just silently generate the SIGILL. >>> Consistency has never been our strong point :-) >> >> That means this patch brings things towards consistency? It's certainly good >> for me then :) > No, this just removes one use of this logging. If you > wanted consistency we should remove all of them... Agreed :) Alex
Alexander Graf <agraf@suse.de> wrote on 2014/07/12 12:41:05: > > On 12.07.14 12:40, Peter Maydell wrote: > > On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote: > >> On 12.07.14 10:58, Peter Maydell wrote: > >>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote: > >>>> What do the other platforms do on illegal instructions during user mode? > >>>> Any way we can get consistency across the board? > >>> Mostly it looks like they just silently generate the SIGILL. > >>> Consistency has never been our strong point :-) > >> > >> That means this patch brings things towards consistency? It's certainly good > >> for me then :) > > No, this just removes one use of this logging. If you > > wanted consistency we should remove all of them... > > Agreed :) So can I infer from this discussion that you will apply the patch?
On Sat, Jul 12, 2014 at 04:06:09PM +0200, Joakim Tjernlund wrote: > Alexander Graf <agraf@suse.de> wrote on 2014/07/12 12:41:05: > > > > On 12.07.14 12:40, Peter Maydell wrote: > > > On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote: > > >> On 12.07.14 10:58, Peter Maydell wrote: > > >>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote: > > >>>> What do the other platforms do on illegal instructions during user > mode? > > >>>> Any way we can get consistency across the board? > > >>> Mostly it looks like they just silently generate the SIGILL. > > >>> Consistency has never been our strong point :-) > > >> > > >> That means this patch brings things towards consistency? It's > certainly good > > >> for me then :) > > > No, this just removes one use of this logging. If you > > > wanted consistency we should remove all of them... > > Agreed :) > So can I infer from this discussion that you will apply the patch? I think Peter and Alex suggest that the EXCP_DUMP() loggings should be removed from all cases in PPC code where TARGET_SIGILL is risen. Your patch removes just once case, and that would make PPC code become internally inconsistent where some SIGILLs are logged and others aren't. Even more dramatically, we remove the whole EXCP_DUMP and users since none of the other archs output anything for SIGFPE/SIGSEGV either. After all, the Linux kernel (ppc or others) doesn't output anything either. And it is the behaviour of linux we try to match. Riku
Riku Voipio <riku.voipio@iki.fi> wrote on 2014/07/16 09:55:50: > From: Riku Voipio <riku.voipio@iki.fi> > To: Joakim Tjernlund <joakim.tjernlund@transmode.se>, > Cc: Alexander Graf <agraf@suse.de>, Peter Maydell <peter.maydell@linaro.org>, "qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>, QEMU Developers <qemu-devel@nongnu.org> > Date: 2014/07/16 09:55 > Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive logging > > On Sat, Jul 12, 2014 at 04:06:09PM +0200, Joakim Tjernlund wrote: > > Alexander Graf <agraf@suse.de> wrote on 2014/07/12 12:41:05: > > > > > > On 12.07.14 12:40, Peter Maydell wrote: > > > > On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote: > > > >> On 12.07.14 10:58, Peter Maydell wrote: > > > >>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote: > > > >>>> What do the other platforms do on illegal instructions during user > > mode? > > > >>>> Any way we can get consistency across the board? > > > >>> Mostly it looks like they just silently generate the SIGILL. > > > >>> Consistency has never been our strong point :-) > > > >> > > > >> That means this patch brings things towards consistency? It's > > certainly good > > > >> for me then :) > > > > No, this just removes one use of this logging. If you > > > > wanted consistency we should remove all of them... > > > > Agreed :) > > > So can I infer from this discussion that you will apply the patch? > > I think Peter and Alex suggest that the EXCP_DUMP() loggings should be > removed from all cases in PPC code where TARGET_SIGILL is risen. Your > patch removes just once case, and that would make PPC code become > internally inconsistent where some SIGILLs are logged and others aren't. Something like that. This is one step in that direction. We(or I cannot) do the consistency for all cases/arches at once. With the patch we become one step closer to the Linux kernel so I don't see why not apply it. > > Even more dramatically, we remove the whole EXCP_DUMP and users since none > of the other archs output anything for SIGFPE/SIGSEGV either. After all, > the Linux kernel (ppc or others) doesn't output anything either. And > it is the behaviour of linux we try to match. hmm, not clear to me what you mean here Jocke
> Am 16.07.2014 um 10:32 schrieb Joakim Tjernlund <joakim.tjernlund@transmode.se>: > > Riku Voipio <riku.voipio@iki.fi> wrote on 2014/07/16 09:55:50: > >> From: Riku Voipio <riku.voipio@iki.fi> >> To: Joakim Tjernlund <joakim.tjernlund@transmode.se>, >> Cc: Alexander Graf <agraf@suse.de>, Peter Maydell > <peter.maydell@linaro.org>, "qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>, > QEMU Developers <qemu-devel@nongnu.org> >> Date: 2014/07/16 09:55 >> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/4] ppc: remove excessive > logging >> >>> On Sat, Jul 12, 2014 at 04:06:09PM +0200, Joakim Tjernlund wrote: >>> Alexander Graf <agraf@suse.de> wrote on 2014/07/12 12:41:05: >>>> >>>>> On 12.07.14 12:40, Peter Maydell wrote: >>>>>> On 12 July 2014 10:39, Alexander Graf <agraf@suse.de> wrote: >>>>>>> On 12.07.14 10:58, Peter Maydell wrote: >>>>>>>> On 12 July 2014 01:39, Alexander Graf <agraf@suse.de> wrote: >>>>>>>> What do the other platforms do on illegal instructions during > user >>> mode? >>>>>>>> Any way we can get consistency across the board? >>>>>>> Mostly it looks like they just silently generate the SIGILL. >>>>>>> Consistency has never been our strong point :-) >>>>>> >>>>>> That means this patch brings things towards consistency? It's >>> certainly good >>>>>> for me then :) >>>>> No, this just removes one use of this logging. If you >>>>> wanted consistency we should remove all of them... >> >>>> Agreed :) >> >>> So can I infer from this discussion that you will apply the patch? >> >> I think Peter and Alex suggest that the EXCP_DUMP() loggings should be >> removed from all cases in PPC code where TARGET_SIGILL is risen. Your >> patch removes just once case, and that would make PPC code become >> internally inconsistent where some SIGILLs are logged and others aren't. > > Something like that. This is one step in that direction. We(or I cannot) > do > the consistency for all cases/arches at once. With the patch we become > one step closer to the Linux kernel so I don't see why not apply it. That's not how it will end up though. If we apply this one patch now, it will stay that way forever and be even more confusing and inconsistent than today. I think what we really want is proper -d int logging on all archs for linux-user. This patch is getting us nowhere close to it. Alex
Joakim Tjernlund writes: > Alexander Graf <agraf@suse.de> wrote on 2014/07/12 02:39:21: >> >> >> On 11.07.14 20:22, Peter Maydell wrote: >> > On 11 July 2014 19:15, Joakim Tjernlund > <joakim.tjernlund@transmode.se> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> wrote on 2014/07/11 > 19:14:25: >> >>> On 11 July 2014 16:18, Joakim Tjernlund > <Joakim.Tjernlund@transmode.se> >> >> wrote: >> >>>> ppc logs every type of Invalid instruction. This generates a lot >> >>> Rather than just deleting this EXCP_DUMP, I would suggest >> >>> changing the EXCP_DUMP macro so it only does anything >> >>> if the user has passed the "-d int" debug logging flag: >> >> I don't think ppc wants that. They want unconditionally >> >> debug on to get relevant bug reports. This one is getting in the >> >> way of normal operations so I think it should be deleted. >> > If the PPC maintainers want that behaviour then they need >> > to defend it. No other architecture's linux-user code spews >> > junk to stderr for exceptions, and PPC shouldn't either. >> > The debug log switches are exactly for allowing us to >> > say "please turn on debug logging" when bugs are reported, >> > and those are what we should use. >> >> I agree - and it's how we behave in system emulation mode already :). >> >> What do the other platforms do on illegal instructions during user mode? > >> Any way we can get consistency across the board? > > In this case it is not an illegal insn, it is a valid insn but not just > for the QEMU emulated CPU. On aarch64 we do: #define unsupported_encoding(s, insn) \ do { \ qemu_log_mask(LOG_UNIMP, \ "%s:%d: unsupported instruction encoding 0x%08x " \ "at pc=%016" PRIx64 "\n", \ __FILE__, __LINE__, insn, s->pc - 4); \ unallocated_encoding(s); \ } while (0); So we signal it's unimplemented before falling through to the signal generating code.
On 16 July 2014 10:17, Alex Bennée <alex.bennee@linaro.org> wrote: > On aarch64 we do: > > #define unsupported_encoding(s, insn) \ > do { \ > qemu_log_mask(LOG_UNIMP, \ > "%s:%d: unsupported instruction encoding 0x%08x " \ > "at pc=%016" PRIx64 "\n", \ > __FILE__, __LINE__, insn, s->pc - 4); \ > unallocated_encoding(s); \ > } while (0); > > So we signal it's unimplemented before falling through to the signal > generating code. Yes, but that macro is only for instructions which exist but which QEMU doesn't implement, not for instructions which exist and which we do have an implementation of but which we generate an exception for because they aren't present on this particular CPU. That's a separate issue from whether you might want to optionally log every SIGILL a process generates. thanks -- PMM
diff --git a/linux-user/main.c b/linux-user/main.c index b453a39..71a33c7 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -1698,7 +1698,6 @@ void cpu_loop(CPUPPCState *env) } break; case POWERPC_EXCP_INVAL: - EXCP_DUMP(env, "Invalid instruction\n"); info.si_signo = TARGET_SIGILL; info.si_errno = 0; switch (env->error_code & 0xF) {
ppc logs every type of Invalid instruction. This generates a lot of garbage on console when sshd/ssh_keygen executes as they try various insn to optimize its performance. The invalid operation log is still there so an unknown insn will still be logged. Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> --- linux-user/main.c | 1 - 1 file changed, 1 deletion(-)