[Qemu-devel] [PATCH v2 0/5] Add Icelake CPU model

[Qemu-devel] [PATCH v2 0/5] Add Icelake CPU model

[Robert Hoo]

This patch set defines the new guest CPU models of Icelake.

The first patch adds support of IA32_PRED_CMD MSR (IBPB) and IA32_ARCH_CAPABILITIES MSR.
Other patches add CPUID bits feature words for new features, like PCONFIG,
WBNOINVD. The final patch defines Icelake-{Server,Client} CPU models.

Changelog:
v2
	Per Paolo's comment, remove unnecessary CPU vmstate check for write/read only
IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs.

Robert Hoo (5):
  i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs
  i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR
  i386: Add CPUID bit for PCONFIG
  i386: Add CPUID bit for WBNOINVD
  i386: Add new CPU model Icelake-{Server,Client}

 target/i386/cpu.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 target/i386/cpu.h |   7 ++++
 target/i386/kvm.c |  27 +++++++++++-
 3 files changed, 152 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Robert Hoo]

IA32_PRED_CMD MSR gives software a way to issue commands that affect the state
of indirect branch predictors. Enumerated by CPUID.(EAX=7H,ECX=0):EDX[26].
IA32_ARCH_CAPABILITIES MSR enumerates architectural features of RDCL_NO and
IBRS_ALL. Enumerated by CPUID.(EAX=07H, ECX=0):EDX[29].

https://software.intel.com/sites/default/files/managed/c5/63/336996-Speculative-Execution-Side-Channel-Mitigations.pdf

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 target/i386/cpu.h |  4 ++++
 target/i386/kvm.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 89c82be..734a73e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -352,6 +352,8 @@ typedef enum X86Seg {
 #define MSR_TSC_ADJUST                  0x0000003b
 #define MSR_IA32_SPEC_CTRL              0x48
 #define MSR_VIRT_SSBD                   0xc001011f
+#define MSR_IA32_PRED_CMD               0x49
+#define MSR_IA32_ARCH_CAPABILITIES      0x10a
 #define MSR_IA32_TSCDEADLINE            0x6e0
 
 #define FEATURE_CONTROL_LOCKED                    (1<<0)
@@ -1210,6 +1212,8 @@ typedef struct CPUX86State {
 
     uint64_t spec_ctrl;
     uint64_t virt_ssbd;
+    uint64_t pred_cmd;
+    uint64_t arch_capabilities;
 
     /* End of state preserved by INIT (dummy marker).  */
     struct {} end_init_save;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 2d174f3..3aab182 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -93,6 +93,8 @@ static bool has_msr_hv_reenlightenment;
 static bool has_msr_xss;
 static bool has_msr_spec_ctrl;
 static bool has_msr_virt_ssbd;
+static bool has_msr_pred_cmd;
+static bool has_msr_arch_capabilities;
 static bool has_msr_smi_count;
 
 static uint32_t has_architectural_pmu_version;
@@ -1265,6 +1267,11 @@ static int kvm_get_supported_msrs(KVMState *s)
                     break;
                 case MSR_VIRT_SSBD:
                     has_msr_virt_ssbd = true;
+                case MSR_IA32_PRED_CMD:
+                    has_msr_pred_cmd = true;
+                    break;
+                case MSR_IA32_ARCH_CAPABILITIES:
+                    has_msr_arch_capabilities = true;
                     break;
                 }
             }
@@ -1757,7 +1764,13 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
     if (has_msr_virt_ssbd) {
         kvm_msr_entry_add(cpu, MSR_VIRT_SSBD, env->virt_ssbd);
     }
-
+    if (has_msr_pred_cmd) {
+        kvm_msr_entry_add(cpu, MSR_IA32_PRED_CMD, env->pred_cmd);
+    }
+    if (has_msr_arch_capabilities) {
+        kvm_msr_entry_add(cpu, MSR_IA32_ARCH_CAPABILITIES,
+            env->arch_capabilities);
+    }
 #ifdef TARGET_X86_64
     if (lm_capable_kernel) {
         kvm_msr_entry_add(cpu, MSR_CSTAR, env->cstar);
@@ -2140,6 +2153,13 @@ static int kvm_get_msrs(X86CPU *cpu)
     if (has_msr_virt_ssbd) {
         kvm_msr_entry_add(cpu, MSR_VIRT_SSBD, 0);
     }
+    if (has_msr_pred_cmd) {
+        kvm_msr_entry_add(cpu, MSR_IA32_PRED_CMD, 0);
+    }
+    if (has_msr_arch_capabilities) {
+        kvm_msr_entry_add(cpu, MSR_IA32_ARCH_CAPABILITIES, 0);
+    }
+
     if (!env->tsc_valid) {
         kvm_msr_entry_add(cpu, MSR_IA32_TSC, 0);
         env->tsc_valid = !runstate_is_running();
@@ -2521,6 +2541,11 @@ static int kvm_get_msrs(X86CPU *cpu)
             break;
         case MSR_VIRT_SSBD:
             env->virt_ssbd = msrs[i].data;
+        case MSR_IA32_PRED_CMD:
+            env->pred_cmd = msrs[i].data;
+            break;
+        case MSR_IA32_ARCH_CAPABILITIES:
+            env->arch_capabilities = msrs[i].data;
             break;
         case MSR_IA32_RTIT_CTL:
             env->msr_rtit_ctrl = msrs[i].data;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/5] i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR

[Robert Hoo]

Support of IA32_PRED_CMD MSR already be enumerated by same CPUID bit as
SPEC_CTRL.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 target/i386/cpu.c | 2 +-
 target/i386/cpu.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index e6c2f8a..953098c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1002,7 +1002,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, "spec-ctrl", NULL,
-            NULL, NULL, NULL, "ssbd",
+            NULL, "arch-capabilities", NULL, "ssbd",
         },
         .cpuid_eax = 7,
         .cpuid_needs_ecx = true, .cpuid_ecx = 0,
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 734a73e..1ef2040 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -688,6 +688,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) /* AVX512 Neural Network Instructions */
 #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */
 #define CPUID_7_0_EDX_SPEC_CTRL     (1U << 26) /* Speculation Control */
