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.
