Re: [PATCH] x86/tsx: fix KVM guest live migration for tsx=on

From: Jon Kohler <jon@nutanix.com>
To: Dave Hansen <dave.hansen@intel.com>
Cc: Jon Kohler <jon@nutanix.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>, Tony Luck <tony.luck@intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Borislav Petkov <bp@suse.de>,
	Neelima Krishnan <neelima.krishnan@intel.com>,
	"kvm @ vger . kernel . org" <kvm@vger.kernel.org>
Subject: Re: [PATCH] x86/tsx: fix KVM guest live migration for tsx=on
Date: Mon, 11 Apr 2022 19:35:37 +0000	[thread overview]
Message-ID: <AE4621FC-0947-4CEF-A1B3-87D4E00C786D@nutanix.com> (raw)
In-Reply-To: <41a3ca80-d3e2-47d2-8f1c-9235c55de8d1@intel.com>

> On Apr 11, 2022, at 3:26 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
> On 4/11/22 11:01, Jon Kohler wrote:
>> static enum tsx_ctrl_states x86_get_tsx_auto_mode(void)
>> {
>> +	/*
>> +	 * Hardware will always abort a TSX transaction if both CPUID bits
>> +	 * RTM_ALWAYS_ABORT and TSX_FORCE_ABORT are set. In this case, it is
>> +	 * better not to enumerate CPUID.RTM and CPUID.HLE bits. Clear them
>> +	 * here.
>> +	 */
>> +	if (boot_cpu_has(X86_FEATURE_RTM_ALWAYS_ABORT) &&
>> +	    boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)) {
>> +		tsx_clear_cpuid();
>> +		setup_clear_cpu_cap(X86_FEATURE_RTM);
>> +		setup_clear_cpu_cap(X86_FEATURE_HLE);
>> +		return TSX_CTRL_RTM_ALWAYS_ABORT;
>> +	}
> 
> I don't really like hiding the setup_clear_cpu_cap() like this.  Right
> now, all of the setup_clear_cpu_cap()'s are in a single function and
> they are pretty easy to figure out.
> 
> This seems like logic that deserves to be appended down to the last if()
> block of code in tsx_init() instead of squirreled away in a "get mode"
> function.  Does this work?

Thanks for the review, Dave. Was trying to make the change simple
with just a cut-n-paste of existing code from one place to the other,
but I see what you’re saying. Yea, I can rework the logic as you
suggested, I’ll send out a v2 patch.

Also, while I’ve got you, I’d also like to send out a patch to simply
force abort all transactions even when tsx=on, and just be done with
TSX. Now that we’ve had the patch that introduced this functionality
I’m patching for roughly a year, combined with the microcode going
out, it seems like TSX’s numbered days have come to an end. 

That could greatly simplify the kernels handling of TAA on systems
that have ARCH_CAP_TSX_CTRL_MSR.

Thoughts?

>        if (tsx_ctrl_state == TSX_CTRL_DISABLE) {
> 		...
>        } else if (tsx_ctrl_state == TSX_CTRL_ENABLE) {
> 		...	
>        } else if (tsx_ctrl_state == TSX_CTRL_RTM_ALWAYS_ABORT) {
> 		tsx_clear_cpuid();
> 
> 		setup_clear_cpu_cap(X86_FEATURE_RTM);
> 		setup_clear_cpu_cap(X86_FEATURE_HLE);
> 	}
> 