+#define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities of RDCL_NO and IBRS_ALL*/
 #define CPUID_7_0_EDX_SPEC_CTRL_SSBD  (1U << 31) /* Speculative Store Bypass Disable */
 
 #define CPUID_8000_0008_EBX_IBPB    (1U << 12) /* Indirect Branch Prediction Barrier */
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/5] i386: Add CPUID bit for PCONFIG

[Robert Hoo]

PCONFIG: Platform configuration, enumerated by CPUID.(EAX=07H, ECX=0):
EDX[bit18].

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 target/i386/cpu.c | 2 +-
 target/i386/cpu.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 953098c..c2c3cdb 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -999,7 +999,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
-            NULL, NULL, NULL, NULL,
+            NULL, NULL, "pconfig", NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, "spec-ctrl", NULL,
             NULL, "arch-capabilities", NULL, "ssbd",
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 1ef2040..61d23e5 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -687,6 +687,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 
 #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) /* AVX512 Neural Network Instructions */
 #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */
+#define CPUID_7_0_EDX_PCONFIG (1U << 18)       /* Platform Configuration */
 #define CPUID_7_0_EDX_SPEC_CTRL     (1U << 26) /* Speculation Control */
 #define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities of RDCL_NO and IBRS_ALL*/
 #define CPUID_7_0_EDX_SPEC_CTRL_SSBD  (1U << 31) /* Speculative Store Bypass Disable */
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/5] i386: Add CPUID bit for WBNOINVD

[Robert Hoo]

WBNOINVD: Write back and do not invalidate cache, enumerated by
CPUID.(EAX=80000008H, ECX=0):EBX[bit 9].

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 target/i386/cpu.c | 2 +-
 target/i386/cpu.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c2c3cdb..92bfbbc 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1029,7 +1029,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
         .feat_names = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
-            NULL, NULL, NULL, NULL,
+            NULL, "wbnoinvd", NULL, NULL,
             "ibpb", NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 61d23e5..c67216d 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -692,6 +692,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities of RDCL_NO and IBRS_ALL*/
 #define CPUID_7_0_EDX_SPEC_CTRL_SSBD  (1U << 31) /* Speculative Store Bypass Disable */
 
+#define CPUID_8000_0008_EBX_WBNOINVD  (1U << 9)  /* Write back and do not invalidate cache */
 #define CPUID_8000_0008_EBX_IBPB    (1U << 12) /* Indirect Branch Prediction Barrier */
 
 #define CPUID_XSAVE_XSAVEOPT   (1U << 0)
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/5] i386: Add new CPU model Icelake-{Server, Client}

[Robert Hoo]

New CPU models mostly inherit features from ancestor Skylake, while addin new
features: UMIP, New Instructions ( PCONIFIG (server only), WBNOINVD,
AVX512_VBMI2, GFNI, AVX512_VNNI, VPCLMULQDQ, VAES, AVX512_BITALG),
Intel PT and 5-level paging (Server only). As well as
IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs, SSBD support for speculative execution
side channel mitigations.

