| Message ID | 1539578845-37944-4-git-send-email-robert.hu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: QEMU side support on MSR based features | expand |
On Mon, Oct 15, 2018 at 12:47:25PM +0800, Robert Hoo wrote: > Note RSBA is specially treated -- no matter host support it or not, qemu > pretends it is supported. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> I am now wondering what else we need to be able to remove CPUID_7_0_EDX_ARCH_CAPABILITIES from feature_word_info[FEAT_7_0_EDX].unmigratable_flags. This series is necessary for that, be I think we still can't let the VM be migrated if arch-capabilities is enabled and we're running on a host that doesn't have MSR_IA32_ARCH_CAPABILITIES on kvm_feature_msrs. Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target/i386/cpu.c | 31 ++++++++++++++++++++++++++++++- > target/i386/cpu.h | 8 ++++++++ > target/i386/kvm.c | 11 +++++++++++ > 3 files changed, 49 insertions(+), 1 deletion(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index d191b9c..51c8fd8 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1141,6 +1141,27 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > }, > .tcg_features = ~0U, > }, > + /*Below are MSR exposed features*/ > + [FEAT_ARCH_CAPABILITIES] = { > + .type = MSR_FEATURE_WORD, > + .feat_names = { > + "rdctl-no", "ibrs-all", "rsba", "skip-l1dfl-vmentry", > + "ssb-no", NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + NULL, NULL, NULL, NULL, > + }, > + .msr = { > + .index = MSR_IA32_ARCH_CAPABILITIES, > + .cpuid_dep = { > + FEAT_7_0_EDX, > + CPUID_7_0_EDX_ARCH_CAPABILITIES > + } > + }, > + }, > }; > > typedef struct X86RegisterInfo32 { > @@ -3696,7 +3717,15 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, > wi->cpuid.reg); > break; > case MSR_FEATURE_WORD: > - r = kvm_arch_get_supported_msr_feature(kvm_state, wi->msr.index); > + /* Special case: > + * No matter host status, IA32_ARCH_CAPABILITIES.RSBA [bit 2] > + * is always supported in guest. > + */ > + if (wi->msr.index == MSR_IA32_ARCH_CAPABILITIES) { > + r = MSR_ARCH_CAP_RSBA; > + } > + r |= kvm_arch_get_supported_msr_feature(kvm_state, > + wi->msr.index); > break; > } > } else if (hvf_enabled()) { > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 730c06f..52a52ec 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -502,6 +502,7 @@ typedef enum FeatureWord { > FEAT_6_EAX, /* CPUID[6].EAX */ > FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */ > FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */ > + FEAT_ARCH_CAPABILITIES, > FEATURE_WORDS, > } FeatureWord; > > @@ -730,6 +731,13 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > #define CPUID_TOPOLOGY_LEVEL_SMT (1U << 8) > #define CPUID_TOPOLOGY_LEVEL_CORE (2U << 8) > > +/* MSR Feature Bits */ > +#define MSR_ARCH_CAP_RDCL_NO (1U << 0) > +#define MSR_ARCH_CAP_IBRS_ALL (1U << 1) > +#define MSR_ARCH_CAP_RSBA (1U << 2) > +#define MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY (1U << 3) > +#define MSR_ARCH_CAP_SSB_NO (1U << 4) > + > #ifndef HYPERV_SPINLOCK_NEVER_RETRY > #define HYPERV_SPINLOCK_NEVER_RETRY 0xFFFFFFFF > #endif > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index db79dad..2f7b40d 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -1928,6 +1928,17 @@ static int kvm_put_msrs(X86CPU *cpu, int level) > } > #endif > > + /* If host supports feature MSR, write down. */ > + if (kvm_feature_msrs) { > + int i; > + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) > + if (kvm_feature_msrs->indices[i] == MSR_IA32_ARCH_CAPABILITIES) { > + kvm_msr_entry_add(cpu, MSR_IA32_ARCH_CAPABILITIES, > + env->features[FEAT_ARCH_CAPABILITIES]); > + break; > + } > + } > + > /* > * The following MSRs have side effects on the guest or are too heavy > * for normal writeback. Limit them to reset or full state updates. > -- > 1.8.3.1 > >
On Wed, 2018-10-24 at 07:06 -0300, Eduardo Habkost wrote: > On Mon, Oct 15, 2018 at 12:47:25PM +0800, Robert Hoo wrote: > > Note RSBA is specially treated -- no matter host support it or not, > > qemu > > pretends it is supported. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > I am now wondering what else we need to be able to remove > CPUID_7_0_EDX_ARCH_CAPABILITIES from > feature_word_info[FEAT_7_0_EDX].unmigratable_flags. Let me know once some thought comes out to you. > > This series is necessary for that, be I think we still can't let > the VM be migrated if arch-capabilities is enabled and we're > running on a host that doesn't have MSR_IA32_ARCH_CAPABILITIES on > kvm_feature_msrs. Agree. So I still keep CPUID_7_0_EDX_ARCH_CAPABILITIES in feature_word_info[FEAT_7_0_EDX].unmigratable_flags for now. > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > target/i386/cpu.c | 31 ++++++++++++++++++++++++++++++- > > target/i386/cpu.h | 8 ++++++++ > > target/i386/kvm.c | 11 +++++++++++ > > 3 files changed, 49 insertions(+), 1 deletion(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index d191b9c..51c8fd8 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -1141,6 +1141,27 @@ static FeatureWordInfo > > feature_word_info[FEATURE_WORDS] = { > > }, > > .tcg_features = ~0U, > > }, > > + /*Below are MSR exposed features*/ > > + [FEAT_ARCH_CAPABILITIES] = { > > + .type = MSR_FEATURE_WORD, > > + .feat_names = { > > + "rdctl-no", "ibrs-all", "rsba", "skip-l1dfl-vmentry", > > + "ssb-no", NULL, NULL, NULL, > > + NULL, NULL, NULL, NULL, > > + NULL, NULL, NULL, NULL, > > + NULL, NULL, NULL, NULL, > > + NULL, NULL, NULL, NULL, > > + NULL, NULL, NULL, NULL, > > + NULL, NULL, NULL, NULL, > > + }, > > + .msr = { > > + .index = MSR_IA32_ARCH_CAPABILITIES, > > + .cpuid_dep = { > > + FEAT_7_0_EDX, > > + CPUID_7_0_EDX_ARCH_CAPABILITIES > > + } > > + }, > > + }, > > }; > > > > typedef struct X86RegisterInfo32 { > > @@ -3696,7 +3717,15 @@ static uint32_t > > x86_cpu_get_supported_feature_word(FeatureWord w, > > wi- > > >cpuid.reg); > > break; > > case MSR_FEATURE_WORD: > > - r = kvm_arch_get_supported_msr_feature(kvm_state, wi- > > >msr.index); > > + /* Special case: > > + * No matter host status, IA32_ARCH_CAPABILITIES.RSBA > > [bit 2] > > + * is always supported in guest. > > + */ > > + if (wi->msr.index == MSR_IA32_ARCH_CAPABILITIES) { > > + r = MSR_ARCH_CAP_RSBA; > > + } > > + r |= kvm_arch_get_supported_msr_feature(kvm_state, > > + wi->msr.index); > > break; > > } > > } else if (hvf_enabled()) { > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index 730c06f..52a52ec 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -502,6 +502,7 @@ typedef enum FeatureWord { > > FEAT_6_EAX, /* CPUID[6].EAX */ > > FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */ > > FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */ > > + FEAT_ARCH_CAPABILITIES, > > FEATURE_WORDS, > > } FeatureWord; > > > > @@ -730,6 +731,13 @@ typedef uint32_t > > FeatureWordArray[FEATURE_WORDS]; > > #define CPUID_TOPOLOGY_LEVEL_SMT (1U << 8) > > #define CPUID_TOPOLOGY_LEVEL_CORE (2U << 8) > > > > +/* MSR Feature Bits */ > > +#define MSR_ARCH_CAP_RDCL_NO (1U << 0) > > +#define MSR_ARCH_CAP_IBRS_ALL (1U << 1) > > +#define MSR_ARCH_CAP_RSBA (1U << 2) > > +#define MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY (1U << 3) > > +#define MSR_ARCH_CAP_SSB_NO (1U << 4) > > + > > #ifndef HYPERV_SPINLOCK_NEVER_RETRY > > #define HYPERV_SPINLOCK_NEVER_RETRY 0xFFFFFFFF > > #endif > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > index db79dad..2f7b40d 100644 > > --- a/target/i386/kvm.c > > +++ b/target/i386/kvm.c > > @@ -1928,6 +1928,17 @@ static int kvm_put_msrs(X86CPU *cpu, int > > level) > > } > > #endif > > > > + /* If host supports feature MSR, write down. */ > > + if (kvm_feature_msrs) { > > + int i; > > + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) > > + if (kvm_feature_msrs->indices[i] == > > MSR_IA32_ARCH_CAPABILITIES) { > > + kvm_msr_entry_add(cpu, MSR_IA32_ARCH_CAPABILITIES, > > + env- > > >features[FEAT_ARCH_CAPABILITIES]); > > + break; > > + } > > + } > > + > > /* > > * The following MSRs have side effects on the guest or are > > too heavy > > * for normal writeback. Limit them to reset or full state > > updates. > > -- > > 1.8.3.1 > > > > > >
On Wed, 2018-10-24 at 07:06 -0300, Eduardo Habkost wrote: > On Mon, Oct 15, 2018 at 12:47:25PM +0800, Robert Hoo wrote: > > Note RSBA is specially treated -- no matter host support it or not, > > qemu > > pretends it is supported. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > I am now wondering what else we need to be able to remove > CPUID_7_0_EDX_ARCH_CAPABILITIES from > feature_word_info[FEAT_7_0_EDX].unmigratable_flags. > > This series is necessary for that, be I think we still can't let > the VM be migrated if arch-capabilities is enabled and we're > running on a host that doesn't have MSR_IA32_ARCH_CAPABILITIES on > kvm_feature_msrs. > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > target/i386/cpu.c | 31 ++++++++++++++++++++++++++++++- > > target/i386/cpu.h | 8 ++++++++ > > target/i386/kvm.c | 11 +++++++++++ > > 3 files changed, 49 insertions(+), 1 deletion(-) > > [...] > > > > typedef struct X86RegisterInfo32 { > > @@ -3696,7 +3717,15 @@ static uint32_t > > x86_cpu_get_supported_feature_word(FeatureWord w, > > wi- > > >cpuid.reg); > > break; > > case MSR_FEATURE_WORD: > > - r = kvm_arch_get_supported_msr_feature(kvm_state, wi- > > >msr.index); > > + /* Special case: > > + * No matter host status, IA32_ARCH_CAPABILITIES.RSBA > > [bit 2] > > + * is always supported in guest. > > + */ > > + if (wi->msr.index == MSR_IA32_ARCH_CAPABILITIES) { > > + r = MSR_ARCH_CAP_RSBA; > > + } > > + r |= kvm_arch_get_supported_msr_feature(kvm_state, > > + wi->msr.index); > > break; After I add the filtering out MSR feature, whose CPUID dependency fails , in x86_cpu_filter_features(), 1 issue comes out here: If running on an old platform that doesn't have ARCH_CAPABILITIES MSR, but we still pretends it here, then qemu will always print out "warning: host doesn't support requested feature: MSR(10AH).rsba [bit 2]", with -cpu 'host', which does not look comfortable. How about remove this hunk for now? leave it to when we fully decide how to handle ARCH_CAPABILITIES live-migration safely. > > } > > } else if (hvf_enabled()) { [...]
On Fri, Oct 26, 2018 at 11:01:25AM +0800, Robert Hoo wrote: > On Wed, 2018-10-24 at 07:06 -0300, Eduardo Habkost wrote: > > On Mon, Oct 15, 2018 at 12:47:25PM +0800, Robert Hoo wrote: > > > Note RSBA is specially treated -- no matter host support it or not, > > > qemu > > > pretends it is supported. > > > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > > > I am now wondering what else we need to be able to remove > > CPUID_7_0_EDX_ARCH_CAPABILITIES from > > feature_word_info[FEAT_7_0_EDX].unmigratable_flags. > > > > This series is necessary for that, be I think we still can't let > > the VM be migrated if arch-capabilities is enabled and we're > > running on a host that doesn't have MSR_IA32_ARCH_CAPABILITIES on > > kvm_feature_msrs. > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > > --- > > > target/i386/cpu.c | 31 ++++++++++++++++++++++++++++++- > > > target/i386/cpu.h | 8 ++++++++ > > > target/i386/kvm.c | 11 +++++++++++ > > > 3 files changed, 49 insertions(+), 1 deletion(-) > > > > [...] > > > > > > typedef struct X86RegisterInfo32 { > > > @@ -3696,7 +3717,15 @@ static uint32_t > > > x86_cpu_get_supported_feature_word(FeatureWord w, > > > wi- > > > >cpuid.reg); > > > break; > > > case MSR_FEATURE_WORD: > > > - r = kvm_arch_get_supported_msr_feature(kvm_state, wi- > > > >msr.index); > > > + /* Special case: > > > + * No matter host status, IA32_ARCH_CAPABILITIES.RSBA > > > [bit 2] > > > + * is always supported in guest. > > > + */ > > > + if (wi->msr.index == MSR_IA32_ARCH_CAPABILITIES) { > > > + r = MSR_ARCH_CAP_RSBA; > > > + } > > > + r |= kvm_arch_get_supported_msr_feature(kvm_state, > > > + wi->msr.index); > > > break; > After I add the filtering out MSR feature, whose CPUID dependency fails > , in x86_cpu_filter_features(), 1 issue comes out here: > > If running on an old platform that doesn't have ARCH_CAPABILITIES MSR, > but we still pretends it here, then qemu will always print out > "warning: host doesn't support requested feature: MSR(10AH).rsba [bit > 2]", with -cpu 'host', which does not look comfortable. > How about remove this hunk for now? leave it to when we fully decide > how to handle ARCH_CAPABILITIES live-migration safely. I will remove that hunk in x86-next, thanks for noting!
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index d191b9c..51c8fd8 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -1141,6 +1141,27 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { }, .tcg_features = ~0U, }, + /*Below are MSR exposed features*/ + [FEAT_ARCH_CAPABILITIES] = { + .type = MSR_FEATURE_WORD, + .feat_names = { + "rdctl-no", "ibrs-all", "rsba", "skip-l1dfl-vmentry", + "ssb-no", NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, + .msr = { + .index = MSR_IA32_ARCH_CAPABILITIES, + .cpuid_dep = { + FEAT_7_0_EDX, + CPUID_7_0_EDX_ARCH_CAPABILITIES + } + }, + }, }; typedef struct X86RegisterInfo32 { @@ -3696,7 +3717,15 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, wi->cpuid.reg); break; case MSR_FEATURE_WORD: - r = kvm_arch_get_supported_msr_feature(kvm_state, wi->msr.index); + /* Special case: + * No matter host status, IA32_ARCH_CAPABILITIES.RSBA [bit 2] + * is always supported in guest. + */ + if (wi->msr.index == MSR_IA32_ARCH_CAPABILITIES) { + r = MSR_ARCH_CAP_RSBA; + } + r |= kvm_arch_get_supported_msr_feature(kvm_state, + wi->msr.index); break; } } else if (hvf_enabled()) { diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 730c06f..52a52ec 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -502,6 +502,7 @@ typedef enum FeatureWord { FEAT_6_EAX, /* CPUID[6].EAX */ FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */ FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */ + FEAT_ARCH_CAPABILITIES, FEATURE_WORDS, } FeatureWord; @@ -730,6 +731,13 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS]; #define CPUID_TOPOLOGY_LEVEL_SMT (1U << 8) #define CPUID_TOPOLOGY_LEVEL_CORE (2U << 8) +/* MSR Feature Bits */ +#define MSR_ARCH_CAP_RDCL_NO (1U << 0) +#define MSR_ARCH_CAP_IBRS_ALL (1U << 1) +#define MSR_ARCH_CAP_RSBA (1U << 2) +#define MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY (1U << 3) +#define MSR_ARCH_CAP_SSB_NO (1U << 4) + #ifndef HYPERV_SPINLOCK_NEVER_RETRY #define HYPERV_SPINLOCK_NEVER_RETRY 0xFFFFFFFF #endif diff --git a/target/i386/kvm.c b/target/i386/kvm.c index db79dad..2f7b40d 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -1928,6 +1928,17 @@ static int kvm_put_msrs(X86CPU *cpu, int level) } #endif + /* If host supports feature MSR, write down. */ + if (kvm_feature_msrs) { + int i; + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) + if (kvm_feature_msrs->indices[i] == MSR_IA32_ARCH_CAPABILITIES) { + kvm_msr_entry_add(cpu, MSR_IA32_ARCH_CAPABILITIES, + env->features[FEAT_ARCH_CAPABILITIES]); + break; + } + } + /* * The following MSRs have side effects on the guest or are too heavy * for normal writeback. Limit them to reset or full state updates.
Note RSBA is specially treated -- no matter host support it or not, qemu pretends it is supported. Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- target/i386/cpu.c | 31 ++++++++++++++++++++++++++++++- target/i386/cpu.h | 8 ++++++++ target/i386/kvm.c | 11 +++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-)