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

[Juergen Gross <jgross@suse.com>]

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

On 30.09.25 10:46, Jürgen Groß wrote:
> 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.

Thinking more about that I believe there are additional problems:

Having overlapping alternatives not starting at the same address will result
in problems with length padding of the outer alternative in case the inner
one starting later is extending past the end of the outer one. This might be
possible to handle, but it will be tedious.

A similar problem occurs with my recent series for merging nested alternative
patching into a temporary buffer. Currently the code relies on all nested
alternatives to start at the same location.

Using your idea with pv_ops could result in the inner alternative not being
at the start of the outer alternative AND being not the initial code. This
would result in patching in the .altinstructions area instead of the normal
.text site, resulting in possible loss of a patching action if the patched
area would have been used as a replacement before.

So in my opinion allowing alternatives to nest without all inner levels
starting at the same location as the outermost level would be a receipt for
failure.

I think I'll write another patch to BUG() in case such a situation is being
detected.


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