| Message ID | 1535888767-154680-4-git-send-email-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: QEMU side support on MSR based features | expand |
On Sun, Sep 02, 2018 at 07:46:07PM +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> > --- > target/i386/cpu.c | 27 ++++++++++++++++++++++++++- > target/i386/cpu.h | 12 ++++++++++++ > 2 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 0160e97..8ec9613 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1129,6 +1129,24 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > .reg = R_EDX, }, > .tcg_features = ~0U, > }, > + /*Below are MSR exposed features*/ > + [FEATURE_WORDS_ARCH_CAPABILITIES] = { > + .type = MSR_FEATURE_WORD, > + .feat_names = { > + "rdctl-no", "ibrs-all", "rsba", NULL, > + "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 } > + }, > + }, One critical piece of the code seems to be missing: where exactly is the MSR value being set on the VCPU before it runs? > }; > > typedef struct X86RegisterInfo32 { > @@ -3680,7 +3698,14 @@ 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, > + /* 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; > } > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index b572a8e..9662730 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -502,9 +502,14 @@ 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 */ > + FEATURE_WORDS_NUM_CPUID, > + FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID, > + FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR, > FEATURE_WORDS, > } FeatureWord; > > +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - FEATURE_WORDS_FIRST_MSR) I don't see FEATURE_WORDS_NUM_CPUID, FEATURE_WORDS_FIRST_MS and FEATURE_WORDS_NUM_MSRS being used anywhere. Why are they being introduced? > + > typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > > /* cpuid_features bits */ > @@ -730,6 +735,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 > -- > 1.8.3.1 > >
On Thu, 2018-09-20 at 00:13 -0300, Eduardo Habkost wrote: > On Sun, Sep 02, 2018 at 07:46:07PM +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> > > --- > > target/i386/cpu.c | 27 ++++++++++++++++++++++++++- > > target/i386/cpu.h | 12 ++++++++++++ > > 2 files changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index 0160e97..8ec9613 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -1129,6 +1129,24 @@ static FeatureWordInfo > > feature_word_info[FEATURE_WORDS] = { > > .reg = R_EDX, }, > > .tcg_features = ~0U, > > }, > > + /*Below are MSR exposed features*/ > > + [FEATURE_WORDS_ARCH_CAPABILITIES] = { > > + .type = MSR_FEATURE_WORD, > > + .feat_names = { > > + "rdctl-no", "ibrs-all", "rsba", NULL, > > + "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 } > > + }, > > + }, > > One critical piece of the code seems to be missing: where exactly > is the MSR value being set on the VCPU before it runs? > I don't quite understand. Isn't such feature MSR read-only, like CPUID, simply to enumerate features? > > > }; > > > > typedef struct X86RegisterInfo32 { > > @@ -3680,7 +3698,14 @@ 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, > > + /* 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; > > } > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index b572a8e..9662730 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -502,9 +502,14 @@ 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 */ > > + FEATURE_WORDS_NUM_CPUID, > > + FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID, > > + FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR, > > FEATURE_WORDS, > > } FeatureWord; > > > > +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - > > FEATURE_WORDS_FIRST_MSR) > > I don't see FEATURE_WORDS_NUM_CPUID, FEATURE_WORDS_FIRST_MS and > FEATURE_WORDS_NUM_MSRS being used anywhere. Why are they being > introduced? > Get rid of them in v5. > > > + > > typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > > > > /* cpuid_features bits */ > > @@ -730,6 +735,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 > > -- > > 1.8.3.1 > > > > > >
On Thu, Sep 20, 2018 at 05:55:48PM +0800, Robert Hoo wrote: > On Thu, 2018-09-20 at 00:13 -0300, Eduardo Habkost wrote: > > On Sun, Sep 02, 2018 at 07:46:07PM +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> > > > --- > > > target/i386/cpu.c | 27 ++++++++++++++++++++++++++- > > > target/i386/cpu.h | 12 ++++++++++++ > > > 2 files changed, 38 insertions(+), 1 deletion(-) > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > index 0160e97..8ec9613 100644 > > > --- a/target/i386/cpu.c > > > +++ b/target/i386/cpu.c > > > @@ -1129,6 +1129,24 @@ static FeatureWordInfo > > > feature_word_info[FEATURE_WORDS] = { > > > .reg = R_EDX, }, > > > .tcg_features = ~0U, > > > }, > > > + /*Below are MSR exposed features*/ > > > + [FEATURE_WORDS_ARCH_CAPABILITIES] = { > > > + .type = MSR_FEATURE_WORD, > > > + .feat_names = { > > > + "rdctl-no", "ibrs-all", "rsba", NULL, > > > + "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 } > > > + }, > > > + }, > > > > One critical piece of the code seems to be missing: where exactly > > is the MSR value being set on the VCPU before it runs? > > > I don't quite understand. Isn't such feature MSR read-only, like CPUID, > simply to enumerate features? The MSR is read-only for the guest, yes. But QEMU needs to call KVM_SET_MSRS somewhere, to tell KVM what's the MSR value the guest should see. I don't see any code doing that. > [...]
On Thu, 2018-09-20 at 14:18 -0300, Eduardo Habkost wrote: > On Thu, Sep 20, 2018 at 05:55:48PM +0800, Robert Hoo wrote: > > On Thu, 2018-09-20 at 00:13 -0300, Eduardo Habkost wrote: > > > On Sun, Sep 02, 2018 at 07:46:07PM +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> > > > > --- > > > > target/i386/cpu.c | 27 ++++++++++++++++++++++++++- > > > > target/i386/cpu.h | 12 ++++++++++++ > > > > 2 files changed, 38 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > > index 0160e97..8ec9613 100644 > > > > --- a/target/i386/cpu.c > > > > +++ b/target/i386/cpu.c > > > > @@ -1129,6 +1129,24 @@ static FeatureWordInfo > > > > feature_word_info[FEATURE_WORDS] = { > > > > .reg = R_EDX, }, > > > > .tcg_features = ~0U, > > > > }, > > > > + /*Below are MSR exposed features*/ > > > > + [FEATURE_WORDS_ARCH_CAPABILITIES] = { > > > > + .type = MSR_FEATURE_WORD, > > > > + .feat_names = { > > > > + "rdctl-no", "ibrs-all", "rsba", NULL, > > > > + "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 } > > > > + }, > > > > + }, > > > > > > One critical piece of the code seems to be missing: where exactly > > > is the MSR value being set on the VCPU before it runs? > > > > > > > I don't quite understand. Isn't such feature MSR read-only, like > > CPUID, > > simply to enumerate features? > > The MSR is read-only for the guest, yes. But QEMU needs to call > KVM_SET_MSRS somewhere, to tell KVM what's the MSR value the > guest should see. I don't see any code doing that. > I think: these feature MSRs are separated from other MSRs. Those MSRs information are stored in X86CPU->kvm_msr_buf, they are set/get through vcpu ioctl KVM_SET_MSRS and KVM_GET_MSRS. While feature MSRs are actually system ioctl, their return value are determined by KVM/Host, i.e. not necessary set by guest, nor to be vcpu level. > > [...] > >
On Fri, Sep 21, 2018 at 01:19:23PM +0800, Robert Hoo wrote: > On Thu, 2018-09-20 at 14:18 -0300, Eduardo Habkost wrote: > > On Thu, Sep 20, 2018 at 05:55:48PM +0800, Robert Hoo wrote: > > > On Thu, 2018-09-20 at 00:13 -0300, Eduardo Habkost wrote: > > > > On Sun, Sep 02, 2018 at 07:46:07PM +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> > > > > > --- > > > > > target/i386/cpu.c | 27 ++++++++++++++++++++++++++- > > > > > target/i386/cpu.h | 12 ++++++++++++ > > > > > 2 files changed, 38 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > > > index 0160e97..8ec9613 100644 > > > > > --- a/target/i386/cpu.c > > > > > +++ b/target/i386/cpu.c > > > > > @@ -1129,6 +1129,24 @@ static FeatureWordInfo > > > > > feature_word_info[FEATURE_WORDS] = { > > > > > .reg = R_EDX, }, > > > > > .tcg_features = ~0U, > > > > > }, > > > > > + /*Below are MSR exposed features*/ > > > > > + [FEATURE_WORDS_ARCH_CAPABILITIES] = { > > > > > + .type = MSR_FEATURE_WORD, > > > > > + .feat_names = { > > > > > + "rdctl-no", "ibrs-all", "rsba", NULL, > > > > > + "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 } > > > > > + }, > > > > > + }, > > > > > > > > One critical piece of the code seems to be missing: where exactly > > > > is the MSR value being set on the VCPU before it runs? > > > > > > > > > > I don't quite understand. Isn't such feature MSR read-only, like > > > CPUID, > > > simply to enumerate features? > > > > The MSR is read-only for the guest, yes. But QEMU needs to call > > KVM_SET_MSRS somewhere, to tell KVM what's the MSR value the > > guest should see. I don't see any code doing that. > > > I think: these feature MSRs are separated from other MSRs. Those MSRs > information are stored in X86CPU->kvm_msr_buf, they are set/get through > vcpu ioctl KVM_SET_MSRS and KVM_GET_MSRS. While feature MSRs are > actually system ioctl, their return value are determined by KVM/Host, > i.e. not necessary set by guest, nor to be vcpu level. I'm confused. What exactly is the goal of this series, exactly, if it is not making QEMU define the MSR values seen by the guest?
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 0160e97..8ec9613 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -1129,6 +1129,24 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .reg = R_EDX, }, .tcg_features = ~0U, }, + /*Below are MSR exposed features*/ + [FEATURE_WORDS_ARCH_CAPABILITIES] = { + .type = MSR_FEATURE_WORD, + .feat_names = { + "rdctl-no", "ibrs-all", "rsba", NULL, + "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 { @@ -3680,7 +3698,14 @@ 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, + /* 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; } diff --git a/target/i386/cpu.h b/target/i386/cpu.h index b572a8e..9662730 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -502,9 +502,14 @@ 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 */ + FEATURE_WORDS_NUM_CPUID, + FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID, + FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR, FEATURE_WORDS, } FeatureWord; +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - FEATURE_WORDS_FIRST_MSR) + typedef uint32_t FeatureWordArray[FEATURE_WORDS]; /* cpuid_features bits */ @@ -730,6 +735,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
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 | 27 ++++++++++++++++++++++++++- target/i386/cpu.h | 12 ++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-)