| Message ID | 20210504171734.1434054-12-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: RDPID/RDTSCP fixes and uret MSR cleanups | expand |
On 04/05/21 19:17, Sean Christopherson wrote: > Tag TSX_CTRL as not needing to be loaded when RTM isn't supported in the > host. Crushing the write mask to '0' has the same effect, but requires > more mental gymnastics to understand. This doesn't explain _why_ this is now possible. What about: Now that user return MSRs is always present in the list, we don't have the problem that the TSX_CTRL MSR needs a slot vmx->guest_uret_msrs even if RTM is not supported in the host (and therefore there is nothing to enable). Thus we can simply tag TSX_CTRL as not needing to be loaded instead of crushing the write mask to '0'. That has the same effect, but requires less mental gymnastics to understand. Paolo > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 4b432d2bbd06..7a53568b34fc 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1771,7 +1771,13 @@ static void setup_msrs(struct vcpu_vmx *vmx) > guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDTSCP) || > guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDPID)); > > - vmx_setup_uret_msr(vmx, MSR_IA32_TSX_CTRL, true); > + /* > + * hle=0, rtm=0, tsx_ctrl=1 can be found with some combinations of new > + * kernel and old userspace. If those guests run on a tsx=off host, do > + * allow guests to use TSX_CTRL, but don't change the value in hardware > + * so that TSX remains always disabled. > + */ > + vmx_setup_uret_msr(vmx, MSR_IA32_TSX_CTRL, boot_cpu_has(X86_FEATURE_RTM)); > > if (cpu_has_vmx_msr_bitmap()) > vmx_update_msr_bitmap(&vmx->vcpu); > @@ -6919,23 +6925,15 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu) > vmx->guest_uret_msrs[i].data = 0; > vmx->guest_uret_msrs[i].mask = -1ull; > } > - tsx_ctrl = vmx_find_uret_msr(vmx, MSR_IA32_TSX_CTRL); > - if (tsx_ctrl) { > + if (boot_cpu_has(X86_FEATURE_RTM)) { > /* > * TSX_CTRL_CPUID_CLEAR is handled in the CPUID interception. > * Keep the host value unchanged to avoid changing CPUID bits > * under the host kernel's feet. > - * > - * hle=0, rtm=0, tsx_ctrl=1 can be found with some combinations > - * of new kernel and old userspace. If those guests run on a > - * tsx=off host, do allow guests to use TSX_CTRL, but do not > - * change the value on the host so that TSX remains always > - * disabled. > */ > - if (boot_cpu_has(X86_FEATURE_RTM)) > + tsx_ctrl = vmx_find_uret_msr(vmx, MSR_IA32_TSX_CTRL); > + if (tsx_ctrl) > vmx->guest_uret_msrs[i].mask = ~(u64)TSX_CTRL_CPUID_CLEAR; > - else > - vmx->guest_uret_msrs[i].mask = 0; > } > > err = alloc_loaded_vmcs(&vmx->vmcs01); >
On Wed, May 05, 2021, Paolo Bonzini wrote: > On 04/05/21 19:17, Sean Christopherson wrote: > > Tag TSX_CTRL as not needing to be loaded when RTM isn't supported in the > > host. Crushing the write mask to '0' has the same effect, but requires > > more mental gymnastics to understand. > > This doesn't explain _why_ this is now possible. What about: > > Now that user return MSRs is always present in the list, we don't have User return MSRs aren't always present in the list; this series doesn't change that behavior at all. > the problem that the TSX_CTRL MSR needs a slot vmx->guest_uret_msrs even > if RTM is not supported in the host (and therefore there is nothing to > enable). Thus we can simply tag TSX_CTRL as not needing to be loaded > instead of crushing the write mask to '0'. Unless I'm missing something, it would have been possible to give TSX_CTRL a slot but not load it even before this refactoring, we just missed that approach when handling the TSX_CTRL without HLE/RTM case. Several other MSRs rely on this behavior, notably the SYSCALL MSRs, which are present in the list so that the guest can read/write the MSRs, but are loaded into hardware iff the guest has enabled SYSCALL. All that said, I certainly have no objection to writing a longer changelog.
On 05/05/21 17:36, Sean Christopherson wrote: > On Wed, May 05, 2021, Paolo Bonzini wrote: >> On 04/05/21 19:17, Sean Christopherson wrote: >>> Tag TSX_CTRL as not needing to be loaded when RTM isn't supported in the >>> host. Crushing the write mask to '0' has the same effect, but requires >>> more mental gymnastics to understand. >> >> This doesn't explain _why_ this is now possible. What about: >> >> Now that user return MSRs is always present in the list, we don't have > > User return MSRs aren't always present in the list; this series doesn't change > that behavior at all. > >> the problem that the TSX_CTRL MSR needs a slot vmx->guest_uret_msrs even >> if RTM is not supported in the host (and therefore there is nothing to >> enable). Thus we can simply tag TSX_CTRL as not needing to be loaded >> instead of crushing the write mask to '0'. > > Unless I'm missing something, it would have been possible to give TSX_CTRL a > slot but not load it even before this refactoring, we just missed that approach > when handling the TSX_CTRL without HLE/RTM case. Several other MSRs rely on > this behavior, notably the SYSCALL MSRs, which are present in the list so that > the guest can read/write the MSRs, but are loaded into hardware iff the guest > has enabled SYSCALL. You're right, it used to be done with vmx->nr_active_uret_msr. Paolo > All that said, I certainly have no objection to writing a longer changelog.
On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote: > Tag TSX_CTRL as not needing to be loaded when RTM isn't supported in the > host. Crushing the write mask to '0' has the same effect, but requires > more mental gymnastics to understand. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 4b432d2bbd06..7a53568b34fc 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1771,7 +1771,13 @@ static void setup_msrs(struct vcpu_vmx *vmx) > guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDTSCP) || > guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDPID)); > > - vmx_setup_uret_msr(vmx, MSR_IA32_TSX_CTRL, true); > + /* > + * hle=0, rtm=0, tsx_ctrl=1 can be found with some combinations of new > + * kernel and old userspace. If those guests run on a tsx=off host, do > + * allow guests to use TSX_CTRL, but don't change the value in hardware > + * so that TSX remains always disabled. > + */ > + vmx_setup_uret_msr(vmx, MSR_IA32_TSX_CTRL, boot_cpu_has(X86_FEATURE_RTM)); > > if (cpu_has_vmx_msr_bitmap()) > vmx_update_msr_bitmap(&vmx->vcpu); > @@ -6919,23 +6925,15 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu) > vmx->guest_uret_msrs[i].data = 0; > vmx->guest_uret_msrs[i].mask = -1ull; > } > - tsx_ctrl = vmx_find_uret_msr(vmx, MSR_IA32_TSX_CTRL); > - if (tsx_ctrl) { > + if (boot_cpu_has(X86_FEATURE_RTM)) { > /* > * TSX_CTRL_CPUID_CLEAR is handled in the CPUID interception. > * Keep the host value unchanged to avoid changing CPUID bits > * under the host kernel's feet. > - * > - * hle=0, rtm=0, tsx_ctrl=1 can be found with some combinations > - * of new kernel and old userspace. If those guests run on a > - * tsx=off host, do allow guests to use TSX_CTRL, but do not > - * change the value on the host so that TSX remains always > - * disabled. > */ > - if (boot_cpu_has(X86_FEATURE_RTM)) > + tsx_ctrl = vmx_find_uret_msr(vmx, MSR_IA32_TSX_CTRL); > + if (tsx_ctrl) > vmx->guest_uret_msrs[i].mask = ~(u64)TSX_CTRL_CPUID_CLEAR; > - else > - vmx->guest_uret_msrs[i].mask = 0; > } > > err = alloc_loaded_vmcs(&vmx->vmcs01); I also agree that commit message should be updated as Paolo said, but other than that: Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky <mlevitsk@redhat.com>
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 4b432d2bbd06..7a53568b34fc 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1771,7 +1771,13 @@ static void setup_msrs(struct vcpu_vmx *vmx) guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDTSCP) || guest_cpuid_has(&vmx->vcpu, X86_FEATURE_RDPID)); - vmx_setup_uret_msr(vmx, MSR_IA32_TSX_CTRL, true); + /* + * hle=0, rtm=0, tsx_ctrl=1 can be found with some combinations of new + * kernel and old userspace. If those guests run on a tsx=off host, do + * allow guests to use TSX_CTRL, but don't change the value in hardware + * so that TSX remains always disabled. + */ + vmx_setup_uret_msr(vmx, MSR_IA32_TSX_CTRL, boot_cpu_has(X86_FEATURE_RTM)); if (cpu_has_vmx_msr_bitmap()) vmx_update_msr_bitmap(&vmx->vcpu); @@ -6919,23 +6925,15 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu) vmx->guest_uret_msrs[i].data = 0; vmx->guest_uret_msrs[i].mask = -1ull; } - tsx_ctrl = vmx_find_uret_msr(vmx, MSR_IA32_TSX_CTRL); - if (tsx_ctrl) { + if (boot_cpu_has(X86_FEATURE_RTM)) { /* * TSX_CTRL_CPUID_CLEAR is handled in the CPUID interception. * Keep the host value unchanged to avoid changing CPUID bits * under the host kernel's feet. - * - * hle=0, rtm=0, tsx_ctrl=1 can be found with some combinations - * of new kernel and old userspace. If those guests run on a - * tsx=off host, do allow guests to use TSX_CTRL, but do not - * change the value on the host so that TSX remains always - * disabled. */ - if (boot_cpu_has(X86_FEATURE_RTM)) + tsx_ctrl = vmx_find_uret_msr(vmx, MSR_IA32_TSX_CTRL); + if (tsx_ctrl) vmx->guest_uret_msrs[i].mask = ~(u64)TSX_CTRL_CPUID_CLEAR; - else - vmx->guest_uret_msrs[i].mask = 0; } err = alloc_loaded_vmcs(&vmx->vmcs01);
Tag TSX_CTRL as not needing to be loaded when RTM isn't supported in the host. Crushing the write mask to '0' has the same effect, but requires more mental gymnastics to understand. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/vmx.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)