Re: x86: CR4 update when IRQs are enabled

[Andy Lutomirski]

I think Nadav is right here.  IMO the right fix is to rename the
functions cr4_set_bits_irqs_off() etc, add a warning (if lockdep is
on) and fix the callers.

On Wed, Nov 15, 2017 at 4:34 PM, Nadav Amit  wrote:
> h...@zytor.com wrote:
>
>> On November 15, 2017 3:31:50 PM PST, Nadav Amit  wrote:
>>> Ping?
>>>
>>> Nadav Amit  wrote:
>>>
 CC’ing more people, and adding a patch to clarify...

 Nadav Amit  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 
 ---
 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);

 

Re: x86: CR4 update when IRQs are enabled

[Andy Lutomirski]

I think Nadav is right here.  IMO the right fix is to rename the
functions cr4_set_bits_irqs_off() etc, add a warning (if lockdep is
on) and fix the callers.

On Wed, Nov 15, 2017 at 4:34 PM, Nadav Amit  wrote:
> h...@zytor.com wrote:
>
>> On November 15, 2017 3:31:50 PM PST, Nadav Amit  wrote:
>>> Ping?
>>>
>>> Nadav Amit  wrote:
>>>
 CC’ing more people, and adding a patch to clarify...

 Nadav Amit  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 
 ---
 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 

Re: x86: CR4 update when IRQs are enabled

[Nadav Amit]

h...@zytor.com wrote:

> On November 15, 2017 3:31:50 PM PST, Nadav Amit  wrote:
>> Ping?
>> 
>> Nadav Amit  wrote:
>> 
>>> CC’ing more people, and adding a patch to clarify...
>>> 
>>> Nadav Amit  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 
>>> ---
>>> 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 

Re: x86: CR4 update when IRQs are enabled

[Nadav Amit]

h...@zytor.com wrote:

> On November 15, 2017 3:31:50 PM PST, Nadav Amit  wrote:
>> Ping?
>> 
>> Nadav Amit  wrote:
>> 
>>> CC’ing more people, and adding a patch to clarify...
>>> 
>>> Nadav Amit  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 
>>> ---
>>> 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 

Re: x86: CR4 update when IRQs are enabled

[hpa]

On November 15, 2017 3:31:50 PM PST, Nadav Amit  wrote:
>Ping?
>
>Nadav Amit  wrote:
>
>> CC’ing more people, and adding a patch to clarify...
>> 
>> Nadav Amit  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 
>> ---
>> 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

Re: x86: CR4 update when IRQs are enabled

[hpa]

On November 15, 2017 3:31:50 PM PST, Nadav Amit  wrote:
>Ping?
>
>Nadav Amit  wrote:
>
>> CC’ing more people, and adding a patch to clarify...
>> 
>> Nadav Amit  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 
>> ---
>> 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
>> --- 

Re: x86: CR4 update when IRQs are enabled

[Nadav Amit]

Ping?

Nadav Amit  wrote:

> CC’ing more people, and adding a patch to clarify...
> 
> Nadav Amit  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 
> ---
> 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 

Re: x86: CR4 update when IRQs are enabled

[Nadav Amit]

Ping?

Nadav Amit  wrote:

> CC’ing more people, and adding a patch to clarify...
> 
> Nadav Amit  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 
> ---
> 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 

Re: x86: CR4 update when IRQs are enabled

[Nadav Amit]

CC’ing more people, and adding a patch to clarify...

Nadav Amit  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 
---
 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)
@@ 

Re: x86: CR4 update when IRQs are enabled

[Nadav Amit]

CC’ing more people, and adding a patch to clarify...

Nadav Amit  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 
---
 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 

x86: CR4 update when IRQs are enabled

[Nadav Amit]

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?

Thanks,
Nadav

x86: CR4 update when IRQs are enabled

[Nadav Amit]

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?

Thanks,
Nadav