[PATCH 0/2] iommu/amd: Misc Update for Virtual APIC support

[PATCH 0/2] iommu/amd: Misc Update for Virtual APIC support

[Suravee Suthikulpanit]

First, this series simplifies existing feature detection and enablement of
GAM and GALog features, which are prerequisite for Virtual APIC (AVIC)
support. 

Second, it fixes the warning reported here https://lkml.org/lkml/2022/7/20/1135.

Last, it introduces new enablement for IOMMU to support AVIC on
SNP-enabled system.

Best Regards,
Suravee

Suravee Suthikulpanit (2):
  iommu/amd: Consolidate Virtual APIC (AVIC) Enablement
  iommu/amd: Add support for AVIC when SNP is enabled

 drivers/iommu/amd/amd_iommu_types.h |  7 +++
 drivers/iommu/amd/init.c            | 94 ++++++++++++++++++++---------
 2 files changed, 71 insertions(+), 30 deletions(-)

-- 
2.34.1

[PATCH 1/2] iommu/amd: Simplify and Consolidate Virtual APIC (AVIC) Enablement

[Suravee Suthikulpanit]

Currently, enabling AVIC requires individually detect and enable GAM and
GALOG features on each IOMMU, which is difficult to keep track on
multi-IOMMU system, where the features needs to be enabled system-wide.

In addition, these features do not need to be enabled in early stage. 
It can be delayed until after amd_iommu_init_pci().

Therefore, consolidate logic for detecting and enabling IOMMU GAM and
GALOG features into a helper function, enable_iommus_vapic(), which uses
the check_feature_on_all_iommus() helper function to ensure system-wide
support of the features before enabling them, and postpone until after
amd_iommu_init_pci().

The new function also check and clean up feature enablement residue from
previous boot (e.g. in case of booting into kdump kernel), which triggers
a WARN_ON (shown below) introduced by the commit a8d4a37d1bb9 ("iommu/amd:
Restore GA log/tail pointer on host resume") in iommu_ga_log_enable().

[    7.731955] ------------[ cut here ]------------
[    7.736575] WARNING: CPU: 0 PID: 1 at drivers/iommu/amd/init.c:829 iommu_ga_log_enable.isra.0+0x16f/0x190
[    7.746135] Modules linked in:
[    7.749193] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W        --------  ---  5.19.0-0.rc7.53.eln120.x86_64 #1
[    7.759706] Hardware name: Dell Inc. PowerEdge R7525/04D5GJ, BIOS 2.1.6 03/09/2021
[    7.767274] RIP: 0010:iommu_ga_log_enable.isra.0+0x16f/0x190
[    7.772931] Code: 20 20 00 00 8b 00 f6 c4 01 74 da 48 8b 44 24 08 65 48 2b 04 25 28 00 00 00 75 13 48 83 c4 10 5b 5d e9 f5 00 72 00 0f 0b eb e1 <0f> 0b eb dd e8 f8 66 42 00 48 8b 15 f1 85 53 01 e9 29 ff ff ff 48
[    7.791679] RSP: 0018:ffffc90000107d20 EFLAGS: 00010206
[    7.796905] RAX: ffffc90000780000 RBX: 0000000000000100 RCX: ffffc90000780000
[    7.804038] RDX: 0000000000000001 RSI: ffffc90000780000 RDI: ffff8880451f9800
[    7.811170] RBP: ffff8880451f9800 R08: ffffffffffffffff R09: 0000000000000000
[    7.818303] R10: 0000000000000000 R11: 0000000000000000 R12: 0008000000000000
[    7.825435] R13: ffff8880462ea900 R14: 0000000000000021 R15: 0000000000000000
[    7.832572] FS:  0000000000000000(0000) GS:ffff888054a00000(0000) knlGS:0000000000000000
[    7.840657] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    7.846400] CR2: ffff888054dff000 CR3: 0000000053210000 CR4: 0000000000350eb0
[    7.853533] Call Trace:
[    7.855979]  <TASK>
[    7.858085]  amd_iommu_enable_interrupts+0x180/0x270
[    7.863051]  ? iommu_setup+0x271/0x271
[    7.866803]  state_next+0x197/0x2c0
[    7.870295]  ? iommu_setup+0x271/0x271
[    7.874049]  iommu_go_to_state+0x24/0x2c
[    7.877976]  amd_iommu_init+0xf/0x29
[    7.881554]  pci_iommu_init+0xe/0x36
[    7.885133]  do_one_initcall+0x44/0x200
[    7.888975]  do_initcalls+0xc8/0xe1
[    7.892466]  kernel_init_freeable+0x14c/0x199
[    7.896826]  ? rest_init+0xd0/0xd0
[    7.900231]  kernel_init+0x16/0x130
[    7.903723]  ret_from_fork+0x22/0x30
[    7.907306]  </TASK>
[    7.909497] ---[ end trace 0000000000000000 ]---