Note: For 5-level paging, Guest physical address width can be configured, with
parameter "phys-bits". Unless explicitly specified, we still use its default
value, even for Icelake-Server cpu model.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 target/i386/cpu.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 92bfbbc..ff3273a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2382,6 +2382,122 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .model_id = "Intel Xeon Processor (Skylake, IBRS)",
     },
     {
+        .name = "Icelake-Client",
+        .level = 0xd,
+        .vendor = CPUID_VENDOR_INTEL,
+        .family = 6,
+        .model = 126,
+        .stepping = 0,
+        .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_8000_0008_EBX] =
+            CPUID_8000_0008_EBX_WBNOINVD,
+        .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 | CPUID_7_0_EBX_INTEL_PT,
+        .features[FEAT_7_0_ECX] =
+            CPUID_7_0_ECX_VBMI | CPUID_7_0_ECX_UMIP | CPUID_7_0_ECX_PKU |
+            CPUID_7_0_ECX_OSPKE | CPUID_7_0_ECX_VBMI2 | CPUID_7_0_ECX_GFNI |
+            CPUID_7_0_ECX_VAES | CPUID_7_0_ECX_VPCLMULQDQ |
+            CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
+            CPUID_7_0_ECX_AVX512_VPOPCNTDQ,
+        .features[FEAT_7_0_EDX] =
+            CPUID_7_0_EDX_SPEC_CTRL | CPUID_7_0_EDX_ARCH_CAPABILITIES |
+            CPUID_7_0_EDX_SPEC_CTRL_SSBD,
+        /* Missing: XSAVES (not supported by some Linux versions,
+                * including v4.1 to v4.12).
+                * KVM doesn't yet expose any XSAVES state save component,
+                * and the only one defined in Skylake (processor tracing)
+                * probably will block migration anyway.
+                */
+        .features[FEAT_XSAVE] =
+            CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
+            CPUID_XSAVE_XGETBV1,
+        .features[FEAT_6_EAX] =
+            CPUID_6_EAX_ARAT,
+        .xlevel = 0x80000008,
+        .model_id = "Intel Core Processor (Icelake)",
+    },
+    {
+        .name = "Icelake-Server",
+        .level = 0xd,
+        .vendor = CPUID_VENDOR_INTEL,
+        .family = 6,
+        .model = 134,
+        .stepping = 0,
+        .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_PDPE1GB | 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_8000_0008_EBX] =
+            CPUID_8000_0008_EBX_WBNOINVD,
+        .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 | CPUID_7_0_EBX_CLWB |
+            CPUID_7_0_EBX_AVX512F | CPUID_7_0_EBX_AVX512DQ |
+            CPUID_7_0_EBX_AVX512BW | CPUID_7_0_EBX_AVX512CD |
+            CPUID_7_0_EBX_AVX512VL | CPUID_7_0_EBX_CLFLUSHOPT |
+            CPUID_7_0_EBX_INTEL_PT,
+        .features[FEAT_7_0_ECX] =
+            CPUID_7_0_ECX_VBMI | CPUID_7_0_ECX_UMIP | CPUID_7_0_ECX_PKU |
+            CPUID_7_0_ECX_OSPKE | CPUID_7_0_ECX_VBMI2 | CPUID_7_0_ECX_GFNI |
+            CPUID_7_0_ECX_VAES | CPUID_7_0_ECX_VPCLMULQDQ |
+            CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
+            CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57,
+        .features[FEAT_7_0_EDX] =
+            CPUID_7_0_EDX_PCONFIG | CPUID_7_0_EDX_SPEC_CTRL |
+            CPUID_7_0_EDX_ARCH_CAPABILITIES | CPUID_7_0_EDX_SPEC_CTRL_SSBD,
+        /* Missing: XSAVES (not supported by some Linux versions,
+                * including v4.1 to v4.12).
+                * KVM doesn't yet expose any XSAVES state save component,
+                * and the only one defined in Skylake (processor tracing)
+                * probably will block migration anyway.
+                */
+        .features[FEAT_XSAVE] =
+            CPUID_XSAVE_XSAVEOPT | CPUID_XSAVE_XSAVEC |
+            CPUID_XSAVE_XGETBV1,
+        .features[FEAT_6_EAX] =
+            CPUID_6_EAX_ARAT,
+        .xlevel = 0x80000008,
+        .model_id = "Intel Xeon Processor (Icelake)",
+    },
+    {
         .name = "KnightsMill",
         .level = 0xd,
         .vendor = CPUID_VENDOR_INTEL,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Eduardo Habkost]

