[MODERATED] Re: [PATCH v4 01/10] TAAv4 1

[MODERATED] Re: [PATCH v4 01/10] TAAv4 1

[Borislav Petkov]

On Tue, Sep 03, 2019 at 02:11:32PM -0700, speck for Pawan Gupta wrote:
> From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Subject: [PATCH v4 01/10] x86/tsx: Add enumeration support for IA32_TSX_CTRL
>  MSR
> 
> Transactional Synchronization Extensions (TSX) may be used on certain
> processors as part of a speculative side channel attack.  A microcode
> update for existing processors that are vulnerable to this attack will
> add a new MSR, IA32_TSX_CTRL to allow the system administrator the option
> to disable TSX as one of the possible mitigations.  [Note that future
> processors that are not vulnerable will also support the IA32_TSX_CTRL
> MSR].  This patch adds the defines for the new IA32_TSX_CTRL MSR and its

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> bits.
> 
> Bit 7 of the IA32_ARCH_CAPABILITIES indicates the presence of the
> IA32_TSX_CTRL MSR.
> 
> There are two control bits in IA32_TSX_CTRL MSR:
> 
>   Bit 0: Can be written to "1" to disable the Restricted Transactional

"Bit 0: When set, it disables ... "

>          Memory (RTM) sub-feature of TSX (will force all transactions
>          to abort on the XBEGIN instruction).
> 
>   Bit 1: Can be written to "1" to disable enumeration of the RTM feature

Ditto.

>          (i.e. will make CPUID(EAX=7).EBX{bit11} read as 0).
> 
> The other TSX sub-feature, Hardware Lock Elision (HLE), is unconditionally
> disabled but still enumerated as present by CPUID(EAX=7).EBX{bit4}.
> 
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> ---
>  arch/x86/include/asm/msr-index.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 271d837d69a8..9163eb67962e 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -93,6 +93,7 @@
>  						  * Microarchitectural Data
>  						  * Sampling (MDS) vulnerabilities.
>  						  */
> +#define ARCH_CAP_TSX_CTRL_MSR		BIT(7)	/* MSR for TSX control is available. */
>  
>  #define MSR_IA32_FLUSH_CMD		0x0000010b
>  #define L1D_FLUSH			BIT(0)	/*
> @@ -103,6 +104,10 @@
>  #define MSR_IA32_BBL_CR_CTL		0x00000119
>  #define MSR_IA32_BBL_CR_CTL3		0x0000011e
>  
> +#define MSR_IA32_TSX_CTRL		0x00000122


> +#define MSR_TSX_CTRL_RTM_DISABLE	BIT(0)	/* Disable RTM feature */
> +#define MSR_TSX_CTRL_CPUID_CLEAR	BIT(1)	/* Disable TSX enumeration */

For those last two:

s/MSR_//

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München
-- 

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Greg KH]

On Tue, Sep 10, 2019 at 04:34:49PM -0700, speck for Luck, Tony wrote:
> On Tue, Sep 10, 2019 at 11:33:34PM +0100, speck for Greg KH wrote:
> > $ ls /sys/devices/cpu/
> > allow_tsx_force_abort  format                      power      type
> > caps                   freeze_on_smi               rdpmc      uevent
> > events                 perf_event_mux_interval_ms  subsystem
> > 
> > Oh look, a tsx-specific cpu sysfs file on my laptop...
> > 
> > But of cource it's not actually documented in Documentation/ABI/ so that
> > does need to be fixed as well, otherwise you could have used:
> > $ ./scripts/get_abi.pl search tsx
> > $
> > 
> > and it would have printed out lots of good information about the
> > allow_tsx_force_abort sysfs file that is there.
> 
> That doesn't look like a super helpful template to follow though.

What template?  Documentation/ABI/ ?  That's required for all sysfs
files (or should be, lots get merged without documentation...)

> If my steering through the twisty maze is correct, then the
> "allow_tsx_force_abort" file appeared there courtesy of this
> chain:
> 
> kernel/events/core.c:pmu_dev_alloc()
> 	if (pmu->attr_update)
>                 ret = sysfs_update_groups(&pmu->dev->kobj, pmu->attr_update);
> 
> pmu->attr_update was set in arch/x86/events/core.c:init_hw_perf_events()
> 	pmu.attr_update = x86_pmu.attr_update;
> 
> x86_pmu.attr_update was set in arch/x86/events/intel/core.c:intel_pmu_init()
> 	x86_pmu.attr_update = attr_update;
> 
> attr_update[] is a static array
> which includes an element
> 
> 	group_default
> 
> which includes an element
> 		intel_pmu_attrs[]
> 
> which includes an element
> 
> 			static DEVICE_ATTR(allow_tsx_force_abort, 0644,
> 
> Pawan wants to include a file to enable/disable TSX ... which has nothing
> to do with PMU ... so bolting his file into the bottom of that call sequence
> isn't going to make anyone happy.
> 
> Does he need to build a similar call sequence from generic code down
> into Intel specific X86 code that attaches somewhere outside of perf/events?

So this is a perf-event-specfic thing?  Then no, that's the wrong place
for this, sorry.  But you have to admit, it does look relevant given the
total lack of documentation here :(

greg k-h

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Greg KH]

On Tue, Sep 10, 2019 at 07:32:24PM -0700, speck for Pawan Gupta wrote:
> On Tue, Sep 10, 2019 at 11:33:34PM +0100, speck for Greg KH wrote:
> > On Tue, Sep 10, 2019 at 11:42:24AM -0700, Pawan Gupta wrote:
> > > On Fri, Sep 06, 2019 at 11:27:27AM +0200, speck for Greg KH wrote:
> > > > On Fri, Sep 06, 2019 at 12:28:35AM -0700, speck for Pawan Gupta wrote:
> > > > > > > +static int __init tsx_sysfs_init(void)
> > > > > > > +{
> > > > > > > +	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
> > > > > > > +		return -ENODEV;
> > > > > > > +
> > > > > > > +	return sysfs_create_file(&cpu_subsys.dev_root->kobj,
> > > > > > > +				 &dev_attr_tsx.attr);
> > > > > > 
> > > > > > Huge hint, if you have "driver" code calling a sysfs_* function,
> > > > > > something is really wrong.
> > > > > > 
> > > > > > This does not feel right, isn't there some attribute group that this
> > > > > > file should be assigned to instead of this "one off" file creation?
> > > > > 
> > > > > Yes there is an attribute group cpu_root_attrs (in drivers/base/cpu.c).
> > > > > Putting an arch specific attribute and supporting functions in a common
> > > > > file doesn't seem right either.
> > > > 
> > > > Then perhaps this isn't correct either as you are putting an
> > > > arch-specific attribute in a generic location :)
> > > 
> > > Firstly, sorry for taking your time.  If it makes your life easier I
> > > will be more than happy to contribute to backporting and testing this
> > > stuff.
> > 
> > That would be most appreciated.
> > 
> > > Adding tsx to cpu_subsys ends up at:
> > > /sys/devices/system/cpu/tsx
> > > 
> > > Which is where /sys/devices/system/cpu/vulnerabilities are.
> > > 
> > > I couldn't find a better place to put this.
> > 
> > $ ls /sys/devices/cpu/
> > allow_tsx_force_abort  format                      power      type
> > caps                   freeze_on_smi               rdpmc      uevent
> > events                 perf_event_mux_interval_ms  subsystem
> > 
> > Oh look, a tsx-specific cpu sysfs file on my laptop...
> 
> Yes, but these are PMU attributes.
> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/events/core.c#L2211
> https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/events/core.c#L1831

Ick, ok, then no, you are back to putting it in /sys/devices/system/cpu/
then.

But that "feels" wrong given there really isn't anything else in that
directory for cpu features like this, they are all buried in the
individual cpuX directories, right?

Is there any other cpu feature you can turn on/off like this today in
the system that is controlled in sysfs?  If so, where is it at?

thanks,

greg k-h

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Pawan Gupta]

On Mon, Sep 23, 2019 at 09:13:12PM +0200, speck for Greg KH wrote:
> > > 
> > > $ ls /sys/devices/cpu/
> > > allow_tsx_force_abort  format                      power      type
> > > caps                   freeze_on_smi               rdpmc      uevent
> > > events                 perf_event_mux_interval_ms  subsystem
> > > 
> > > Oh look, a tsx-specific cpu sysfs file on my laptop...
> > 
> > Yes, but these are PMU attributes.
> > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/events/core.c#L2211
> > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/events/core.c#L1831
> 
> Ick, ok, then no, you are back to putting it in /sys/devices/system/cpu/
> then.
> 
> But that "feels" wrong given there really isn't anything else in that
> directory for cpu features like this, they are all buried in the
> individual cpuX directories, right?
> 
> Is there any other cpu feature you can turn on/off like this today in
> the system that is controlled in sysfs?  If so, where is it at?

I don't know if it counts, but there is one for turning SMT on/off.

	$ echo off > /sys/devices/system/cpu/smt/control

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Greg KH]

On Mon, Sep 23, 2019 at 03:25:53PM -0700, speck for Pawan Gupta wrote:
> On Mon, Sep 23, 2019 at 09:13:12PM +0200, speck for Greg KH wrote:
> > > > 
> > > > $ ls /sys/devices/cpu/
> > > > allow_tsx_force_abort  format                      power      type
> > > > caps                   freeze_on_smi               rdpmc      uevent
> > > > events                 perf_event_mux_interval_ms  subsystem
> > > > 
> > > > Oh look, a tsx-specific cpu sysfs file on my laptop...
> > > 
> > > Yes, but these are PMU attributes.
> > > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/events/core.c#L2211
> > > https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/x86/events/core.c#L1831
> > 
> > Ick, ok, then no, you are back to putting it in /sys/devices/system/cpu/
> > then.
> > 
> > But that "feels" wrong given there really isn't anything else in that
> > directory for cpu features like this, they are all buried in the
> > individual cpuX directories, right?
> > 
> > Is there any other cpu feature you can turn on/off like this today in
> > the system that is controlled in sysfs?  If so, where is it at?
> 
> I don't know if it counts, but there is one for turning SMT on/off.
> 
> 	$ echo off > /sys/devices/system/cpu/smt/control

{sigh}

Yeah, it looks like you copied some of the logic for creating that file
(and that shouldn't be calling sysfs_create_group either, I'll add it to
my todo file...)

But again, those files are created from arch-independent code.  Is tsx
in any other processors and should it go into the same file where these
smt sysfs files are at, or is it really an Intel-only thing?

Either way, my original comment of "do not use sysfs_create_group() when
you have a 'struct device'" still stands, that needs to be fixed up no
matter what here.

greg k-h

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Jiri Kosina]

On Tue, 24 Sep 2019, speck for Greg KH wrote:

