Re: [PATCH 17/21] x86/fpu: Use proper mask to replace full instruction mask

Liang, Kan
2020-06-22T17:47:31Z

On 6/22/2020 11:02 AM, Dave Hansen wrote:
> On 6/22/20 7:52 AM, Liang, Kan wrote:
>>>> --- a/arch/x86/kernel/fpu/xstate.c
>>>> +++ b/arch/x86/kernel/fpu/xstate.c
>>>> @@ -58,6 +58,7 @@ static short xsave_cpuid_features[] __initdata = {
>>>>    * XSAVE buffer, both supervisor and user xstates.
>>>>    */
>>>>   u64 xfeatures_mask_all __read_mostly;
>>>> +EXPORT_SYMBOL_GPL(xfeatures_mask_all);
>>>
>>> *groan*...
>>>
>>> AFAICT KVM doesn't actually use any of those functions,
>>
>> It seems KVM may eventually invoke copy_xregs_to_kernel() as below.
>>
>> kvm_save_current_fpu()
>>     copy_fpregs_to_fpstate()
>>         copy_xregs_to_kernel()
>>
>> I think we have to export the xfeatures_mask_all.
>
> I'm wondering if we should just take these copy_*regs_to_*() functions
> and uninline them. Yeah, they are basically wrapping one instruction,
> but it might literally be the most heavyweight instruction in the whole ISA.
>

Thanks for the suggestions, but I'm not sure if I follow these methods.

I don't think simply removing the "inline" key word for the
copy_xregs_to_kernel() functions would help here.
Do you mean exporting the copy_*regs_to_*()?

> Or, maybe just make an out-of-line version for KVM to call?
>

I think the out-of-line version for KVM still needs the
xfeatures_mask_all. Because the size of vcpu's XSAVE buffer
(&vcpu->arch.guest_fpu) is the same as other kernel XSAVE buffers, such
as task->fpu. The xfeatures_mask_all is required for KVM to filter out
the dynamic supervisor feature as well. I think even if we make an
out-of-line version for KVM, we still have to export the
xfeatures_mask_all for KVM.

Thanks,
Kan