On Wed, Jun 27, 2018 at 07:27:20PM +0800, Robert Hoo wrote:
> IA32_PRED_CMD MSR gives software a way to issue commands that affect the state
> of indirect branch predictors. Enumerated by CPUID.(EAX=7H,ECX=0):EDX[26].
> IA32_ARCH_CAPABILITIES MSR enumerates architectural features of RDCL_NO and
> IBRS_ALL. Enumerated by CPUID.(EAX=07H, ECX=0):EDX[29].
> 
> https://software.intel.com/sites/default/files/managed/c5/63/336996-Speculative-Execution-Side-Channel-Mitigations.pdf
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> ---
>  target/i386/cpu.h |  4 ++++
>  target/i386/kvm.c | 27 ++++++++++++++++++++++++++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 89c82be..734a73e 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -352,6 +352,8 @@ typedef enum X86Seg {
>  #define MSR_TSC_ADJUST                  0x0000003b
>  #define MSR_IA32_SPEC_CTRL              0x48
>  #define MSR_VIRT_SSBD                   0xc001011f
> +#define MSR_IA32_PRED_CMD               0x49
> +#define MSR_IA32_ARCH_CAPABILITIES      0x10a
>  #define MSR_IA32_TSCDEADLINE            0x6e0
>  
>  #define FEATURE_CONTROL_LOCKED                    (1<<0)
> @@ -1210,6 +1212,8 @@ typedef struct CPUX86State {
>  
>      uint64_t spec_ctrl;
>      uint64_t virt_ssbd;
> +    uint64_t pred_cmd;
> +    uint64_t arch_capabilities;

What's the purpose of those CPUX86State fields, if the migration
sections were removed in v2?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Robert Hoo]

On Wed, 2018-06-27 at 14:03 -0300, Eduardo Habkost wrote:
> On Wed, Jun 27, 2018 at 07:27:20PM +0800, Robert Hoo wrote:
> > IA32_PRED_CMD MSR gives software a way to issue commands that affect the state
> > of indirect branch predictors. Enumerated by CPUID.(EAX=7H,ECX=0):EDX[26].
> > IA32_ARCH_CAPABILITIES MSR enumerates architectural features of RDCL_NO and
> > IBRS_ALL. Enumerated by CPUID.(EAX=07H, ECX=0):EDX[29].
> > 
> > https://software.intel.com/sites/default/files/managed/c5/63/336996-Speculative-Execution-Side-Channel-Mitigations.pdf
> > 
> > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > ---
> >  target/i386/cpu.h |  4 ++++
> >  target/i386/kvm.c | 27 ++++++++++++++++++++++++++-
> >  2 files changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 89c82be..734a73e 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -352,6 +352,8 @@ typedef enum X86Seg {
> >  #define MSR_TSC_ADJUST                  0x0000003b
> >  #define MSR_IA32_SPEC_CTRL              0x48
> >  #define MSR_VIRT_SSBD                   0xc001011f
> > +#define MSR_IA32_PRED_CMD               0x49
> > +#define MSR_IA32_ARCH_CAPABILITIES      0x10a
> >  #define MSR_IA32_TSCDEADLINE            0x6e0
> >  
> >  #define FEATURE_CONTROL_LOCKED                    (1<<0)
> > @@ -1210,6 +1212,8 @@ typedef struct CPUX86State {
> >  
> >      uint64_t spec_ctrl;
> >      uint64_t virt_ssbd;
> > +    uint64_t pred_cmd;
> > +    uint64_t arch_capabilities;
> 
> What's the purpose of those CPUX86State fields, if the migration
> sections were removed in v2?
> 
Thanks Eduardo. Going to clean up in v3. Any more comments, regarding
other patches?

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Eduardo Habkost]

On Thu, Jun 28, 2018 at 05:25:56PM +0800, Robert Hoo wrote:
> On Wed, 2018-06-27 at 14:03 -0300, Eduardo Habkost wrote:
> > On Wed, Jun 27, 2018 at 07:27:20PM +0800, Robert Hoo wrote:
> > > IA32_PRED_CMD MSR gives software a way to issue commands that affect the state
> > > of indirect branch predictors. Enumerated by CPUID.(EAX=7H,ECX=0):EDX[26].
> > > IA32_ARCH_CAPABILITIES MSR enumerates architectural features of RDCL_NO and
> > > IBRS_ALL. Enumerated by CPUID.(EAX=07H, ECX=0):EDX[29].
> > > 
> > > https://software.intel.com/sites/default/files/managed/c5/63/336996-Speculative-Execution-Side-Channel-Mitigations.pdf
> > > 
> > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > ---
> > >  target/i386/cpu.h |  4 ++++
> > >  target/i386/kvm.c | 27 ++++++++++++++++++++++++++-
> > >  2 files changed, 30 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 89c82be..734a73e 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -352,6 +352,8 @@ typedef enum X86Seg {
> > >  #define MSR_TSC_ADJUST                  0x0000003b
> > >  #define MSR_IA32_SPEC_CTRL              0x48
> > >  #define MSR_VIRT_SSBD                   0xc001011f
> > > +#define MSR_IA32_PRED_CMD               0x49
> > > +#define MSR_IA32_ARCH_CAPABILITIES      0x10a
> > >  #define MSR_IA32_TSCDEADLINE            0x6e0
> > >  
> > >  #define FEATURE_CONTROL_LOCKED                    (1<<0)
> > > @@ -1210,6 +1212,8 @@ typedef struct CPUX86State {
> > >  
> > >      uint64_t spec_ctrl;
> > >      uint64_t virt_ssbd;
> > > +    uint64_t pred_cmd;
> > > +    uint64_t arch_capabilities;
> > 
> > What's the purpose of those CPUX86State fields, if the migration
> > sections were removed in v2?
> > 
> Thanks Eduardo. Going to clean up in v3. Any more comments, regarding
> other patches?

The other patches look good, assuming that the bit offsets are
all correct.  Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Paolo Bonzini]

On 28/06/2018 11:25, Robert Hoo wrote:
>>> +    uint64_t pred_cmd;
>>> +    uint64_t arch_capabilities;
>> What's the purpose of those CPUX86State fields, if the migration
>> sections were removed in v2?
>>
> Thanks Eduardo. Going to clean up in v3. Any more comments, regarding
> other patches?

Yes - something like arch_capabilities must stay, as it will be filled
in with "CPUID-like" feature bits that are actually visible to the guest
via the MSR.

However, I suggest adding it to the FeatureWord enum, since everything
that handles FeatureWord applies to this new kind of MSR as well.
Currently FeatureWord is only for CPUID leaves, but it doesn't have to
be like that.

Paolo

Re: [Qemu-devel] [PATCH v2 2/5] i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR

[Eduardo Habkost]

On Wed, Jun 27, 2018 at 07:27:21PM +0800, Robert Hoo wrote:
> Support of IA32_PRED_CMD MSR already be enumerated by same CPUID bit as
> SPEC_CTRL.
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>

Based on kernel commit 1eaafe91, it looks like we must always set
IA32_ARCH_CAPABILITIES.RSBA[bit 2] unless we're really sure the
VM will not be migrated to a vulnerable processor.

Considering this, I'd like to make "+arch-capabilities" set
IA32_ARCH_CAPABILITIES.RSBA by default, unless RSBA is explicitly
disabled by management software.

> ---
>  target/i386/cpu.c | 2 +-
>  target/i386/cpu.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index e6c2f8a..953098c 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1002,7 +1002,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              NULL, NULL, NULL, NULL,
>              NULL, NULL, NULL, NULL,
>              NULL, NULL, "spec-ctrl", NULL,
> -            NULL, NULL, NULL, "ssbd",
> +            NULL, "arch-capabilities", NULL, "ssbd",
>          },
>          .cpuid_eax = 7,
>          .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 734a73e..1ef2040 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -688,6 +688,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) /* AVX512 Neural Network Instructions */
>  #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */
>  #define CPUID_7_0_EDX_SPEC_CTRL     (1U << 26) /* Speculation Control */
> +#define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities of RDCL_NO and IBRS_ALL*/
>  #define CPUID_7_0_EDX_SPEC_CTRL_SSBD  (1U << 31) /* Speculative Store Bypass Disable */
>  
>  #define CPUID_8000_0008_EBX_IBPB    (1U << 12) /* Indirect Branch Prediction Barrier */
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 0/5] Add Icelake CPU model

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1530098844-236851-1-git-send-email-robert.hu@linux.intel.com
Subject: [Qemu-devel] [PATCH v2 0/5] Add Icelake CPU model

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
0a526568ba i386: Add new CPU model Icelake-{Server, Client}
4d6628a55b i386: Add CPUID bit for WBNOINVD
16a4ad0cfe i386: Add CPUID bit for PCONFIG
244c40c403 i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR
3139b1bf60 i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

