On 9/23/2021 5:41 PM, Thomas Gleixner wrote:
On Mon, Sep 13 2021 at 13:01, Sohil Mehta wrote:
User interrupt state is saved and restored using xstate supervisor feature support. This includes the MSR state and the User Interrupt Flag (UIF) value.
During context switch update the UPID for a uintr task to reflect the current state of the task; namely whether the task should receive interrupt notifications and which cpu the task is currently running on.
XSAVES clears the notification vector (UINV) in the MISC MSR to prevent interrupts from being recognized in the UIRR MSR while the task is being context switched. The UINV is restored back when the kernel does an XRSTORS.
However, this conflicts with the kernel's lazy restore optimization which skips an XRSTORS if the kernel is scheduling the same user task back and the underlying MSR state hasn't been modified. Special handling is needed for a uintr task in the context switch path to keep using this optimization.
And this special handling is?
By special handling I meant programming the MSR when XRSTORS doesn't happen on return to userspace. The pseudo code you provided in patch 5 comments handles this well.
*/ void save_fpregs_to_fpstate(struct fpu *fpu) {
- cleared.
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c index ec0d836a13b1..62b82137db9c 100644 --- a/arch/x86/kernel/process_64.c +++ b/arch/x86/kernel/process_64.c @@ -53,6 +53,7 @@ #include <asm/xen/hypervisor.h> #include <asm/vdso.h> #include <asm/resctrl.h> +#include <asm/uintr.h> #include <asm/unistd.h> #include <asm/fsgsbase.h> #ifdef CONFIG_IA32_EMULATION @@ -565,6 +566,9 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p) WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ENTRY) && this_cpu_read(hardirq_stack_inuse));
- if (static_cpu_has(X86_FEATURE_UINTR))
cpu_feature_enabled() please.
I'll fix this and the other issues that you mentioned.
switch_uintr_prepare(prev_p);- if (!test_thread_flag(TIF_NEED_FPU_LOAD)) switch_fpu_prepare(prev_fpu, cpu);
diff --git a/arch/x86/kernel/uintr_core.c b/arch/x86/kernel/uintr_core.c index 2c6042a6840a..7a29888050ad 100644 --- a/arch/x86/kernel/uintr_core.c +++ b/arch/x86/kernel/uintr_core.c @@ -238,3 +238,78 @@ int do_uintr_register_handler(u64 handler) return 0; }
+/* Suppress notifications since this task is being context switched out */ +void switch_uintr_prepare(struct task_struct *prev) +{
- struct uintr_upid *upid;
- if (is_uintr_receiver(prev)) {
upid = prev->thread.ui_recv->upid_ctx->upid;set_bit(UPID_SN, (unsigned long *)&upid->nc.status);Please add a comment why this needs to be a locked instruction.
Ok, will do. The SN bit could be read concurrently on another CPU executing SENDUIPI.
Of course this is invoked unconditionally when the CPU has X86_FEATURE_UINTR:
- if (static_cpu_has(X86_FEATURE_UINTR))
switch_uintr_return();Why?
If the sequence is:
syscall() do_stuff() return_to_user()then what on earth has modified that MSR state? Nothing at all, but you still run this code. What for?
The pseudo code in patch 5 covers this. I'll fix the code based on that.
Thanks,
Sohil