Re: [PATCH] KVM: SVM: Update destination when updating pi irte

[Sean Christopherson]

On Fri, Jul 14, 2023, Joao Martins wrote:
> +Suravee, +Alejandro
> 
> On 29/06/2023 23:35, Sean Christopherson wrote:
> > On Thu, May 18, 2023, Joao Martins wrote:
> >> On 18/05/2023 09:19, Maxim Levitsky wrote:
> >>> I think that we do need to a flag indicating if the vCPU is currently
> >>> running and if yes, then use svm->vcpu.cpu (or put -1 to it when it not
> >>> running or something) (currently the vcpu->cpu remains set when vCPU is
> >>> put)
> >>>
> >>> In other words if a vCPU is running, then avic_pi_update_irte should put
> >>> correct pCPU number, and if it raced with vCPU put/load, then later should
> >>> win and put the correct value.  This can be done either with a lock or
> >>> barriers.
> >>>
> >> If this could be done, it could remove cost from other places and avoid this
> >> little dance of the galog (and avoid its usage as it's not the greatest design
> >> aspect of the IOMMU). We anyways already need to do IRT flushes in all these
> >> things with regards to updating any piece of the IRTE, but we need some care
> >> there two to avoid invalidating too much (which is just as expensive and per-VCPU).
> > 
> > ...
> > 
> >> But still quite expensive (as many IPIs as vCPUs updated), but it works as
> >> intended and guest will immediately see the right vcpu affinity. But I honestly
> >> prefer going towards your suggestion (via vcpu.pcpu) if we can have some
> >> insurance that vcpu.cpu is safe to use in pi_update_irte if protected against
> >> preemption/blocking of the VCPU.
> > 
> > I think we have all the necessary info, and even a handy dandy spinlock to ensure
> > ordering.  Disclaimers: compile tested only, I know almost nothing about the IOMMU
> > side of things, and I don't know if I understood the needs for the !IsRunning cases.
> > 
> I was avoiding grabbing that lock, but now that I think about it it shouldn't do
> much harm.
> 
> My only concern has mostly been whether we mark the IRQ isRunning=1 on a vcpu
> that is about to block as then the doorbell rang by the IOMMU won't do anything
> to the guest. But IIUC the physical ID cache read-once should cover that

Acquiring ir_list_lock in avic_vcpu_{load,put}() when modifying
AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK is the key to avoiding ordering issues.
E.g. without the spinlock, READ_ONCE() wouldn't prevent svm_ir_list_add() from
racing with avic_vcpu_{load,put}() and ultimately shoving stale data into the IRTE.

It *should* actually be safe to drop the READ_ONCE() since acquiring/releasing
the spinlock will prevent multiple loads from observing different values.  I kept
them mostly to keep the diff small, and to be conservative.

The WRITE_ONCE() needs to stay to ensure that hardware doesn't see inconsitent
information due to store tearing.

If this patch works, I think it makes sense to follow-up with a cleanup patch to
drop the READ_ONCE() and add comments explaining why KVM uses WRITE_ONCE() but
not READ_ONCE().