=== OUTPUT BEGIN ===
Checking PATCH 1/5: i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs...
Checking PATCH 2/5: i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR...
ERROR: line over 90 characters
#34: FILE: target/i386/cpu.h:691:
+#define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities of RDCL_NO and IBRS_ALL*/

total: 1 errors, 0 warnings, 15 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/5: i386: Add CPUID bit for PCONFIG...
Checking PATCH 4/5: i386: Add CPUID bit for WBNOINVD...
ERROR: line over 90 characters
#33: FILE: target/i386/cpu.h:695:
+#define CPUID_8000_0008_EBX_WBNOINVD  (1U << 9)  /* Write back and do not invalidate cache */

total: 1 errors, 0 warnings, 15 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/5: i386: Add new CPU model Icelake-{Server, Client}...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 2/5] i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR

[Robert Hoo]

On Thu, 2018-06-28 at 15:28 -0300, Eduardo Habkost wrote:
> On Wed, Jun 27, 2018 at 07:27:21PM +0800, Robert Hoo wrote:
> > Support of IA32_PRED_CMD MSR already be enumerated by same CPUID bit as
> > SPEC_CTRL.
> > 
> > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> 
> Based on kernel commit 1eaafe91, it looks like we must always set
> IA32_ARCH_CAPABILITIES.RSBA[bit 2] unless we're really sure the
> VM will not be migrated to a vulnerable processor.
> 
> Considering this, I'd like to make "+arch-capabilities" set
> IA32_ARCH_CAPABILITIES.RSBA by default, unless RSBA is explicitly
> disabled by management software.
> 
Agree. But this seems beyond Icelake CPU model scope. How about I think
about this carefully and compose another patch (set) for this?
And you'd like to set  IA32_ARCH_CAPABILITIES.RSBA by default in qemu or
kvm layer?
> > ---
> >  target/i386/cpu.c | 2 +-
> >  target/i386/cpu.h | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index e6c2f8a..953098c 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -1002,7 +1002,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> >              NULL, NULL, NULL, NULL,
> >              NULL, NULL, NULL, NULL,
> >              NULL, NULL, "spec-ctrl", NULL,
> > -            NULL, NULL, NULL, "ssbd",
> > +            NULL, "arch-capabilities", NULL, "ssbd",
> >          },
> >          .cpuid_eax = 7,
> >          .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 734a73e..1ef2040 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -688,6 +688,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> >  #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) /* AVX512 Neural Network Instructions */
> >  #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */
> >  #define CPUID_7_0_EDX_SPEC_CTRL     (1U << 26) /* Speculation Control */
> > +#define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities of RDCL_NO and IBRS_ALL*/
> >  #define CPUID_7_0_EDX_SPEC_CTRL_SSBD  (1U << 31) /* Speculative Store Bypass Disable */
> >  
> >  #define CPUID_8000_0008_EBX_IBPB    (1U << 12) /* Indirect Branch Prediction Barrier */
> > -- 
> > 1.8.3.1
> > 
> > 
> 

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Robert Hoo]

On Thu, 2018-06-28 at 16:20 +0200, Paolo Bonzini wrote:
> On 28/06/2018 11:25, Robert Hoo wrote:
> >>> +    uint64_t pred_cmd;
> >>> +    uint64_t arch_capabilities;
> >> What's the purpose of those CPUX86State fields, if the migration
> >> sections were removed in v2?
> >>
> > Thanks Eduardo. Going to clean up in v3. Any more comments, regarding
> > other patches?
> 
> Yes - something like arch_capabilities must stay, as it will be filled
> in with "CPUID-like" feature bits that are actually visible to the guest
> via the MSR.
> 
> However, I suggest adding it to the FeatureWord enum, since everything
> that handles FeatureWord applies to this new kind of MSR as well.
> Currently FeatureWord is only for CPUID leaves, but it doesn't have to
> be like that.
> 
I think this will be changing struct FeatureWordInfo, which is designed
for cpuid enumerations. You must not want to do that. May I know more
details about your thought?

