Messages in this thread | | | Subject | Re: x86: CR4 update when IRQs are enabled | From | Nadav Amit <> | Date | Wed, 15 Nov 2017 16:34:39 -0800 |
| |
hpa@zytor.com wrote:
> On November 15, 2017 3:31:50 PM PST, Nadav Amit <nadav.amit@gmail.com> wrote: >> Ping? >> >> Nadav Amit <nadav.amit@gmail.com> wrote: >> >>> CC’ing 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: >>>> >>>> /* >>>> * Access to this CR4 shadow and to H/W CR4 is protected by >>>> * disabling interrupts when modifying either one. >>>> */ >>>> >>>> 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. >>>> >>>> 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. >>> >>> -- >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()); >>> } >>> >>> +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 = this_cpu_read(cpu_tlbstate.cr4); >>> if ((cr4 | mask) != cr4) { >>> cr4 |= mask; >>> - this_cpu_write(cpu_tlbstate.cr4, cr4); >>> - __write_cr4(cr4); >>> + update_cr4(cr4); >>> } >>> } >>> >>> @@ -110,8 +118,7 @@ static inline void cr4_clear_bits(unsigned long >> mask) >>> cr4 = this_cpu_read(cpu_tlbstate.cr4); >>> if ((cr4 & ~mask) != cr4) { >>> cr4 &= ~mask; >>> - this_cpu_write(cpu_tlbstate.cr4, cr4); >>> - __write_cr4(cr4); >>> + update_cr4(cr4); >>> } >>> } >>> >>> @@ -121,8 +128,7 @@ static inline void cr4_toggle_bits(unsigned long >> mask) >>> cr4 = this_cpu_read(cpu_tlbstate.cr4); >>> cr4 ^= mask; >>> - this_cpu_write(cpu_tlbstate.cr4, cr4); >>> - __write_cr4(cr4); >>> + update_cr4(cr4); >>> } >>> >>> /* 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; >>> >>> 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; >>> >>> + 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; >>> >>> c->loops_per_jiffy = 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); >>> >>> - /* 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); >>> >>> /* >>> * 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 = 0; >>> mce_banks_t all_banks; >>> + unsigned long flags; >>> u64 cap; >>> >>> 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); >>> >>> + local_irq_save(flags); >>> cr4_set_bits(X86_CR4_MCE); >>> + local_irq_restore(flags); >>> >>> 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; >>> >>> /* 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"); >>> >>> /* 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; >>> >>> machine_check_vector = winchip_machine_check; >>> @@ -37,7 +38,9 @@ void winchip_mcheck_init(struct cpuinfo_x86 *c) >>> lo &= ~(1<<4); /* Enable MCE */ >>> wrmsr(MSR_IDT_FCR1, lo, hi); >>> >>> + local_irq_save(flags); >>> cr4_set_bits(X86_CR4_MCE); >>> + local_irq_restore(flags); >>> >>> 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) >>> >>> 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(); >>> } >>> >>> 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(); >>> } > > This is wrong on at least two levels colon first of all, this should not be wrapped around the abstracted operations but put inside them if relevant. Second, I suspect that this is not at all a requirement but rather that as long as the hardware register is written second, I think we should always be safe.
Thanks for your reply.
Can you please explain your suspicion? Here is a simple execution that may fail (assume cr4 is initially zero):
CPU0 ==== cr4_set_bits(1) => cpu_tlbstate.cr4 = 1 => interrupt cr4_set_bits(2) cpu_tlbstate.cr4 = 3 __write_cr4(3) => __write_cr4(1) [ and should have been 3 ]
Am I missing anything?
Now, it may be a theoretical issue right now, since I did not find any change of CR4 bits that happens in an interrupt handler.
Anyhow, if you want me to submit a fix, please let me know what other levels of wrongness you had in mind.
Regards, Nadav
| |