Re: [RFC PATCH 07/13] x86/process/64: Add uintr task context switch support

[Sohil Mehta]

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.


> > + * cleared.
> >    */
> >   void save_fpregs_to_fpstate(struct fpu *fpu)
> >   {
> > 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