And, if I implemented ARCH_CAPABILITIES-bits features in FeatureWord,
then no necessity of having it in kvm_msr_entries, right?
> Paolo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Paolo Bonzini]

On 03/07/2018 10:48, Robert Hoo wrote:
>>
>> However, I suggest adding it to the FeatureWord enum, since everything
>> that handles FeatureWord applies to this new kind of MSR as well.
>> Currently FeatureWord is only for CPUID leaves, but it doesn't have to
>> be like that.
>>
> I think this will be changing struct FeatureWordInfo, which is designed
> for cpuid enumerations. You must not want to do that. May I know more
> details about your thought?

The simplest way is to put CPUIDs first and MSRs second in FeatureWord.
Then you can do

     FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
     FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
+    FEATURE_WORDS_NUM_CPUID,
+    FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
+    FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
     FEATURE_WORDS,
};

#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
                                FEATURE_WORDS_FIRST_MSR)

Then the existing loops that use FeatureWordInfo can go up to
FEATURE_WORDS_NUM_CPUID.

Thanks,

Paolo

> And, if I implemented ARCH_CAPABILITIES-bits features in FeatureWord,
> then no necessity of having it in kvm_msr_entries, right?

Re: [Qemu-devel] [PATCH v2 2/5] i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR

[Eduardo Habkost]

On Tue, Jul 03, 2018 at 03:35:13PM +0800, Robert Hoo wrote:
> On Thu, 2018-06-28 at 15:28 -0300, Eduardo Habkost wrote:
> > On Wed, Jun 27, 2018 at 07:27:21PM +0800, Robert Hoo wrote:
> > > Support of IA32_PRED_CMD MSR already be enumerated by same CPUID bit as
> > > SPEC_CTRL.
> > > 
> > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > 
> > Based on kernel commit 1eaafe91, it looks like we must always set
> > IA32_ARCH_CAPABILITIES.RSBA[bit 2] unless we're really sure the
> > VM will not be migrated to a vulnerable processor.
> > 
> > Considering this, I'd like to make "+arch-capabilities" set
> > IA32_ARCH_CAPABILITIES.RSBA by default, unless RSBA is explicitly
> > disabled by management software.
> > 
> Agree. But this seems beyond Icelake CPU model scope. How about I think
> about this carefully and compose another patch (set) for this?

This plan makes sense to me, as I don't want to make this
decision block IceLake from being in QEMU 3.0.

However, enabling CPUID_7_0_EDX_ARCH_CAPABILITIES in IceLake but
setting the MSR to 0 seems pointless.

I think we should add IceLake without
CPUID_7_0_EDX_ARCH_CAPABILITIES first, and later (after deciding
on a reasonable default value for MSR_IA32_ARCH_CAPABILITIES),
enable the CPUID bit on IceLake (hopefully in time for QEMU 3.0).


> And you'd like to set  IA32_ARCH_CAPABILITIES.RSBA by default in qemu or
> kvm layer?

Probably we need to make this decision in QEMU.  If KVM set RSBA
automatically on .get_msr_feature(), QEMU won't be able to
differentiate a host with RSBA set from a host with RSBA unset.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Eduardo Habkost]

On Tue, Jul 03, 2018 at 11:06:00AM +0200, Paolo Bonzini wrote:
> On 03/07/2018 10:48, Robert Hoo wrote:
> >>
> >> However, I suggest adding it to the FeatureWord enum, since everything
> >> that handles FeatureWord applies to this new kind of MSR as well.
> >> Currently FeatureWord is only for CPUID leaves, but it doesn't have to
> >> be like that.
> >>
> > I think this will be changing struct FeatureWordInfo, which is designed
> > for cpuid enumerations. You must not want to do that. May I know more
> > details about your thought?
> 
> The simplest way is to put CPUIDs first and MSRs second in FeatureWord.
> Then you can do
> 
>      FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
>      FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
> +    FEATURE_WORDS_NUM_CPUID,
> +    FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
> +    FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
>      FEATURE_WORDS,
> };
> 
> #define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
>                                 FEATURE_WORDS_FIRST_MSR)
> 
> Then the existing loops that use FeatureWordInfo can go up to
> FEATURE_WORDS_NUM_CPUID.

I assume we want to make some (or most) of the loops go up to
FEATURE_WORDS (e.g. make x86_cpu_get_supported_feature_word()
support MSRs too), otherwise it would be pointless to reuse the
same array.

I would be OK with both approaches, though.  If the first version
doesn't use the `features[]` array and implements this with a
separate `msr_features[]` or `msr_arch_capabilities` field, it
would work for me (especially if this means we can get this
implemented in time for QEMU 3.0).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Robert Hoo]

