Re: [PATCH v2 1/4] x86/split_lock: Enumerate #AC exception for split locked access feature

From: Fenghua Yu <fenghua.yu@intel.com>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Dave Hansen <dave.hansen@intel.com>,
	Fenghua Yu <fenghua.yu@intel.com>, Ingo Molnar <mingo@redhat.com>,
	H Peter Anvin <hpa@zytor.com>, Ashok Raj <ashok.raj@intel.com>,
	Alan Cox <alan@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Rafael Wysocki <rafael.j.wysocki@intel.com>,
	Tony Luck <tony.luck@intel.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
	Vedvyas Shanbhogue <vedvyas.shanbhogue@intel.com>
Subject: Re: [PATCH v2 1/4] x86/split_lock: Enumerate #AC exception for split locked access feature
Date: Tue, 10 Jul 2018 11:45:07 -0700	[thread overview]
Message-ID: <20180710184506.GA37857@romley-ivt3.sc.intel.com> (raw)
In-Reply-To: <20180704200742.GD7451@localhost.localdomain>

On Wed, Jul 04, 2018 at 05:07:42PM -0300, Eduardo Habkost wrote:
> On Fri, Jun 29, 2018 at 06:23:35PM +0200, Thomas Gleixner wrote:
> > On Fri, 29 Jun 2018, Dave Hansen wrote:
> > > On 06/29/2018 07:33 AM, Fenghua Yu wrote:
> > > > +/* Detect feature of #AC for split lock by probing bit 29 in MSR_TEST_CTL. */
> > > > +void detect_ac_split_lock(void)
> > > > +{
> > > > +	u64 val, orig_val;
> > > > +	int ret;
> > > > +
> > > > +	/* Attempt to read the MSR. If the MSR doesn't exist, reading fails. */
> > > > +	ret = rdmsrl_safe(MSR_TEST_CTL, &val);
> > > > +	if (ret)
> > > > +		return;
> > > 
> > > This is a bit fast and loose with how the feature is detected, which
> > > might be OK, but can we call out why we are doing this, please?
> > > 
> > > Is this MSR not really model-specific?  Is it OK to go poking at it on
> > > all x86 variants?  Or, do we at _least_ need a check for Intel cpus in here?
> > 
> > That definitely needs a vendor check. Also the whole code needs to be
> > compiled out if CONFIG_INTEL=n.
> > 
> > Aside of that this wants to be enumerated. CPUID or MISC_FEATURES and not
> > this guess work detection logic. Why do I have to ask for that for every
> > other new feature thingy?
> 
> Yes, please.  KVM hosts normally expect guests to not touch MSRs
> unless we explicitly tell them the MSR is available (normally
> through CPUID).  This is important to ensure live migration
> between different host kernel versions works reliably.

The problem is the hardware design for the feature is complete. The
hardware designer cannot change the feature enumeration to CPUID or
MISC_FEATURES. In the future, the designer will put future model-
specific feature bits in a processor feature MSR and hopefully we will
have a nice standard enumeration way for all future model specific
features. But split lock is too late (or too early) to be in the feature
MSR.

Considering the current situation, can we do split lock as following?

By default, #AC for split lock is not enabled by kernel. It's disabled
by BIOS by default (bit29=0). Kernel doesn't clear bit 29.

For IOT/real time users, they know the feature is in the processor and
they know the consequence of enabling the feature (e.g. some apps will
be killed because of split lock issues). They want to enable #AC for
split lock by kernel parameter "split_lock_detect=on". Kernel calls
wrmsr_safe(), which can fail on processor not supporting
the feature, to enable the feature.

There is no enumeration and no flag in /proc/cpuinfo flag for the feature.

Is this a right way to go? Please advise.

Thanks.

-Fenghua