| Message ID | 20190927021927.23057-8-weijiang.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce support for Guest CET feature | expand |
On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > There're two different places storing Guest CET states, the states > managed with XSAVES/XRSTORS, as restored/saved > in previous patch, can be read/write directly from/to the MSRs. > For those stored in VMCS fields, they're access via vmcs_read/ > vmcs_write. > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 83 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 83 insertions(+) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 44913e4ab558..5265db7cd2af 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1671,6 +1671,49 @@ static int vmx_get_msr_feature(struct kvm_msr_entry *msr) > return 0; > } > > +static int check_cet_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) I'd suggest changing return type to bool, since you are essentially returning true or false. > +{ > + u64 kvm_xss = kvm_supported_xss(); > + > + switch (msr_info->index) { > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP: > + if (!(kvm_xss | XFEATURE_MASK_CET_KERNEL)) '|' should be '&' > + return 1; > + if (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) > + return 1; > + break; > + case MSR_IA32_PL3_SSP: > + if (!(kvm_xss | XFEATURE_MASK_CET_USER)) '|' should be '&' > + return 1; > + if (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) > + return 1; > + break; > + case MSR_IA32_U_CET: > + if (!(kvm_xss | XFEATURE_MASK_CET_USER)) '|' should be '&' > + return 1; > + if (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) && > + !guest_cpuid_has(vcpu, X86_FEATURE_IBT)) > + return 1; > + break; > + case MSR_IA32_S_CET: > + if (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) && > + !guest_cpuid_has(vcpu, X86_FEATURE_IBT)) > + return 1; > + break; > + case MSR_IA32_INT_SSP_TAB: > + if (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) > + return 1; > + break; > + default: > + return 1; > + } > + return 0; > +} > /* > * Reads an msr value (of 'msr_index') into 'pdata'. > * Returns 0 on success, non-0 otherwise. > @@ -1788,6 +1831,26 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > else > msr_info->data = vmx->pt_desc.guest.addr_a[index / 2]; > break; > + case MSR_IA32_S_CET: > + if (check_cet_msr(vcpu, msr_info)) > + return 1; > + msr_info->data = vmcs_readl(GUEST_S_CET); Have we ensured that this VMCS field exists? > + break; > + case MSR_IA32_INT_SSP_TAB: > + if (check_cet_msr(vcpu, msr_info)) > + return 1; > + msr_info->data = vmcs_readl(GUEST_INTR_SSP_TABLE); Have we ensured that this VMCS field exists? > + break; > + case MSR_IA32_U_CET: Can this be lumped together with MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP, below? > + if (check_cet_msr(vcpu, msr_info)) > + return 1; > + rdmsrl(MSR_IA32_U_CET, msr_info->data); > + break; > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP: > + if (check_cet_msr(vcpu, msr_info)) > + return 1; > + rdmsrl(msr_info->index, msr_info->data); > + break; > case MSR_TSC_AUX: > if (!msr_info->host_initiated && > !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP)) > @@ -2039,6 +2102,26 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > else > vmx->pt_desc.guest.addr_a[index / 2] = data; > break; > + case MSR_IA32_S_CET: > + if (check_cet_msr(vcpu, msr_info)) > + return 1; Bits 9:6 must be zero. > + vmcs_writel(GUEST_S_CET, data); Have we ensured that this VMCS field exists? > + break; > + case MSR_IA32_INT_SSP_TAB: > + if (check_cet_msr(vcpu, msr_info)) > + return 1; Must be canonical. vCPU must support longmode. > + vmcs_writel(GUEST_INTR_SSP_TABLE, data); Have we ensured that this VMCS field exists? > + break; > + case MSR_IA32_U_CET: > + if (check_cet_msr(vcpu, msr_info)) > + return 1; Bits 9:6 must be zero. > + wrmsrl(MSR_IA32_U_CET, data); > + break; > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP: > + if (check_cet_msr(vcpu, msr_info)) > + return 1; 'Data' must be canonical and 4-byte aligned. High dword must be zero on vCPUs that don't support longmode. > + wrmsrl(msr_info->index, data); > + break; > case MSR_TSC_AUX: > if (!msr_info->host_initiated && > !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP)) > -- > 2.17.2 >
On Wed, Oct 02, 2019 at 01:57:50PM -0700, Jim Mattson wrote: > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > There're two different places storing Guest CET states, the states > > managed with XSAVES/XRSTORS, as restored/saved > > in previous patch, can be read/write directly from/to the MSRs. > > For those stored in VMCS fields, they're access via vmcs_read/ > > vmcs_write. > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 83 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 83 insertions(+) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 44913e4ab558..5265db7cd2af 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -1671,6 +1671,49 @@ static int vmx_get_msr_feature(struct kvm_msr_entry *msr) > > return 0; > > } > > > > +static int check_cet_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > I'd suggest changing return type to bool, since you are essentially > returning true or false. > Sure, will change it. > > +{ > > + u64 kvm_xss = kvm_supported_xss(); > > + > > + switch (msr_info->index) { > > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP: > > + if (!(kvm_xss | XFEATURE_MASK_CET_KERNEL)) > '|' should be '&' Oops, thanks for the capture! > > + return 1; > > + if (!msr_info->host_initiated && > > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) > > + return 1; > > + break; > > + case MSR_IA32_PL3_SSP: > > + if (!(kvm_xss | XFEATURE_MASK_CET_USER)) > '|' should be '&' > > + return 1; > > + if (!msr_info->host_initiated && > > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) > > + return 1; > > + break; > > + case MSR_IA32_U_CET: > > + if (!(kvm_xss | XFEATURE_MASK_CET_USER)) > '|' should be '&' > > + return 1; > > + if (!msr_info->host_initiated && > > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) && > > + !guest_cpuid_has(vcpu, X86_FEATURE_IBT)) > > + return 1; > > + break; > > + case MSR_IA32_S_CET: > > + if (!msr_info->host_initiated && > > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) && > > + !guest_cpuid_has(vcpu, X86_FEATURE_IBT)) > > + return 1; > > + break; > > + case MSR_IA32_INT_SSP_TAB: > > + if (!msr_info->host_initiated && > > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) > > + return 1; > > + break; > > + default: > > + return 1; > > + } > > + return 0; > > +} > > /* > > * Reads an msr value (of 'msr_index') into 'pdata'. > > * Returns 0 on success, non-0 otherwise. > > @@ -1788,6 +1831,26 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > else > > msr_info->data = vmx->pt_desc.guest.addr_a[index / 2]; > > break; > > + case MSR_IA32_S_CET: > > + if (check_cet_msr(vcpu, msr_info)) > > + return 1; > > + msr_info->data = vmcs_readl(GUEST_S_CET); > Have we ensured that this VMCS field exists? > > + break; > > + case MSR_IA32_INT_SSP_TAB: > > + if (check_cet_msr(vcpu, msr_info)) > > + return 1; > > + msr_info->data = vmcs_readl(GUEST_INTR_SSP_TABLE); > Have we ensured that this VMCS field exists? If host kernel sets correct info, we can make sure here. > > + break; > > + case MSR_IA32_U_CET: > Can this be lumped together with MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP, below? Unfortunately, this MSR is not adjacent to below MSRs. > > + if (check_cet_msr(vcpu, msr_info)) > > + return 1; > > + rdmsrl(MSR_IA32_U_CET, msr_info->data); > > + break; > > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP: > > + if (check_cet_msr(vcpu, msr_info)) > > + return 1; > > + rdmsrl(msr_info->index, msr_info->data); > > + break; > > case MSR_TSC_AUX: > > if (!msr_info->host_initiated && > > !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP)) > > @@ -2039,6 +2102,26 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > else > > vmx->pt_desc.guest.addr_a[index / 2] = data; > > break; > > + case MSR_IA32_S_CET: > > + if (check_cet_msr(vcpu, msr_info)) > > + return 1; > Bits 9:6 must be zero. > > + vmcs_writel(GUEST_S_CET, data); > Have we ensured that this VMCS field exists? > > + break; > > + case MSR_IA32_INT_SSP_TAB: > > + if (check_cet_msr(vcpu, msr_info)) > > + return 1; > Must be canonical. vCPU must support longmode. > > + vmcs_writel(GUEST_INTR_SSP_TABLE, data); > Have we ensured that this VMCS field exists? > > + break; > > + case MSR_IA32_U_CET: > > + if (check_cet_msr(vcpu, msr_info)) > > + return 1; > Bits 9:6 must be zero. > > + wrmsrl(MSR_IA32_U_CET, data); > > + break; > > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP: > > + if (check_cet_msr(vcpu, msr_info)) > > + return 1; > 'Data' must be canonical and 4-byte aligned. High dword must be zero > on vCPUs that don't support longmode. Thanks a lot for all above captures, will add sanity checks in next version. > > + wrmsrl(msr_info->index, data); > > + break; > > case MSR_TSC_AUX: > > if (!msr_info->host_initiated && > > !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP)) > > -- > > 2.17.2 > >
On Wed, Oct 02, 2019 at 01:57:50PM -0700, Jim Mattson wrote: > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > There're two different places storing Guest CET states, the states > > managed with XSAVES/XRSTORS, as restored/saved > > in previous patch, can be read/write directly from/to the MSRs. > > For those stored in VMCS fields, they're access via vmcs_read/ > > vmcs_write. > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > --- > > arch/x86/kvm/vmx/vmx.c | 83 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 83 insertions(+) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 44913e4ab558..5265db7cd2af 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -1671,6 +1671,49 @@ static int vmx_get_msr_feature(struct kvm_msr_entry *msr) > > return 0; > > } > > > > +static int check_cet_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > I'd suggest changing return type to bool, since you are essentially > returning true or false. I'd also be more explicit in the name, e.g. is_valid_cet_msr().
On Thu, Oct 17, 2019 at 12:58:11PM -0700, Sean Christopherson wrote: > On Wed, Oct 02, 2019 at 01:57:50PM -0700, Jim Mattson wrote: > > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@intel.com> wrote: > > > > > > There're two different places storing Guest CET states, the states > > > managed with XSAVES/XRSTORS, as restored/saved > > > in previous patch, can be read/write directly from/to the MSRs. > > > For those stored in VMCS fields, they're access via vmcs_read/ > > > vmcs_write. > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> > > > --- > > > arch/x86/kvm/vmx/vmx.c | 83 ++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 83 insertions(+) > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > index 44913e4ab558..5265db7cd2af 100644 > > > --- a/arch/x86/kvm/vmx/vmx.c > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > @@ -1671,6 +1671,49 @@ static int vmx_get_msr_feature(struct kvm_msr_entry *msr) > > > return 0; > > > } > > > > > > +static int check_cet_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > > > I'd suggest changing return type to bool, since you are essentially > > returning true or false. > > I'd also be more explicit in the name, e.g. is_valid_cet_msr(). Sure, thank you two.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 44913e4ab558..5265db7cd2af 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1671,6 +1671,49 @@ static int vmx_get_msr_feature(struct kvm_msr_entry *msr) return 0; } +static int check_cet_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) +{ + u64 kvm_xss = kvm_supported_xss(); + + switch (msr_info->index) { + case MSR_IA32_PL0_SSP ... MSR_IA32_PL2_SSP: + if (!(kvm_xss | XFEATURE_MASK_CET_KERNEL)) + return 1; + if (!msr_info->host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) + return 1; + break; + case MSR_IA32_PL3_SSP: + if (!(kvm_xss | XFEATURE_MASK_CET_USER)) + return 1; + if (!msr_info->host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) + return 1; + break; + case MSR_IA32_U_CET: + if (!(kvm_xss | XFEATURE_MASK_CET_USER)) + return 1; + if (!msr_info->host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) && + !guest_cpuid_has(vcpu, X86_FEATURE_IBT)) + return 1; + break; + case MSR_IA32_S_CET: + if (!msr_info->host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) && + !guest_cpuid_has(vcpu, X86_FEATURE_IBT)) + return 1; + break; + case MSR_IA32_INT_SSP_TAB: + if (!msr_info->host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK)) + return 1; + break; + default: + return 1; + } + return 0; +} /* * Reads an msr value (of 'msr_index') into 'pdata'. * Returns 0 on success, non-0 otherwise. @@ -1788,6 +1831,26 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) else msr_info->data = vmx->pt_desc.guest.addr_a[index / 2]; break; + case MSR_IA32_S_CET: + if (check_cet_msr(vcpu, msr_info)) + return 1; + msr_info->data = vmcs_readl(GUEST_S_CET); + break; + case MSR_IA32_INT_SSP_TAB: + if (check_cet_msr(vcpu, msr_info)) + return 1; + msr_info->data = vmcs_readl(GUEST_INTR_SSP_TABLE); + break; + case MSR_IA32_U_CET: + if (check_cet_msr(vcpu, msr_info)) + return 1; + rdmsrl(MSR_IA32_U_CET, msr_info->data); + break; + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP: + if (check_cet_msr(vcpu, msr_info)) + return 1; + rdmsrl(msr_info->index, msr_info->data); + break; case MSR_TSC_AUX: if (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP)) @@ -2039,6 +2102,26 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) else vmx->pt_desc.guest.addr_a[index / 2] = data; break; + case MSR_IA32_S_CET: + if (check_cet_msr(vcpu, msr_info)) + return 1; + vmcs_writel(GUEST_S_CET, data); + break; + case MSR_IA32_INT_SSP_TAB: + if (check_cet_msr(vcpu, msr_info)) + return 1; + vmcs_writel(GUEST_INTR_SSP_TABLE, data); + break; + case MSR_IA32_U_CET: + if (check_cet_msr(vcpu, msr_info)) + return 1; + wrmsrl(MSR_IA32_U_CET, data); + break; + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP: + if (check_cet_msr(vcpu, msr_info)) + return 1; + wrmsrl(msr_info->index, data); + break; case MSR_TSC_AUX: if (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
There're two different places storing Guest CET states, the states managed with XSAVES/XRSTORS, as restored/saved in previous patch, can be read/write directly from/to the MSRs. For those stored in VMCS fields, they're access via vmcs_read/ vmcs_write. Signed-off-by: Yang Weijiang <weijiang.yang@intel.com> --- arch/x86/kvm/vmx/vmx.c | 83 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)