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!
