| Message ID | 20240522131703.30839-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/shadow: Trace fixes and cleanup | expand |
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@suse.com> Jan
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@suse.com> 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
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@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
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index bcd02b2d0037..1775952d7e18 100644 --- 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); } static inline void trace_shadow_fixup(guest_l1e_t gl1e, @@ -2239,7 +2243,7 @@ static int cf_check sh_page_fault( sh_reset_early_unshadow(v); perfc_incr(shadow_fault_fast_gnp); SHADOW_PRINTK("fast path not-present\n"); - trace_shadow_gen(TRC_SHADOW_FAST_PROPAGATE, va); + sh_trace_va(TRC_SHADOW_FAST_PROPAGATE, va); return 0; } #ifdef CONFIG_HVM @@ -2250,7 +2254,7 @@ static int cf_check sh_page_fault( perfc_incr(shadow_fault_fast_mmio); SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); sh_reset_early_unshadow(v); - trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); + sh_trace_va(TRC_SHADOW_FAST_MMIO, va); return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, access) ? EXCRET_fault_fixed : 0; #else @@ -2265,7 +2269,7 @@ static int cf_check sh_page_fault( * Retry and let the hardware give us the right fault next time. */ perfc_incr(shadow_fault_fast_fail); SHADOW_PRINTK("fast path false alarm!\n"); - trace_shadow_gen(TRC_SHADOW_FALSE_FAST_PATH, va); + sh_trace_va(TRC_SHADOW_FALSE_FAST_PATH, va); return EXCRET_fault_fixed; } } @@ -2481,7 +2485,7 @@ static int cf_check sh_page_fault( #endif paging_unlock(d); put_gfn(d, gfn_x(gfn)); - trace_shadow_gen(TRC_SHADOW_DOMF_DYING, va); + sh_trace_va(TRC_SHADOW_DOMF_DYING, va); return 0; } @@ -2569,7 +2573,7 @@ static int cf_check sh_page_fault( put_gfn(d, gfn_x(gfn)); perfc_incr(shadow_fault_mmio); - trace_shadow_gen(TRC_SHADOW_MMIO, va); + sh_trace_va(TRC_SHADOW_MMIO, va); return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, access) ? EXCRET_fault_fixed : 0;
The ((GUEST_PAGING_LEVELS - 2) << 8) expression in the event field is common to all shadow trace events, so introduce sh_trace() as a very thin wrapper around trace(). Then, rename trace_shadow_gen() to sh_trace_va() to better describe what it is doing, and to be more consistent with later cleanup. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: George Dunlap <george.dunlap@citrix.com> v2: * New --- xen/arch/x86/mm/shadow/multi.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)