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 <jbeul...@suse.com>
>
> 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
