[PATCH v2 0/2] iommu/amd: Fix GAM IRTEs affinity and GALog restart

[PATCH v2 0/2] iommu/amd: Fix GAM IRTEs affinity and GALog restart

[Joao Martins]

Hey,

This small series expands from v1 with one more patch:

Patch 1) Fix affinity changes to already-in-guest-mode IRTEs which would
         otherwise be nops.

Patch 2) Handle the GALog overflow condition by restarting it

I have a follow-up 3-patch series but being an potential optimization[0]
I prefer making it separate. This series just tackles bugs.

Comments appreciated.

Thanks,
	Joao

Changes since v1[1]:
- Adjust commit message in first patch (Suravee)
- Add Rb in the first patch (Suravee)

[0] https://lore.kernel.org/linux-iommu/b39d505c-8d2b-d90b-f52d-ceabde8225cf@oracle.com/
[1] https://lore.kernel.org/linux-iommu/20230208131938.39898-1-joao.m.martins@oracle.com/

Joao Martins (2):
  iommu/amd: Don't block updates to GATag if guest mode is on
  iommu/amd: Handle GALog overflows

 drivers/iommu/amd/amd_iommu.h |  1 +
 drivers/iommu/amd/init.c      | 24 ++++++++++++++++++++++++
 drivers/iommu/amd/iommu.c     | 11 +++++++++--
 3 files changed, 34 insertions(+), 2 deletions(-)

-- 
2.17.2

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

[Joao Martins]

On KVM GSI routing table updates, specially those where they have vIOMMUs
with interrupt remapping enabled (to boot >255vcpus setups without relying
on KVM_FEATURE_MSI_EXT_DEST_ID), a VMM may update the backing VF MSIs
with a new VCPU affinity.

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 they were first configured, and guest
will be unable to either VF interrupts and receive messages like this
from spuruious 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 the fact that amd_ir_set_vcpu_affinity() uses
amd_iommu_activate_guest_mode() underneath it essentially means that VCPU
affinity changes of IRTEs are nops. 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@oracle.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 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 5a505ba5467e..bf3ebc9d6cde 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3485,7 +3485,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;
-- 
2.17.2

[PATCH v2 2/2] iommu/amd: Handle GALog overflows

[Joao Martins]

GALog exists to propagate interrupts into all vCPUs in the system when
interrupts are marked as non running (e.g. when vCPUs aren't running). A
GALog overflow happens when there's in no space in the log to record the
GATag of the interrupt. So when the GALOverflow condition happens, the
GALog queue is processed and the GALog is restarted, as the IOMMU
manual indicates in section "2.7.4 Guest Virtual APIC Log Restart
Procedure":

| * Wait until MMIO Offset 2020h[GALogRun]=0b so that all request
|   entries are completed as circumstances allow. GALogRun must be 0b to
|   modify the guest virtual APIC log registers safely.
| * Write MMIO Offset 0018h[GALogEn]=0b.
| * As necessary, change the following values (e.g., to relocate or
| resize the guest virtual APIC event log):
|   - the Guest Virtual APIC Log Base Address Register
|      [MMIO Offset 00E0h],
|   - the Guest Virtual APIC Log Head Pointer Register
|      [MMIO Offset 2040h][GALogHead], and
|   - the Guest Virtual APIC Log Tail Pointer Register
|      [MMIO Offset 2048h][GALogTail].
| * Write MMIO Offset 2020h[GALOverflow] = 1b to clear the bit (W1C).
| * Write MMIO Offset 0018h[GALogEn] = 1b, and either set
|   MMIO Offset 0018h[GAIntEn] to enable the GA log interrupt or clear
|   the bit to disable it.

Failing to handle the GALog overflow means that none of the VFs (in any
guest) will work with IOMMU AVIC forcing the user to power cycle the
host. When handling the event it resumes the GALog without resizing
much like how it is done in the event handler overflow. The
[MMIO Offset 2020h][GALOverflow] bit might be set in status register
without the [MMIO Offset 2020h][GAInt] bit, so when deciding to poll
for GA events (to clear space in the galog), also check the overflow
bit.

[suravee: Check for GAOverflow without GAInt, toggle CONTROL_GAINT_EN]
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/amd/amd_iommu.h |  1 +
 drivers/iommu/amd/init.c      | 24 ++++++++++++++++++++++++
 drivers/iommu/amd/iommu.c     |  9 ++++++++-
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index c160a332ce33..24c7e6c6c0de 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -15,6 +15,7 @@ extern irqreturn_t amd_iommu_int_thread(int irq, void *data);
 extern irqreturn_t amd_iommu_int_handler(int irq, void *data);
 extern void amd_iommu_apply_erratum_63(struct amd_iommu *iommu, u16 devid);
 extern void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
+extern void amd_iommu_restart_ga_log(struct amd_iommu *iommu);
 extern int amd_iommu_init_devices(void);
 extern void amd_iommu_uninit_devices(void);
 extern void amd_iommu_init_notifier(void);
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 19a46b9f7357..fd487c33b28a 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -751,6 +751,30 @@ void amd_iommu_restart_event_logging(struct amd_iommu *iommu)
 	iommu_feature_enable(iommu, CONTROL_EVT_LOG_EN);
 }
 