> But again, those files are created from arch-independent code.  Is tsx 
> in any other processors and should it go into the same file where these 
> smt sysfs files are at, or is it really an Intel-only thing?

FWIW there are other processors that support transactional memory (e.g 
powerpc), but it's not called TSX; that's Intel specific.

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Greg KH]

On Tue, Sep 24, 2019 at 12:48:02PM +0200, speck for Jiri Kosina wrote:
> On Tue, 24 Sep 2019, speck for Greg KH wrote:
> 
> > But again, those files are created from arch-independent code.  Is tsx 
> > in any other processors and should it go into the same file where these 
> > smt sysfs files are at, or is it really an Intel-only thing?
> 
> FWIW there are other processors that support transactional memory (e.g 
> powerpc), but it's not called TSX; that's Intel specific.

What does powerpc call it, and do they export the "turn it on or off"
attribute anywhere?

Is there a "generic" name for this type of processor feature, like SMT
is?

thanks,

greg k-h

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Jiri Kosina]

On Tue, 24 Sep 2019, speck for Greg KH wrote:

> > FWIW there are other processors that support transactional memory (e.g 
> > powerpc), but it's not called TSX; that's Intel specific.
> 
> What does powerpc call it, 

Hardware Trasnactional Memory (HTM).

> and do they export the "turn it on or off" attribute anywhere?

I think you can only do that using 'ppc_tm' kernel cmdline parameter 
during boot.

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Greg KH]

On Tue, Sep 24, 2019 at 03:38:59PM +0200, speck for Jiri Kosina wrote:
> On Tue, 24 Sep 2019, speck for Greg KH wrote:
> 
> > > FWIW there are other processors that support transactional memory (e.g 
> > > powerpc), but it's not called TSX; that's Intel specific.
> > 
> > What does powerpc call it, 
> 
> Hardware Trasnactional Memory (HTM).

Ok, nice, that's what should probably be used here in sysfs as well, as
this is the "generic" cpu location in the sysfs tree, right?

thanks,

greg k-h

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Pawan Gupta]

On Tue, Sep 24, 2019 at 07:04:59AM +0200, speck for Greg KH wrote:
> Either way, my original comment of "do not use sysfs_create_group() when
> you have a 'struct device'" still stands, that needs to be fixed up no
> matter what here.

Thanks for your inputs. As we have the 'struct device' are you
suggesting to use device_create_file()?

Approach 1 - sysfs_create_file() => device_create_file()

-----------------
diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
index 40fec8ab82b1..4799c081198c 100644
--- a/arch/x86/kernel/cpu/tsx.c
+++ b/arch/x86/kernel/cpu/tsx.c
@@ -162,7 +162,7 @@ static void tsx_update_on_each_cpu(bool val)
 	put_online_cpus();
 }
 
