Re: [Qemu-devel] XSAVES in GET_SUPPORTED_CPUID (was Re: [PATCH] target-i386: add Skylake-Client cpu mode)

[Xiao Guangrong]

On 05/04/2016 12:11 AM, Eduardo Habkost wrote:
> (CCing KVM mailing list)
>
> On Tue, May 03, 2016 at 02:38:44PM +0800, Xiao Guangrong wrote:
> > On 04/29/2016 01:34 AM, Eduardo Habkost wrote:
> > > On Wed, Apr 27, 2016 at 04:13:06PM +0800, Xiao Guangrong wrote:
> [...]
> > > > 1. As XSAVES is disabled in upstream linux kernel by commit e88221c50
> > > > (x86/fpu: Disable XSAVES* support for now), QEMU will complain about
> > > > "warning: host doesn't support requested feature: CPUID.0DH:EAX.xsaves [bit 3]"
> > >
> > > I have been looking at the GET_SUPPORTED_CPUID code and I am not
> > > sure if commit e88221c50 is supposed to be affecting
> > > GET_SUPPORTED_CPUID, or not. It looks like it shouldn't, so I
> > > don't know why QEMU is reporting xsaves as unsupported.
> > >
> > > For reference, GET_SUPPORTED_CPUID code for function == 0xd &&
> > > idx == 1 will run:
> > >
> > >    unsigned f_xsaves = kvm_x86_ops->xsaves_supported() ? F(XSAVES) : 0;
> > >    const u32 kvm_cpuid_D_1_eax_x86_features =
> > >            F(XSAVEOPT) | F(XSAVEC) | F(XGETBV1) | f_xsaves;
> > >    /* [...] */
> > >    do_cpuid_1_ent(&entry[i], function, idx);
> > >    entry[i].eax &= kvm_cpuid_D_1_eax_x86_features;
> > >
> > > do_cpuid_1_ent() just executes the CPUID instruction.
> > >
> > > kvm_x86_ops->xsaves_supported is:
> > >
> > >    static bool vmx_xsaves_supported(void)
> > >    {
> > >            return vmcs_config.cpu_based_2nd_exec_ctrl &
> > >                    SECONDARY_EXEC_XSAVES;
> > >    }
> > >
> > > Is GET_SUPPORTED_CPUID returning XSAVES as unsupported in the
> > > system where you are running tests?
> >
> > No, it returns that XSAVES is supported.
>
> You mean it returns it as unsupported, right?

Sorry for the typo. GET_SUPPORTED_CPUID returns XSAVES is not supported if the 
feature
is cleared by host.

> > Actually F(SAVES) bit is cleared later, in __do_cpuid_ent():
> > 536                                 entry[i].eax &= 
> > kvm_cpuid_D_1_eax_x86_features;
> > 537                                 cpuid_mask(&entry[i].eax, CPUID_D_1_EAX);
> >
> > The bits unsupported by boot_cpu_data.x86_capability[CPUID_D_1_EAX] will be 
> > cleared.
>
> Oh, I didn't notice the cpuid_mask() call. You're right.
>
> > During boot, the capability is adjusted by cpu_caps_cleared, in 
> > arch/x86/kernel/cpu/common.c:
> >   971         /* Clear/Set all flags overriden by options, after probe */
> >   972         for (i = 0; i < NCAPINTS; i++) {
> >   973                 c->x86_capability[i] &= ~cpu_caps_cleared[i];
> >   974                 c->x86_capability[i] |= cpu_caps_set[i];
> >   975         }
> >
> > Actually, setup_clear_cpu_cap() exactly acts on cpu_caps_cleared():
> > 112 #define setup_clear_cpu_cap(bit) do { \
> > 113         clear_cpu_cap(&boot_cpu_data, bit);     \
> > 114         set_bit(bit, (unsigned long *)cpu_caps_cleared); \
> > 115 } while (0)
> >
> > This is the reason why setup_clear_cpu_cap(X86_FEATURE_XSAVES) introduced by 
> > commit e88221c50
> > caused XSAVES unreported by KVM.
>
> So, is this the right behavior, or KVM can support exposing
> XSAVES to guests even if the cpu_cap bit is cleared? I don't know
> if exposing XSAVES to KVM guests depend on reworking kernel xsave
> code or not.

I think current behavior is right. KVM uses kernel's APIs to save/restore FPU 
context that may
cause XSAVE not properly switched if XSAVES is used in VM but host does not see 
it.