Re: [PATCH v10 03/38] x86/msr: Add the WRMSRNS instruction support

["H. Peter Anvin" <hpa@xxxxxxxxx>]

> WRMSR has one complexity that most other PV-ops don't, and that's the
> exception table reference for the instruction itself.
>
> In a theoretical future ought to look like:
>
>     mov    $msr, %ecx
>     mov    $lo, %eax
>     mov    $hi, %edx
>     1: {call paravirt_blah(%rip) | cs...cs wrmsr | cs...cs wrmsrns }
>     _ASM_EXTABLE(1b, ...)
>
> In paravirt builds, the CALL needs to be the emitted form, because it
> needs to function in very early boot.
>
> But once the paravirt-ness has been chosen and alternatives run, the
> as-native paths are fully inline.
>
> The alternative which processes this site wants to conclude that, in the
> case it does not alter from the CALL, to clobber the EXTABLE reference. 
> CALL instructions can #GP, and you don't want to end up thinking you're
> handling a WRMSR #GP when in fact it was a non-canonical function pointer.

On 9/14/23 17:36, andrew.cooper3@xxxxxxxxxx wrote:> On 15/09/2023 1:07 
am, H. Peter Anvin wrote:
>> Is *that* your concern?! Just put a NOP before WRMSR – you need 
padding NOP bytes anyway – and the extable entry is no longer at the 
same address. Problem solved.
>>
>> Either that, or use a direct call, which can't #GP in the address 
range used by the kernel.
>
> For non-paravirt builds, I really hope the inlining DoesTheRightThing.
> If it doesn't lets fix it to do so.
>
> For paravirt builds, the emitted form must be the indirect call so it
> can function in boot prior to alternatives running [1].
>
No, it doesn't. You always have the option of a direct call to an 
indirect JMP. This is in fact exactly what userspace does in the form of 
the PLT.
> So you still need some way of putting the EXTABLE reference at the
> emitted site, not in the .altintr_replacement section where the
> WRMSR{,NS} instruction lives.  This needs to be at build time because
> the EXTABLE references aren't shuffled at runtime.
>
> How else do you propose getting an extable reference to midway through
> an instruction on the "wrong" part of an alternative?
Well, obviously there has to be a magic inline at the patch site. It 
ends up looking something like this:
        asm volatile("1:"
                     ALTERNATIVE_2("call pv_wrmsr(%%rip)",
                        "nop; wrmsr", X86_FEATURE_NATIVE_MSR,
                        "nop; wrmsrns", X86_FEATURE_WRMSRNS)
                     "2:"
                     _ASM_EXTABLE_TYPE(1b+1, 2b, EX_TYPE_WRMSR)
                     : : "c" (msr), "a" (low), "d" (high) : "memory");


[one can argue whether or not WRMSRNS specifically should require 
"memory" or not.]
The whole bit with alternatives and pvops being separate is a major 
maintainability problem, and honestly it never made any sense in the 
first place. Never have two mechanisms to do one job; it makes it harder 
to grok their interactions.
As an alternative to the NOP, the EX_TYPE_*MSR* handlers could simply 
look for a CALL opcode at the origin.
        -hpa