-static ssize_t tsx_show(struct device *cdev,
+static ssize_t htm_show(struct device *cdev,
 			struct device_attribute *attr,
 			char *buf)
 {
@@ -171,7 +171,7 @@ static ssize_t tsx_show(struct device *cdev,
 
 static DEFINE_MUTEX(tsx_mutex);
 
-static ssize_t tsx_store(struct device *cdev,
+static ssize_t htm_store(struct device *cdev,
 			 struct device_attribute *attr,
 			 const char *buf, size_t count)
 {
@@ -197,15 +197,14 @@ static ssize_t tsx_store(struct device *cdev,
 	return count;
 }
 
-static DEVICE_ATTR_RW(tsx);
+static const DEVICE_ATTR_RW(htm);
 
 static int __init tsx_sysfs_init(void)
 {
 	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
 		return -ENODEV;
 
-	return sysfs_create_file(&cpu_subsys.dev_root->kobj,
-				 &dev_attr_tsx.attr);
+	return device_create_file(cpu_subsys.dev_root, &dev_attr_htm);
 }
 
-late_initcall(tsx_sysfs_init);
+device_initcall(tsx_sysfs_init);



-------------------------------------------------------------------

If the first approach is not what you are expecting, is the below
patch to add the attribute to the cpu_root_attrs list okay?

Approach 2 - Add tsx attribute to cpu_root_attrs in drivers/base/cpu.c

--------------------
diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
index da75021daa1d..631996e129d5 100644
--- a/arch/x86/kernel/cpu/tsx.c
+++ b/arch/x86/kernel/cpu/tsx.c
@@ -158,17 +158,15 @@ static void tsx_update_on_each_cpu(bool val)
 	put_online_cpus();
 }
 
-static ssize_t tsx_show(struct device *cdev,
-			struct device_attribute *attr,
-			char *buf)
+ssize_t htm_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	return sprintf(buf, "%d\n", tsx_ctrl_state == TSX_CTRL_ENABLE ? 1 : 0);
 }
 
 static DEFINE_MUTEX(tsx_mutex);
 
-static ssize_t tsx_store(struct device *cdev, struct device_attribute *attr,
-			 const char *buf, size_t count)
+ssize_t htm_store(struct device *dev, struct device_attribute *attr,
+		  const char *buf, size_t count)
 {
 	enum tsx_ctrl_states requested_state;
 	ssize_t ret;
@@ -229,32 +227,10 @@ static ssize_t tsx_store(struct device *cdev, struct device_attribute *attr,
 	return count;
 }
 
-static DEVICE_ATTR_RW(tsx);
-
-static struct attribute *tsx_ctrl_attrs[] = {
-	&dev_attr_tsx.attr,
-	NULL
-};
-
-static umode_t tsx_ctrl_attr_is_visible(struct kobject *kobj,
-					struct attribute *attr, int index)
+umode_t htm_is_visible(void)
 {
-	if ((attr == &dev_attr_tsx.attr) &&
-	    (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED))
+	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
 		return 0;
 
-	return attr->mode;
+	return 0644;
 }
-
-static struct attribute_group tsx_ctrl_attr_group = {
-	.attrs		= tsx_ctrl_attrs,
-	.is_visible	= tsx_ctrl_attr_is_visible,
-};
-
-static int __init tsx_sysfs_init(void)
-{
-	return sysfs_create_group(&cpu_subsys.dev_root->kobj,
-				  &tsx_ctrl_attr_group);
-}
-
-late_initcall(tsx_sysfs_init);
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 80d43ff9a7ec..2b1579c7f22d 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -460,6 +460,34 @@ struct device *cpu_device_create(struct device *parent, void *drvdata,
 }
 EXPORT_SYMBOL_GPL(cpu_device_create);
 
+ssize_t __weak htm_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	return -ENODEV;
+}
+
+ssize_t __weak htm_store(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	return -ENODEV;
+}
+
+DEVICE_ATTR_RW(htm);
+
+umode_t __weak htm_is_visible(void)
+{
+	return 0;
+}
+
+static umode_t cpu_root_attrs_is_visible(struct kobject *kobj,
+					 struct attribute *attr, int index)
+{
+	if (attr == &dev_attr_htm.attr)
+		return htm_is_visible();
+
+	return attr->mode;
+}
+
 #ifdef CONFIG_GENERIC_CPU_AUTOPROBE
 static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL);
 #endif
@@ -481,11 +509,13 @@ static struct attribute *cpu_root_attrs[] = {
 #ifdef CONFIG_GENERIC_CPU_AUTOPROBE
 	&dev_attr_modalias.attr,
 #endif
+	&dev_attr_htm.attr,
 	NULL
 };
 
 static struct attribute_group cpu_root_attr_group = {
-	.attrs = cpu_root_attrs,
+	.attrs		= cpu_root_attrs,
+	.is_visible	= cpu_root_attrs_is_visible,
 };
 
 static const struct attribute_group *cpu_root_attr_groups[] = {

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Greg KH]

On Tue, Sep 24, 2019 at 04:25:03PM -0700, speck for Pawan Gupta wrote:
> On Tue, Sep 24, 2019 at 07:04:59AM +0200, speck for Greg KH wrote:
> > Either way, my original comment of "do not use sysfs_create_group() when
> > you have a 'struct device'" still stands, that needs to be fixed up no
> > matter what here.
> 
> Thanks for your inputs. As we have the 'struct device' are you
> suggesting to use device_create_file()?

You should never use sysfs_create_file() if you have a 'struct device'
so yes, that's at least one thing that needs to be fixed.

> Approach 1 - sysfs_create_file() => device_create_file()
> 
> -----------------
> diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> index 40fec8ab82b1..4799c081198c 100644
> --- a/arch/x86/kernel/cpu/tsx.c
> +++ b/arch/x86/kernel/cpu/tsx.c
> @@ -162,7 +162,7 @@ static void tsx_update_on_each_cpu(bool val)
>  	put_online_cpus();
>  }
>  
> -static ssize_t tsx_show(struct device *cdev,
> +static ssize_t htm_show(struct device *cdev,
>  			struct device_attribute *attr,
>  			char *buf)
>  {
> @@ -171,7 +171,7 @@ static ssize_t tsx_show(struct device *cdev,
>  
>  static DEFINE_MUTEX(tsx_mutex);
>  
> -static ssize_t tsx_store(struct device *cdev,
> +static ssize_t htm_store(struct device *cdev,
>  			 struct device_attribute *attr,
>  			 const char *buf, size_t count)
>  {
> @@ -197,15 +197,14 @@ static ssize_t tsx_store(struct device *cdev,
>  	return count;
>  }
>  
> -static DEVICE_ATTR_RW(tsx);
> +static const DEVICE_ATTR_RW(htm);
>  
>  static int __init tsx_sysfs_init(void)
>  {
>  	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
>  		return -ENODEV;
>  
> -	return sysfs_create_file(&cpu_subsys.dev_root->kobj,
> -				 &dev_attr_tsx.attr);
> +	return device_create_file(cpu_subsys.dev_root, &dev_attr_htm);
>  }
>  
> -late_initcall(tsx_sysfs_init);
> +device_initcall(tsx_sysfs_init);

You are renaming the file here too, but that's fine, it's not my file :)

> -------------------------------------------------------------------
> 
> If the first approach is not what you are expecting, is the below
> patch to add the attribute to the cpu_root_attrs list okay?
> 
> Approach 2 - Add tsx attribute to cpu_root_attrs in drivers/base/cpu.c
> 
> --------------------
> diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> index da75021daa1d..631996e129d5 100644
> --- a/arch/x86/kernel/cpu/tsx.c
> +++ b/arch/x86/kernel/cpu/tsx.c
> @@ -158,17 +158,15 @@ static void tsx_update_on_each_cpu(bool val)
>  	put_online_cpus();
>  }
>  
> -static ssize_t tsx_show(struct device *cdev,
> -			struct device_attribute *attr,
> -			char *buf)
> +ssize_t htm_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	return sprintf(buf, "%d\n", tsx_ctrl_state == TSX_CTRL_ENABLE ? 1 : 0);
>  }
>  
>  static DEFINE_MUTEX(tsx_mutex);
>  
> -static ssize_t tsx_store(struct device *cdev, struct device_attribute *attr,
> -			 const char *buf, size_t count)
> +ssize_t htm_store(struct device *dev, struct device_attribute *attr,
> +		  const char *buf, size_t count)
>  {
>  	enum tsx_ctrl_states requested_state;
>  	ssize_t ret;
> @@ -229,32 +227,10 @@ static ssize_t tsx_store(struct device *cdev, struct device_attribute *attr,
>  	return count;
>  }
>  
> -static DEVICE_ATTR_RW(tsx);
> -
> -static struct attribute *tsx_ctrl_attrs[] = {
> -	&dev_attr_tsx.attr,
> -	NULL
> -};
> -
> -static umode_t tsx_ctrl_attr_is_visible(struct kobject *kobj,
> -					struct attribute *attr, int index)
> +umode_t htm_is_visible(void)
>  {
> -	if ((attr == &dev_attr_tsx.attr) &&
> -	    (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED))
> +	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
>  		return 0;
>  
> -	return attr->mode;
> +	return 0644;
>  }
> -
> -static struct attribute_group tsx_ctrl_attr_group = {
> -	.attrs		= tsx_ctrl_attrs,
> -	.is_visible	= tsx_ctrl_attr_is_visible,
> -};
> -
> -static int __init tsx_sysfs_init(void)
> -{
> -	return sysfs_create_group(&cpu_subsys.dev_root->kobj,
> -				  &tsx_ctrl_attr_group);
> -}
> -
> -late_initcall(tsx_sysfs_init);
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index 80d43ff9a7ec..2b1579c7f22d 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -460,6 +460,34 @@ struct device *cpu_device_create(struct device *parent, void *drvdata,
>  }
>  EXPORT_SYMBOL_GPL(cpu_device_create);
>  
> +ssize_t __weak htm_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	return -ENODEV;
> +}
> +
> +ssize_t __weak htm_store(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	return -ENODEV;
> +}
> +
> +DEVICE_ATTR_RW(htm);
> +
> +umode_t __weak htm_is_visible(void)
> +{
> +	return 0;
> +}
> +
> +static umode_t cpu_root_attrs_is_visible(struct kobject *kobj,
> +					 struct attribute *attr, int index)
> +{
> +	if (attr == &dev_attr_htm.attr)
> +		return htm_is_visible();
> +
> +	return attr->mode;
> +}
> +
>  #ifdef CONFIG_GENERIC_CPU_AUTOPROBE
>  static DEVICE_ATTR(modalias, 0444, print_cpu_modalias, NULL);
>  #endif
> @@ -481,11 +509,13 @@ static struct attribute *cpu_root_attrs[] = {
>  #ifdef CONFIG_GENERIC_CPU_AUTOPROBE
>  	&dev_attr_modalias.attr,
>  #endif
> +	&dev_attr_htm.attr,
>  	NULL
>  };
>  
>  static struct attribute_group cpu_root_attr_group = {
> -	.attrs = cpu_root_attrs,
> +	.attrs		= cpu_root_attrs,
> +	.is_visible	= cpu_root_attrs_is_visible,
>  };
>  
>  static const struct attribute_group *cpu_root_attr_groups[] = {


I like this second one a _lot_ better.  You are creating the file at the
correct time, in the correct location, and handling if you should expose
the file or not to userspace correctly.  Nice work!

thanks,

greg k-h

[MODERATED] Re: [PATCH v4 06/10] TAAv4 6

[Kanth Ghatraju]

[-- Attachment #1: Type: text/plain, Size: 11413 bytes --]



> On Sep 3, 2019, at 5:16 PM, speck for Pawan Gupta <speck@linutronix.de> wrote:
> 
> From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Subject: [PATCH v4 06/10] x86/speculation/taa: Add mitigation for TSX Async
> Abort
> 
> TSX Async Abort (TAA) is a side channel attack on internal buffers in
> some Intel processors similar to Microachitectural Data Sampling (MDS).
> In this case certain loads may speculatively pass invalid data to
> dependent operations when an asynchronous abort condition is pending in
> a TSX transaction.  This includes loads with no fault or assist
> condition.  Such loads may speculatively expose stale data from the
> uarch data structures as in MDS.  Scope of exposure is within the
> same-thread and cross-thread.  This issue affects all current processors
> that support TSX.
> 
> On CPUs which have their IA32_ARCH_CAPABILITIES MSR bit MDS_NO=0 and the
> MDS mitigation is clearing the CPU buffers using VERW, there is no
> additional mitigation needed for TAA.
Could you please explicitly state that for the processors that enumerate MD_CLEAR and using the VERW instruction or L1D_FLUSH command to mitigate MDS, no additional mitigation is required. Thanks.
> 
> On affected CPUs with MDS_NO=1 this issue can be mitigated by disabling
> Transactional Synchronization Extensions (TSX) feature.  A new MSR
> IA32_TSX_CTRL in future and current processors after a microcode update
> can be used to control TSX feature.  TSX_CTRL_RTM_DISABLE bit disables
> the TSX sub-feature Restricted Transactional Memory (RTM).
> TSX_CTRL_CPUID_CLEAR bit clears the RTM enumeration in CPUID.  The other
> TSX sub-feature, Hardware Lock Elision (HLE), is unconditionally
> disabled but still enumerated as present by CPUID(EAX=7).EBX{bit4}.
> 
> The second mitigation approach is similar to MDS which is clearing the
> affected CPU buffers on return to user space and when entering a guest.
> Relevant microcode update is required for the mitigation to work.  More
> details on this approach can be found here:
> https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/mds.html
> 
> TSX feature can be controlled by the "tsx" command line parameter.  If
> the TSX feature is forced to be enabled then "Clear CPU buffers" (MDS
> mitigation) is deployed. The effective mitigation state can be read from
> sysfs.
> 
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> ---
> arch/x86/include/asm/cpufeatures.h   |   1 +
> arch/x86/include/asm/msr-index.h     |   4 +
> arch/x86/include/asm/nospec-branch.h |   4 +-
> arch/x86/include/asm/processor.h     |   7 ++
> arch/x86/kernel/cpu/bugs.c           | 110 ++++++++++++++++++++++++++-
> arch/x86/kernel/cpu/common.c         |   3 +
> 6 files changed, 125 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index e880f2408e29..138512ecc975 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -397,5 +397,6 @@
> #define X86_BUG_MDS			X86_BUG(19) /* CPU is affected by Microarchitectural data sampling */
> #define X86_BUG_MSBDS_ONLY		X86_BUG(20) /* CPU is only affected by the  MSDBS variant of BUG_MDS */
> #define X86_BUG_SWAPGS			X86_BUG(21) /* CPU is affected by speculation through SWAPGS */
> +#define X86_BUG_TAA			X86_BUG(22) /* CPU is affected by TSX Async Abort(TAA) */
> 
> #endif /* _ASM_X86_CPUFEATURES_H */
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 9163eb67962e..506056cb4db8 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -94,6 +94,10 @@
> 						  * Sampling (MDS) vulnerabilities.
> 						  */
> #define ARCH_CAP_TSX_CTRL_MSR		BIT(7)	/* MSR for TSX control is available. */
> +#define ARCH_CAP_TAA_NO			BIT(8)	/*
> +						 * Not susceptible to
> +						 * TSX Async Abort (TAA) vulnerabilities.
> +						 */
> 
> #define MSR_IA32_FLUSH_CMD		0x0000010b
> #define L1D_FLUSH			BIT(0)	/*
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index dbbf22be900b..def152e0cf42 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -314,7 +314,7 @@ DECLARE_STATIC_KEY_FALSE(verw_idle_clear);
> #include <asm/segment.h>
> 
> /**
> - * verw_clear_cpu_buffers - Mitigation for MDS vulnerability
> + * verw_clear_cpu_buffers - Mitigation for MDS and TAA vulnerability
>  *
>  * This uses the otherwise unused and obsolete VERW instruction in
>  * combination with microcode which triggers a CPU buffer flush when the
> @@ -337,7 +337,7 @@ static inline void verw_clear_cpu_buffers(void)
> }
> 
> /**
> - * verw_user_clear_cpu_buffers - Mitigation for MDS vulnerability
> + * verw_user_clear_cpu_buffers - Mitigation for MDS and TAA vulnerability
>  *
>  * Clear CPU buffers if the corresponding static key is enabled
>  */
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 6e0a3b43d027..999b85039128 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -988,4 +988,11 @@ enum mds_mitigations {
> 	MDS_MITIGATION_VMWERV,
> };
> 
> +enum taa_mitigations {
> +	TAA_MITIGATION_OFF,
> +	TAA_MITIGATION_UCODE_NEEDED,
> +	TAA_MITIGATION_VERW,
> +	TAA_MITIGATION_TSX_DISABLE,
> +};
> +
> #endif /* _ASM_X86_PROCESSOR_H */
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 3bb8564da271..1c0e670c3262 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -39,6 +39,7 @@ static void __init spectre_v2_select_mitigation(void);
> static void __init ssb_select_mitigation(void);
> static void __init l1tf_select_mitigation(void);
> static void __init mds_select_mitigation(void);
> +static void __init taa_select_mitigation(void);
> 
> /* The base value of the SPEC_CTRL MSR that always has to be preserved. */
> u64 x86_spec_ctrl_base;
> @@ -105,6 +106,7 @@ void __init check_bugs(void)
> 	ssb_select_mitigation();
> 	l1tf_select_mitigation();
> 	mds_select_mitigation();
> +	taa_select_mitigation();
> 
> 	arch_smt_update();
> 
> @@ -268,6 +270,94 @@ static int __init mds_cmdline(char *str)
> }
> early_param("mds", mds_cmdline);
> 
> +#undef pr_fmt
> +#define pr_fmt(fmt)	"TAA: " fmt
> +
> +/* Default mitigation for TAA-affected CPUs */
> +static enum taa_mitigations taa_mitigation __ro_after_init = TAA_MITIGATION_VERW;
> +static bool taa_nosmt __ro_after_init;
> +
> +static const char * const taa_strings[] = {
> +	[TAA_MITIGATION_OFF]		= "Vulnerable",
> +	[TAA_MITIGATION_UCODE_NEEDED]	= "Vulnerable: Clear CPU buffers attempted, no microcode",
> +	[TAA_MITIGATION_VERW]		= "Mitigation: Clear CPU buffers",
> +	[TAA_MITIGATION_TSX_DISABLE]	= "Mitigation: TSX disabled",
> +};
> +
> +static void __init taa_select_mitigation(void)
> +{
> +	u64 ia32_cap = 0;
> +
> +	/*
> +	 * Turn off TAA mitigation if X86_BUG_TAA was not set during arch setup
> +	 * or the global mitigation switch is off.
> +	 */
> +	if (!boot_cpu_has_bug(X86_BUG_TAA) || cpu_mitigations_off()) {
> +		taa_mitigation = TAA_MITIGATION_OFF;
> +		return;
> +	}
> +
> +	if (taa_mitigation == TAA_MITIGATION_OFF) {
> +		pr_info("%s\n", taa_strings[taa_mitigation]);
> +		return;
> +	}
> +
> +	/*
> +	 * TSX is supported by the hardware but was disabled during boot,
> +	 * select TSX_DISABLE as mitigation.
> +	 */
> +	if (!boot_cpu_has(X86_FEATURE_RTM)) {
> +		taa_mitigation = TAA_MITIGATION_TSX_DISABLE;
> +		pr_info("%s\n", taa_strings[taa_mitigation]);
> +		return;
> +	}
> +
> +	if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
> +		taa_mitigation = TAA_MITIGATION_VERW;
> +	else
> +		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> +
> +	if (boot_cpu_has(X86_FEATURE_ARCH_CAPABILITIES))
> +		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> +
> +	/*
> +	 * If CPU is not vulnerable to MDS, and TSX control is not supported,
> +	 * microcode update is required.
> +	 */
> +	if ((ia32_cap & ARCH_CAP_MDS_NO) &&
> +	    !(ia32_cap & ARCH_CAP_TSX_CTRL_MSR))
> +		taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> +
> +	/* Enable VERW static branch for CPU buffer clearing */
> +	static_branch_enable(&verw_user_clear);
> +
> +	if (taa_nosmt || cpu_mitigations_auto_nosmt())
> +		cpu_smt_disable(false);
> +
> +	pr_info("%s\n", taa_strings[taa_mitigation]);
> +}
> +
> +static int __init taa_cmdline(char *str)
> +{
> +	if (!boot_cpu_has_bug(X86_BUG_TAA))
> +		return 0;
> +
> +	if (!str)
> +		return -EINVAL;
> +
> +	if (!strcmp(str, "off")) {
> +		taa_mitigation = TAA_MITIGATION_OFF;
> +	} else if (!strcmp(str, "full")) {
> +		taa_mitigation = TAA_MITIGATION_VERW;
> +	} else if (!strcmp(str, "full,nosmt")) {
> +		taa_mitigation = TAA_MITIGATION_VERW;
> +		taa_nosmt = true;
> +	}
> +
> +	return 0;
> +}
> +early_param("taa", taa_cmdline);
> +
> #undef pr_fmt
> #define pr_fmt(fmt)     "Spectre V1 : " fmt
> 
> @@ -765,7 +855,7 @@ static void update_indir_branch_cond(void)
> #undef pr_fmt
> #define pr_fmt(fmt) fmt
> 
> -/* Update the static key controlling the MDS CPU buffer clear in idle */
> +/* Update the static key controlling the MDS and TAA CPU buffer clear in idle */
> static void update_verw_branch_idle(void)
> {
> 	/*
> @@ -775,8 +865,11 @@ static void update_verw_branch_idle(void)
> 	 * The other variants cannot be mitigated when SMT is enabled, so
> 	 * clearing the buffers on idle just to prevent the Store Buffer
> 	 * repartitioning leak would be a window dressing exercise.
> +	 *
> +	 * Apply idle buffer clearing to TAA affected CPUs also.
> 	 */
> -	if (!boot_cpu_has_bug(X86_BUG_MSBDS_ONLY))
> +	if (!boot_cpu_has_bug(X86_BUG_MSBDS_ONLY) &&
> +	    !boot_cpu_has_bug(X86_BUG_TAA))
> 		return;
> 
> 	if (sched_smt_active())
> @@ -786,6 +879,7 @@ static void update_verw_branch_idle(void)
> }
> 
> #define MDS_MSG_SMT "MDS CPU bug present and SMT on, data leak possible. See https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/mds.html for more details.\n"
> +#define TAA_MSG_SMT "TAA CPU bug present and SMT on, data leak possible. See https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/taa.html for more details.\n"
> 
> void arch_smt_update(void)
> {
> @@ -819,6 +913,18 @@ void arch_smt_update(void)
> 		break;
> 	}
> 
> +	switch (taa_mitigation) {
> +	case TAA_MITIGATION_VERW:
> +	case TAA_MITIGATION_UCODE_NEEDED:
> +		if (sched_smt_active())
> +			pr_warn_once(TAA_MSG_SMT);
> +		update_verw_branch_idle();
> +		break;
> +	case TAA_MITIGATION_TSX_DISABLE:
> +	case TAA_MITIGATION_OFF:
> +		break;
> +	}
> +
> 	mutex_unlock(&spec_ctrl_mutex);
> }
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index f125bf7ecb6f..1b24eca685d7 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1120,6 +1120,9 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
> 	if (!cpu_matches(NO_SWAPGS))
> 		setup_force_cpu_bug(X86_BUG_SWAPGS);
> 
> +	if (!(ia32_cap & ARCH_CAP_TAA_NO) && boot_cpu_has(X86_FEATURE_RTM))
> +		setup_force_cpu_bug(X86_BUG_TAA);
> +
> 	if (cpu_matches(NO_MELTDOWN))
> 		return;
> 
> --
> 2.20.1


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[MODERATED] [AUTOREPLY] [AUTOREPLY] Automatic reply: [PATCH v4 06/10] TAAv4 6

[Hatle, Mark]

Hi. I am no longer with Wind River. Please contact Stephanie.Moscrip@windriver.com with any questions. Thank you

[MODERATED] Re: [PATCH v4 06/10] TAAv4 6

[Pawan Gupta]

On Wed, Sep 25, 2019 at 05:10:54PM -0400, speck for Kanth Ghatraju wrote:
> 
> 
> > On Sep 3, 2019, at 5:16 PM, speck for Pawan Gupta <speck@linutronix.de> wrote:
> > 
> > From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > Subject: [PATCH v4 06/10] x86/speculation/taa: Add mitigation for TSX Async
> > Abort
> > 
> > TSX Async Abort (TAA) is a side channel attack on internal buffers in
> > some Intel processors similar to Microachitectural Data Sampling (MDS).
> > In this case certain loads may speculatively pass invalid data to
> > dependent operations when an asynchronous abort condition is pending in
> > a TSX transaction.  This includes loads with no fault or assist
> > condition.  Such loads may speculatively expose stale data from the
> > uarch data structures as in MDS.  Scope of exposure is within the
> > same-thread and cross-thread.  This issue affects all current processors
> > that support TSX.
> > 
> > On CPUs which have their IA32_ARCH_CAPABILITIES MSR bit MDS_NO=0 and the
> > MDS mitigation is clearing the CPU buffers using VERW, there is no
> > additional mitigation needed for TAA.
> Could you please explicitly state that for the processors that enumerate MD_CLEAR and using the VERW instruction or L1D_FLUSH command to mitigate MDS, no additional mitigation is required. Thanks.

L1D_FLUSH also clears the TAA affected CPU buffers, I will add this.

Thanks,
Pawan

[MODERATED] Re: ***UNCHECKED*** Re: [PATCH v4 03/10] TAAv4 3

[Josh Poimboeuf]

On Wed, Sep 04, 2019 at 01:27:58PM +0200, speck for Michal Hocko wrote:
> On Wed 04-09-19 10:43:06, speck for Greg KH wrote:
> > On Wed, Sep 04, 2019 at 12:58:47AM -0700, speck for Pawan Gupta wrote:
> > > On Wed, Sep 04, 2019 at 08:11:55AM +0200, speck for Greg KH wrote:
> > > > On Wed, Sep 04, 2019 at 08:01:47AM +0200, speck for Jiri Kosina wrote:
> > > > > On Wed, 4 Sep 2019, speck for Greg KH wrote:
> > > > > 
> > > > > > Did we ever have a reason to enable TSX?  I thought no one used it as it 
> > > > > > just didn't make any sense (as per Linus's old email about this)
> > > > > > 
> > > > > > Who will be turning this on?
> > > > > 
> > > > > We know about certain database vendor(s) who are somehow making use of TSX 
> > > > > for whatever reason, so having such a knob would be nice of us.
> > > > 
> > > > So they now need to explicitly enable it?  Would that not break their
> > > > existing systems by forcing them to do an additional startup step, or is
> > > > that somehow ok here?
> > > 
> > > For cases when the additional startup step is a problem, there is also
> > > a sysfs interface to control TSX after boot.
> > 
> > And that extra step could be a problem.  You had a machine that had TSX
> > enabled and the program used it.  Upgrade your kernel and now you have
> > to add an additional step in order to use this (command line or sysfs).
> > 
> > I am asking if this is going to be an issue for those systems that are
> > expecting a working TSX.
> 
> Indeed. I was expecting auto|on|off semantic like other knobs with auto
> preserving the current semantic (disable on broken models) and off to
> override.

In the previous version of this patch set, Thomas requested the default
be 'tsx=off', which is what Pawan has done now in this version.

But I agree with Greg, that would break existing workloads.  So I think
the default needs to be 'tsx=on', along with 'taa=full'.

-- 
Josh

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Pawan Gupta]

On Wed, Sep 25, 2019 at 05:05:15PM -0500, speck for Josh Poimboeuf wrote:
> On Wed, Sep 04, 2019 at 01:27:58PM +0200, speck for Michal Hocko wrote:
> > On Wed 04-09-19 10:43:06, speck for Greg KH wrote:
> > > On Wed, Sep 04, 2019 at 12:58:47AM -0700, speck for Pawan Gupta wrote:
> > > > On Wed, Sep 04, 2019 at 08:11:55AM +0200, speck for Greg KH wrote:
> > > > > On Wed, Sep 04, 2019 at 08:01:47AM +0200, speck for Jiri Kosina wrote:
> > > > > > On Wed, 4 Sep 2019, speck for Greg KH wrote:
> > > > > > 
> > > > > > > Did we ever have a reason to enable TSX?  I thought no one used it as it 
> > > > > > > just didn't make any sense (as per Linus's old email about this)
> > > > > > > 
> > > > > > > Who will be turning this on?
> > > > > > 
> > > > > > We know about certain database vendor(s) who are somehow making use of TSX 
> > > > > > for whatever reason, so having such a knob would be nice of us.
> > > > > 
> > > > > So they now need to explicitly enable it?  Would that not break their
> > > > > existing systems by forcing them to do an additional startup step, or is
> > > > > that somehow ok here?
> > > > 
> > > > For cases when the additional startup step is a problem, there is also
> > > > a sysfs interface to control TSX after boot.
> > > 
> > > And that extra step could be a problem.  You had a machine that had TSX
> > > enabled and the program used it.  Upgrade your kernel and now you have
> > > to add an additional step in order to use this (command line or sysfs).
> > > 
> > > I am asking if this is going to be an issue for those systems that are
> > > expecting a working TSX.
> > 
> > Indeed. I was expecting auto|on|off semantic like other knobs with auto
> > preserving the current semantic (disable on broken models) and off to
> > override.
> 
> In the previous version of this patch set, Thomas requested the default
> be 'tsx=off', which is what Pawan has done now in this version.
> 
> But I agree with Greg, that would break existing workloads.  So I think
> the default needs to be 'tsx=on', along with 'taa=full'.

