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
