| Message ID | 1530098844-236851-2-git-send-email-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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?
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!
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
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
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?
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).
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?
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
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
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
(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?
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
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
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.
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;
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(-)