Fixes: commit a8d4a37d1bb9 ("iommu/amd: Restore GA log/tail pointer on host resume")
Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Maxim Levitsky <mlevitsk@redhat.com>
Cc: Will Deacon <will@kernel.org> (maintainer:IOMMU DRIVERS)
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 85 ++++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index d86496114ca5..4cd94d716122 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -908,11 +908,6 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
 	if (!iommu->ga_log)
 		return -EINVAL;
 
-	/* Check if already running */
-	status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
-	if (WARN_ON(status & (MMIO_STATUS_GALOG_RUN_MASK)))
-		return 0;
-
 	entry = iommu_virt_to_phys(iommu->ga_log) | GA_LOG_SIZE_512;
 	memcpy_toio(iommu->mmio_base + MMIO_GA_LOG_BASE_OFFSET,
 		    &entry, sizeof(entry));
@@ -2068,10 +2063,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 	if (iommu_feature(iommu, FEATURE_PPR) && alloc_ppr_log(iommu))
 		return -ENOMEM;
 
-	ret = iommu_init_ga_log(iommu);
-	if (ret)
-		return ret;
-
 	if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE)) {
 		pr_info("Using strict mode due to virtualization\n");
 		iommu_set_dma_strict();
@@ -2155,8 +2146,6 @@ static void print_iommu_info(void)
 	}
 	if (irq_remapping_enabled) {
 		pr_info("Interrupt remapping enabled\n");
-		if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
-			pr_info("Virtual APIC enabled\n");
 		if (amd_iommu_xt_mode == IRQ_REMAP_X2APIC_MODE)
 			pr_info("X2APIC enabled\n");
 	}
@@ -2446,9 +2435,6 @@ static int iommu_init_irq(struct amd_iommu *iommu)
 
 	if (iommu->ppr_log != NULL)
 		iommu_feature_enable(iommu, CONTROL_PPRINT_EN);
-
-	iommu_ga_log_enable(iommu);
-
 	return 0;
 }
 
@@ -2678,8 +2664,6 @@ static void iommu_enable_ga(struct amd_iommu *iommu)
 #ifdef CONFIG_IRQ_REMAP
 	switch (amd_iommu_guest_ir) {
 	case AMD_IOMMU_GUEST_IR_VAPIC:
-		iommu_feature_enable(iommu, CONTROL_GAM_EN);
-		fallthrough;
 	case AMD_IOMMU_GUEST_IR_LEGACY_GA:
 		iommu_feature_enable(iommu, CONTROL_GA_EN);
 		iommu->irte_ops = &irte_128_ops;
@@ -2759,19 +2743,6 @@ static void early_enable_iommus(void)
 			iommu_flush_all_caches(iommu);
 		}
 	}
-
-#ifdef CONFIG_IRQ_REMAP
-	/*
-	 * Note: We have already checked GASup from IVRS table.
-	 *       Now, we need to make sure that GAMSup is set.
-	 */
-	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
-	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC))
-		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
-
-	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
-		amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
-#endif
 }
 
 static void enable_iommus_v2(void)
@@ -2784,10 +2755,63 @@ static void enable_iommus_v2(void)
 	}
 }
 
