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

[Sean Christopherson <seanjc@google.com>]

On Tue, Mar 28, 2023, Joao Martins wrote:
> [I was out sick, hence the delay]
> 
> On 24/03/2023 14:31, Sean Christopherson wrote:
> > 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);
> > 	}
> > 
> > 
> 
> I recall one instance of the 'garbage pi data' issue but this was due to
> prev_ga_tag not being initialized (see commit f6426ab9c957).

Yep, that's the one I was trying to recall.

> As far as I understand, AMD implementation on irq_vcpu_set_affinity will
> write back to caller the following fields of pi:
> 
> - prev_ga_tag
> - ir_data
> - guest_mode (sometimes when it is unsupported or disabled by the host via cmdline)
> 
> On legacy interrupt remap path (no iommu avic) the IRQ update just uses irq data
> mostly. It's the avic path that uses more things (vcpu_data, ga_tag, base,
> ga_root_ptr, ga_vector), but all of which are initialized by KVM properly already.

Ya, on my Nth read through, I don't see any issues with KVM's behavior.  I was
thinking that KVM's "pi" could bleed into amd_iommu_deactivate_guest_mode(), but
I had just gotten turned around by the many "data" variables.  Bummer.