Re: [PATCH 3/4] x86/uaccess: rework user access speculative harden guards

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.11.2024 16:31, Andrew Cooper wrote:
> On 19/11/2024 10:34 am, Roger Pau Monne wrote:
> Overall this is far more legible, and I'm tempted to take it on that
> justification alone.  But this is Jan's pile of magic.
> 
> There is a weird effect from this change:
> 
> add/remove: 2/0 grow/shrink: 2/2 up/down: 740/-739 (1)
> Function                                     old     new   delta
> build_symbol_table                             -     686    +686
> build_symbol_table.cold                        -      48     +48
> pv_map_ldt_shadow_page                       641     644      +3
> pv_emulate_gate_op                          2919    2922      +3
> livepatch_op.cold                            557     509     -48
> livepatch_op                                5952    5261    -691
> 
> which is clearly changing inlining decisions.  I suspect it's related to...
> 
>> --- a/xen/arch/x86/usercopy.c
>> +++ b/xen/arch/x86/usercopy.c
>> @@ -11,23 +11,23 @@
>>  #include <asm/uaccess.h>
>>  
>>  #ifndef GUARD
>> -# define GUARD UA_KEEP
>> +# define GUARD 1
>>  #endif
>>  
>>  unsigned int copy_to_guest_ll(void __user *to, const void *from, unsigned 
>> int n)
>>  {
>> -    GUARD(unsigned dummy);
>> +    unsigned __maybe_unused dummy;
> 
> ... this.  This doesn't need to be __maybe_unused, because ...
> 
>>  
>>      stac();
>>      asm volatile (
>> -        GUARD(
>> +        ".if " STR(GUARD) "\n"
>>          "    guest_access_mask_ptr %[to], %q[scratch1], %q[scratch2]\n"
>> -        )
>> +        ".endif\n"
>>          "1:  rep movsb\n"
>>          "2:\n"
>>          _ASM_EXTABLE(1b, 2b)
>> -        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from)
>> -          GUARD(, [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy))
>> +        : [cnt] "+c" (n), [to] "+D" (to), [from] "+S" (from),
>> +          [scratch1] "=&r" (dummy), [scratch2] "=&r" (dummy)
> 
> ... these parameters are referenced unconditionally.
> 
> However, it does mean the compiler is spilling the scratch registers
> even when guard is 0.  I expect this is what is affecting the inlining
> decisions.

Or the increased number of newlines that the asm() expands to (or the
combination of both).

Jan