Messages in this thread | | | From | "Bae, Chang Seok" <> | Subject | Re: [PATCH v9 25/26] intel_idle/amx: Add SPR support with XTILEDATA capability | Date | Tue, 3 Aug 2021 21:32:29 +0000 |
| |
On Jul 30, 2021, at 11:41, Hansen, Dave <dave.hansen@intel.com> wrote: >> +/** >> + * idle_tile - Initialize TILE registers in INIT-state >> + * >> + * Leaving state in the dirty TILE registers may prevent the processor from >> + * entering lower-power idle states. Use TILERELEASE to initialize the >> + * state. Destroying fpregs state is safe after the fpstate update. >> + */ >> +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()
> I'm also not sure TILERELEASE is *technically* what you want here. > xgetbv(1) tells you whether a feature is being tracked by the processor > as being in its init state. tile_release() gets a feature into its init > state, but does not guarantee that the processor will *track* it as > being in the init state. > > TILERELEASE is not documented to have an effect on XINUSE (init tracking). > > XRSTOR, on the other hand, is at least documented to affect XINUSE. > > It sounds like we either need a documentation update, or a clear > explanation why TILERELEASE is being used over XRSTOR.
TILERELEASE impacts INIT tracking at least on the first AMX implementation. I agree that the documentation needs some update.
Thanks, Chang
| |