Re: [PATCH v2 2/2] x86: disable IRQs before changing CR4

[Nadav Amit]

Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> On Sat, 25 Nov 2017, Nadav Amit wrote:
>> Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> 
>>> On Fri, 24 Nov 2017, Nadav Amit wrote:
>>>> /* Set in this cpu's CR4. */
>>>> -static inline void cr4_set_bits(unsigned long mask)
>>>> +static inline void cr4_set_bits_irqs_off(unsigned long mask)
>>> 
>>> This change is kinda weird. I'd expect that there is a corresponding
>>> function cr4_set_bits() which takes care of disabling interrupts. But there
>>> is not. All it does is creating a lot of pointless churn.
>>> 
>>>> static __always_inline void setup_smep(struct cpuinfo_x86 *c)
>>>> static __init int setup_disable_smap(char *arg)
>>>> static __always_inline void setup_pku(struct cpuinfo_x86 *c)
>>> 
>>> Why are you not doing this at the call site around all calls which fiddle
>>> with cr4, i.e. in identify_cpu() ?
>>> 
>>> identify_cpu() is called from two places:
>>> 
>>>   identify_boot_cpu() and identify_secondary_cpu()
>>> 
>>> identify_secondary_cpu is called with interrupts disabled anyway and there
>>> is no reason why we can't enforce interrupts being disabled around
>>> identify_cpu() completely.
>>> 
>>> But if we actually do the right thing, i.e. having cr4_set_bit() and
>>> cr4_set_bit_irqsoff() all of this churn goes away magically.
>>> 
>>> Then the only place which needs to be changed is the context switch because
>>> here interrupts are already disabled and we really care about performance.
>>> 
>>>> @@ -293,7 +303,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
>>>> 	}
>>>> 
>>>> 	if ((tifp ^ tifn) & _TIF_NOTSC)
>>>> -		cr4_toggle_bits(X86_CR4_TSD);
>>>> +		cr4_toggle_bits_irqs_off(X86_CR4_TSD);
>> 
>> You make a good point. I will add cr4_set_bit(). I will leave identify_cpu()
>> as is, since it is rather hard to maintain code that enables/disables irqs
>> at one point and rely on these operations at a completely different place.
>> As you said, it is less of an issue once cr4_set_bit() and friends are
>> introduced.
> 
> I fixed that up already as I wanted to have it done, see the tip-bot mail
> in your inbox.

Thanks, your changes made it much better. At some point it might be better
to make the MTRR code to use these interfaces too instead of meddling with
CR4 directly. Anyhow, that is a different story.

Regards,
Nadav

Attachment: signature.asc
Description: Message signed with OpenPGP