| Message ID | 1461744786-47643-1-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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]" Seeing this warning means live migration will be broken (maybe we should mention this explicitly in the warning output). If XSAVES is still not supported, we shouldn't add it yet. I see 2 options: 1) Add a CPU model without XSAVES today (e.g. "Skylake-Client-noXSAVES"), and add a new one with XSAVES ("Skylake-Client") once we have XSAVES working on some hosts. Then both cases would be supported out of the box. 2) Wait until we have XSAVES working, and only add a CPU model with XSAVES. This means -cpu Skylake-Client would work out of the box only on hosts with XSAVES. Older systems would require using an older CPU model (e.g. Broadwell) or "-cpu Skylake-Client,-XSAVES". I guess it depends on how long we expect to take for XSAVES support to become available. If it won't take long, we can do (2). If it will take a few kernel releases, we could do (1). We've already had at least 5 kernel releases without XSAVES support (v4.1 to v4.6-rc5), so I believe we should do (1). What do others think? > > 2. We will post out the patch introducing Skylake-Server mode once it > has been verified on a Skylake Sever machine Thanks! > > [ Xiao: largely based on Eduardo's work, introduce Skylake-Client and > Skylake-Server cpu modes separately. ] > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > target-i386/cpu.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index ddae932..2f2c647 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1228,6 +1228,45 @@ static X86CPUDefinition builtin_x86_defs[] = { > .model_id = "Intel Core Processor (Broadwell)", > }, > { > + .name = "Skylake-Client", > + .level = 0xd, > + .vendor = CPUID_VENDOR_INTEL, > + .family = 6, > + .model = 94, > + .stepping = 3, > + .features[FEAT_1_EDX] = > + CPUID_VME | CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX | > + CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA | > + CPUID_PGE | CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 | > + CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | > + CPUID_DE | CPUID_FP87, > + .features[FEAT_1_ECX] = > + CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES | > + CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 | > + CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | > + CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 | > + CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_FMA | CPUID_EXT_MOVBE | > + CPUID_EXT_PCID | CPUID_EXT_F16C | CPUID_EXT_RDRAND, > + .features[FEAT_8000_0001_EDX] = > + CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_NX | > + CPUID_EXT2_SYSCALL, > + .features[FEAT_8000_0001_ECX] = > + CPUID_EXT3_ABM | CPUID_EXT3_LAHF_LM | CPUID_EXT3_3DNOWPREFETCH, > + .features[FEAT_7_0_EBX] = > + CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | > + CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | > + CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | > + CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | > + CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_MPX, > + .features[FEAT_XSAVE] = > + CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC | CPUID_XSAVE_XSAVES | > + CPUID_XSAVE_XGETBV1, > + .features[FEAT_6_EAX] = > + CPUID_6_EAX_ARAT, > + .xlevel = 0x80000008, > + .model_id = "Intel Core Processor (Skylake)", > + }, > + { > .name = "Opteron_G1", > .level = 5, > .vendor = CPUID_VENDOR_AMD, > -- > 1.8.3.1 >
(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?
Hi Guangrong, How about also add SGX to Skylake? Thanks, -Kai On 4/27/2016 8:13 PM, 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]" > > 2. We will post out the patch introducing Skylake-Server mode once it > has been verified on a Skylake Sever machine > > [ Xiao: largely based on Eduardo's work, introduce Skylake-Client and > Skylake-Server cpu modes separately. ] > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > target-i386/cpu.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index ddae932..2f2c647 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1228,6 +1228,45 @@ static X86CPUDefinition builtin_x86_defs[] = { > .model_id = "Intel Core Processor (Broadwell)", > }, > { > + .name = "Skylake-Client", > + .level = 0xd, > + .vendor = CPUID_VENDOR_INTEL, > + .family = 6, > + .model = 94, > + .stepping = 3, > + .features[FEAT_1_EDX] = > + CPUID_VME | CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX | > + CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA | > + CPUID_PGE | CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 | > + CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | > + CPUID_DE | CPUID_FP87, > + .features[FEAT_1_ECX] = > + CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES | > + CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 | > + CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | > + CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 | > + CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_FMA | CPUID_EXT_MOVBE | > + CPUID_EXT_PCID | CPUID_EXT_F16C | CPUID_EXT_RDRAND, > + .features[FEAT_8000_0001_EDX] = > + CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_NX | > + CPUID_EXT2_SYSCALL, > + .features[FEAT_8000_0001_ECX] = > + CPUID_EXT3_ABM | CPUID_EXT3_LAHF_LM | CPUID_EXT3_3DNOWPREFETCH, > + .features[FEAT_7_0_EBX] = > + CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | > + CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | > + CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | > + CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | > + CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_MPX, > + .features[FEAT_XSAVE] = > + CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC | CPUID_XSAVE_XSAVES | > + CPUID_XSAVE_XGETBV1, > + .features[FEAT_6_EAX] = > + CPUID_6_EAX_ARAT, > + .xlevel = 0x80000008, > + .model_id = "Intel Core Processor (Skylake)", > + }, > + { > .name = "Opteron_G1", > .level = 5, > .vendor = CPUID_VENDOR_AMD, >
No, i am afraid it is not a Skylake Client feature. On 04/29/2016 11:11 AM, Huang, Kai wrote: > Hi Guangrong, > > How about also add SGX to Skylake? > > Thanks, > -Kai > > On 4/27/2016 8:13 PM, 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]" >> >> 2. We will post out the patch introducing Skylake-Server mode once it >> has been verified on a Skylake Sever machine >> >> [ Xiao: largely based on Eduardo's work, introduce Skylake-Client and >> Skylake-Server cpu modes separately. ] >> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> target-i386/cpu.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index ddae932..2f2c647 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -1228,6 +1228,45 @@ static X86CPUDefinition builtin_x86_defs[] = { >> .model_id = "Intel Core Processor (Broadwell)", >> }, >> { >> + .name = "Skylake-Client", >> + .level = 0xd, >> + .vendor = CPUID_VENDOR_INTEL, >> + .family = 6, >> + .model = 94, >> + .stepping = 3, >> + .features[FEAT_1_EDX] = >> + CPUID_VME | CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX | >> + CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA | >> + CPUID_PGE | CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 | >> + CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | >> + CPUID_DE | CPUID_FP87, >> + .features[FEAT_1_ECX] = >> + CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES | >> + CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 | >> + CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | >> + CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 | >> + CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_FMA | CPUID_EXT_MOVBE | >> + CPUID_EXT_PCID | CPUID_EXT_F16C | CPUID_EXT_RDRAND, >> + .features[FEAT_8000_0001_EDX] = >> + CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_NX | >> + CPUID_EXT2_SYSCALL, >> + .features[FEAT_8000_0001_ECX] = >> + CPUID_EXT3_ABM | CPUID_EXT3_LAHF_LM | CPUID_EXT3_3DNOWPREFETCH, >> + .features[FEAT_7_0_EBX] = >> + CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | >> + CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | >> + CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | >> + CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | >> + CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_MPX, >> + .features[FEAT_XSAVE] = >> + CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC | CPUID_XSAVE_XSAVES | >> + CPUID_XSAVE_XGETBV1, >> + .features[FEAT_6_EAX] = >> + CPUID_6_EAX_ARAT, >> + .xlevel = 0x80000008, >> + .model_id = "Intel Core Processor (Skylake)", >> + }, >> + { >> .name = "Opteron_G1", >> .level = 5, >> .vendor = CPUID_VENDOR_AMD, >> >
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.
(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? > > 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.
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.
On 05/03/2016 10:44 AM, Xiao Guangrong wrote: >>> 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. I don't think this is a correct statement. The guest's use of XSAVES is completely independent of what instructions the host (kernel) uses for its xsave buffer. For instance, just because the kernel doesn't use XSAVES to context switch Processor Trace, it does not make Processor Trace unusable to a guest. Guests are still free to do what they want with it (including using XSAVES for its MSRs too btw...).
On Tue, May 03, 2016 at 10:53:43AM -0700, Dave Hansen wrote: > On 05/03/2016 10:44 AM, Xiao Guangrong wrote: > >>> 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. > > I don't think this is a correct statement. > > The guest's use of XSAVES is completely independent of what instructions > the host (kernel) uses for its xsave buffer. > > For instance, just because the kernel doesn't use XSAVES to context > switch Processor Trace, it does not make Processor Trace unusable to a > guest. Guests are still free to do what they want with it (including > using XSAVES for its MSRs too btw...). > In this case, is it better to replace the setup_clear_cpu_cap(X86_FEATURE_XSAVES) quirk with some other workaround that won't affect GET_SUPPORTED_CPUID, or should we make GET_SUPPORTED_CPUID ignore cpu_cap in the case of X86_FEATURE_XSAVES?
On 05/03/2016 11:23 AM, Eduardo Habkost wrote: > On Tue, May 03, 2016 at 10:53:43AM -0700, Dave Hansen wrote: >> On 05/03/2016 10:44 AM, Xiao Guangrong wrote: >>>>> 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. >> >> I don't think this is a correct statement. >> >> The guest's use of XSAVES is completely independent of what instructions >> the host (kernel) uses for its xsave buffer. >> >> For instance, just because the kernel doesn't use XSAVES to context >> switch Processor Trace, it does not make Processor Trace unusable to a >> guest. Guests are still free to do what they want with it (including >> using XSAVES for its MSRs too btw...). >> > > In this case, is it better to replace the > setup_clear_cpu_cap(X86_FEATURE_XSAVES) quirk with some other > workaround that won't affect GET_SUPPORTED_CPUID, or should we > make GET_SUPPORTED_CPUID ignore cpu_cap in the case of > X86_FEATURE_XSAVES? I think we should introduce a X86_FEATURE_ in word 3 (the Linux-defined ones) that says whether Linux is *using* XSAVES for its FPU buffers. Then make sure that we leave X86_FEATURE_XSAVES alone so that it accurately represents what the _hardware_ supports. We might even want to change the name of X86_FEATURE_XSAVES so we don't confuse it with the software bit.
On 03/05/2016 19:53, Dave Hansen wrote: > The guest's use of XSAVES is completely independent of what instructions > the host (kernel) uses for its xsave buffer. > > For instance, just because the kernel doesn't use XSAVES to context > switch Processor Trace, it does not make Processor Trace unusable to a > guest. Guests are still free to do what they want with it (including > using XSAVES for its MSRs too btw...). True. In addition, there are other considerations to make: 1) right now, KVM is *not* planning to use XSAVES to marshal/unmarshal MSRs out of the hypervisor and back in. Instead it will use KVM_GET_MSR/KVM_SET_MSR. This is for at least two reasons. First, because KVM_GET_XSAVE/KVM_SET_XSAVE is still using the non-compacted XSAVE/XSAVEOPT format and there's simply no room (as far as I understasnd) for supervisor state save components in the non-compacted format. Second, because QEMU anway doesn't like treating the XSAVE area as a black box so we'd be parsing the fields around KVM_GET_XSAVE/KVM_SET_XSAVE. 2) KVM doesn't yet expose any XSAVES state save component, and the only one defined in Skylake (processor tracing) probably will block migration and will have to be added separately. Item number 1 means that it is particularly easy to re-enable XSAVES for guests only (and Dave's proposal later in the threaad sounds great). Item number 2 on the other hand means that it's okay to add Skylake CPU models without XSAVES. Because of the large number of kernels in the wild that block XSAVES, I'm inclined to do that. Thanks, Paolo
On Mon, May 09, 2016 at 03:44:57PM +0200, Paolo Bonzini wrote: [...] > 2) KVM doesn't yet expose any XSAVES state save component, and the only > one defined in Skylake (processor tracing) probably will block migration > and will have to be added separately. > [...] > Item number 2 on the other hand means that it's okay to add Skylake CPU > models without XSAVES. Because of the large number of kernels in the > wild that block XSAVES, I'm inclined to do that. Agreed. Now, should we name the CPU model without XSAVES "Skylake" or "Skylake-noXSAVES"? I'm inclined towards the latter, to follow the same pattern we used for "Haswell-noTSX".
On 12/05/2016 14:03, Eduardo Habkost wrote: > > Item number 2 on the other hand means that it's okay to add Skylake CPU > > models without XSAVES. Because of the large number of kernels in the > > wild that block XSAVES, I'm inclined to do that. > > Agreed. Now, should we name the CPU model without XSAVES > "Skylake" or "Skylake-noXSAVES"? I'm inclined towards the latter, > to follow the same pattern we used for "Haswell-noTSX". Do we have a plan to add Skylake with XSAVES? I think no, so it should be fine to do .name = "Skylake", .features[FEATURE_XSAVE] = CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XGETBV1 | CPUID_XSAVE_XSAVEC /* omitting CPUID_XSAVE_XSAVES because... */ Haswell-noTSX was added only because we already had a model with TSX. If we hadn't we probably would have had: - Haswell without TSX - Broadwell without TSX - Broadwell-EX with TSX (or something like that). Thanks, Paolo
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index ddae932..2f2c647 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1228,6 +1228,45 @@ static X86CPUDefinition builtin_x86_defs[] = { .model_id = "Intel Core Processor (Broadwell)", }, { + .name = "Skylake-Client", + .level = 0xd, + .vendor = CPUID_VENDOR_INTEL, + .family = 6, + .model = 94, + .stepping = 3, + .features[FEAT_1_EDX] = + CPUID_VME | CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX | + CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA | + CPUID_PGE | CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 | + CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE | + CPUID_DE | CPUID_FP87, + .features[FEAT_1_ECX] = + CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES | + CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 | + CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 | + CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 | + CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_FMA | CPUID_EXT_MOVBE | + CPUID_EXT_PCID | CPUID_EXT_F16C | CPUID_EXT_RDRAND, + .features[FEAT_8000_0001_EDX] = + CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_NX | + CPUID_EXT2_SYSCALL, + .features[FEAT_8000_0001_ECX] = + CPUID_EXT3_ABM | CPUID_EXT3_LAHF_LM | CPUID_EXT3_3DNOWPREFETCH, + .features[FEAT_7_0_EBX] = + CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | + CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | + CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID | + CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX | + CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_MPX, + .features[FEAT_XSAVE] = + CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC | CPUID_XSAVE_XSAVES | + CPUID_XSAVE_XGETBV1, + .features[FEAT_6_EAX] = + CPUID_6_EAX_ARAT, + .xlevel = 0x80000008, + .model_id = "Intel Core Processor (Skylake)", + }, + { .name = "Opteron_G1", .level = 5, .vendor = CPUID_VENDOR_AMD,