Re: [Qemu-devel] [PATCH] target-i386: add Skylake-Client cpu mode

From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: pbonzini@redhat.com, imammedo@redhat.com, kvm@vger.kernel.org,
	mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com,
	qemu-devel@nongnu.org, stefanha@redhat.com,
	dan.j.williams@intel.com, rth@twiddle.net,
	Dave Hansen <dave.hansen@intel.com>,
	"Yu, Yu-cheng" <yu-cheng.yu@intel.com>,
	"Wang, Yong Y" <yong.y.wang@intel.com>,
	"virt-intel-list@redhat.com" <virt-intel-list@redhat.com>,
	"Kasten, Robert A" <robert.a.kasten@intel.com>,
	"Lai, Paul C" <paul.c.lai@intel.com>,
	"Xiao, Guangrong" <guangrong.xiao@intel.com>,
	"ruwang@redhat.com" <ruwang@redhat.com>,
	"Shankar, Ravi V" <ravi.v.shankar@intel.com>,
	"Yu, Fenghua" <fenghua.yu@intel.com>
Subject: Re: [Qemu-devel] [PATCH] target-i386: add Skylake-Client cpu mode
Date: Tue, 3 May 2016 14:38:44 +0800	[thread overview]
Message-ID: <57284774.7010401@linux.intel.com> (raw)
In-Reply-To: <20160428173443.GA24153@thinpad.lan.raisama.net>

On 04/29/2016 01:34 AM, Eduardo Habkost wrote:
> (CCing some additional Intel people)
>
> On Wed, Apr 27, 2016 at 04:13:06PM +0800, Xiao Guangrong wrote:
>> From: Eduardo Habkost <ehabkost@redhat.com>
>>
>> Introduce Skylake-Client cpu mode which inherits the features from
>> Broadwell and supports some additional features that are: MPX,
>> XSAVEC, XSAVES and XGETBV1
>>
>> Note:
>> 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.

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.

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.