+/*
+ * This function restarts event logging in case the IOMMU experienced
+ * an GA log overflow.
+ */
+void amd_iommu_restart_ga_log(struct amd_iommu *iommu)
+{
+	u32 status;
+
+	status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
+	if (status & MMIO_STATUS_GALOG_RUN_MASK)
+		return;
+
+	pr_info_ratelimited("IOMMU GA Log restarting\n");
+
+	iommu_feature_disable(iommu, CONTROL_GALOG_EN);
+	iommu_feature_disable(iommu, CONTROL_GAINT_EN);
+
+	writel(MMIO_STATUS_GALOG_OVERFLOW_MASK,
+	       iommu->mmio_base + MMIO_STATUS_OFFSET);
+
+	iommu_feature_enable(iommu, CONTROL_GAINT_EN);
+	iommu_feature_enable(iommu, CONTROL_GALOG_EN);
+}
+
 /*
  * This function resets the command buffer if the IOMMU stopped fetching
  * commands from it.
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index bf3ebc9d6cde..ebb155bfef15 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -845,6 +845,7 @@ amd_iommu_set_pci_msi_domain(struct device *dev, struct amd_iommu *iommu) { }
 	(MMIO_STATUS_EVT_OVERFLOW_INT_MASK | \
 	 MMIO_STATUS_EVT_INT_MASK | \
 	 MMIO_STATUS_PPR_INT_MASK | \
+	 MMIO_STATUS_GALOG_OVERFLOW_MASK | \
 	 MMIO_STATUS_GALOG_INT_MASK)
 
 irqreturn_t amd_iommu_int_thread(int irq, void *data)
@@ -868,10 +869,16 @@ irqreturn_t amd_iommu_int_thread(int irq, void *data)
 		}
 
 #ifdef CONFIG_IRQ_REMAP
-		if (status & MMIO_STATUS_GALOG_INT_MASK) {
+		if (status & (MMIO_STATUS_GALOG_INT_MASK |
+			      MMIO_STATUS_GALOG_OVERFLOW_MASK)) {
 			pr_devel("Processing IOMMU GA Log\n");
 			iommu_poll_ga_log(iommu);
 		}
+
+		if (status & MMIO_STATUS_GALOG_OVERFLOW_MASK) {
+			pr_info_ratelimited("IOMMU GA Log overflow\n");
+			amd_iommu_restart_ga_log(iommu);
+		}
 #endif
 
 		if (status & MMIO_STATUS_EVT_OVERFLOW_INT_MASK) {
-- 
2.17.2

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

[Sean Christopherson]

On Thu, Mar 16, 2023, Joao Martins wrote:
> On KVM GSI routing table updates, specially those where they have vIOMMUs
> with interrupt remapping enabled (to boot >255vcpus setups without relying
> on KVM_FEATURE_MSI_EXT_DEST_ID), a VMM may update the backing VF MSIs
> with a new VCPU affinity.
> 
> 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 they were first configured, and guest
> will be unable to either VF interrupts and receive messages like this
> from spuruious 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 the fact that amd_ir_set_vcpu_affinity() uses
> amd_iommu_activate_guest_mode() underneath it essentially means that VCPU
> affinity changes of IRTEs are nops. 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.

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

> Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation code")
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  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 5a505ba5467e..bf3ebc9d6cde 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3485,7 +3485,7 @@ int amd_iommu_activate_guest_mode(void *data)

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

>  	u64 valid;
>  
>  	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) ||
> -	    !entry || entry->lo.fields_vapic.guest_mode)
> +	    !entry)

This can easily fit on the previous line.

	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || !entry)
		return 0;

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

[Joao Martins]

On 16/03/2023 21:01, Sean Christopherson wrote:
> On Thu, Mar 16, 2023, Joao Martins wrote:
>> On KVM GSI routing table updates, specially those where they have vIOMMUs
>> with interrupt remapping enabled (to boot >255vcpus setups without relying
>> on KVM_FEATURE_MSI_EXT_DEST_ID), a VMM may update the backing VF MSIs
>> with a new VCPU affinity.
>>
>> 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 they were first configured, and guest
>> will be unable to either VF interrupts and receive messages like this
>> from spuruious 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 the fact that amd_ir_set_vcpu_affinity() uses
>> amd_iommu_activate_guest_mode() underneath it essentially means that VCPU
>> affinity changes of IRTEs are nops. 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.
> 
> 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).

>> Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation code")
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>  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 5a505ba5467e..bf3ebc9d6cde 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -3485,7 +3485,7 @@ int amd_iommu_activate_guest_mode(void *data)
> 
> 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.

Part of the problem here (as you also hint) is instead the reusal of the helpers
used in the (correct) transition to/from guest-mode *externally* by callers
mixed from *internal* usage in amd iommu code for IRQ vcpu affinity using the
same said helpers. And that'a also the reason I put the Fixes tag as that patch
introduced such "reusal" and which could be useful for stable trees. Here we are
mainly concerned with the updates (the internal usage) and actually exercising
the IRTE update instead of skipping it such that when you have interrupts on
blocked vCPUS that you actually wakeup the right one (and not doing so has a
rather drastic effect for VFs within the guest).

>>  	u64 valid;
>>  
>>  	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) ||
>> -	    !entry || entry->lo.fields_vapic.guest_mode)
>> +	    !entry)
> 
> This can easily fit on the previous line.
> 
> 	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || !entry)
> 		return 0;

True, I can move it to the previous line.

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

[Sean Christopherson]

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!

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

[Alexey Kardashevskiy]

On 17/3/23 07:02, Joao Martins wrote:
> On KVM GSI routing table updates, specially those where they have vIOMMUs
> with interrupt remapping enabled (to boot >255vcpus setups without relying
> on KVM_FEATURE_MSI_EXT_DEST_ID), a VMM may update the backing VF MSIs
> with a new VCPU affinity.
> 
> 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 they were first configured,and guest
> will be unable to either VF interrupts and receive messages like this
> from spuruious interrupts (e.g. from waking the wrong vCPU in GALog):

The "either" above sounds like there should be a verb which it is not, 
or is it? (my english skills are meh). I kinda get the idea anyway (I hope).

btw s/spuruious/spurious/, says my vim. Thanks,

> 
> [  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 the fact that amd_ir_set_vcpu_affinity() uses
> amd_iommu_activate_guest_mode() underneath it essentially means that VCPU
> affinity changes of IRTEs are nops. 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@oracle.com>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   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 5a505ba5467e..bf3ebc9d6cde 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3485,7 +3485,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;

-- 
Alexey

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

[Joao Martins]

On 28/03/2023 10:07, Alexey Kardashevskiy wrote:
> On 17/3/23 07:02, Joao Martins wrote:
>> On KVM GSI routing table updates, specially those where they have vIOMMUs
>> with interrupt remapping enabled (to boot >255vcpus setups without relying
>> on KVM_FEATURE_MSI_EXT_DEST_ID), a VMM may update the backing VF MSIs
>> with a new VCPU affinity.
>>
>> 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 they were first configured,and guest
>> will be unable to either VF interrupts and receive messages like this
>> from spuruious interrupts (e.g. from waking the wrong vCPU in GALog):
> 
> The "either" above sounds like there should be a verb which it is not, or is it?
> (my english skills are meh). I kinda get the idea anyway (I hope).
> 
It should be 'issue'. I'll delete the 'either'

> btw s/spuruious/spurious/, says my vim. Thanks,
> 
/me nods

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

[Joao Martins]

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

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

[Sean Christopherson]

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.

Re: [PATCH v2 2/2] iommu/amd: Handle GALog overflows

[Suthikulpanit, Suravee]

On 3/17/2023 3:02 AM, Joao Martins wrote:
> GALog exists to propagate interrupts into all vCPUs in the system when
> interrupts are marked as non running (e.g. when vCPUs aren't running). A
> GALog overflow happens when there's in no space in the log to record the
> GATag of the interrupt. So when the GALOverflow condition happens, the
> GALog queue is processed and the GALog is restarted, as the IOMMU
> manual indicates in section "2.7.4 Guest Virtual APIC Log Restart
> Procedure":
> 
> | * Wait until MMIO Offset 2020h[GALogRun]=0b so that all request
> |   entries are completed as circumstances allow. GALogRun must be 0b to
> |   modify the guest virtual APIC log registers safely.
> | * Write MMIO Offset 0018h[GALogEn]=0b.
> | * As necessary, change the following values (e.g., to relocate or
> | resize the guest virtual APIC event log):
> |   - the Guest Virtual APIC Log Base Address Register
> |      [MMIO Offset 00E0h],
> |   - the Guest Virtual APIC Log Head Pointer Register
> |      [MMIO Offset 2040h][GALogHead], and
> |   - the Guest Virtual APIC Log Tail Pointer Register
> |      [MMIO Offset 2048h][GALogTail].
> | * Write MMIO Offset 2020h[GALOverflow] = 1b to clear the bit (W1C).
> | * Write MMIO Offset 0018h[GALogEn] = 1b, and either set
> |   MMIO Offset 0018h[GAIntEn] to enable the GA log interrupt or clear
> |   the bit to disable it.
> 
> Failing to handle the GALog overflow means that none of the VFs (in any
> guest) will work with IOMMU AVIC forcing the user to power cycle the
> host. When handling the event it resumes the GALog without resizing
> much like how it is done in the event handler overflow. The
> [MMIO Offset 2020h][GALOverflow] bit might be set in status register
> without the [MMIO Offset 2020h][GAInt] bit, so when deciding to poll
> for GA events (to clear space in the galog), also check the overflow
> bit.
> 
> [suravee: Check for GAOverflow without GAInt, toggle CONTROL_GAINT_EN]

According to the AMD IOMMU spec,

* The GAInt is set when the virtual interrupt request is written to the 
GALog and the IOMMU hardware generates an interrupt when GAInt changes 
from 0 to 1.

* The GAOverflow bit is set when a new guest virtual APIC event is to be 
written to the GALog and there is no usable entry in the GALog, causing 
the new event information to be discarded. No interrupt is generated 
when GALOverflow is changed from 0b to 1b.

So, whenever the IOMMU driver detects GALogOverflow, it should also 
ensure to process any existing entries in the GALog.

Please note that we are working on another patch series to isolate the 
interrupts for Event, PPR, and GALog so that each one can be handled 
separately in a similar fashion.

Thanks,
Suravee

Re: [PATCH v2 2/2] iommu/amd: Handle GALog overflows

[Joao Martins]

On 13/04/2023 11:24, Suthikulpanit, Suravee wrote:
> On 3/17/2023 3:02 AM, Joao Martins wrote:
>> GALog exists to propagate interrupts into all vCPUs in the system when
>> interrupts are marked as non running (e.g. when vCPUs aren't running). A
>> GALog overflow happens when there's in no space in the log to record the
>> GATag of the interrupt. So when the GALOverflow condition happens, the
>> GALog queue is processed and the GALog is restarted, as the IOMMU
>> manual indicates in section "2.7.4 Guest Virtual APIC Log Restart
>> Procedure":
>>
>> | * Wait until MMIO Offset 2020h[GALogRun]=0b so that all request
>> |   entries are completed as circumstances allow. GALogRun must be 0b to
>> |   modify the guest virtual APIC log registers safely.
>> | * Write MMIO Offset 0018h[GALogEn]=0b.
>> | * As necessary, change the following values (e.g., to relocate or
>> | resize the guest virtual APIC event log):
>> |   - the Guest Virtual APIC Log Base Address Register
>> |      [MMIO Offset 00E0h],
>> |   - the Guest Virtual APIC Log Head Pointer Register
>> |      [MMIO Offset 2040h][GALogHead], and
>> |   - the Guest Virtual APIC Log Tail Pointer Register
>> |      [MMIO Offset 2048h][GALogTail].
>> | * Write MMIO Offset 2020h[GALOverflow] = 1b to clear the bit (W1C).
>> | * Write MMIO Offset 0018h[GALogEn] = 1b, and either set
>> |   MMIO Offset 0018h[GAIntEn] to enable the GA log interrupt or clear
>> |   the bit to disable it.
>>
>> Failing to handle the GALog overflow means that none of the VFs (in any
>> guest) will work with IOMMU AVIC forcing the user to power cycle the
>> host. When handling the event it resumes the GALog without resizing
>> much like how it is done in the event handler overflow. The
>> [MMIO Offset 2020h][GALOverflow] bit might be set in status register
>> without the [MMIO Offset 2020h][GAInt] bit, so when deciding to poll
>> for GA events (to clear space in the galog), also check the overflow
>> bit.
>>
>> [suravee: Check for GAOverflow without GAInt, toggle CONTROL_GAINT_EN]
> 
> According to the AMD IOMMU spec,
> 
> * The GAInt is set when the virtual interrupt request is written to the GALog
> and the IOMMU hardware generates an interrupt when GAInt changes from 0 to 1.
> 
> * The GAOverflow bit is set when a new guest virtual APIC event is to be written
> to the GALog and there is no usable entry in the GALog, causing the new event
> information to be discarded. No interrupt is generated when GALOverflow is
> changed from 0b to 1b.
> 
> So, whenever the IOMMU driver detects GALogOverflow, it should also ensure to
> process any existing entries in the GALog.
> 

... And I am doing all that aren't I?

Or do you want me edit the commit message to quote these two bullet points from
the IOMMU manual?

> Please note that we are working on another patch series to isolate the
> interrupts for Event, PPR, and GALog so that each one can be handled separately
> in a similar fashion.
> 
Cool, if possible please CC me on that series.

	Joao

Re: [PATCH v2 2/2] iommu/amd: Handle GALog overflows

[Suthikulpanit, Suravee]

On 4/13/2023 5:30 PM, Joao Martins wrote:
> On 13/04/2023 11:24, Suthikulpanit, Suravee wrote:
>> On 3/17/2023 3:02 AM, Joao Martins wrote:
>>> GALog exists to propagate interrupts into all vCPUs in the system when
>>> interrupts are marked as non running (e.g. when vCPUs aren't running). A
>>> GALog overflow happens when there's in no space in the log to record the
>>> GATag of the interrupt. So when the GALOverflow condition happens, the
>>> GALog queue is processed and the GALog is restarted, as the IOMMU
>>> manual indicates in section "2.7.4 Guest Virtual APIC Log Restart
>>> Procedure":
>>>
>>> | * Wait until MMIO Offset 2020h[GALogRun]=0b so that all request
>>> |   entries are completed as circumstances allow. GALogRun must be 0b to
>>> |   modify the guest virtual APIC log registers safely.
>>> | * Write MMIO Offset 0018h[GALogEn]=0b.
>>> | * As necessary, change the following values (e.g., to relocate or
>>> | resize the guest virtual APIC event log):
>>> |   - the Guest Virtual APIC Log Base Address Register
>>> |      [MMIO Offset 00E0h],
>>> |   - the Guest Virtual APIC Log Head Pointer Register
>>> |      [MMIO Offset 2040h][GALogHead], and
>>> |   - the Guest Virtual APIC Log Tail Pointer Register
>>> |      [MMIO Offset 2048h][GALogTail].
>>> | * Write MMIO Offset 2020h[GALOverflow] = 1b to clear the bit (W1C).
>>> | * Write MMIO Offset 0018h[GALogEn] = 1b, and either set
>>> |   MMIO Offset 0018h[GAIntEn] to enable the GA log interrupt or clear
>>> |   the bit to disable it.
>>>
>>> Failing to handle the GALog overflow means that none of the VFs (in any
>>> guest) will work with IOMMU AVIC forcing the user to power cycle the
>>> host. When handling the event it resumes the GALog without resizing
>>> much like how it is done in the event handler overflow. The
>>> [MMIO Offset 2020h][GALOverflow] bit might be set in status register
>>> without the [MMIO Offset 2020h][GAInt] bit, so when deciding to poll
>>> for GA events (to clear space in the galog), also check the overflow
>>> bit.
>>>
>>> [suravee: Check for GAOverflow without GAInt, toggle CONTROL_GAINT_EN]
>> According to the AMD IOMMU spec,
>>
>> * The GAInt is set when the virtual interrupt request is written to the GALog
>> and the IOMMU hardware generates an interrupt when GAInt changes from 0 to 1.
>>
>> * The GAOverflow bit is set when a new guest virtual APIC event is to be written
>> to the GALog and there is no usable entry in the GALog, causing the new event
>> information to be discarded. No interrupt is generated when GALOverflow is
>> changed from 0b to 1b.
>>
>> So, whenever the IOMMU driver detects GALogOverflow, it should also ensure to
>> process any existing entries in the GALog.
>>
> ... And I am doing all that aren't I?

Correct. I am just following up to clarify details on the GAOverflow and 
GAInt.


> Or do you want me edit the commit message to quote these two bullet points from
> the IOMMU manual?

No need since this is already documented in the spec.

Thanks,
Suravee

Re: [PATCH v2 2/2] iommu/amd: Handle GALog overflows

[Vasant Hegde]

On 3/17/2023 1:32 AM, Joao Martins wrote:
> GALog exists to propagate interrupts into all vCPUs in the system when
> interrupts are marked as non running (e.g. when vCPUs aren't running). A
> GALog overflow happens when there's in no space in the log to record the
> GATag of the interrupt. So when the GALOverflow condition happens, the
> GALog queue is processed and the GALog is restarted, as the IOMMU
> manual indicates in section "2.7.4 Guest Virtual APIC Log Restart
> Procedure":
> 
> | * Wait until MMIO Offset 2020h[GALogRun]=0b so that all request
> |   entries are completed as circumstances allow. GALogRun must be 0b to
> |   modify the guest virtual APIC log registers safely.
> | * Write MMIO Offset 0018h[GALogEn]=0b.
> | * As necessary, change the following values (e.g., to relocate or
> | resize the guest virtual APIC event log):
> |   - the Guest Virtual APIC Log Base Address Register
> |      [MMIO Offset 00E0h],
> |   - the Guest Virtual APIC Log Head Pointer Register
> |      [MMIO Offset 2040h][GALogHead], and
> |   - the Guest Virtual APIC Log Tail Pointer Register
> |      [MMIO Offset 2048h][GALogTail].
> | * Write MMIO Offset 2020h[GALOverflow] = 1b to clear the bit (W1C).
> | * Write MMIO Offset 0018h[GALogEn] = 1b, and either set
> |   MMIO Offset 0018h[GAIntEn] to enable the GA log interrupt or clear
> |   the bit to disable it.
> 
> Failing to handle the GALog overflow means that none of the VFs (in any
> guest) will work with IOMMU AVIC forcing the user to power cycle the
> host. When handling the event it resumes the GALog without resizing
> much like how it is done in the event handler overflow. The
> [MMIO Offset 2020h][GALOverflow] bit might be set in status register
> without the [MMIO Offset 2020h][GAInt] bit, so when deciding to poll
> for GA events (to clear space in the galog), also check the overflow
> bit.
> 
> [suravee: Check for GAOverflow without GAInt, toggle CONTROL_GAINT_EN]
> Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

Patch looks good to me.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant