Re: [PATCH] ppc: remove excessive logging

[Joakim Tjernlund]

On Sun, 2019-12-15 at 11:59 +0100, BALATON Zoltan wrote:
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open attachments unless you recognize the sender and know the 
> content is safe.
> 
> 
> On Sun, 15 Dec 2019, Philippe Mathieu-Daudé wrote:
> > Hi Joakim,
> > 
> > I'm cc'ing the PPC maintainers for you, so they won't miss your patch (see
> > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.qemu.org%2FContribute%2FSubmitAPatch%23CC_the_relevant_maintainer&data=02%7C01%7Cjoakim.tjernlund%40infinera.com%7C8fd615a611ec4bd9cce408d7814dda1a%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637120043688298205&sdata=d433DXO8SEaFJqAu73VTQwkZptmvK2eMAxivELGMcMI%3D&reserved=0
> >  and
> > the output of ./scripts/get_maintainer.pl -f target/ppc/excp_helper.c).
> > 
> > On 12/14/19 1:13 PM, Joakim Tjernlund wrote:
> > > From: Joakim Tjernlund <address@hidden>
> > > 
> > > 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 <address@hidden>
> > > ---
> > > 
> > >      As far as I can see, ppc is the only one emiting thsi
> > >      debug msg for Invalid insns.
> > > 
> > > target/ppc/excp_helper.c | 1 -
> > >   1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > > index 50b004d00d..45c2fa3ff9 100644
> > > --- a/target/ppc/excp_helper.c
> > > +++ b/target/ppc/excp_helper.c
> > > @@ -326,7 +326,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
> > > excp_model, int excp)
> > >               env->spr[SPR_BOOKE_ESR] = ESR_FP;
> > >               break;
> > >           case POWERPC_EXCP_INVAL:
> > > -            LOG_EXCP("Invalid instruction at " TARGET_FMT_lx "\n",
> > > env->nip);
> > 
> > I don't think we want to remove a such important log. Since it make sense to
> > not disturb the console, maybe we can replace some of the LOG_EXCP() calls 
> > by
> > qemu_log_mask(LOG_GUEST_ERROR,...) instead?

Why is that so important? As far as I can tell ppc is the only arch doing this?

> 
> I don't think that's a good idea. That would flood the -d guest_errors log
> with unwanted messages that are normal not really due to guest errors. The

It not OK to flood some log but OK to flood the console?

> LOG_EXCP() is not enabled by default, you have to edit source to enable it

LOG_EXCP is enabled on Gentoo, what about other distros?

> so you can also then edit the unwanted messages as well (like commenting
> this one out if you don't like it). If this is removed, invalid opcodes
> are still logged from translate.c but the exception generated for them is
> not logged. I don't know if that's useful or not but these are debug logs
> so depends on what do you want to debug. I don't mind this being removed
> but would be also happy leaving it as it is as it's only shown for
> developers who enable it and might help debugging. Or maybe these could be
> converted to traces (although I generally find traces to be less
> convenient to work with than debug logs and not sure about potential
> performance impact). So my preferences would be in order: 1. leave it
> alone, 2. remove it, 3. convert to traces.
> 
> Regards,
> BALATON Zoltan
> 
> > >               msr |= 0x00080000;
> > >               env->spr[SPR_BOOKE_ESR] = ESR_PIL;
> > >               break;