Message-ID: <B93766B5-563E-401D-A1EF-579077CAEE82@zytor.com>
Date:   Thu, 14 Sep 2023 16:34:40 -0700
From:   "H. Peter Anvin" <hpa@...or.com>
To:     Thomas Gleixner <tglx@...utronix.de>, andrew.cooper3@...rix.com,
        Xin Li <xin3.li@...el.com>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-edac@...r.kernel.org,
        linux-hyperv@...r.kernel.org, kvm@...r.kernel.org,
        xen-devel@...ts.xenproject.org
CC:     mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        x86@...nel.org, luto@...nel.org, pbonzini@...hat.com,
        seanjc@...gle.com, peterz@...radead.org, jgross@...e.com,
        ravi.v.shankar@...el.com, mhiramat@...nel.org,
        jiangshanlai@...il.com
Subject: Re: [PATCH v10 03/38] x86/msr: Add the WRMSRNS instruction support

On September 14, 2023 4:00:39 PM PDT, Thomas Gleixner <tglx@...utronix.de> wrote:
>Andrew!
>
>On Thu, Sep 14 2023 at 15:05, andrew wrote:
>> On 14/09/2023 5:47 am, Xin Li wrote:
>>> +static __always_inline void wrmsrns(u32 msr, u64 val)
>>> +{
>>> +	__wrmsrns(msr, val, val >> 32);
>>> +}
>>
>> This API works in terms of this series where every WRMSRNS is hidden
>> behind a FRED check, but it's an awkward interface to use anywhere else
>> in the kernel.
>
>Agreed.
>
>> I fully understand that you expect all FRED capable systems to have
>> WRMSRNS, but it is not a hard requirement and you will end up with
>> simpler (and therefore better) logic by deleting the dependency.
>
>According to the CPU folks FRED systems are guaranteed to have WRMSRNS -
>I asked for that :). It's just not yet documented.
>
>But that I aside, I agree that we should opt for the safe side with a
>fallback like the one you have in XEN even for the places which are
>strictly FRED dependent.
>
>> As a "normal" user of the WRMSR APIs, the programmer only cares about:
>>
>> 1) wrmsr() -> needs to be serialising
>> 2) wrmsr_ns() -> safe to be non-serialising
>
>Correct.
>
>> In Xen, I added something of the form:
>>
>> /* Non-serialising WRMSR, when available.  Falls back to a serialising
>> WRMSR. */
>> static inline void wrmsr_ns(uint32_t msr, uint32_t lo, uint32_t hi)
>> {
>>     /*
>>      * WRMSR is 2 bytes.  WRMSRNS is 3 bytes.  Pad WRMSR with a redundant CS
>>      * prefix to avoid a trailing NOP.
>>      */
>>     alternative_input(".byte 0x2e; wrmsr",
>>                       ".byte 0x0f,0x01,0xc6", X86_FEATURE_WRMSRNS,
>>                       "c" (msr), "a" (lo), "d" (hi));
>> }
>>
>> and despite what Juergen said, I'm going to recommend that you do wire
>> this through the paravirt infrastructure, for the benefit of regular
>> users having a nice API, not because XenPV is expecting to do something
>> wildly different here.
>
>I fundamentaly hate adding this to the PV infrastructure. We don't want
>more PV ops, quite the contrary.
>
>For the initial use case at hand, there is an explicit FRED dependency
>and the code in question really wants to use WRMSRNS directly and not
>through a PV function call.
>
>I agree with your reasoning for the more generic use case where we can
>gain performance independent of FRED by using WRMSRNS for cases where
>the write has no serialization requirements.
>
>But this made me look into PV ops some more. For actual performance
>relevant code the current PV ops mechanics are a horrorshow when the op
>defaults to the native instruction.
>
>Let's 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().
>
>The call makes only sense, when the native default is an actual
>function, but for the trivial cases it's a blatant engineering
>trainwreck.
>
>I wouldn't care at all if CONFIG_PARAVIRT_XXL would be the esoteric use
>case, but AFAICT it's default enabled on all major distros.
>
>So no. I'm fundamentally disagreeing with your recommendation. The way
>forward is:
>
>  1) Provide the native variant for wrmsrns(), i.e. rename the proposed
>     wrmsrns() to native_wrmsr_ns() and have the X86_FEATURE_WRMSRNS
>     safety net as you pointed out.
>
>     That function can be used in code which is guaranteed to be not
>     affected by the PV_XXL madness.
>
>  2) Come up with a sensible solution for the PV_XXL horrorshow
>
>  3) Implement a sane general variant of wrmsr_ns() which handles
>     both X86_FEATURE_WRMSRNS and X86_MISFEATURE_PV_XXL
>
>  4) Convert other code which benefits from the non-serializing variant
>     to wrmsr_ns()
>
>Thanks,
>
>        tglx
>

With regards to (2), the IMO only sensible solution is to have the ABI be the one of the native instruction, and to have the PVXXL users take the full brunt of the overhead. That means that on a native or HVM machine, the proper code gets patched in inline, and the PVXXL code becomes a call to a stub to do the parameter marshalling before walking off back into C ABI land. The pvop further has to bear the full cost of providing the full native semantics unless otherwise agreed with the native maintainers and explicitly documented what the modified semantics are (notably, having excplicit stubs for certain specific MSRs is entirely reasonable.)

In case this sounds familiar, it is the pvops we were promised over 15 years ago, and yet somehow never really got there. It *also* is similar in an inside-out way of the ABI marshalling used for legacy BIOS functions in the 16-bit startup code.

In-place "fat" paravirtualization in the Linux kernel has been a horror show, with the notable exception of UML, which is almost universally ignored but yet manages to stay out of the way and keep working.

This is a classic case of Tragedy of The Commons, much like burning coal for power.

Powered by blists - more mailing lists