On Thu, Jan 28, 2021, Paolo Bonzini wrote: > Userspace that does not know about KVM_GET_MSR_FEATURE_INDEX_LIST will > generally use the default value for MSR_IA32_ARCH_CAPABILITIES. > When this happens and the host has tsx=on, it is possible to end up > with virtual machines that have HLE and RTM disabled, but TSX_CTRL > disabled.
Thos wording is confusing the heck out of me. I think what you're saying is "but TSX disabled in the guest via TSX_CTRL". I read "but TSX_CTRL disabled" as saying the the TSX_CTRL itself was disabled/unsupported. > If the fleet is then switched to tsx=off, kvm_get_arch_capabilities() > will clear the ARCH_CAP_TSX_CTRL_MSR bit and it will not be possible > to use the tsx=off as migration destinations, even though the guests > indeed do not have TSX enabled. > > When tsx=off is used, however, we know that guests will not have > HLE and RTM (or if userspace sets bogus CPUID data, we do not > expect HLE and RTM to work in guests). Therefore we can keep > TSX_CTRL_RTM_DISABLE set for the entire life of the guests and > save MSR reads and writes on KVM_RUN and in the user return > notifiers. > > Cc: [email protected] > Fixes: cbbaa2727aa3 ("KVM: x86: fix presentation of TSX feature in > ARCH_CAPABILITIES") > Signed-off-by: Paolo Bonzini <[email protected]> > --- > arch/x86/kvm/vmx/vmx.c | 12 +++++++++++- > arch/x86/kvm/x86.c | 2 +- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index cc60b1fc3ee7..80491a729408 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6863,8 +6863,18 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu) > * No need to pass TSX_CTRL_CPUID_CLEAR through, so > * let's avoid changing CPUID bits under the host > * kernel's feet. > + * > + * If the host disabled RTM, we may still need TSX_CTRL > + * to be supported in the guest; for example the guest > + * could have been created on a tsx=on host with hle=0, > + * rtm=0, tsx_ctrl=1 and later migrate to a tsx=off > host. > + * In that case however do not change the value on the > host, > + * so that TSX remains always disabled. Oof, can you reword this to clarify what "the value" refers to? The previous paragraphs talks about TSX_CTRL_CPUID_CLEAR, and the obvious "value" in the code is also TSX_CTRL_CPUID_CLEAR, and so I thought the comment was saying "don't change the value of CPUID_CLEAR", which is non-sensical because that's the the RTM-enabled case does... > */ > - vmx->guest_uret_msrs[j].mask = > ~(u64)TSX_CTRL_CPUID_CLEAR; > + if (boot_cpu_has(X86_FEATURE_RTM)) > + vmx->guest_uret_msrs[j].mask = > ~(u64)TSX_CTRL_CPUID_CLEAR; > + else > + vmx->guest_uret_msrs[j].mask = 0; IMO, this is an unnecessarily confusing way to "remove" the user return MSR. Changing the ordering to do a 'continue' would also provide a separate chunk of code for the new comment. And maybe replace the switch with an if-statement to avoid a 'continue' buried in a switch? vmx->guest_uret_msrs[j].slot = i; vmx->guest_uret_msrs[j].data = 0; if (index == MSR_IA32_TSX_CTRL) { /* Fancy new comment here. */ if (!boot_cpu_has(X86_FEATURE_RTM)) continue; /* * No need to pass TSX_CTRL_CPUID_CLEAR through, so * let's avoid changing CPUID bits under the host * kernel's feet. */ vmx->guest_uret_msrs[j].mask = ~(u64)TSX_CTRL_CPUID_CLEAR; } else { vmx->guest_uret_msrs[j].mask = -1ull; } > break; > default: > vmx->guest_uret_msrs[j].mask = -1ull; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 76bce832cade..15733013b266 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1401,7 +1401,7 @@ static u64 kvm_get_arch_capabilities(void) This comments needs to be rewritten, it reflects the old behavior of exposing the feature iff RTM/TSC is supported by the host. > * This lets the guest use VERW to clear CPU buffers. > */ > if (!boot_cpu_has(X86_FEATURE_RTM)) > - data &= ~(ARCH_CAP_TAA_NO | ARCH_CAP_TSX_CTRL_MSR); > + data &= ~ARCH_CAP_TAA_NO; > else if (!boot_cpu_has_bug(X86_BUG_TAA)) > data |= ARCH_CAP_TAA_NO; > > -- > 2.26.2 >
