Re: [PATCH 02/67] KVM: x86: Reset IRTE to host control if *new* route isn't postable

[Paolo Bonzini]

On 4/11/25 16:16, Sean Christopherson wrote:
> On Fri, Apr 11, 2025, Sairaj Kodilkar wrote:
> > On 4/5/2025 1:08 AM, Sean Christopherson wrote:
> > > @@ -991,7 +967,36 @@ int avic_pi_update_irte(struct kvm *kvm, unsigned int host_irq,
> > >    		}
> > >    	}
> > > -	ret = 0;
> > > +	if (enable_remapped_mode) {
> > > +		/* Use legacy mode in IRTE */
> > > +		struct amd_iommu_pi_data pi;
> > > +
> > > +		/**
> > > +		 * Here, pi is used to:
> > > +		 * - Tell IOMMU to use legacy mode for this interrupt.
> > > +		 * - Retrieve ga_tag of prior interrupt remapping data.
> > > +		 */
> > > +		pi.prev_ga_tag = 0;
> > > +		pi.is_guest_mode = false;
> > > +		ret = irq_set_vcpu_affinity(host_irq, &pi);
> > > +
> > > +		/**
> > > +		 * Check if the posted interrupt was previously
> > > +		 * setup with the guest_mode by checking if the ga_tag
> > > +		 * was cached. If so, we need to clean up the per-vcpu
> > > +		 * ir_list.
> > > +		 */
> > > +		if (!ret && pi.prev_ga_tag) {
> > > +			int id = AVIC_GATAG_TO_VCPUID(pi.prev_ga_tag);
> > > +			struct kvm_vcpu *vcpu;
> > > +
> > > +			vcpu = kvm_get_vcpu_by_id(kvm, id);
> > > +			if (vcpu)
> > > +				svm_ir_list_del(to_svm(vcpu), &pi);
> > > +		}
> > > +	} else {
> > > +		ret = 0;
> > > +	}
> >
> > Hi Sean,
> > I think you can remove this else and "ret = 0". Because Code will come to
> > this point when irq_set_vcpu_affinity() is successful, ensuring that ret is
> > 0.
>
> Ah, nice, because of this:
>
> 		if (ret < 0) {
> 			pr_err("%s: failed to update PI IRTE\n", __func__);
> 			goto out;
> 		}
>
> However, looking at this again, I'm very tempted to simply leave the "ret = 0;"
> that's already there so as to minimize the change.  It'll get cleaned up later on
> no matter what, so safety for LTS kernels is the driving factor as of this patch.
>
> Paolo, any preference?

If you mean squashing this in:

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index ef08356fdb1c..8e09f6ae98fd 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -967,6 +967,7 @@ int avic_pi_update_irte(struct kvm *kvm, unsigned int host_irq,
                }
        }
 
+       ret = 0;
        if (enable_remapped_mode) {
                /* Use legacy mode in IRTE */
                struct amd_iommu_pi_data pi;
@@ -994,8 +995,6 @@ int avic_pi_update_irte(struct kvm *kvm, unsigned int host_irq,
                        if (vcpu)
                                svm_ir_list_del(to_svm(vcpu), &pi);
                }
-       } else {
-               ret = 0;
        }
 out:
        srcu_read_unlock(&kvm->irq_srcu, idx);

to undo the moving of "ret = 0", that's a good idea yes.

Paolo