| Message ID | 20210504171734.1434054-15-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: RDPID/RDTSCP fixes and uret MSR cleanups | expand |
On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote: > Squish the Intel and AMD emulation of MSR_TSC_AUX together and tie it to > the guest CPU model instead of the host CPU behavior. While not strictly > necessary to avoid guest breakage, emulating cross-vendor "architecture" > will provide consistent behavior for the guest, e.g. WRMSR fault behavior > won't change if the vCPU is migrated to a host with divergent behavior. > > Note, the "new" kvm_is_supported_user_return_msr() checks do not add new > functionality on either SVM or VMX. On SVM, the equivalent was > "tsc_aux_uret_slot < 0", and on VMX the check was buried in the > vmx_find_uret_msr() call at the find_uret_msr label. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 5 +++++ > arch/x86/kvm/svm/svm.c | 24 ---------------------- > arch/x86/kvm/vmx/vmx.c | 15 -------------- > arch/x86/kvm/x86.c | 36 +++++++++++++++++++++++++++++++++ > 4 files changed, 41 insertions(+), 39 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index a4b912f7e427..00fb9efb9984 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1782,6 +1782,11 @@ int kvm_add_user_return_msr(u32 msr); > int kvm_find_user_return_msr(u32 msr); > int kvm_set_user_return_msr(unsigned index, u64 val, u64 mask); > > +static inline bool kvm_is_supported_user_return_msr(u32 msr) > +{ > + return kvm_find_user_return_msr(msr) >= 0; > +} > + > u64 kvm_scale_tsc(struct kvm_vcpu *vcpu, u64 tsc); > u64 kvm_read_l1_tsc(struct kvm_vcpu *vcpu, u64 host_tsc); > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index de921935e8de..6c7c6a303cc5 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -2663,12 +2663,6 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > msr_info->data |= (u64)svm->sysenter_esp_hi << 32; > break; > case MSR_TSC_AUX: > - if (tsc_aux_uret_slot < 0) > - return 1; > - if (!msr_info->host_initiated && Not related to this patch, but I do wonder why do we need to always allow writing this msr if done by the host, since if neither RDTSPC nor RDPID are supported, the guest won't be able to read this msr at all. > - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > - return 1; > msr_info->data = svm->tsc_aux; > break; > /* > @@ -2885,24 +2879,6 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > svm->sysenter_esp_hi = guest_cpuid_is_intel(vcpu) ? (data >> 32) : 0; > break; > case MSR_TSC_AUX: > - if (tsc_aux_uret_slot < 0) > - return 1; > - > - if (!msr->host_initiated && > - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > - return 1; > - > - /* > - * Per Intel's SDM, bits 63:32 are reserved, but AMD's APM has > - * incomplete and conflicting architectural behavior. Current > - * AMD CPUs completely ignore bits 63:32, i.e. they aren't > - * reserved and always read as zeros. Emulate AMD CPU behavior > - * to avoid explosions if the vCPU is migrated from an AMD host > - * to an Intel host. > - */ > - data = (u32)data; > - > /* > * TSC_AUX is usually changed only during boot and never read > * directly. Intercept TSC_AUX instead of exposing it to the > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 26f82f302391..d85ac5876982 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1981,12 +1981,6 @@ 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_TSC_AUX: > - if (!msr_info->host_initiated && > - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > - return 1; > - goto find_uret_msr; > case MSR_IA32_DEBUGCTLMSR: > msr_info->data = vmcs_read64(GUEST_IA32_DEBUGCTL); > break; > @@ -2302,15 +2296,6 @@ 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_TSC_AUX: > - if (!msr_info->host_initiated && > - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > - return 1; > - /* Check reserved bit, higher 32 bits should be zero */ > - if ((data >> 32) != 0) > - return 1; > - goto find_uret_msr; > case MSR_IA32_PERF_CAPABILITIES: > if (data && !vcpu_to_pmu(vcpu)->version) > return 1; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index adca491d3b4b..896127ea4d4f 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1642,6 +1642,30 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data, > * invokes 64-bit SYSENTER. > */ > data = get_canonical(data, vcpu_virt_addr_bits(vcpu)); > + break; > + case MSR_TSC_AUX: > + if (!kvm_is_supported_user_return_msr(MSR_TSC_AUX)) > + return 1; > + > + if (!host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > + !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > + return 1; > + > + /* > + * Per Intel's SDM, bits 63:32 are reserved, but AMD's APM has > + * incomplete and conflicting architectural behavior. Current > + * AMD CPUs completely ignore bits 63:32, i.e. they aren't > + * reserved and always read as zeros. Enforce Intel's reserved > + * bits check if and only if the guest CPU is Intel, and clear > + * the bits in all other cases. This ensures cross-vendor > + * migration will provide consistent behavior for the guest. > + */ > + if (guest_cpuid_is_intel(vcpu) && (data >> 32) != 0) > + return 1; > + > + data = (u32)data; > + break; > } > > msr.data = data; > @@ -1678,6 +1702,18 @@ int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, > if (!host_initiated && !kvm_msr_allowed(vcpu, index, KVM_MSR_FILTER_READ)) > return KVM_MSR_RET_FILTERED; > > + switch (index) { > + case MSR_TSC_AUX: > + if (!kvm_is_supported_user_return_msr(MSR_TSC_AUX)) > + return 1; > + > + if (!host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > + !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > + return 1; > + break; > + } > + > msr.index = index; > msr.host_initiated = host_initiated; > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On Mon, May 10, 2021, Maxim Levitsky wrote: > On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote: > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > > index de921935e8de..6c7c6a303cc5 100644 > > --- a/arch/x86/kvm/svm/svm.c > > +++ b/arch/x86/kvm/svm/svm.c > > @@ -2663,12 +2663,6 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > msr_info->data |= (u64)svm->sysenter_esp_hi << 32; > > break; > > case MSR_TSC_AUX: > > - if (tsc_aux_uret_slot < 0) > > - return 1; > > - if (!msr_info->host_initiated && > Not related to this patch, but I do wonder why do we need > to always allow writing this msr if done by the host, > since if neither RDTSPC nor RDPID are supported, the guest > won't be able to read this msr at all. It's an ordering thing and not specific to MSR_TSC_AUX. Exempting host userspace from guest CPUID checks allows userspace to set MSR state, e.g. during migration, before setting the guest CPUID model. > > - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > > - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > > - return 1; > > msr_info->data = svm->tsc_aux; > > break; > > /*
On Mon, May 10, 2021 at 9:50 AM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, May 10, 2021, Maxim Levitsky wrote: > > On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote: > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > > > index de921935e8de..6c7c6a303cc5 100644 > > > --- a/arch/x86/kvm/svm/svm.c > > > +++ b/arch/x86/kvm/svm/svm.c > > > @@ -2663,12 +2663,6 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > > msr_info->data |= (u64)svm->sysenter_esp_hi << 32; > > > break; > > > case MSR_TSC_AUX: > > > - if (tsc_aux_uret_slot < 0) > > > - return 1; > > > - if (!msr_info->host_initiated && > > Not related to this patch, but I do wonder why do we need > > to always allow writing this msr if done by the host, > > since if neither RDTSPC nor RDPID are supported, the guest > > won't be able to read this msr at all. > > It's an ordering thing and not specific to MSR_TSC_AUX. Exempting host userspace > from guest CPUID checks allows userspace to set MSR state, e.g. during migration, > before setting the guest CPUID model. I thought the rule was that if an MSR was enumerated by KVM_GET_MSR_INDEX_LIST, then KVM had to accept legal writes from the host. The only "ordering thing" is that KVM_GET_MSR_INDEX_LIST is a device ioctl, so it can't take guest CPUID information into account. > > > - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > > > - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > > > - return 1; > > > msr_info->data = svm->tsc_aux; > > > break; > > > /*
On Mon, 2021-05-10 at 10:11 -0700, Jim Mattson wrote: > On Mon, May 10, 2021 at 9:50 AM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, May 10, 2021, Maxim Levitsky wrote: > > > On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote: > > > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > > > > index de921935e8de..6c7c6a303cc5 100644 > > > > --- a/arch/x86/kvm/svm/svm.c > > > > +++ b/arch/x86/kvm/svm/svm.c > > > > @@ -2663,12 +2663,6 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > > > msr_info->data |= (u64)svm->sysenter_esp_hi << 32; > > > > break; > > > > case MSR_TSC_AUX: > > > > - if (tsc_aux_uret_slot < 0) > > > > - return 1; > > > > - if (!msr_info->host_initiated && > > > Not related to this patch, but I do wonder why do we need > > > to always allow writing this msr if done by the host, > > > since if neither RDTSPC nor RDPID are supported, the guest > > > won't be able to read this msr at all. > > > > It's an ordering thing and not specific to MSR_TSC_AUX. Exempting host userspace > > from guest CPUID checks allows userspace to set MSR state, e.g. during migration, > > before setting the guest CPUID model. > > I thought the rule was that if an MSR was enumerated by > KVM_GET_MSR_INDEX_LIST, then KVM had to accept legal writes from the > host. The only "ordering thing" is that KVM_GET_MSR_INDEX_LIST is a > device ioctl, so it can't take guest CPUID information into account. This makes sense. Thanks! Best regards, Maxim Levitsky > > > > > - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && > > > > - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) > > > > - return 1; > > > > msr_info->data = svm->tsc_aux; > > > > break; > > > > /*
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index a4b912f7e427..00fb9efb9984 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1782,6 +1782,11 @@ int kvm_add_user_return_msr(u32 msr); int kvm_find_user_return_msr(u32 msr); int kvm_set_user_return_msr(unsigned index, u64 val, u64 mask); +static inline bool kvm_is_supported_user_return_msr(u32 msr) +{ + return kvm_find_user_return_msr(msr) >= 0; +} + u64 kvm_scale_tsc(struct kvm_vcpu *vcpu, u64 tsc); u64 kvm_read_l1_tsc(struct kvm_vcpu *vcpu, u64 host_tsc); diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index de921935e8de..6c7c6a303cc5 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -2663,12 +2663,6 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) msr_info->data |= (u64)svm->sysenter_esp_hi << 32; break; case MSR_TSC_AUX: - if (tsc_aux_uret_slot < 0) - return 1; - if (!msr_info->host_initiated && - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) - return 1; msr_info->data = svm->tsc_aux; break; /* @@ -2885,24 +2879,6 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) svm->sysenter_esp_hi = guest_cpuid_is_intel(vcpu) ? (data >> 32) : 0; break; case MSR_TSC_AUX: - if (tsc_aux_uret_slot < 0) - return 1; - - if (!msr->host_initiated && - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) - return 1; - - /* - * Per Intel's SDM, bits 63:32 are reserved, but AMD's APM has - * incomplete and conflicting architectural behavior. Current - * AMD CPUs completely ignore bits 63:32, i.e. they aren't - * reserved and always read as zeros. Emulate AMD CPU behavior - * to avoid explosions if the vCPU is migrated from an AMD host - * to an Intel host. - */ - data = (u32)data; - /* * TSC_AUX is usually changed only during boot and never read * directly. Intercept TSC_AUX instead of exposing it to the diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 26f82f302391..d85ac5876982 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1981,12 +1981,6 @@ 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_TSC_AUX: - if (!msr_info->host_initiated && - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) - return 1; - goto find_uret_msr; case MSR_IA32_DEBUGCTLMSR: msr_info->data = vmcs_read64(GUEST_IA32_DEBUGCTL); break; @@ -2302,15 +2296,6 @@ 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_TSC_AUX: - if (!msr_info->host_initiated && - !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && - !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) - return 1; - /* Check reserved bit, higher 32 bits should be zero */ - if ((data >> 32) != 0) - return 1; - goto find_uret_msr; case MSR_IA32_PERF_CAPABILITIES: if (data && !vcpu_to_pmu(vcpu)->version) return 1; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index adca491d3b4b..896127ea4d4f 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1642,6 +1642,30 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data, * invokes 64-bit SYSENTER. */ data = get_canonical(data, vcpu_virt_addr_bits(vcpu)); + break; + case MSR_TSC_AUX: + if (!kvm_is_supported_user_return_msr(MSR_TSC_AUX)) + return 1; + + if (!host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && + !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) + return 1; + + /* + * Per Intel's SDM, bits 63:32 are reserved, but AMD's APM has + * incomplete and conflicting architectural behavior. Current + * AMD CPUs completely ignore bits 63:32, i.e. they aren't + * reserved and always read as zeros. Enforce Intel's reserved + * bits check if and only if the guest CPU is Intel, and clear + * the bits in all other cases. This ensures cross-vendor + * migration will provide consistent behavior for the guest. + */ + if (guest_cpuid_is_intel(vcpu) && (data >> 32) != 0) + return 1; + + data = (u32)data; + break; } msr.data = data; @@ -1678,6 +1702,18 @@ int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, if (!host_initiated && !kvm_msr_allowed(vcpu, index, KVM_MSR_FILTER_READ)) return KVM_MSR_RET_FILTERED; + switch (index) { + case MSR_TSC_AUX: + if (!kvm_is_supported_user_return_msr(MSR_TSC_AUX)) + return 1; + + if (!host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP) && + !guest_cpuid_has(vcpu, X86_FEATURE_RDPID)) + return 1; + break; + } + msr.index = index; msr.host_initiated = host_initiated;
Squish the Intel and AMD emulation of MSR_TSC_AUX together and tie it to the guest CPU model instead of the host CPU behavior. While not strictly necessary to avoid guest breakage, emulating cross-vendor "architecture" will provide consistent behavior for the guest, e.g. WRMSR fault behavior won't change if the vCPU is migrated to a host with divergent behavior. Note, the "new" kvm_is_supported_user_return_msr() checks do not add new functionality on either SVM or VMX. On SVM, the equivalent was "tsc_aux_uret_slot < 0", and on VMX the check was buried in the vmx_find_uret_msr() call at the find_uret_msr label. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_host.h | 5 +++++ arch/x86/kvm/svm/svm.c | 24 ---------------------- arch/x86/kvm/vmx/vmx.c | 15 -------------- arch/x86/kvm/x86.c | 36 +++++++++++++++++++++++++++++++++ 4 files changed, 41 insertions(+), 39 deletions(-)