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

H. Peter Anvin
2023-09-15T01:01:57Z

> 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@citrix.com 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