On 2/8/2023 8:19 PM, Joao Martins wrote:
On KVM GSI routing table updates, specially those where they have vIOMMUs
with interrupt remapping enabled (e.g. to boot >255vcpus guests without
relying on KVM_FEATURE_MSI_EXT_DEST_ID), a VMM may update the backing VF
MSIs with new VCPU affinities.
On AMD this translates to calls to amd_ir_set_vcpu_affinity() and
eventually to amd_iommu_{de}activate_guest_mode() with a new GATag
outlining the VM ID and (new) VCPU ID. On vCPU blocking and unblocking
paths it disables AVIC, and rely on GALog to convey the wakeups to any
sleeping vCPUs. KVM will store a list of GA-mode IR entries to each
running/blocked vCPU. So any vCPU Affinity update to a VF interrupt happen
via KVM, and it will change already-configured-guest-mode IRTEs with a new
GATag.
Could we simplify this paragraph to:
On AMD with AVIC enabled, the new vcpu affinity info is updated via:
avic_pi_update_irte()
irq_set_vcpu_affinity()
amd_ir_set_vcpu_affinity()
amd_iommu_{de}activate_guest_mode()
where the IRTE[GATag] is updated with the new vcpu affinity. The GATag
contains VM ID and VCPU ID, and is used by IOMMU hardware to signal KVM
(via GALog) when interrupt cannot be delivered due to vCPU is in
blocking state.
The issue is that amd_iommu_activate_guest_mode() will essentially only
change IRTE fields on transitions from non-guest-mode to guest-mode and
otherwise returns *with no changes to IRTE* on already configured
guest-mode interrupts. To the guest this means that the VF interrupts
remain affined to the first vCPU these were first configured, and guest
will be unable to either VF interrupts and receive messages like this from
spurious interrupts (e.g. from waking the wrong vCPU in GALog):
[ 167.759472] __common_interrupt: 3.34 No irq handler for vector
[ 230.680927] mlx5_core 0000:00:02.0: mlx5_cmd_eq_recover:247:(pid
3122): Recovered 1 EQEs on cmd_eq
[ 230.681799] mlx5_core 0000:00:02.0:
wait_func_handle_exec_timeout:1113:(pid 3122): cmd[0]: CREATE_CQ(0x400)
recovered after timeout
[ 230.683266] __common_interrupt: 3.34 No irq handler for vector
Given that amd_ir_set_vcpu_affinity() uses amd_iommu_activate_guest_mode()
underneath it essentially means that VCPU affinity changes of IRTEs are
nops if it was called once for the IRTE already (on VMENTER). Fix it by
dropping the check for guest-mode at amd_iommu_activate_guest_mode(). Same
thing is applicable to amd_iommu_deactivate_guest_mode() although, even if
the IRTE doesn't change underlying DestID on the host, the VFIO IRQ handler
will still be able to poke at the right guest-vCPU.
Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation code")
Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
---
Some notes in other related flaws as I looked at this:
1) amd_iommu_deactivate_guest_mode() suffers from the same issue as this patch,
but it should only matter for the case where you rely on irqbalance-like
daemons balancing VFIO IRQs in the hypervisor. Though, it doesn't translate
into guest failures, more like performance "misdirection". Happy to fix it, if
folks also deem it as a problem.
2) This patch doesn't attempt at changing semantics around what
amd_iommu_activate_guest_mode() has been doing for a long time [since v5.4]
(i.e. clear the whole IRTE and then changes its fields). As such when
updating the IRTEs the interrupts get isRunning and DestId cleared, thus
we rely on the GALog to inject IRQs into vCPUs /until/ the vCPUs block
and unblock again (which is when they update the IOMMU affinity), or the
AVIC gets momentarily disabled. I have patches that improve this part as a
follow-up, but I thought that this patch had value on its own onto fixing
what has been broken since v5.4 ... and that it could be easily carried
to stable trees.
---
drivers/iommu/amd/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index cbeaab55c0db..afe1f35a4dd9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3476,7 +3476,7 @@ int amd_iommu_activate_guest_mode(void *data)
u64 valid;
if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) ||
- !entry || entry->lo.fields_vapic.guest_mode)
+ !entry)
return 0;
valid = entry->lo.fields_vapic.valid;
Apart from the commit message change:
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
Thanks,
Suravee