+static void enable_iommus_vapic(void)
+{
+#ifdef CONFIG_IRQ_REMAP
+	u32 status, i;
+	struct amd_iommu *iommu;
+
+	for_each_iommu(iommu) {
+		/*
+		 * Disable GALog if already running. It could have been enabled
+		 * in the previous boot before kdump.
+		 */
+		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
+		if (!(status & MMIO_STATUS_GALOG_RUN_MASK))
+			continue;
+
+		iommu_feature_disable(iommu, CONTROL_GALOG_EN);
+		iommu_feature_disable(iommu, CONTROL_GAINT_EN);
+
+		/*
+		 * Need to set and poll check the GALOGRun bit to zero before
+		 * we can set/ modify GA Log registers safely.
+		 */
+		for (i = 0; i < LOOP_TIMEOUT; ++i) {
+			status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
+			if (!(status & MMIO_STATUS_GALOG_RUN_MASK))
+				break;
+			udelay(10);
+		}
+
+		if (WARN_ON(i >= LOOP_TIMEOUT))
+			return;
+	}
+
+	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
+	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC)) {
+		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
+		return;
+	}
+
+	/* Enabling GAM support */
+	for_each_iommu(iommu) {
+		if (iommu_init_ga_log(iommu) ||
+		    iommu_ga_log_enable(iommu))
+			return;
+
+		iommu_feature_enable(iommu, CONTROL_GAM_EN);
+	}
+
+	amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
+	pr_info("Virtual APIC enabled\n");
+#endif
+}
+
 static void enable_iommus(void)
 {
 	early_enable_iommus();
-
+	enable_iommus_vapic();
 	enable_iommus_v2();
 }
 
@@ -3126,6 +3150,7 @@ static int __init state_next(void)
 		register_syscore_ops(&amd_iommu_syscore_ops);
 		ret = amd_iommu_init_pci();
 		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_PCI_INIT;
+		enable_iommus_vapic();
 		enable_iommus_v2();
 		break;
 	case IOMMU_PCI_INIT:
-- 
2.34.1

Re: [PATCH 1/2] iommu/amd: Simplify and Consolidate Virtual APIC (AVIC) Enablement

[Jerry Snitselaar]

On Tue, Jul 26, 2022 at 08:43:47AM -0500, Suravee Suthikulpanit wrote:
> Currently, enabling AVIC requires individually detect and enable GAM and
> GALOG features on each IOMMU, which is difficult to keep track on
> multi-IOMMU system, where the features needs to be enabled system-wide.
> 
> In addition, these features do not need to be enabled in early stage. 
> It can be delayed until after amd_iommu_init_pci().
> 
> Therefore, consolidate logic for detecting and enabling IOMMU GAM and
> GALOG features into a helper function, enable_iommus_vapic(), which uses
> the check_feature_on_all_iommus() helper function to ensure system-wide
> support of the features before enabling them, and postpone until after
> amd_iommu_init_pci().
> 
> The new function also check and clean up feature enablement residue from
> previous boot (e.g. in case of booting into kdump kernel), which triggers
> a WARN_ON (shown below) introduced by the commit a8d4a37d1bb9 ("iommu/amd:
> Restore GA log/tail pointer on host resume") in iommu_ga_log_enable().
> 
> [    7.731955] ------------[ cut here ]------------
> [    7.736575] WARNING: CPU: 0 PID: 1 at drivers/iommu/amd/init.c:829 iommu_ga_log_enable.isra.0+0x16f/0x190
> [    7.746135] Modules linked in:
> [    7.749193] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W        --------  ---  5.19.0-0.rc7.53.eln120.x86_64 #1
> [    7.759706] Hardware name: Dell Inc. PowerEdge R7525/04D5GJ, BIOS 2.1.6 03/09/2021
> [    7.767274] RIP: 0010:iommu_ga_log_enable.isra.0+0x16f/0x190
> [    7.772931] Code: 20 20 00 00 8b 00 f6 c4 01 74 da 48 8b 44 24 08 65 48 2b 04 25 28 00 00 00 75 13 48 83 c4 10 5b 5d e9 f5 00 72 00 0f 0b eb e1 <0f> 0b eb dd e8 f8 66 42 00 48 8b 15 f1 85 53 01 e9 29 ff ff ff 48
> [    7.791679] RSP: 0018:ffffc90000107d20 EFLAGS: 00010206
> [    7.796905] RAX: ffffc90000780000 RBX: 0000000000000100 RCX: ffffc90000780000
> [    7.804038] RDX: 0000000000000001 RSI: ffffc90000780000 RDI: ffff8880451f9800
> [    7.811170] RBP: ffff8880451f9800 R08: ffffffffffffffff R09: 0000000000000000
> [    7.818303] R10: 0000000000000000 R11: 0000000000000000 R12: 0008000000000000
> [    7.825435] R13: ffff8880462ea900 R14: 0000000000000021 R15: 0000000000000000
> [    7.832572] FS:  0000000000000000(0000) GS:ffff888054a00000(0000) knlGS:0000000000000000
> [    7.840657] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    7.846400] CR2: ffff888054dff000 CR3: 0000000053210000 CR4: 0000000000350eb0
> [    7.853533] Call Trace:
> [    7.855979]  <TASK>
> [    7.858085]  amd_iommu_enable_interrupts+0x180/0x270
> [    7.863051]  ? iommu_setup+0x271/0x271
> [    7.866803]  state_next+0x197/0x2c0
> [    7.870295]  ? iommu_setup+0x271/0x271
> [    7.874049]  iommu_go_to_state+0x24/0x2c
> [    7.877976]  amd_iommu_init+0xf/0x29
> [    7.881554]  pci_iommu_init+0xe/0x36
> [    7.885133]  do_one_initcall+0x44/0x200
> [    7.888975]  do_initcalls+0xc8/0xe1
> [    7.892466]  kernel_init_freeable+0x14c/0x199
> [    7.896826]  ? rest_init+0xd0/0xd0
> [    7.900231]  kernel_init+0x16/0x130
> [    7.903723]  ret_from_fork+0x22/0x30
> [    7.907306]  </TASK>
> [    7.909497] ---[ end trace 0000000000000000 ]---
> 
> Fixes: commit a8d4a37d1bb9 ("iommu/amd: Restore GA log/tail pointer on host resume")
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Maxim Levitsky <mlevitsk@redhat.com>
> Cc: Will Deacon <will@kernel.org> (maintainer:IOMMU DRIVERS)
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

> ---
>  drivers/iommu/amd/init.c | 85 ++++++++++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index d86496114ca5..4cd94d716122 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -908,11 +908,6 @@ static int iommu_ga_log_enable(struct amd_iommu *iommu)
>  	if (!iommu->ga_log)
>  		return -EINVAL;
>  
> -	/* Check if already running */
> -	status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> -	if (WARN_ON(status & (MMIO_STATUS_GALOG_RUN_MASK)))
> -		return 0;
> -
>  	entry = iommu_virt_to_phys(iommu->ga_log) | GA_LOG_SIZE_512;
>  	memcpy_toio(iommu->mmio_base + MMIO_GA_LOG_BASE_OFFSET,
>  		    &entry, sizeof(entry));
> @@ -2068,10 +2063,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
>  	if (iommu_feature(iommu, FEATURE_PPR) && alloc_ppr_log(iommu))
>  		return -ENOMEM;
>  
> -	ret = iommu_init_ga_log(iommu);
> -	if (ret)
> -		return ret;
> -
>  	if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE)) {
>  		pr_info("Using strict mode due to virtualization\n");
>  		iommu_set_dma_strict();
> @@ -2155,8 +2146,6 @@ static void print_iommu_info(void)
>  	}
>  	if (irq_remapping_enabled) {
>  		pr_info("Interrupt remapping enabled\n");
> -		if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
> -			pr_info("Virtual APIC enabled\n");
>  		if (amd_iommu_xt_mode == IRQ_REMAP_X2APIC_MODE)
>  			pr_info("X2APIC enabled\n");
>  	}
> @@ -2446,9 +2435,6 @@ static int iommu_init_irq(struct amd_iommu *iommu)
>  
>  	if (iommu->ppr_log != NULL)
>  		iommu_feature_enable(iommu, CONTROL_PPRINT_EN);
> -
> -	iommu_ga_log_enable(iommu);
> -
>  	return 0;
>  }
>  
> @@ -2678,8 +2664,6 @@ static void iommu_enable_ga(struct amd_iommu *iommu)
>  #ifdef CONFIG_IRQ_REMAP
>  	switch (amd_iommu_guest_ir) {
>  	case AMD_IOMMU_GUEST_IR_VAPIC:
> -		iommu_feature_enable(iommu, CONTROL_GAM_EN);
> -		fallthrough;
>  	case AMD_IOMMU_GUEST_IR_LEGACY_GA:
>  		iommu_feature_enable(iommu, CONTROL_GA_EN);
>  		iommu->irte_ops = &irte_128_ops;
> @@ -2759,19 +2743,6 @@ static void early_enable_iommus(void)
>  			iommu_flush_all_caches(iommu);
>  		}
>  	}
> -
> -#ifdef CONFIG_IRQ_REMAP
> -	/*
> -	 * Note: We have already checked GASup from IVRS table.
> -	 *       Now, we need to make sure that GAMSup is set.
> -	 */
> -	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
> -	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC))
> -		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> -
> -	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
> -		amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
> -#endif
>  }
>  
>  static void enable_iommus_v2(void)
> @@ -2784,10 +2755,63 @@ static void enable_iommus_v2(void)
>  	}
>  }
>  
> +static void enable_iommus_vapic(void)
> +{
> +#ifdef CONFIG_IRQ_REMAP
> +	u32 status, i;
> +	struct amd_iommu *iommu;
> +
> +	for_each_iommu(iommu) {
> +		/*
> +		 * Disable GALog if already running. It could have been enabled
> +		 * in the previous boot before kdump.
> +		 */
> +		status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> +		if (!(status & MMIO_STATUS_GALOG_RUN_MASK))
> +			continue;
> +
> +		iommu_feature_disable(iommu, CONTROL_GALOG_EN);
> +		iommu_feature_disable(iommu, CONTROL_GAINT_EN);
> +
> +		/*
> +		 * Need to set and poll check the GALOGRun bit to zero before
> +		 * we can set/ modify GA Log registers safely.
> +		 */
> +		for (i = 0; i < LOOP_TIMEOUT; ++i) {
> +			status = readl(iommu->mmio_base + MMIO_STATUS_OFFSET);
> +			if (!(status & MMIO_STATUS_GALOG_RUN_MASK))
> +				break;
> +			udelay(10);
> +		}
> +
> +		if (WARN_ON(i >= LOOP_TIMEOUT))
> +			return;
> +	}
> +
> +	if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) &&
> +	    !check_feature_on_all_iommus(FEATURE_GAM_VAPIC)) {
> +		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> +		return;
> +	}
> +
> +	/* Enabling GAM support */
> +	for_each_iommu(iommu) {
> +		if (iommu_init_ga_log(iommu) ||
> +		    iommu_ga_log_enable(iommu))
> +			return;
> +
> +		iommu_feature_enable(iommu, CONTROL_GAM_EN);
> +	}
> +
> +	amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
> +	pr_info("Virtual APIC enabled\n");
> +#endif
> +}
> +
>  static void enable_iommus(void)
>  {
>  	early_enable_iommus();
> -
> +	enable_iommus_vapic();
>  	enable_iommus_v2();
>  }
>  
> @@ -3126,6 +3150,7 @@ static int __init state_next(void)
>  		register_syscore_ops(&amd_iommu_syscore_ops);
>  		ret = amd_iommu_init_pci();
>  		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_PCI_INIT;
> +		enable_iommus_vapic();
>  		enable_iommus_v2();
>  		break;
>  	case IOMMU_PCI_INIT:
> -- 
> 2.34.1
> 