On Tue, 2018-07-03 at 11:06 +0200, Paolo Bonzini wrote:
> On 03/07/2018 10:48, Robert Hoo wrote:
> >>
> >> However, I suggest adding it to the FeatureWord enum, since everything
> >> that handles FeatureWord applies to this new kind of MSR as well.
> >> Currently FeatureWord is only for CPUID leaves, but it doesn't have to
> >> be like that.
> >>
> > I think this will be changing struct FeatureWordInfo, which is designed
> > for cpuid enumerations. You must not want to do that. May I know more
> > details about your thought?
> 
> The simplest way is to put CPUIDs first and MSRs second in FeatureWord.
> Then you can do
> 
>      FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
>      FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
> +    FEATURE_WORDS_NUM_CPUID,
> +    FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
> +    FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
>      FEATURE_WORDS,
> };
> 
> #define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
>                                 FEATURE_WORDS_FIRST_MSR)
> 
> Then the existing loops that use FeatureWordInfo can go up to
> FEATURE_WORDS_NUM_CPUID.

Emm... Understand your point now. It is a little risky, all references
to FEATURE_WORDS need to be updated carefully.
OK, let me try to think in this way.
Perhaps, I'll need to define a new 'struct FeautureWordMsrInfo' to
describe feature words from MSR, in parallel to current FeatureWordInfo
(or better rename it to FeatureWordCpuidInfo).
> 
> Thanks,
> 
> Paolo
> 
> > And, if I implemented ARCH_CAPABILITIES-bits features in FeatureWord,
> > then no necessity of having it in kvm_msr_entries, right?
> 
And would you help confirm with my this point?

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Paolo Bonzini]

On 03/07/2018 13:07, Robert Hoo wrote:
>>      FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
>>      FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
>> +    FEATURE_WORDS_NUM_CPUID,
>> +    FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
>> +    FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
>>      FEATURE_WORDS,
>> };
>>
>> #define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
>>                                 FEATURE_WORDS_FIRST_MSR)
>>
>> Then the existing loops that use FeatureWordInfo can go up to
>> FEATURE_WORDS_NUM_CPUID.
> Emm... Understand your point now. It is a little risky, all references
> to FEATURE_WORDS need to be updated carefully.
> OK, let me try to think in this way.
> Perhaps, I'll need to define a new 'struct FeautureWordMsrInfo' to
> describe feature words from MSR, in parallel to current FeatureWordInfo
> (or better rename it to FeatureWordCpuidInfo).

Yes, probably.  The plan seems fine.

Paolo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Robert Hoo]

On Tue, 2018-07-03 at 15:38 +0200, Paolo Bonzini wrote:
> On 03/07/2018 13:07, Robert Hoo wrote:
> >>      FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */
> >>      FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */
> >> +    FEATURE_WORDS_NUM_CPUID,
> >> +    FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID,
> >> +    FEAT_MSR_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR,
> >>      FEATURE_WORDS,
> >> };
> >>
> >> #define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - \
> >>                                 FEATURE_WORDS_FIRST_MSR)
> >>
> >> Then the existing loops that use FeatureWordInfo can go up to
> >> FEATURE_WORDS_NUM_CPUID.
> > Emm... Understand your point now. It is a little risky, all references
> > to FEATURE_WORDS need to be updated carefully.
> > OK, let me try to think in this way.
> > Perhaps, I'll need to define a new 'struct FeautureWordMsrInfo' to
> > describe feature words from MSR, in parallel to current FeatureWordInfo
> > (or better rename it to FeatureWordCpuidInfo).
> 
> Yes, probably.  The plan seems fine.
> 

> > And, if I implemented ARCH_CAPABILITIES-bits features in
FeatureWord,
> > then no necessity of having it in kvm_msr_entries, right?
> 
Hi Paolo, would you confirm this? I mean your previous patch "KVM: VMX:
support MSR_IA32_ARCH_CAPABILITIES as a feature MSR" is not necessary
now?

> Paolo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Paolo Bonzini]

On 04/07/2018 08:33, Robert Hoo wrote:
>>> And, if I implemented ARCH_CAPABILITIES-bits features in
> FeatureWord,
>>> then no necessity of having it in kvm_msr_entries, right?
> Hi Paolo, would you confirm this? I mean your previous patch "KVM: VMX:
> support MSR_IA32_ARCH_CAPABILITIES as a feature MSR" is not necessary
> now?
> 

The patch is necessary.  However, ARCH_CAPABILITIES is not needed in
kvm_msr.  It is retrieved with KVM_GET_MSR on the *virtual machine* file
descriptor, while kvm_msr is for the KVM_GET/SET_MSR on the *vCPU* file
descriptor.

You still need to do KVM_SET_MSR on each vCPU when it is initialized;
however, that is done separately from the other MSRs.

Paolo

Re: [Qemu-devel] [PATCH v2 2/5] i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR

[Robert Hoo]

