Messages in this thread | | | Subject | Re: [PATCH v9 25/26] intel_idle/amx: Add SPR support with XTILEDATA capability | From | Dave Hansen <> | Date | Tue, 3 Aug 2021 14:38:26 -0700 |
| |
On 8/3/21 2:32 PM, Bae, Chang Seok wrote: >>> +static inline void idle_tile(void) >>> +{ >>> + if (boot_cpu_has(X86_FEATURE_XGETBV1) && (xgetbv(1) & XFEATURE_MASK_XTILE)) { >>> + tile_release(); >>> + fpregs_deactivate(¤t->thread.fpu); >>> + } >>> +} >> This isn't obviously safe code. There's a window in there when we have >> bogus, destroyed FPU register state but where we might be rescheduled. >> >> I would assume that preempt is off *somewhere* in this, but it would be >> nice to make sure of that, or at least mention the requirement for it to >> be off before this code is safe. > I can see preempt_disable() in this path: > > $kernel/sched/idle.c::play_idle_precise() > --> preempt_disable() > ... > --> do_idle() > --> cpuidle_idle_call() > --> call_cpuidle() > --> $drivers/cpuidle/cpuidle.c::cpuidle_enter() > --> cpuidle_enter_state() > --> target_state->enter() > --> $drivers/idle/intel_idle.c::intel_idle_tile() > --> idle_tile() > ... > --> preempt_enable()
OK, that's good. Can we comment about the preempt requirement somewhere? Or, maybe add a !in_atomic() warning?
Also, should this have something like a fpregs_state_valid() check? If the registers are invalid, should this be calling tile_release()?
| |