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.
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.
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.
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().
} else { // Manually restore UIRR and UINV wrmsrl(IA32_UINTR_RR, pir);I believe read-modify-write here as well.
Sigh, yes.
Thanks,
tglx