linux-kernel - Re: [PATCH v1 2/4] x86/fred: Write to FRED MSRs with wrmsrns()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <246c77ec-1911-4f43-9ce6-7e087b3a4562@zytor.com>
Date: Tue, 9 Jul 2024 06:58:07 -0700
From: Xin Li <xin@...or.com>
To: Borislav Petkov <bp@...en8.de>, Andrew Cooper <andrew.cooper3@...rix.com>
Cc: "H. Peter Anvin" <hpa@...or.com>, dave.hansen@...el.com,
        linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
        dave.hansen@...ux.intel.com, x86@...nel.org, peterz@...radead.org,
        nik.borisov@...e.com, houwenlong.hwl@...group.com,
        Juergen Gross <jgross@...e.com>
Subject: Re: [PATCH v1 2/4] x86/fred: Write to FRED MSRs with wrmsrns()

On 7/5/2024 6:45 AM, Borislav Petkov wrote:
> On Fri, Jul 05, 2024 at 11:30:16AM +0100, Andrew Cooper wrote:
>> You cite perf.  Look at the disassembly of the two approaches...
>>
>> cpu_feature_enabled() might give you warm fuzzy feelings that you've
>> eekd out every ounce of performance, but it's an absolute disaster at a
>> code generation level by forcing the compiler to lay out both side and
>> preventing any kind of CSE.  As I've reported before, count the number
>> of RDPKRU instructions in trivial-looking xsave handling functions for a
>> glimpse of the practical consequences.
> 
> Yes, I do cite perf because what you have above is not saying: "yes, this is
> a fast path and doing an alternative is warranted." If that is the case, sure,
> by all means. If not, make the C readable and ignore code generation. Who
> cares.
> 
>> Anyway, none of this is the complicated aspect.  The complicated issue
>> is the paravirt wrmsr().
>>
>> TGLX's complaint is that everyone turns on CONFIG_PARAVIRT, and the
>> paravirt hook for wmsr() is a code generation disaster WRT parameter
>> handling.  I agree that it's not great, although it's got nothing on the
>> damage done by cpu_feature_enabled().
>>
>>
>> But, seeing as I've got everyone's attention, I'll repeat my proposal
>> for fixing this nicely, in the hope of any feedback on the 3rd posting...
>>
>> The underlying problem is that parameter setup for the paravirt wrmsr()
>> follows a C calling convention, so the index/data are manifested into
>> %rdi/%rsi.  Then, the out-line "native" hook shuffles the index/data
>> back into %ecx/%edx/%eax, and this cost is borne in all kernels.
> 
> A handful of reg ops per a WRMSRNS? Meh, same argument as above. But...
> 
>> Instead, the better way would be to have a hook with a non-standard
>> calling convention which happens to match the WRMSR instruction.
>>
>> That way, the native, and simple paravirt paths inline to a single
>> instruction with no extraneous parameter shuffling, and the shuffling
>> cost is borne by PARAVIRT_XXL only, where a reg/reg move is nothing
>> compared to the hypercall involved.
>>
>> The only complication is the extable #GP hook, but that's fine to place
>> at the paravirt site as long as the extable handler confirms the #GP
>> came from a WRMSR{NS,} and not a branch.
> 
> ... yes, I'd gladly review patches which address that and make the whole deal
> cleaner. I'm still sceptical those handful of regs shuffling ops would matter
> in any benchmark but sure, if it can be done in a cleaner way, why not...
> 
> Unless I'm missing some use case where that overhead really matters. Then by
> all means...
> 

It looks that it no longer makes sense to include this patch in this
patchset; it is not something that can be done as a small cleanup.

Any objection?

Thanks!
     Xin