target-i386: add Skylake-Client cpu mode
diff mbox

Message ID 1461744786-47643-1-git-send-email-guangrong.xiao@linux.intel.com
State New
Headers show

Commit Message

Xiao Guangrong April 27, 2016, 8:13 a.m. UTC
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(+)

Comments

Eduardo Habkost April 27, 2016, 11:56 a.m. UTC | #1
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
>
Eduardo Habkost April 28, 2016, 5:34 p.m. UTC | #2
(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?
Kai Huang April 29, 2016, 3:11 a.m. UTC | #3
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,
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiao Guangrong April 29, 2016, 3:28 a.m. UTC | #4
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,
>>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiao Guangrong May 3, 2016, 6:38 a.m. UTC | #5
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Habkost May 3, 2016, 4:11 p.m. UTC | #6
(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.
Xiao Guangrong May 3, 2016, 5:44 p.m. UTC | #7
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.


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Hansen May 3, 2016, 5:53 p.m. UTC | #8
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...).

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Habkost May 3, 2016, 6:23 p.m. UTC | #9
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?
Dave Hansen May 3, 2016, 6:29 p.m. UTC | #10
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini May 9, 2016, 1:44 p.m. UTC | #11
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eduardo Habkost May 12, 2016, 12:03 p.m. UTC | #12
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".
Paolo Bonzini May 12, 2016, 12:06 p.m. UTC | #13
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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,