Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: x86: CR4 update when IRQs are enabled
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <E7DBFC7E-3904-483A-89E1-56EC17F14EF1@gmail.com>
Date:   Wed, 8 Nov 2017 22:29:34 -0800
Cc:     LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <2DE32618-CB44-43C7-BAFB-F6967248F97C@gmail.com>
References: <E7DBFC7E-3904-483A-89E1-56EC17F14EF1@gmail.com>
To:     Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Borislav Petkov <bp@alien8.de>,
        the arch/x86 maintainers <x86@kernel.org>,
        Andy Lutomirski <luto@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

CC=E2=80=99ing more people, and adding a patch to clarify...

Nadav Amit <nadav.amit@gmail.com> wrote:

> I am puzzled by the comment in tlb_state.cr4 , which says:
>=20
>        /*
>         * Access to this CR4 shadow and to H/W CR4 is protected by
>         * disabling interrupts when modifying either one.
>         */
>=20
> This does not seem to be true and adding a warning to CR4 writes when
> !irqs_disabled() immediately fires in identify_cpu() and
> __mcheck_cpu_init_generic(). While these two are called during boot, I =
think
> there other CR4 changes with enabled IRQs, for example, PR_SET_TSC.
>=20
> So my question(s): Is the comment correct? Is the current behavior =
correct?

So here is what I have in mind. I am not sure whether =
CONFIG_DEBUG_PREEMPT is
the right #ifdef. Let me know what you think.
=20
-- >8 --

Subject: [PATCH] x86: disable IRQs before changing CR4

CR4 changes need to be performed while IRQs are disabled in order to
update the CR4 shadow and the actual register atomically.

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 arch/x86/include/asm/tlbflush.h      | 18 ++++++++++++------
 arch/x86/kernel/cpu/common.c         | 13 ++++++++++++-
 arch/x86/kernel/cpu/mcheck/mce.c     |  3 +++
 arch/x86/kernel/cpu/mcheck/p5.c      |  4 ++++
 arch/x86/kernel/cpu/mcheck/winchip.c |  3 +++
 arch/x86/kernel/process.c            | 14 ++++++++++++--
 6 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/tlbflush.h =
b/arch/x86/include/asm/tlbflush.h
index 50ea3482e1d1..bc70dd1cc7c6 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -89,6 +89,15 @@ static inline void cr4_init_shadow(void)
 	this_cpu_write(cpu_tlbstate.cr4, __read_cr4());
 }
=20
+static inline void update_cr4(unsigned long cr4)
+{
+#ifdef CONFIG_DEBUG_PREEMPT
+	WARN_ON_ONCE(!irqs_disabled());
+#endif
+	this_cpu_write(cpu_tlbstate.cr4, cr4);
+	__write_cr4(cr4);
+}
+
 /* Set in this cpu's CR4. */
 static inline void cr4_set_bits(unsigned long mask)
 {
@@ -97,8 +106,7 @@ static inline void cr4_set_bits(unsigned long mask)
 	cr4 =3D this_cpu_read(cpu_tlbstate.cr4);
 	if ((cr4 | mask) !=3D cr4) {
 		cr4 |=3D mask;
-		this_cpu_write(cpu_tlbstate.cr4, cr4);
-		__write_cr4(cr4);
+		update_cr4(cr4);
 	}
 }
=20
@@ -110,8 +118,7 @@ static inline void cr4_clear_bits(unsigned long =
mask)
 	cr4 =3D this_cpu_read(cpu_tlbstate.cr4);
 	if ((cr4 & ~mask) !=3D cr4) {
 		cr4 &=3D ~mask;
-		this_cpu_write(cpu_tlbstate.cr4, cr4);
-		__write_cr4(cr4);
+		update_cr4(cr4);
 	}
 }
=20
@@ -121,8 +128,7 @@ static inline void cr4_toggle_bits(unsigned long =
mask)
=20
 	cr4 =3D this_cpu_read(cpu_tlbstate.cr4);
 	cr4 ^=3D mask;
-	this_cpu_write(cpu_tlbstate.cr4, cr4);
-	__write_cr4(cr4);
+	update_cr4(cr4);
 }
=20
 /* Read the CR4 shadow. */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index c8b39870f33e..82e6b41fd5e9 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -318,6 +318,8 @@ static bool pku_disabled;
