Sohil,

On Mon, Sep 27 2021 at 12:07, Sohil Mehta wrote:

On 9/26/2021 5:39 AM, Thomas Gleixner wrote:

The User-interrupt notification processing moves all the pending 
interrupts from UPID.PIR to the UIRR.

Indeed that makes sense. Should have thought about that myself.

quoted

Also the restore portion on the way back to user space has to be coupled
more tightly:

arch_exit_to_user_mode_prepare()
{
         ...
         if (unlikely(ti_work & _TIF_UPID))
         	uintr_restore_upid(ti_work & _TIF_NEED_FPU_LOAD);
         if (unlikely(ti_work & _TIF_NEED_FPU_LOAD))
         	switch_fpu_return();
}

I am assuming _TIF_UPID would be set everytime SN is set and XSTATE is 
saved.

Yes.

quoted

upid_set_ndst(upid)
{
	apicid = __this_cpu_read(x86_cpu_to_apicid);

         if (x2apic_enabled())
             upid->ndst.x2apic = apicid;
         else
             upid->ndst.apic = apicid;
}

uintr_restore_upid(bool xrstors_pending)
{
         clear_thread_flag(TIF_UPID);
         
	// Update destination
         upid_set_ndst(upid);

         // Do we need something stronger here?
         barrier();

         clear_bit(SN, upid->status);

         // Any SENDUIPI after this point sends to this CPU
            
         // Any bit which was set in upid->pir after SN was set
         // and/or UINV was cleared by XSAVES up to the point
         // where SN was cleared above is not reflected in UIRR.

	// As this runs with interrupts disabled the current state
         // of upid->pir can be read and used for restore. A SENDUIPI
         // which sets a bit in upid->pir after that read will send
         // the notification vector which is going to be handled once
         // the task reenables interrupts on return to user space.
         // If the SENDUIPI set the bit before the read then the
         // notification vector handling will just observe the same
         // PIR state.

         // Needs to be a locked access as there might be a
         // concurrent SENDUIPI modiying it.
         pir = read_locked(upid->pir);

         if (xrstors_pending)) {
         	// Update the saved xstate for xrstors
            	current->xstate.uintr.uinv = UINTR_NOTIFICATION_VECTOR;

XSAVES saves the UINV value into the XSTATE buffer. I am not sure if we 
need this again. Is it because it could have been overwritten by calling 
XSAVES twice?

Yes that can happen AFAICT. I haven't done a deep analysis, but this
needs to looked at.

quoted

                 current->xstate.uintr.uirr = pir;

I believe PIR should be ORed. There could be some bits already set in 
the UIRR.

Also, shouldn't UPID->PIR be cleared? If not, we would detect these 
interrupts all over again during the next ring transition.

Right. So that PIR read above needs to be a locked cmpxchg().

quoted

         } else {
                 // Manually restore UIRR and UINV
                 wrmsrl(IA32_UINTR_RR, pir);

I believe read-modify-write here as well.

Sigh, yes.

Thanks,

        tglx