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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.05.2024 15:47, Andrew Cooper wrote:
> 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.

Not here, but I can see how that could happen in principle, when ...

> 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.

... inlining indeed doesn't happen. Patch 2 fits the one here in this regard
(no function calls); I have yet to look at patch 3, though.

But anyway, the present placement, while likely a little redundant, is not
the end of the world, so my R-b holds either way.

Jan