Do we have a consensus on the default, tsx=off, tsx=on or tsx=auto?

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Borislav Petkov]

On Mon, Sep 30, 2019 at 05:20:56PM -0700, speck for Pawan Gupta wrote:
> Do we have a consensus on the default, tsx=off, tsx=on or tsx=auto?

Let's do the =on thing for now so that we can review v5 in the meantime.
We can always change that when tglx gets back and we all agree on the
default setting then.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 247165, AG München
-- 

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Pawan Gupta]

On Wed, Oct 02, 2019 at 04:55:30PM +0200, speck for Borislav Petkov wrote:
> On Mon, Sep 30, 2019 at 05:20:56PM -0700, speck for Pawan Gupta wrote:
> > Do we have a consensus on the default, tsx=off, tsx=on or tsx=auto?
> 
> Let's do the =on thing for now so that we can review v5 in the meantime.
> We can always change that when tglx gets back and we all agree on the
> default setting then.

v5 was already under test so keeping tsx=off.

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Josh Poimboeuf]

On Fri, Oct 04, 2019 at 10:16:53PM -0700, speck for Pawan Gupta wrote:
> On Wed, Oct 02, 2019 at 04:55:30PM +0200, speck for Borislav Petkov wrote:
> > On Mon, Sep 30, 2019 at 05:20:56PM -0700, speck for Pawan Gupta wrote:
> > > Do we have a consensus on the default, tsx=off, tsx=on or tsx=auto?
> > 
> > Let's do the =on thing for now so that we can review v5 in the meantime.
> > We can always change that when tglx gets back and we all agree on the
> > default setting then.
> 
> v5 was already under test so keeping tsx=off.

