Re: [PATCH v2 1/2] iommu/amd: Don't block updates to GATag if guest mode is on

[Sean Christopherson <seanjc@google.com>]

On Thu, Mar 16, 2023, Joao Martins wrote:
> On 16/03/2023 21:01, Sean Christopherson wrote:
> > Is there any harm in giving deactivate the same treatement?  If the worst case
> > scenario is a few wasted cycles, having symmetric flows and eliminating benign
> > bugs seems like a worthwhile tradeoff (assuming this is indeed a relatively slow
> > path like I think it is).
> > 
> 
> I wanna say there's no harm, but initially I had such a patch, and on testing it
> broke the classic interrupt remapping case but I didn't investigate further --
> my suspicion is that the only case that should care is the updates (not the
> actual deactivation of guest-mode).

Ugh, I bet this is due to KVM invoking irq_set_vcpu_affinity() with garbage when
AVIC is enabled, but KVM can't use a posted interrupt due to the how the IRQ is
configured.  I vaguely recall a bug report about uninitialized data in "pi" being
consumed, but I can't find it at the moment.

	if (!get_pi_vcpu_info(kvm, e, &vcpu_info, &svm) && set &&
		    kvm_vcpu_apicv_active(&svm->vcpu)) {

		...

	} else {
			/* 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);
	}


> > Any chance you (or anyone) would want to create a follow-up series to rename and/or
> > rework these flows to make it more obvious that the helpers handle updates as well
> > as transitions between "guest mode" and "host mode"?  E.g. I can see KVM getting
> > clever and skipping the "activation" when KVM knows AVIC is already active (though
> > I can't tell for certain whether or not that would actually be problematic).
> > 
> 
> To be honest, I think the function naming is correct.

After looking more closely at the KVM code, I agree.  I was thinking KVM invoked
the (de)activate helpers somewhat spuriously, but that's not actually the case,
KVM just has a few less-than-perfect names due to conflicting requirements.

Thanks!