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

[Jürgen Groß]

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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature