Message-ID: <20190211191643.GB103371@romley-ivt3.sc.intel.com>
Date:   Mon, 11 Feb 2019 11:16:43 -0800
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
        H Peter Anvin <hpa@...or.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Ricardo Neri <ricardo.neri@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v3 08/10] x86/setcpuid: Add kernel option setcpuid

On Sun, Feb 10, 2019 at 08:20:20PM +0100, Thomas Gleixner wrote:
> On Tue, 5 Feb 2019, Peter Zijlstra wrote:
> > On Tue, Feb 05, 2019 at 07:19:16AM -0800, Dave Hansen wrote:
> > > On 2/5/19 12:48 AM, Peter Zijlstra wrote:
> > > This isn't something we want everybody and their grandma to turn on;
> > > it's a rather specialized feature.  It's really only for folks that care
> > > about the latency incurred across the entire system on split lock
> > > operations.
> > 
> > That really should be everyone. That split lock stuff is horrible. There
> > is no real down-side to having it always enabled. Code that breaks is
> > bad code you want fixed anyway.
> > 
> > Like I said elsewhere, I wish it would #AC for any unaligned LOCK
> > prefix, not just cross-line. I see why we'd not want to traditional RISC
> > #AC for every load/store, but atomics really had better be aligned.
> 
> Right, we should really make this default enabled.

Yes, I agree.

> 
> > > > Is this some transient state; where a few (early) models will not have
> > > > the enumeration sorted but all later models will have it all neat and
> > > > tidy?
> > > 
> > > From my understanding, it's not just an early stepping.  It's a
> > > generational thing.  The current generation lacks the enumeration and
> > > the next generation will get it.  Both have the silicon to implement the
> > > feature itself.
> > 
> > I never said stepping, in fact I explicitly said model.
> > 
> > > > If so, we can easily do the FMS solution for this.
> > > 
> > > Yeah, we can.  I honestly forget why we didn't do FMS. :)
> > 
> > Right so FMS is fairly horrible; but when it is a stop-gap for a limited
> > number of models it's waaay better than dodgy cmdline things.
> 
> One or two is fine. And _IF_ we get the enumeration sorted before we merge
> that, then we can declare the FM list as immutable :)

There will be about 8 models that have the split lock feature but
don't have the IA32_CORE_CAPABILITY enumeration. The models will NOT
have the IA32_CORE_CAPABILITY enumerate in the future as planned. All
of other models than these ones will have the IA32_CORE_CAPABILITY
enumeration.

I don't see any of the model numbers are public yet as of now.

In the next version of patches, I will do the following changes from
v3:

1. Remove patch #11 that implements "setcpuid=".
2. But I will not implement the patch that enables the split lock feature
based on FMS because there is no public FMS numbers of those models. The
patch/patches will be implemented only after the FMS numbers are public.
3. The new patches enables the feature once it enumerates the feature
by bit 5 in IA32_CORE_CAPABILITY.
4. The feature can be disabled by kernel option
"clearcpuid=split_lock_detection" during early boot time.
5. The feature can be disabled/enabled during run time by
sysfs interface "/sys/kernel/split_lock_detection"

Does that make sense?

Thanks.

-Fenghua

Powered by blists - more mailing lists