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

[H. Peter Anvin]

On July 9, 2024 6:58:07 AM PDT, Xin Li <xin@xxxxxxxxx> wrote:
>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

I agree, this is desirable but separate.