Do you plan to change the default to tsx=on for v6?

-- 
Josh

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Pawan Gupta]

On Mon, Oct 07, 2019 at 09:59:23PM -0500, speck for Josh Poimboeuf wrote:
> On Fri, Oct 04, 2019 at 10:16:53PM -0700, speck for Pawan Gupta wrote:
> > On Wed, Oct 02, 2019 at 04:55:30PM +0200, speck for Borislav Petkov wrote:
> > > On Mon, Sep 30, 2019 at 05:20:56PM -0700, speck for Pawan Gupta wrote:
> > > > Do we have a consensus on the default, tsx=off, tsx=on or tsx=auto?
> > > 
> > > Let's do the =on thing for now so that we can review v5 in the meantime.
> > > We can always change that when tglx gets back and we all agree on the
> > > default setting then.
> > 
> > v5 was already under test so keeping tsx=off.
> 
> Do you plan to change the default to tsx=on for v6?

If Thomas and others don't have an objection to tsx=on.

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On 10/7/19 7:59 PM, speck for Josh Poimboeuf wrote:
> On Fri, Oct 04, 2019 at 10:16:53PM -0700, speck for Pawan Gupta wrote:
>> On Wed, Oct 02, 2019 at 04:55:30PM +0200, speck for Borislav Petkov wrote:
>>> On Mon, Sep 30, 2019 at 05:20:56PM -0700, speck for Pawan Gupta wrote:
>>>> Do we have a consensus on the default, tsx=off, tsx=on or tsx=auto?
>>> Let's do the =on thing for now so that we can review v5 in the meantime.
>>> We can always change that when tglx gets back and we all agree on the
>>> default setting then.
>> v5 was already under test so keeping tsx=off.
> Do you plan to change the default to tsx=on for v6?

I'd be curious what the distributions plan on doing.  You folks can
obviously pick any default you want separately from mainline.  You're
also the ones that will get the first phone calls if it's set "wrong".

[MODERATED] Re: ***UNCHECKED*** Re: [PATCH v4 03/10] TAAv4 3

[Jiri Kosina]

On Tue, 8 Oct 2019, speck for Dave Hansen wrote:

> >> v5 was already under test so keeping tsx=3Doff.
> > Do you plan to change the default to tsx=3Don for v6?
> 
> I'd be curious what the distributions plan on doing.  You folks can
> obviously pick any default you want separately from mainline.  You're
> also the ones that will get the first phone calls if it's set "wrong".

As I said in some other mail, we're trying to pro-actively figure this out 
with some usual suspects among our customers, and we should know much more 
in the coming few days.

But generally, we always try to stay as close to mainline defaults as 
possible, for various reasons; so we might still provide some more input 
into this discussion in the coming days, once we actually do we a 
qualified opinion :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [PATCH v4 02/10] TAAv4 2

[Josh Poimboeuf]

On Fri, Sep 06, 2019 at 02:07:12PM -0700, speck for Dave Hansen wrote:
> On 9/4/19 12:43 AM, speck for Pawan Gupta wrote:
> > On Wed, Sep 04, 2019 at 07:54:06AM +0200, speck for Greg KH wrote:
> >>> +static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
> >>> +{
> >>> +	u64 ia32_cap = 0;
> >>> +
> >>> +	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES))
> >>> +		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> >>> +
> >>> +	if (!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR))
> >>> +		tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
> >> Why isn't this if statement under the if statement above?
> > Because we still have to set tsx_ctrl_state when cpu doesn't support
> > X86_FEATURE_ARCH_CAPABILITIES.
> 
> FWIW, I struggled to read this the first time I read it, too.  It might
> make sense to break it up like this:
> 
> u64 read_ia32_arch_cap(void)
> {
> 	u64 ia32_cap = 0;
> 
> 	/* Leave the MSR set to all 0's when not supported */
> 	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES))
> 		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> 
> 	return ia32_cap;
> }
> 
> You could even replace the other places that we read the MSR.  They have
> the same pattern.  Then do:
> 
> static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
> {
> 	u64 ia32_cap = read_ia32_arch_cap();
> 
> 	if (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)
> 		tsx_ctrl_state = TSX_CTRL_ENABLE;
> }
> 
> which takes tsx_ctrl_state out of the 'disable' mode.

I think it would be even clearer if the compile-time default were
changed to TSX_CTRL_NOT_SUPPORTED.

Then the logic would be less confusing.  For example the reader of the
code doesn't have to remember that ia32_cap is initialized to zero.  And
IMO, "not supported" conceptually makes the most sense as a compile-time
default anyway.

static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
{
	u64 ia32_cap;

	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES)) {
		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);

		if (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)
			tsx_ctrl_state = TSX_CTRL_DISABLE;
	}
}

-- 
Josh

[MODERATED] Re: [PATCH v4 02/10] TAAv4 2

[Pawan Gupta]

On Wed, Sep 25, 2019 at 05:13:19PM -0500, speck for Josh Poimboeuf wrote:
> On Fri, Sep 06, 2019 at 02:07:12PM -0700, speck for Dave Hansen wrote:
> > On 9/4/19 12:43 AM, speck for Pawan Gupta wrote:
> > > On Wed, Sep 04, 2019 at 07:54:06AM +0200, speck for Greg KH wrote:
> > >>> +static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
> > >>> +{
> > >>> +	u64 ia32_cap = 0;
> > >>> +
> > >>> +	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES))
> > >>> +		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> > >>> +
> > >>> +	if (!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR))
> > >>> +		tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
> > >> Why isn't this if statement under the if statement above?
> > > Because we still have to set tsx_ctrl_state when cpu doesn't support
> > > X86_FEATURE_ARCH_CAPABILITIES.
> > 
> > FWIW, I struggled to read this the first time I read it, too.  It might
> > make sense to break it up like this:
> > 
> > u64 read_ia32_arch_cap(void)
> > {
> > 	u64 ia32_cap = 0;
> > 
> > 	/* Leave the MSR set to all 0's when not supported */
> > 	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES))
> > 		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> > 
> > 	return ia32_cap;
> > }
> > 
> > You could even replace the other places that we read the MSR.  They have
> > the same pattern.  Then do:
> > 
> > static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
> > {
> > 	u64 ia32_cap = read_ia32_arch_cap();
> > 
> > 	if (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)
> > 		tsx_ctrl_state = TSX_CTRL_ENABLE;
> > }
> > 
> > which takes tsx_ctrl_state out of the 'disable' mode.
> 
> I think it would be even clearer if the compile-time default were
> changed to TSX_CTRL_NOT_SUPPORTED.
> 
> Then the logic would be less confusing.  For example the reader of the
> code doesn't have to remember that ia32_cap is initialized to zero.  And
> IMO, "not supported" conceptually makes the most sense as a compile-time
> default anyway.
> 
> static void tsx_ctrl_check_support(struct cpuinfo_x86 *c)
> {
> 	u64 ia32_cap;
> 
> 	if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES)) {
> 		rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap);
> 
> 		if (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)
> 			tsx_ctrl_state = TSX_CTRL_DISABLE;

Below is the order in which tsx_ctrl_state can be modified:
1. Compile time
2. Cmdline parsing (early_param())
3. TSX_CTRL MSR support check (tsx_ctrl_check_support())

Here we need to handle tsx=on case as well. If we set tsx_ctrl_state to
ENABLE/DISABLE in tsx_ctrl_check_support() we could override what user
asked for via cmdline. Unless we keep another copy of tsx_ctrl_state,
which might add more complexity.

I believe tsx_ctrl_check_support() should only change state to
NOT_SUPPORTED when TSX_CTRL MSR is not supported.

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 02/10] TAAv4 2

[Josh Poimboeuf]

On Tue, Sep 03, 2019 at 02:12:32PM -0700, speck for Pawan Gupta wrote:
> From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Subject: [PATCH v4 02/10] x86/tsx: Add TSX control initialization
> 
> Add a new file to host TSX feature controls.  TSX control has three

"Add a new file" makes it sound like a sysfs file rather than a .c file.

And it would probably be better to lead off with a description of the
*functional* change of the patch, rather than the not-so-interesting
aspect of adding a .c file.  For example, this patch disables TSX by
default.  That should also be reflected in the patch subject.

(This is of course pending the other discussion about what defaults make
sense.)

> valid states ENABLE, DISABLE and NOT_SUPPORTED.  TSX may be used on
> certain processors as part of a speculative side channel attack.

How is that relevant to this patch?

> Set the boot time default to DISABLE or NOT_SUPPORTED based on the
> presence of IA32_TSX_CTRL MSR.

This should say *why* it's changing the default to disabled, assuming
that's what we decide to do.

