Re: [PATCH v5 04/17] x86/msr: Move MSR trace calls one function level up

[Shreshth Srivastava <shreshth.srivastava@intel.com>]

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?

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.

Thanks,
Shreshth