Re: [PATCH] ppc: remove excessive logging

[BALATON Zoltan <balaton@eik.bme.hu>]

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

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://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer 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 <Joakim.Tjernlund@infinera.com>
>> 
>> 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@infinera.com>
>> ---
>>
>> 	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?

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 
LOG_EXCP() is not enabled by default, you have to edit source to enable it 
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;