[PATCH 2/2] iommu/amd: Add support for AVIC when SNP is enabled

[Suravee Suthikulpanit]

In order to support AVIC on SNP-enabled system, The IOMMU driver needs to
check EFR2[SNPAVICSup] and enables the support by setting SNPAVICEn bit
in the IOMMU control register (MMIO offset 18h).

For detail, please see section "SEV-SNP Guest Virtual APIC Support" of the
AMD I/O Virtualization Technology (IOMMU) Specification.
(https://www.amd.com/system/files/TechDocs/48882_IOMMU.pdf)

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h |  7 +++++++
 drivers/iommu/amd/init.c            | 11 ++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 3c1205ba636a..5b1019dab328 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -103,6 +103,12 @@
 #define FEATURE_GLXVAL_SHIFT	14
 #define FEATURE_GLXVAL_MASK	(0x03ULL << FEATURE_GLXVAL_SHIFT)
 
+/* Extended Feature 2 Bits */
+#define FEATURE_SNPAVICSUP_SHIFT	5
+#define FEATURE_SNPAVICSUP_MASK		(0x07ULL << FEATURE_SNPAVICSUP_SHIFT)
+#define FEATURE_SNPAVICSUP_GAM(x) \
+	((x & FEATURE_SNPAVICSUP_MASK) >> FEATURE_SNPAVICSUP_SHIFT == 0x1)
+
 /* Note:
  * The current driver only support 16-bit PASID.
  * Currently, hardware only implement upto 16-bit PASID
@@ -165,6 +171,7 @@
 #define CONTROL_GAINT_EN	29
 #define CONTROL_XT_EN		50
 #define CONTROL_INTCAPXT_EN	51
+#define CONTROL_SNPAVIC_EN	61
 
 #define CTRL_INV_TO_MASK	(7 << CONTROL_INV_TIMEOUT)
 #define CTRL_INV_TO_NONE	0
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 4cd94d716122..6bbaf6b971e8 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2794,13 +2794,22 @@ static void enable_iommus_vapic(void)
 		return;
 	}
 
-	/* Enabling GAM support */
+	if (amd_iommu_snp_en &&
+	    !FEATURE_SNPAVICSUP_GAM(amd_iommu_efr2)) {
+		pr_warn("Force to disable Virtual APIC due to SNP\n");
+		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
+		return;
+	}
+
+	/* Enabling GAM and SNPAVIC support */
 	for_each_iommu(iommu) {
 		if (iommu_init_ga_log(iommu) ||
 		    iommu_ga_log_enable(iommu))
 			return;
 
 		iommu_feature_enable(iommu, CONTROL_GAM_EN);
+		if (amd_iommu_snp_en)
+			iommu_feature_enable(iommu, CONTROL_SNPAVIC_EN);
 	}
 
 	amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
-- 
2.34.1

Re: [PATCH 2/2] iommu/amd: Add support for AVIC when SNP is enabled

[Jerry Snitselaar]

On Tue, Jul 26, 2022 at 08:43:48AM -0500, Suravee Suthikulpanit wrote:
> In order to support AVIC on SNP-enabled system, The IOMMU driver needs to
> check EFR2[SNPAVICSup] and enables the support by setting SNPAVICEn bit
> in the IOMMU control register (MMIO offset 18h).
> 
> For detail, please see section "SEV-SNP Guest Virtual APIC Support" of the
> AMD I/O Virtualization Technology (IOMMU) Specification.
> (https://www.amd.com/system/files/TechDocs/48882_IOMMU.pdf)
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Is this a typo in the 3.06 spec for SNPAVIC_EN?

"001b-111b = Reserved."

Or I guess maybe there is a newer revision that isn't available yet
that describes 001b?

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

> ---
>  drivers/iommu/amd/amd_iommu_types.h |  7 +++++++
>  drivers/iommu/amd/init.c            | 11 ++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 3c1205ba636a..5b1019dab328 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -103,6 +103,12 @@
>  #define FEATURE_GLXVAL_SHIFT	14
>  #define FEATURE_GLXVAL_MASK	(0x03ULL << FEATURE_GLXVAL_SHIFT)
>  
> +/* Extended Feature 2 Bits */
> +#define FEATURE_SNPAVICSUP_SHIFT	5
> +#define FEATURE_SNPAVICSUP_MASK		(0x07ULL << FEATURE_SNPAVICSUP_SHIFT)
> +#define FEATURE_SNPAVICSUP_GAM(x) \
> +	((x & FEATURE_SNPAVICSUP_MASK) >> FEATURE_SNPAVICSUP_SHIFT == 0x1)
> +
>  /* Note:
>   * The current driver only support 16-bit PASID.
>   * Currently, hardware only implement upto 16-bit PASID
> @@ -165,6 +171,7 @@
>  #define CONTROL_GAINT_EN	29
>  #define CONTROL_XT_EN		50
>  #define CONTROL_INTCAPXT_EN	51
> +#define CONTROL_SNPAVIC_EN	61
>  
>  #define CTRL_INV_TO_MASK	(7 << CONTROL_INV_TIMEOUT)
>  #define CTRL_INV_TO_NONE	0
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 4cd94d716122..6bbaf6b971e8 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -2794,13 +2794,22 @@ static void enable_iommus_vapic(void)
>  		return;
>  	}
>  
> -	/* Enabling GAM support */
> +	if (amd_iommu_snp_en &&
> +	    !FEATURE_SNPAVICSUP_GAM(amd_iommu_efr2)) {
> +		pr_warn("Force to disable Virtual APIC due to SNP\n");
> +		amd_iommu_guest_ir = AMD_IOMMU_GUEST_IR_LEGACY_GA;
> +		return;
> +	}
> +
> +	/* Enabling GAM and SNPAVIC support */
>  	for_each_iommu(iommu) {
>  		if (iommu_init_ga_log(iommu) ||
>  		    iommu_ga_log_enable(iommu))
>  			return;
>  
>  		iommu_feature_enable(iommu, CONTROL_GAM_EN);
> +		if (amd_iommu_snp_en)
> +			iommu_feature_enable(iommu, CONTROL_SNPAVIC_EN);
>  	}
>  
>  	amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] iommu/amd: Add support for AVIC when SNP is enabled

[Suravee Suthikulpanit]

On 7/26/22 10:32 PM, Jerry Snitselaar wrote:
> On Tue, Jul 26, 2022 at 08:43:48AM -0500, Suravee Suthikulpanit wrote:
>> In order to support AVIC on SNP-enabled system, The IOMMU driver needs to
>> check EFR2[SNPAVICSup] and enables the support by setting SNPAVICEn bit
>> in the IOMMU control register (MMIO offset 18h).
>>
>> For detail, please see section "SEV-SNP Guest Virtual APIC Support" of the
>> AMD I/O Virtualization Technology (IOMMU) Specification.
>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fsystem%2Ffiles%2FTechDocs%2F48882_IOMMU.pdf&data=05%7C01%7Csuravee.suthikulpanit%40amd.com%7C844cc78ad4674484b1f208da6f1c0ee1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637944463702725122%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=U%2Fe%2FxqqQ14UmlF5ThaB55Col4mbNzuqNj8IqEyxmsxo%3D&reserved=0)
>>
>> Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
> Is this a typo in the 3.06 spec for SNPAVIC_EN?
> 
> "001b-111b = Reserved."
> 
> Or I guess maybe there is a newer revision that isn't available yet
> that describes 001b?

That's correct. The newer version should be coming out soon.

> Reviewed-by: Jerry Snitselaar<jsnitsel@redhat.com>

Thank you,
Suravee

Re: [PATCH 0/2] iommu/amd: Misc Update for Virtual APIC support

[Joerg Roedel]

On Tue, Jul 26, 2022 at 08:43:46AM -0500, Suravee Suthikulpanit wrote:
> Suravee Suthikulpanit (2):
>   iommu/amd: Consolidate Virtual APIC (AVIC) Enablement
>   iommu/amd: Add support for AVIC when SNP is enabled

Applied, thanks.