Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 19 Jul 2023 08:57:44 -0700
In-Reply-To: <bae58fd3-34b0-641a-a18b-010d48c792f0@oracle.com>
Mime-Version: 1.0
References: <3d05fcf1-dad3-826e-03e9-599ced7524b4@oracle.com>
 <20230518035806.938517-1-dengqiao.joey@bytedance.com> <2f6210acca81090146bc1decf61996aae2a0bfcf.camel@redhat.com>
 <36295675-2139-266d-4b07-9e029ac88fef@oracle.com> <ZJ4HJhQytonABUMl@google.com>
 <bae58fd3-34b0-641a-a18b-010d48c792f0@oracle.com>
Message-ID: <ZLgH+LGl+eC4hFdx@google.com>
Subject: Re: [PATCH] KVM: SVM: Update destination when updating pi irte
From:   Sean Christopherson <seanjc@google.com>
To:     Joao Martins <joao.m.martins@oracle.com>
Cc:     Maxim Levitsky <mlevitsk@redhat.com>,
        "dengqiao.joey" <dengqiao.joey@bytedance.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, pbonzini@redhat.com,
        Alejandro Jimenez <alejandro.j.jimenez@oracle.com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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().