Re: [PATCH v2 1/4] x86/shadow: Rework trace_shadow_gen() into sh_trace_va()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/05/2024 2:40 pm, Jan Beulich wrote:
> On 22.05.2024 15:17, Andrew Cooper wrote:
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -1974,13 +1974,17 @@ typedef u32 guest_va_t;
>>  typedef u32 guest_pa_t;
>>  #endif
>>  
>> -static inline void trace_shadow_gen(u32 event, guest_va_t va)
>> +/* Shadow trace event with GUEST_PAGING_LEVELS folded into the event field. 
>> */
>> +static void sh_trace(uint32_t event, unsigned int extra, const void 
>> *extra_data)
>> +{
>> +    trace(event | ((GUEST_PAGING_LEVELS - 2) << 8), extra, extra_data);
>> +}
>> +
>> +/* Shadow trace event with the guest's linear address. */
>> +static void sh_trace_va(uint32_t event, guest_va_t va)
>>  {
>>      if ( tb_init_done )
>> -    {
>> -        event |= (GUEST_PAGING_LEVELS-2)<<8;
>> -        trace(event, sizeof(va), &va);
>> -    }
>> +        sh_trace(event, sizeof(va), &va);
>>  }
> If any tb_init_done check, then perhaps rather in sh_trace()? With that
> (and provided you agree)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Sadly not.  That leads to double reads of tb_init_done when tracing is
compiled in.

When GCC can't fully inline the structure initialisation, it can't prove
that a function call modified tb_init_done.  This is why I arranged all
the trace cleanup in this way.

~Andrew