> +void tsx_init(struct cpuinfo_x86 *c)
> +{
> +	tsx_ctrl_check_support(c);
> +
> +	switch (tsx_ctrl_state) {
> +	case TSX_CTRL_DISABLE:
> +		tsx_disable();
> +		clear_cpu_cap(c, X86_FEATURE_RTM);
> +		setup_clear_cpu_cap(X86_FEATURE_RTM);

Can you clarify why setup_clear_cpu_cap() is needed?

AFAICT, setup_clear_cpu_cap() would be used for forcing a feature to be
cleared on all CPUs, but this function already calls clear_cpu_cap() for
every CPU anyway.

-- 
Josh

[MODERATED] Re: [PATCH v4 02/10] TAAv4 2

[Pawan Gupta]

On Wed, Sep 25, 2019 at 05:30:14PM -0500, speck for Josh Poimboeuf wrote:
> On Tue, Sep 03, 2019 at 02:12:32PM -0700, speck for Pawan Gupta wrote:
> > From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > Subject: [PATCH v4 02/10] x86/tsx: Add TSX control initialization
> > 
> > Add a new file to host TSX feature controls.  TSX control has three
> 
> "Add a new file" makes it sound like a sysfs file rather than a .c file.
> 
> And it would probably be better to lead off with a description of the
> *functional* change of the patch, rather than the not-so-interesting
> aspect of adding a .c file.  For example, this patch disables TSX by
> default.  That should also be reflected in the patch subject.
> 
> (This is of course pending the other discussion about what defaults make
> sense.)
> 
> > valid states ENABLE, DISABLE and NOT_SUPPORTED.  TSX may be used on
> > certain processors as part of a speculative side channel attack.
> 
> How is that relevant to this patch?

This is why we are defaulting to off.

> 
> > Set the boot time default to DISABLE or NOT_SUPPORTED based on the
> > presence of IA32_TSX_CTRL MSR.
> 
> This should say *why* it's changing the default to disabled, assuming
> that's what we decide to do.

Ok. Updating commit log to below:

    x86/tsx: Disable TSX by default
    
    Disable TSX by default on bootup. If IA32_TSX_CTRL MSR is not present,
    TSX state stays the compile time default which is NOT_SUPPORTED,
    otherwise change TSX state to DISABLE.  This is because on certain
    processsors TSX may be used as a part of a speculative side channel
    attack.

> 
> > +void tsx_init(struct cpuinfo_x86 *c)
> > +{
> > +	tsx_ctrl_check_support(c);
> > +
> > +	switch (tsx_ctrl_state) {
> > +	case TSX_CTRL_DISABLE:
> > +		tsx_disable();
> > +		clear_cpu_cap(c, X86_FEATURE_RTM);
> > +		setup_clear_cpu_cap(X86_FEATURE_RTM);
> 
> Can you clarify why setup_clear_cpu_cap() is needed?
> 
> AFAICT, setup_clear_cpu_cap() would be used for forcing a feature to be
> cleared on all CPUs, but this function already calls clear_cpu_cap() for
> every CPU anyway.

clear_cpu_cap() would update per-cpu capability bit for a particular CPU (used
by this_cpu_has()).  setup_clear_cpu_cap() updates the common (boot_cpu_data)
capability bit so that boot_cpu_has() tests are evaluated correctly. We need to
update both data structures.

Thanks,
Pawan

[MODERATED] [AUTOREPLY] [MODERATED] [AUTOREPLY] Automatic reply: [PATCH v4 02/10] TAAv4 2

[James, Hengameh M]

Hi,
Thank you for your email. I'm on a vacation from 9/4 through 9/18 and will be back to the office on the 19th.
Regards,
Hengameh

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Josh Poimboeuf]

On Fri, Sep 06, 2019 at 12:46:45AM -0700, speck for Pawan Gupta wrote:
> On Wed, Sep 04, 2019 at 05:19:43AM -0700, speck for Dave Hansen wrote:
> > > diff --git a/arch/x86/kernel/cpu/tsx.c b/arch/x86/kernel/cpu/tsx.c
> > > index e7b1fe929cab..cdfc6c3d11d1 100644
> > > --- a/arch/x86/kernel/cpu/tsx.c
> > > +++ b/arch/x86/kernel/cpu/tsx.c
> > > @@ -29,6 +29,18 @@ static void tsx_disable(void)
> > >  	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
> > >  }
> > >  
> > > +static void tsx_enable(void)
> > > +{
> > > +	u64 tsx;
> > > +
> > > +	rdmsrl(MSR_IA32_TSX_CTRL, tsx);
> > > +
> > > +	tsx &= ~MSR_TSX_CTRL_RTM_DISABLE;
> > > +	tsx &= ~MSR_TSX_CTRL_CPUID_CLEAR;
> > > +
> > > +	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
> > > +}
> > 
> > OK, so in the last patch we went through all the steps to enumerate this
> > sucker.  Is that still being respected here?
> 
> This doesn't affect the enumeration, only adds support for tsx=on case.
> 
> > 
> > Also, how would these bits have gotten set so that we need to clear them
> > here?  kexec?
> 
> Yes.

So you're saying we need to support the case where we booted with
tsx=off, but then want to kexec with tsx=on?

I don't know, that sounds a little esoteric to me.  Do we actually
support that type of scenario for other cmdline options?

> > > +static int __init tsx_cmdline(char *str)
> > > +{
> > > +	if (!str)
> > > +		return -EINVAL;
> > > +
> > > +	if (!strcmp(str, "on"))
> > > +		tsx_ctrl_state = TSX_CTRL_ENABLE;
> > > +	else if (!strcmp(str, "off"))
> > > +		tsx_ctrl_state = TSX_CTRL_DISABLE;
> > > +
> > > +	return 0;
> > > +}
> > > +early_param("tsx", tsx_cmdline);
> > 
> > Hmm...  Let's say I have a non-TSX system.  I specify tsx=on.  This will
> > set tsx_ctrl_state=TSX_CTRL_ENABLE, then tsx_ctrl_check_support() will
> > set it to tsx_ctrl_state=TSX_CTRL_NOT_SUPPORTED.
> > 
> > I *think* this all works, but I'd love a comment or two about it.  Maybe
> > even something that makes it clear that tsx_en/disable() are both only
> > called when TSX *AND* TSX_CTRL_MSR are supported.
> > 
> > This is all kinda weird and confusing because we're doing this all
> > without the X86_FEATURE bits for TSX.
> 
> If we add another X86_FEATURE bit it will have exact same state as
> X86_FEATURE_RTM.  Do we really need this?

How about moving the tsx_init() call to early_init_intel()?  Then you'd
be able to check the feature bit above.  And that might make the
ordering a little easier to follow.

-- 
Josh

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On 9/25/19 3:48 PM, speck for Josh Poimboeuf wrote:
>>>> +static void tsx_enable(void)
>>>> +{
>>>> +	u64 tsx;
>>>> +
>>>> +	rdmsrl(MSR_IA32_TSX_CTRL, tsx);
>>>> +
>>>> +	tsx &= ~MSR_TSX_CTRL_RTM_DISABLE;
>>>> +	tsx &= ~MSR_TSX_CTRL_CPUID_CLEAR;
>>>> +
>>>> +	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
>>>> +}
>>> OK, so in the last patch we went through all the steps to enumerate this
>>> sucker.  Is that still being respected here?
>> This doesn't affect the enumeration, only adds support for tsx=on case.
>>
>>> Also, how would these bits have gotten set so that we need to clear them
>>> here?  kexec?
>> Yes.
> So you're saying we need to support the case where we booted with
> tsx=off, but then want to kexec with tsx=on?
> 
> I don't know, that sounds a little esoteric to me.  Do we actually
> support that type of scenario for other cmdline options?

I can't think of another case where an earlier kernel's actions change a
later kernel's ability to enumerate CPU features.  We always start from
scratch in our enumeration.

I look at it this way: the new MSR_IA32_TSX_CTRL effectively brings new
enabling to enable TSX.  This patch effectively brings TSX in line with
other features, starting the enabling from scratch every time.

It's certainly a weird situation, though.

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On 26/09/2019 00:12, speck for Dave Hansen wrote:
> On 9/25/19 3:48 PM, speck for Josh Poimboeuf wrote:
>>>>> +static void tsx_enable(void)
>>>>> +{
>>>>> +	u64 tsx;
>>>>> +
>>>>> +	rdmsrl(MSR_IA32_TSX_CTRL, tsx);
>>>>> +
>>>>> +	tsx &= ~MSR_TSX_CTRL_RTM_DISABLE;
>>>>> +	tsx &= ~MSR_TSX_CTRL_CPUID_CLEAR;
>>>>> +
>>>>> +	wrmsrl(MSR_IA32_TSX_CTRL, tsx);
>>>>> +}
>>>> OK, so in the last patch we went through all the steps to enumerate this
>>>> sucker.  Is that still being respected here?
>>> This doesn't affect the enumeration, only adds support for tsx=on case.
>>>
>>>> Also, how would these bits have gotten set so that we need to clear them
>>>> here?  kexec?
>>> Yes.
>> So you're saying we need to support the case where we booted with
>> tsx=off, but then want to kexec with tsx=on?
>>
>> I don't know, that sounds a little esoteric to me.  Do we actually
>> support that type of scenario for other cmdline options?
> I can't think of another case where an earlier kernel's actions change a
> later kernel's ability to enumerate CPU features.

MISC_ENABLES.{LIMIT_CPUID,XD_DISABLE}

Any kernel with knowledge of this new TSX MSR should set it to a sane
state as part of coming up, whether it is transitioning out of the
firmware, or from a previous kernel.

~Andrew

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Pawan Gupta]

> > > > +static int __init tsx_cmdline(char *str)
> > > > +{
> > > > +	if (!str)
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (!strcmp(str, "on"))
> > > > +		tsx_ctrl_state = TSX_CTRL_ENABLE;
> > > > +	else if (!strcmp(str, "off"))
> > > > +		tsx_ctrl_state = TSX_CTRL_DISABLE;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +early_param("tsx", tsx_cmdline);
> > > 
> > > Hmm...  Let's say I have a non-TSX system.  I specify tsx=on.  This will
> > > set tsx_ctrl_state=TSX_CTRL_ENABLE, then tsx_ctrl_check_support() will
> > > set it to tsx_ctrl_state=TSX_CTRL_NOT_SUPPORTED.
> > > 
> > > I *think* this all works, but I'd love a comment or two about it.  Maybe
> > > even something that makes it clear that tsx_en/disable() are both only
> > > called when TSX *AND* TSX_CTRL_MSR are supported.
> > > 
> > > This is all kinda weird and confusing because we're doing this all
> > > without the X86_FEATURE bits for TSX.
> > 
> > If we add another X86_FEATURE bit it will have exact same state as
> > X86_FEATURE_RTM.  Do we really need this?
> 
> How about moving the tsx_init() call to early_init_intel()?  Then you'd
> be able to check the feature bit above.  And that might make the
> ordering a little easier to follow.

Sorry, I am failing to understand how it would make a difference.
early_init_intel() is also called from init_init().

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Josh Poimboeuf]

On Wed, Sep 25, 2019 at 06:13:05PM -0700, speck for Pawan Gupta wrote:
> > > > > +static int __init tsx_cmdline(char *str)
> > > > > +{
> > > > > +	if (!str)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if (!strcmp(str, "on"))
> > > > > +		tsx_ctrl_state = TSX_CTRL_ENABLE;
> > > > > +	else if (!strcmp(str, "off"))
> > > > > +		tsx_ctrl_state = TSX_CTRL_DISABLE;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +early_param("tsx", tsx_cmdline);
> > > > 
> > > > Hmm...  Let's say I have a non-TSX system.  I specify tsx=on.  This will
> > > > set tsx_ctrl_state=TSX_CTRL_ENABLE, then tsx_ctrl_check_support() will
> > > > set it to tsx_ctrl_state=TSX_CTRL_NOT_SUPPORTED.
> > > > 
> > > > I *think* this all works, but I'd love a comment or two about it.  Maybe
> > > > even something that makes it clear that tsx_en/disable() are both only
> > > > called when TSX *AND* TSX_CTRL_MSR are supported.
> > > > 
> > > > This is all kinda weird and confusing because we're doing this all
> > > > without the X86_FEATURE bits for TSX.
> > > 
> > > If we add another X86_FEATURE bit it will have exact same state as
> > > X86_FEATURE_RTM.  Do we really need this?
> > 
> > How about moving the tsx_init() call to early_init_intel()?  Then you'd
> > be able to check the feature bit above.  And that might make the
> > ordering a little easier to follow.
> 
> Sorry, I am failing to understand how it would make a difference.
> early_init_intel() is also called from init_init().

Regardless, shouldn't tsx_init() at least be called before
cpu_set_bug_bits(), so that X86_BUG_TAA can get set appropriately?

In that case, instead of early_param(), tsx_init() could just use
cmdline_find_option() to find the 'tsx=' option.  That would also have
the advantage of making it much easier to untangle the initialization
order.

If early_init_intel() isn't the right spot, then maybe
early_identify_cpu().

-- 
Josh

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Pawan Gupta]

On Wed, Sep 25, 2019 at 09:34:45PM -0500, speck for Josh Poimboeuf wrote:
> On Wed, Sep 25, 2019 at 06:13:05PM -0700, speck for Pawan Gupta wrote:
> > > > > > +static int __init tsx_cmdline(char *str)
> > > > > > +{
> > > > > > +	if (!str)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	if (!strcmp(str, "on"))
> > > > > > +		tsx_ctrl_state = TSX_CTRL_ENABLE;
> > > > > > +	else if (!strcmp(str, "off"))
> > > > > > +		tsx_ctrl_state = TSX_CTRL_DISABLE;
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +early_param("tsx", tsx_cmdline);
> > > > > 
> > > > > Hmm...  Let's say I have a non-TSX system.  I specify tsx=on.  This will
> > > > > set tsx_ctrl_state=TSX_CTRL_ENABLE, then tsx_ctrl_check_support() will
> > > > > set it to tsx_ctrl_state=TSX_CTRL_NOT_SUPPORTED.
> > > > > 
> > > > > I *think* this all works, but I'd love a comment or two about it.  Maybe
> > > > > even something that makes it clear that tsx_en/disable() are both only
> > > > > called when TSX *AND* TSX_CTRL_MSR are supported.
> > > > > 
> > > > > This is all kinda weird and confusing because we're doing this all
> > > > > without the X86_FEATURE bits for TSX.
> > > > 
> > > > If we add another X86_FEATURE bit it will have exact same state as
> > > > X86_FEATURE_RTM.  Do we really need this?
> > > 
> > > How about moving the tsx_init() call to early_init_intel()?  Then you'd
> > > be able to check the feature bit above.  And that might make the
> > > ordering a little easier to follow.
> > 
> > Sorry, I am failing to understand how it would make a difference.
> > early_init_intel() is also called from init_init().
> 
> Regardless, shouldn't tsx_init() at least be called before
> cpu_set_bug_bits(), so that X86_BUG_TAA can get set appropriately?

X86_BUG_TAA should be set even when tsx_init() disables TSX. This is to
indicate that the CPU has the bug but was mitigated by disabling TSX.
That is why we are calling cpu_set_bug_bits() first to set X86_BUG_TAA
and then disabling TSX in tsx_init(). Moreover cpu_set_bug_bits() is
called only by the boot-cpu, and tsx_init() is called per-cpu to disable
TSX on each CPU.

> In that case, instead of early_param(), tsx_init() could just use
> cmdline_find_option() to find the 'tsx=' option.  That would also have
> the advantage of making it much easier to untangle the initialization
> order.

Thanks for the suggestion. Does this look okay?

static enum tsx_ctrl_states {
	TSX_CTRL_ENABLE,
	TSX_CTRL_DISABLE,
	TSX_CTRL_NOT_SUPPORTED,
} tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;

static bool tsx_ctrl_is_supported(void)
{
	u64 ia32_cap = read_ia32_arch_cap();

	/*
	 * TSX is controlled via MSR_IA32_TSX_CTRL.  However,
	 * support for this MSR is enumerated by ARCH_CAP_TSX_MSR bit
	 * in MSR_IA32_ARCH_CAPABILITIES.
	 */
	return !!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR);
}

void tsx_init(struct cpuinfo_x86 *c)
{
	char arg[20];
	int ret;

	/* return if TSX_CTRL is not supported */
	if (!tsx_ctrl_is_supported())
		return;

	ret = cmdline_find_option(boot_command_line, "tsx", arg, sizeof(arg));
	if (ret >= 0) {
		if (!strcmp(arg, "on"))
			tsx_ctrl_state = TSX_CTRL_ENABLE;
		else if (!strcmp(arg, "off"))
			tsx_ctrl_state = TSX_CTRL_DISABLE;
		else
			pr_info("tsx: invalid option, defaulting to off\n");
	}

	/*
	 * If user provided an invalid option or tsx= is not provided on cmdline,
	 * default to TSX_CTRL_DISABLE. This is because on certain processors
	 * TSX may be used as part of a speculative side channel attack.
	 */
	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
		tsx_ctrl_state = TSX_CTRL_DISABLE;

	switch (tsx_ctrl_state) {
	case TSX_CTRL_DISABLE:
		tsx_disable();
		[...]

> If early_init_intel() isn't the right spot, then maybe
> early_identify_cpu().

early_identify_cpu() is called for other vendors as well, I think
init_intel() is the ideal place for calling an Intel specific function.
Why do we want to move tsx_init() from init_intel()?

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Josh Poimboeuf]

On Thu, Sep 26, 2019 at 12:15:55AM -0700, speck for Pawan Gupta wrote:
> > Regardless, shouldn't tsx_init() at least be called before
> > cpu_set_bug_bits(), so that X86_BUG_TAA can get set appropriately?
> 
> X86_BUG_TAA should be set even when tsx_init() disables TSX. This is to
> indicate that the CPU has the bug but was mitigated by disabling TSX.
> That is why we are calling cpu_set_bug_bits() first to set X86_BUG_TAA
> and then disabling TSX in tsx_init(). Moreover cpu_set_bug_bits() is
> called only by the boot-cpu, and tsx_init() is called per-cpu to disable
> TSX on each CPU.

Hm, I guess that makes sense.  But what if TSX enumeration is disabled
due to a previous kexec?  Then the CPUID feature bit (X86_FEATURE_RTM)
won't be set, and thus X86_BUG_TAA won't get set, right?

> > In that case, instead of early_param(), tsx_init() could just use
> > cmdline_find_option() to find the 'tsx=' option.  That would also have
> > the advantage of making it much easier to untangle the initialization
> > order.
> 
> Thanks for the suggestion. Does this look okay?
> 
> static enum tsx_ctrl_states {
> 	TSX_CTRL_ENABLE,
> 	TSX_CTRL_DISABLE,
> 	TSX_CTRL_NOT_SUPPORTED,
> } tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED;
> 
> static bool tsx_ctrl_is_supported(void)
> {
> 	u64 ia32_cap = read_ia32_arch_cap();
> 
> 	/*
> 	 * TSX is controlled via MSR_IA32_TSX_CTRL.  However,
> 	 * support for this MSR is enumerated by ARCH_CAP_TSX_MSR bit
> 	 * in MSR_IA32_ARCH_CAPABILITIES.
> 	 */
> 	return !!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR);
> }
> 
> void tsx_init(struct cpuinfo_x86 *c)
> {
> 	char arg[20];
> 	int ret;
> 
> 	/* return if TSX_CTRL is not supported */
> 	if (!tsx_ctrl_is_supported())
> 		return;
> 
> 	ret = cmdline_find_option(boot_command_line, "tsx", arg, sizeof(arg));
> 	if (ret >= 0) {
> 		if (!strcmp(arg, "on"))
> 			tsx_ctrl_state = TSX_CTRL_ENABLE;
> 		else if (!strcmp(arg, "off"))
> 			tsx_ctrl_state = TSX_CTRL_DISABLE;
> 		else
> 			pr_info("tsx: invalid option, defaulting to off\n");
> 	}
> 
> 	/*
> 	 * If user provided an invalid option or tsx= is not provided on cmdline,
> 	 * default to TSX_CTRL_DISABLE. This is because on certain processors
> 	 * TSX may be used as part of a speculative side channel attack.
> 	 */
> 	if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED)
> 		tsx_ctrl_state = TSX_CTRL_DISABLE;
> 
> 	switch (tsx_ctrl_state) {
> 	case TSX_CTRL_DISABLE:
> 		tsx_disable();
> 		[...]

Yeah, that does look better to me.

> > If early_init_intel() isn't the right spot, then maybe
> > early_identify_cpu().
> 
> early_identify_cpu() is called for other vendors as well, I think
> init_intel() is the ideal place for calling an Intel specific function.
> Why do we want to move tsx_init() from init_intel()?

IIUC, the fact that the X86_FEATURE_RTM enumeration can be removed
before kexec means that tsx_init() needs to run early, so that it can
properly detect the enumeration so that X86_BUG_TAA can get set
properly.

Since tsx_init() can clear X86_FEATURE_RTM even though the CPU supports
it, cpu_set_bug_bits() could check TSX_CTRL_NOT_SUPPORTED instead of
X86_FEATURE_RTM, like:

	if (cpu_has_tsx() && !(ia32_cap & ARCH_CAP_TAA_NO))
		setup_force_cpu_bug(X86_BUG_TAA);


where cpu_has_tsx() is just !TSX_CTRL_NOT_SUPPORTED.

-- 
Josh

[MODERATED] Re: [PATCH v4 03/10] TAAv4 3

[Pawan Gupta]

On Thu, Sep 26, 2019 at 08:54:09AM -0500, speck for Josh Poimboeuf wrote:
> On Thu, Sep 26, 2019 at 12:15:55AM -0700, speck for Pawan Gupta wrote:
> > > Regardless, shouldn't tsx_init() at least be called before
> > > cpu_set_bug_bits(), so that X86_BUG_TAA can get set appropriately?
> > 
> > X86_BUG_TAA should be set even when tsx_init() disables TSX. This is to
> > indicate that the CPU has the bug but was mitigated by disabling TSX.
> > That is why we are calling cpu_set_bug_bits() first to set X86_BUG_TAA
> > and then disabling TSX in tsx_init(). Moreover cpu_set_bug_bits() is
> > called only by the boot-cpu, and tsx_init() is called per-cpu to disable
> > TSX on each CPU.
> 
> Hm, I guess that makes sense.  But what if TSX enumeration is disabled
> due to a previous kexec?  Then the CPUID feature bit (X86_FEATURE_RTM)
> won't be set, and thus X86_BUG_TAA won't get set, right?

That is correct.

> > > If early_init_intel() isn't the right spot, then maybe
> > > early_identify_cpu().
> > 
> > early_identify_cpu() is called for other vendors as well, I think
> > init_intel() is the ideal place for calling an Intel specific function.
> > Why do we want to move tsx_init() from init_intel()?
> 
> IIUC, the fact that the X86_FEATURE_RTM enumeration can be removed
> before kexec means that tsx_init() needs to run early, so that it can
> properly detect the enumeration so that X86_BUG_TAA can get set
> properly.
> 
> Since tsx_init() can clear X86_FEATURE_RTM even though the CPU supports
> it, cpu_set_bug_bits() could check TSX_CTRL_NOT_SUPPORTED instead of
> X86_FEATURE_RTM, like:
> 
> 	if (cpu_has_tsx() && !(ia32_cap & ARCH_CAP_TAA_NO))
> 		setup_force_cpu_bug(X86_BUG_TAA);
> 
> 
> where cpu_has_tsx() is just !TSX_CTRL_NOT_SUPPORTED.

Yes we need something like this to address kexec like cases where TSX is
disabled before the kernel boot. We can probably add a check for
TSX_CTRL in cpu_set_bug_bits() as well.

cpu_set_bug_bits()
{
[...]
	/*
	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
	 *	- TSX is supported or
	 *	- TSX_CTRL is supported
	 *
	 * TSX_CTRL check is needed for cases when TSX could be disabled before
	 * the kernel boot e.g. kexec
	 * TSX_CTRL check alone is not sufficient for cases when the microcode
	 * update is not present; running as guest that don't get TSX_CTRL.
	 */
	if ((boot_cpu_has(X86_FEATURE_RTM) || (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)) &&
	    !(ia32_cap & ARCH_CAP_TAA_NO))
		setup_force_cpu_bug(X86_BUG_TAA);

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Josh Poimboeuf]

On Mon, Sep 09, 2019 at 08:42:54AM -0700, speck for Dave Hansen wrote:
> >>> +static void tsx_update_this_cpu(void *arg)
> >>> +{
> >>> +	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> >>> +	unsigned long enable = (unsigned long)arg;
> >>> +
> >>> +	if (enable) {
> >>> +		tsx_enable();
> >>> +		set_cpu_cap(c, X86_FEATURE_RTM);
> >>> +	} else {
> >>> +		tsx_disable();
> >>> +		clear_cpu_cap(c, X86_FEATURE_RTM);
> >>> +	}
> >>> +}
> >>
> >> This makes me really nervous.
> >>
> >> Do we have any other *runtime* (after early boot) manipulation of
> >> X86_FEATURE_* flags?
> > 
> > I don't think we have an option.  TSX_CTRL MSR write would change the
> > CPUID anyways.  It is probably better to have X86_FEATURE_RTM reflect
> > the correct state.
> 
> We have a choice.
> 
> If we don't have the infrastructure to do something right, we don't do
> it.  Poking at the bits and praying nothing breaks is not my preferred
> approach.  Could you please take a look and try to answer my question:
> 
> 	Do we have any other *runtime* (after early boot) manipulation
> 	of X86_FEATURE_* flags?
> 
> If the answer is no, then there's some homework that needs to be done to
> see what the implications are here and to make sure that nothing will
> break.  From the changelog and comments, it appears that research hasn't
> been done.

Right.  This patch feels dangerous and fragile...

And what's the point anyway?  There's hardly any justification in the
patch description.  Is it a theoretical problem or is there a real-world
production use case?

Please drop this patch, or at least, put it at the very end of the set.

-- 
Josh

[MODERATED] Re: [PATCH v4 04/10] TAAv4 4

[Pawan Gupta]

On Wed, Sep 25, 2019 at 05:58:31PM -0500, speck for Josh Poimboeuf wrote:
> On Mon, Sep 09, 2019 at 08:42:54AM -0700, speck for Dave Hansen wrote:
> > >>> +static void tsx_update_this_cpu(void *arg)
> > >>> +{
> > >>> +	struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> > >>> +	unsigned long enable = (unsigned long)arg;
> > >>> +
> > >>> +	if (enable) {
> > >>> +		tsx_enable();
> > >>> +		set_cpu_cap(c, X86_FEATURE_RTM);
> > >>> +	} else {
> > >>> +		tsx_disable();
> > >>> +		clear_cpu_cap(c, X86_FEATURE_RTM);
> > >>> +	}
> > >>> +}
> > >>
> > >> This makes me really nervous.
> > >>
> > >> Do we have any other *runtime* (after early boot) manipulation of
> > >> X86_FEATURE_* flags?
> > > 
> > > I don't think we have an option.  TSX_CTRL MSR write would change the
> > > CPUID anyways.  It is probably better to have X86_FEATURE_RTM reflect
> > > the correct state.
> > 
> > We have a choice.
> > 
> > If we don't have the infrastructure to do something right, we don't do
> > it.  Poking at the bits and praying nothing breaks is not my preferred
> > approach.  Could you please take a look and try to answer my question:
> > 
> > 	Do we have any other *runtime* (after early boot) manipulation
> > 	of X86_FEATURE_* flags?
> > 
> > If the answer is no, then there's some homework that needs to be done to
> > see what the implications are here and to make sure that nothing will
> > break.  From the changelog and comments, it appears that research hasn't
> > been done.
> 
> Right.  This patch feels dangerous and fragile...
> 
> And what's the point anyway?  There's hardly any justification in the
> patch description.  Is it a theoretical problem or is there a real-world
> production use case?
> 
> Please drop this patch, or at least, put it at the very end of the set.

Updating the changelog and moving this to the end.

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 06/10] TAAv4 6

[Josh Poimboeuf]

On Tue, Sep 03, 2019 at 02:16:33PM -0700, speck for Pawan Gupta wrote:
> +static int __init taa_cmdline(char *str)
> +{
> +	if (!boot_cpu_has_bug(X86_BUG_TAA))
> +		return 0;
> +
> +	if (!str)
> +		return -EINVAL;
> +
> +	if (!strcmp(str, "off")) {
> +		taa_mitigation = TAA_MITIGATION_OFF;
> +	} else if (!strcmp(str, "full")) {
> +		taa_mitigation = TAA_MITIGATION_VERW;
> +	} else if (!strcmp(str, "full,nosmt")) {
> +		taa_mitigation = TAA_MITIGATION_VERW;
> +		taa_nosmt = true;
> +	}
> +
> +	return 0;
> +}
> +early_param("taa", taa_cmdline);

Should the cmdline option be called "tsx_async_abort" for consistency
with the sysfs file name?

-- 
Josh

[MODERATED] Re: [PATCH v4 06/10] TAAv4 6

[Pawan Gupta]

> > +static int __init taa_cmdline(char *str)
> > +{
> > +	if (!boot_cpu_has_bug(X86_BUG_TAA))
> > +		return 0;
> > +
> > +	if (!str)
> > +		return -EINVAL;
> > +
> > +	if (!strcmp(str, "off")) {
> > +		taa_mitigation = TAA_MITIGATION_OFF;
> > +	} else if (!strcmp(str, "full")) {
> > +		taa_mitigation = TAA_MITIGATION_VERW;
> > +	} else if (!strcmp(str, "full,nosmt")) {
> > +		taa_mitigation = TAA_MITIGATION_VERW;
> > +		taa_nosmt = true;
> > +	}
> > +
> > +	return 0;
> > +}
> > +early_param("taa", taa_cmdline);
> 
> Should the cmdline option be called "tsx_async_abort" for consistency
> with the sysfs file name?

Yes, will change cmdline parameter to tsx_async_abort.

Thanks,
Pawan

[MODERATED] Re: [PATCH v4 05/10] TAAv4 5

[Pawan Gupta]

On Tue, Sep 03, 2019 at 02:15:33PM -0700, speck for Pawan Gupta wrote:
> From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Subject: [PATCH v4 05/10] x86/speculation/mds: Rename MDS buffer clear
>  functions
> 
> In preparation for reusing the MDS cpu buffer clear functions and static
> keys by the following commits, rename them from mds_* --> verw_* to make
> them more generic.  VERW is a legacy cpu instruction which was
> overloaded to clear the CPU buffers for MDS.
> 
> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> ---
>  Documentation/x86/mds.rst            |  6 +++---
>  arch/x86/entry/common.c              |  2 +-
>  arch/x86/include/asm/irqflags.h      |  4 ++--
>  arch/x86/include/asm/mwait.h         |  4 ++--
>  arch/x86/include/asm/nospec-branch.h | 24 ++++++++++++------------
>  arch/x86/kernel/cpu/bugs.c           | 22 +++++++++++-----------
>  arch/x86/kernel/nmi.c                |  2 +-
>  arch/x86/kvm/vmx/vmx.c               |  4 ++--
>  8 files changed, 34 insertions(+), 34 deletions(-)

This patch (s/mds/verw/) doesn't add much value compared to the amount
of changes needed. It also doesn't bring any functional change. Dropping
this patch in favor of below comment.

taa_select_mitigation()
{
	[...]
	/*
	 * TSX is enabled, select alternate mitigation for TAA which is
	 * same as MDS. Enable MDS static branch to clear CPU buffers.
	 *
	 * For guests that can't determine whether the correct microcode is
	 * present on host, enable the mitigation for UCODE_NEEDED as well.
	 */
	static_branch_enable(&mds_user_clear);

Thanks,
Pawan