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

[Xin Li <xin@zytor.com>]

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