=20
 static __always_inline void setup_pku(struct cpuinfo_x86 *c)
 {
+	unsigned long flags;
+
 	/* check the boot processor, plus compile options for PKU: */
 	if (!cpu_feature_enabled(X86_FEATURE_PKU))
 		return;
@@ -327,7 +329,10 @@ static __always_inline void setup_pku(struct =
cpuinfo_x86 *c)
 	if (pku_disabled)
 		return;
=20
+	local_irq_save(flags);
 	cr4_set_bits(X86_CR4_PKE);
+	local_irq_restore(flags);
+
 	/*
 	 * Seting X86_CR4_PKE will cause the X86_FEATURE_OSPKE
 	 * cpuid bit to be set.  We need to ensure that we
@@ -1069,6 +1074,7 @@ static void validate_apic_and_package_id(struct =
cpuinfo_x86 *c)
  */
 static void identify_cpu(struct cpuinfo_x86 *c)
 {
+	unsigned long flags;
 	int i;
=20
 	c->loops_per_jiffy =3D loops_per_jiffy;
@@ -1121,9 +1127,14 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	/* Disable the PN if appropriate */
 	squash_the_stupid_serial_number(c);
=20
-	/* Set up SMEP/SMAP */
+	/*
+	 * Set up SMEP/SMAP. Disable interrupts to prevent triggering a =
warning
+	 * as CR4 changes must be done with disabled interrupts.
+	 */
+	local_irq_save(flags);
 	setup_smep(c);
 	setup_smap(c);
+	local_irq_restore(flags);
=20
 	/*
 	 * The vendor-specific functions might have changed features.
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c =
b/arch/x86/kernel/cpu/mcheck/mce.c
index 3b413065c613..497c07e33c25 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1508,6 +1508,7 @@ static void __mcheck_cpu_init_generic(void)
 {
 	enum mcp_flags m_fl =3D 0;
 	mce_banks_t all_banks;
+	unsigned long flags;
 	u64 cap;
=20
 	if (!mca_cfg.bootlog)
@@ -1519,7 +1520,9 @@ static void __mcheck_cpu_init_generic(void)
 	bitmap_fill(all_banks, MAX_NR_BANKS);
 	machine_check_poll(MCP_UC | m_fl, &all_banks);
=20
+	local_irq_save(flags);
 	cr4_set_bits(X86_CR4_MCE);
+	local_irq_restore(flags);
=20
 	rdmsrl(MSR_IA32_MCG_CAP, cap);
 	if (cap & MCG_CTL_P)
diff --git a/arch/x86/kernel/cpu/mcheck/p5.c =
b/arch/x86/kernel/cpu/mcheck/p5.c
index 2a0717bf8033..d5d4963415e9 100644
--- a/arch/x86/kernel/cpu/mcheck/p5.c
+++ b/arch/x86/kernel/cpu/mcheck/p5.c
@@ -42,6 +42,7 @@ static void pentium_machine_check(struct pt_regs =
*regs, long error_code)
 /* Set up machine check reporting for processors with Intel style MCE: =
*/
 void intel_p5_mcheck_init(struct cpuinfo_x86 *c)
 {
+	unsigned long flags;
 	u32 l, h;
=20
 	/* Default P5 to off as its often misconnected: */
@@ -62,7 +63,10 @@ void intel_p5_mcheck_init(struct cpuinfo_x86 *c)
 	pr_info("Intel old style machine check architecture =
supported.\n");
=20
 	/* Enable MCE: */
+	local_irq_save(flags);
 	cr4_set_bits(X86_CR4_MCE);
+	local_irq_restore(flags);
+
 	pr_info("Intel old style machine check reporting enabled on =
CPU#%d.\n",
 		smp_processor_id());
 }
diff --git a/arch/x86/kernel/cpu/mcheck/winchip.c =
b/arch/x86/kernel/cpu/mcheck/winchip.c
index c6a722e1d011..6dd985e3849d 100644
--- a/arch/x86/kernel/cpu/mcheck/winchip.c
+++ b/arch/x86/kernel/cpu/mcheck/winchip.c
@@ -26,6 +26,7 @@ static void winchip_machine_check(struct pt_regs =
*regs, long error_code)
 /* Set up machine check reporting on the Winchip C6 series */
 void winchip_mcheck_init(struct cpuinfo_x86 *c)
 {
+	unsigned long flags;
 	u32 lo, hi;
=20
 	machine_check_vector =3D winchip_machine_check;
@@ -37,7 +38,9 @@ void winchip_mcheck_init(struct cpuinfo_x86 *c)
 	lo &=3D ~(1<<4);	/* Enable MCE */
 	wrmsr(MSR_IDT_FCR1, lo, hi);
=20
+	local_irq_save(flags);
 	cr4_set_bits(X86_CR4_MCE);
+	local_irq_restore(flags);
=20
 	pr_info("Winchip machine check reporting enabled on CPU#0.\n");
 }
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 3ca198080ea9..09e44e784745 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -127,25 +127,35 @@ void flush_thread(void)
=20
 void disable_TSC(void)
 {
+	unsigned long flags;
+
 	preempt_disable();
-	if (!test_and_set_thread_flag(TIF_NOTSC))
+	if (!test_and_set_thread_flag(TIF_NOTSC)) {
 		/*
 		 * Must flip the CPU state synchronously with
 		 * TIF_NOTSC in the current running context.
 		 */
+		local_irq_save(flags);
 		cr4_set_bits(X86_CR4_TSD);
+		local_irq_restore(flags);
+	}
 	preempt_enable();
 }
=20
 static void enable_TSC(void)
 {
+	unsigned long flags;
+
 	preempt_disable();
-	if (test_and_clear_thread_flag(TIF_NOTSC))
+	if (test_and_clear_thread_flag(TIF_NOTSC)) {
 		/*
 		 * Must flip the CPU state synchronously with
 		 * TIF_NOTSC in the current running context.
 		 */
+		local_irq_save(flags);
 		cr4_clear_bits(X86_CR4_TSD);
+		local_irq_restore(flags);
+	}
 	preempt_enable();
 }


From 1583536401885830151@xxx Wed Nov 08 21:53:15 +0000 2017
X-GM-THRID: 1583536401885830151
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread