On 23.09.26 22:26, Shreshth Srivastava wrote:
On 11.09.26 10:41, Juergen Gross wrote:In order to prepare paravirt inlining of the MSR access instructions move the calls of MSR trace functions one function level up. Introduce {read|write}_msr[_safe]() helpers allowing to have common definitions in msr.h doing the trace calls.Hi Juergen, read_msr() and write_msr() get a single wrapper below the CONFIG_PARAVIRT_XXL ifdef holding the tracepoint, so it fires for either implementation. rdpmc() moved into the same ifdef but kept the old arrangement: still defined twice, once per arm, with do_trace_rdpmc() still called from native_read_pmc() above the ifdef. With CONFIG_PARAVIRT_XXL=y that leaves rdpmc() on a path with no trace call: rdpmc() => paravirt-msr.h, PVOP_CALL1(pv_ops_msr, read_pmc) xen_read_pmc() => Xen PV sets pv_ops_msr.read_pmc to this reads the value out of the Xen shared PMU page => returns without ever calling native_read_pmc(), which is where do_trace_rdpmc() sits So msr:rdpmc doesn't fire under Xen PV, while msr:read_msr and msr:write_msr now do. Was that deliberate? If it wasn't, here is a diff that treats rdpmc the same way as the other six: rename both definitions to read_pmc(), matching the pv_ops_msr member they dispatch to, and add one rdpmc() below the endif holding the tracepoint. native_read_pmc() is then an untraced primitive next to native_rdmsrq() and native_wrmsrq(). Would something like this help?
My patch doesn't change anything in this regard, as the Xen PV case didn't write trace entries for read_pmc() before. OTOH I agree that this is more like an oversight than a design decision, so I'll do something along the lines you are suggesting below. Thanks for the review!
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h index eba325ecfe4c..6f50b703fba9 100644 --- a/arch/x86/include/asm/msr.h +++ b/arch/x86/include/asm/msr.h @@ -305,8 +305,7 @@ static __always_inline u64 native_read_pmc(int counter) EAX_EDX_DECLARE_ARGS(val, low, high);asm volatile("rdpmc" : EAX_EDX_RET(val, low, high) : "c" (counter));- if (tracepoint_enabled(rdpmc)) - do_trace_rdpmc(counter, EAX_EDX_VAL(val, low, high), 0); + return EAX_EDX_VAL(val, low, high); }@@ -343,7 +342,7 @@ static __always_inline int write_msrns_safe(u32 msr, u64 val)return native_wrmsrns_safe(msr, val); }-static __always_inline u64 rdpmc(int counter)+static __always_inline u64 read_pmc(int counter) { return native_read_pmc(counter); } @@ -413,6 +412,16 @@ static __always_inline int wrmsrns_safe(u32 msr, u64 val) return err; }+static __always_inline u64 rdpmc(int counter)+{ + u64 val = read_pmc(counter); + + if (tracepoint_enabled(rdpmc)) + do_trace_rdpmc(counter, val, 0); + + return val; +} + static __always_inline void sync_cpu_after_wrmsrns(void) { if (cpu_feature_enabled(X86_FEATURE_WRMSRNS)) diff --git a/arch/x86/include/asm/paravirt-msr.h b/arch/x86/include/asm/paravirt-msr.h index ba3ee64446db..47220bf16cf3 100644 --- a/arch/x86/include/asm/paravirt-msr.h +++ b/arch/x86/include/asm/paravirt-msr.h @@ -172,7 +172,7 @@ static __always_inline int write_msrns_safe(u32 msr, u64 val) return err ? -EIO : 0; }-static __always_inline u64 rdpmc(int counter)+static __always_inline u64 read_pmc(int counter) { return PVOP_CALL1(u64, pv_ops_msr, read_pmc, counter); } I have no Xen PV guest, so that path is untested. What I did check is that the __tracepoint_rdpmc relocation shows up in arch/x86/events/core.o with CONFIG_PARAVIRT_XXL=y, where it previously did not, and that x86_64 defconfig still builds clean with gcc and clang.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
