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
