Re: [PATCH v2 09/12] x86/msr: Use the alternatives mechanism for WRMSR

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

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

On 30.09.25 10:31, Peter Zijlstra wrote:
> On Tue, Sep 30, 2025 at 09:03:53AM +0200, Juergen Gross wrote:
> 
>> +static __always_inline bool __wrmsrq_constant(u32 msr, u64 val, int type)
>> +{
>> +	BUILD_BUG_ON(!__builtin_constant_p(msr));
>> +
>> +	asm_inline volatile goto(
>> +		"1:\n"
>> +		ALTERNATIVE_2(PREPARE_RCX_RDX_FOR_WRMSR
>> +			      "2: ds wrmsr",
>> +			      PREPARE_RCX_RDX_FOR_WRMSR
>> +			      ASM_WRMSRNS,
>> +			      X86_FEATURE_WRMSRNS,
>> +			      ASM_WRMSRNS_IMM,
>> +			      X86_FEATURE_MSR_IMM)
>> +		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
>> +		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
>> +
>> +		:
>> +		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
>> +		: "memory", "ecx", "rdx"
>> +		: badmsr);
>> +
>> +	return false;
>> +
>> +badmsr:
>> +	return true;
>> +}
> 
> Just wondering, would something this work?
> 
> 	asm_inline volatile goto(
> 		"1:\n"
> 		ALTERNATIVE(PREPARE_RCX_RDX_FOR_WRMSR
> 			    "2:\n"
> 			    ALTERNATIVE("ds wrmsr",
> 			                ASM_WRMSRNS, X86_FEATURE_WRMSRNS),
> 			    ASM_WRMSRNS_IMM, X86_FEATURE_MSR_IMM);
> 		_ASM_EXTABLE_TYPE(1b, %l[badmsr], %c[type])	/* For WRMSRNS immediate */
> 		_ASM_EXTABLE_TYPE(2b, %l[badmsr], %c[type])	/* For WRMSR(NS) */
> 
> 		:
> 		: [val] "a" (val), [msr] "i" (msr), [type] "i" (type)
> 		: "memory", "ecx", "rdx"
> 		: badmsr);
> 
> Its a bit weird because the nested alternative isn't for the exact same
> position I suppose. But I find it a more readable form.

I don't think it would work. Nested ALTERNATIVE()s do work only with
all of them starting at the same location. Have a look at the
ALTERNATIVE() macro, which is defining the label "771" via OLDINSTR()
and then referring to this label via ALTINSTR_ENTRY(). In your case
the ALTINSTR_ENTRY() of the outer ALTERNATIVE() invocation would find
the wrong "771" label (the one of the inner ALTERNATIVE()).

Allowing such constructs would probably require switching from preprocessor
macros to assembler macros.


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 --]