Re: [RFC PATCH v2 21/34] x86/msr: Utilize the alternatives mechanism to write MSR

Jürgen Groß
2025-04-22T09:57:05Z

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

On 22.04.25 10:22, Xin Li (Intel) wrote:
> The story started from tglx's reply in [1]:
>
> For actual performance relevant code the current PV ops mechanics
> are a horrorshow when the op defaults to the native instruction.
>
> look at wrmsrl():
>
> wrmsrl(msr, val
> wrmsr(msr, (u32)val, (u32)val >> 32))
> paravirt_write_msr(msr, low, high)
> PVOP_VCALL3(cpu.write_msr, msr, low, high)
>
> Which results in
>
> mov $msr, %edi
> mov $val, %rdx
> mov %edx, %esi
> shr $0x20, %rdx
> call native_write_msr
>
> and native_write_msr() does at minimum:
>
> mov %edi,%ecx
> mov %esi,%eax
> wrmsr
> ret
>
> In the worst case 'ret' is going through the return thunk. Not to
> talk about function prologues and whatever.
>
> This becomes even more silly for trivial instructions like STI/CLI
> or in the worst case paravirt_nop().

This is nonsense.

In the non-Xen case the initial indirect call is directly replaced with
STI/CLI via alternative patching, while for Xen it is replaced by a direct
call.

The paravirt_nop() case is handled in alt_replace_call() by replacing the
indirect call with a nop in case the target of the call was paravirt_nop()
(which is in fact no_func()).

>
> The call makes only sense, when the native default is an actual
> function, but for the trivial cases it's a blatant engineering
> trainwreck.

The trivial cases are all handled as stated above: a direct replacement
instruction is placed at the indirect call position.

> Later a consensus was reached to utilize the alternatives mechanism to
> eliminate the indirect call overhead introduced by the pv_ops APIs:
>
> 1) When built with !CONFIG_XEN_PV, X86_FEATURE_XENPV becomes a
> disabled feature, preventing the Xen code from being built
> and ensuring the native code is executed unconditionally.

This is the case today already. There is no need for any change to have
this in place.

>
> 2) When built with CONFIG_XEN_PV:
>
> 2.1) If not running on the Xen hypervisor (!X86_FEATURE_XENPV),
> the kernel runtime binary is patched to unconditionally
> jump to the native MSR write code.
>
> 2.2) If running on the Xen hypervisor (X86_FEATURE_XENPV), the
> kernel runtime binary is patched to unconditionally jump
> to the Xen MSR write code.

I can't see what is different here compared to today's state.

>
> The alternatives mechanism is also used to choose the new immediate
> form MSR write instruction when it's available.

Yes, this needs to be added.

> Consequently, remove the pv_ops MSR write APIs and the Xen callbacks.

I still don't see a major difference to today's solution.

Only the "paravirt" term has been eliminated.

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