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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4122 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]