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

[Joao Martins <joao.m.martins@oracle.com>]

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