Re: [PATCH v2 02/14] x86/traps: Factor out extable_fixup() and make printing consistent

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/05/2020 10:50, Jan Beulich wrote:
> On 27.05.2020 21:18, Andrew Cooper wrote:
>> UD faults never had any diagnostics printed, and the others were 
>> inconsistent.
>>
>> Don't use dprintk() because identifying traps.c is actively unhelpful in the
>> message, as it is the location of the fixup, not the fault.  Use the new
>> vec_name() infrastructure, rather than leaving raw numbers for the log.
>>
>>   (XEN) Running stub recovery selftests...
>>   (XEN) Fixup #UD[0000]: ffff82d07fffd040 [ffff82d07fffd040] -> 
>> ffff82d0403ac9d6
>>   (XEN) Fixup #GP[0000]: ffff82d07fffd041 [ffff82d07fffd041] -> 
>> ffff82d0403ac9d6
>>   (XEN) Fixup #SS[0000]: ffff82d07fffd040 [ffff82d07fffd040] -> 
>> ffff82d0403ac9d6
>>   (XEN) Fixup #BP[0000]: ffff82d07fffd041 [ffff82d07fffd041] -> 
>> ffff82d0403ac9d6
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> As before
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> albeit I realize I have one more suggestion:
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -772,10 +772,31 @@ static void do_reserved_trap(struct cpu_user_regs 
>> *regs)
>>            trapnr, vec_name(trapnr), regs->error_code);
>>  }
>>  
>> +static bool extable_fixup(struct cpu_user_regs *regs, bool print)
>> +{
>> +    unsigned long fixup = search_exception_table(regs);
>> +
>> +    if ( unlikely(fixup == 0) )
>> +        return false;
>> +
>> +    /*
>> +     * Don't use dprintk() because the __FILE__ reference is unhelpful.
>> +     * Can currently be triggered by guests.  Make sure we ratelimit.
>> +     */
>> +    if ( IS_ENABLED(CONFIG_DEBUG) && print )
> How about pulling the IS_ENABLED(CONFIG_DEBUG) into the call sites
> currently passing "true"?

That is an obfuscation, not an improvement, in code legibility.

It is however a transformation that the compiler does (as part of
dropping the print parameter entirely).

~Andrew