On Tue, 2018-07-03 at 08:00 -0300, Eduardo Habkost wrote:
> On Tue, Jul 03, 2018 at 03:35:13PM +0800, Robert Hoo wrote:
> > On Thu, 2018-06-28 at 15:28 -0300, Eduardo Habkost wrote:
> > > On Wed, Jun 27, 2018 at 07:27:21PM +0800, Robert Hoo wrote:
> > > > Support of IA32_PRED_CMD MSR already be enumerated by same CPUID bit as
> > > > SPEC_CTRL.
> > > > 
> > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > 
> > > Based on kernel commit 1eaafe91, it looks like we must always set
> > > IA32_ARCH_CAPABILITIES.RSBA[bit 2] unless we're really sure the
> > > VM will not be migrated to a vulnerable processor.
> > > 
> > > Considering this, I'd like to make "+arch-capabilities" set
> > > IA32_ARCH_CAPABILITIES.RSBA by default, unless RSBA is explicitly
> > > disabled by management software.
> > > 
> > Agree. But this seems beyond Icelake CPU model scope. How about I think
> > about this carefully and compose another patch (set) for this?
> 
> This plan makes sense to me, as I don't want to make this
> decision block IceLake from being in QEMU 3.0.
> 
> However, enabling CPUID_7_0_EDX_ARCH_CAPABILITIES in IceLake but
> setting the MSR to 0 seems pointless.
> 
> I think we should add IceLake without
> CPUID_7_0_EDX_ARCH_CAPABILITIES first, and later (after deciding
> on a reasonable default value for MSR_IA32_ARCH_CAPABILITIES),
> enable the CPUID bit on IceLake (hopefully in time for QEMU 3.0).
> 
> 
> > And you'd like to set  IA32_ARCH_CAPABILITIES.RSBA by default in qemu or
> > kvm layer?
> 
> Probably we need to make this decision in QEMU.  If KVM set RSBA
> automatically on .get_msr_feature(), QEMU won't be able to
> differentiate a host with RSBA set from a host with RSBA unset.
> 
What's the default value for MSR IA32_ARCH_CAPABILITIES? is it clear
now?

Re: [Qemu-devel] [PATCH v2 2/5] i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR

[Paolo Bonzini]

On 12/07/2018 11:18, Robert Hoo wrote:
>>> And you'd like to set  IA32_ARCH_CAPABILITIES.RSBA by default in qemu or
>>> kvm layer?
>> Probably we need to make this decision in QEMU.  If KVM set RSBA
>> automatically on .get_msr_feature(), QEMU won't be able to
>> differentiate a host with RSBA set from a host with RSBA unset.
>>
> What's the default value for MSR IA32_ARCH_CAPABILITIES? is it clear
> now?
> 

It's the host value.  However, QEMU never sets the CPUID bit so it
doesn't matter if you are using QEMU.

Paolo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[konrad.wilk]

(Apologies if this comes out as HTML, using Thunderbird instead of mutt here)..

> +    uint64_t pred_cmd;
> +    uint64_t arch_capabilities;

Could this be 'arch_cap' ?

>   
>       /* End of state preserved by INIT (dummy marker).  */
>       struct {} end_init_save;
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 2d174f3..3aab182 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -93,6 +93,8 @@ static bool has_msr_hv_reenlightenment;
>   static bool has_msr_xss;
>   static bool has_msr_spec_ctrl;
>   static bool has_msr_virt_ssbd;
> +static bool has_msr_pred_cmd;
> +static bool has_msr_arch_capabilities;

Ditto here?

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Paolo Bonzini]

On 13/07/2018 16:11, konrad.wilk@oracle.com wrote:
> (Apologies if this comes out as HTML, using Thunderbird instead of mutt
> here)..
> 
>> +    uint64_t pred_cmd;
>> +    uint64_t arch_capabilities;
> 
> Could this be 'arch_cap' ?
> 

Why?  Intel chose a verbose name, we should not abbrev. it unnecessarily. :)

Paolo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Konrad Rzeszutek Wilk]

On Fri, Jul 13, 2018 at 04:44:49PM +0200, Paolo Bonzini wrote:
> On 13/07/2018 16:11, konrad.wilk@oracle.com wrote:
> > (Apologies if this comes out as HTML, using Thunderbird instead of mutt
> > here)..
> > 
> >> +    uint64_t pred_cmd;
> >> +    uint64_t arch_capabilities;
> > 
> > Could this be 'arch_cap' ?
> > 
> 
> Why?  Intel chose a verbose name, we should not abbrev. it unnecessarily. :)

Oh you are right. Gosh, so many more characters to type :-(
> 
> Paolo

Re: [Qemu-devel] [PATCH v2 1/5] i386: Add support for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES MSRs

[Robert Hoo]

On Fri, 2018-07-13 at 10:11 -0400, konrad.wilk@oracle.com wrote:
> (Apologies if this comes out as HTML, using Thunderbird instead of mutt here)..
> 
> > +    uint64_t pred_cmd;
> > +    uint64_t arch_capabilities;
> 
> Could this be 'arch_cap' ?
> 
> >   
> >       /* End of state preserved by INIT (dummy marker).  */
> >       struct {} end_init_save;
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index 2d174f3..3aab182 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -93,6 +93,8 @@ static bool has_msr_hv_reenlightenment;
> >   static bool has_msr_xss;
> >   static bool has_msr_spec_ctrl;
> >   static bool has_msr_virt_ssbd;
> > +static bool has_msr_pred_cmd;
> > +static bool has_msr_arch_capabilities;
> 
> Ditto here?
> 

Per Paolo and Eduardo's comments, the 2 fields/variables are gone in new
versions.