diff mbox series

[1/4] iommu/amd: Check SNP support before enabling IOMMU

Message ID 0b74c3fce90ea464621c0be1dbf681bf46f1aadd.1737505394.git.ashish.kalra@amd.com (mailing list archive)
State New
Headers show
Series Fix broken SNP support with KVM module built-in | expand

Commit Message

Kalra, Ashish Jan. 22, 2025, 1 a.m. UTC 
From: Vasant Hegde <vasant.hegde@amd.com>

iommu_snp_enable() checks for IOMMU feature support and page table
compatibility. Ideally this check should be done before enabling
IOMMUs. Currently its done after enabling IOMMUs. Also its causes
issue if kvm_amd is builtin.

Hence move SNP enable check before enabling IOMMUs.

Fixes: 04d65a9dbb33 ("iommu/amd: Don't rely on external callers to enable IOMMU SNP support")
Cc: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Tom Lendacky Jan. 22, 2025, 3:22 p.m. UTC | #1 
On 1/21/25 19:00, Ashish Kalra wrote:
> From: Vasant Hegde <vasant.hegde@amd.com>
> 
> iommu_snp_enable() checks for IOMMU feature support and page table
> compatibility. Ideally this check should be done before enabling
> IOMMUs. Currently its done after enabling IOMMUs. Also its causes

Why should it be done before enabling the IOMMUs? In other words, at
some more detail here.

> issue if kvm_amd is builtin.
> 
> Hence move SNP enable check before enabling IOMMUs.
> 
> Fixes: 04d65a9dbb33 ("iommu/amd: Don't rely on external callers to enable IOMMU SNP support")
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>

Ashish, as the submitter, this requires your Signed-off-by:.

> ---
>  drivers/iommu/amd/init.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index c5cd92edada0..419a0bc8eeea 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3256,13 +3256,14 @@ static int __init state_next(void)
>  		}
>  		break;
>  	case IOMMU_ACPI_FINISHED:
> +		/* SNP enable has to be called after early_amd_iommu_init() */

This comment doesn't really explain anything, so I think it should
either be improved or just remove it.

Thanks,
Tom

> +		iommu_snp_enable();
>  		early_enable_iommus();
>  		x86_platform.iommu_shutdown = disable_iommus;
>  		init_state = IOMMU_ENABLED;
>  		break;
>  	case IOMMU_ENABLED:
>  		register_syscore_ops(&amd_iommu_syscore_ops);
> -		iommu_snp_enable();
>  		ret = amd_iommu_init_pci();
>  		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_PCI_INIT;
>  		break;
Vasant Hegde Jan. 22, 2025, 5:07 p.m. UTC | #2 
Hi Tom,


On 1/22/2025 8:52 PM, Tom Lendacky wrote:
> On 1/21/25 19:00, Ashish Kalra wrote:
>> From: Vasant Hegde <vasant.hegde@amd.com>
>>
>> iommu_snp_enable() checks for IOMMU feature support and page table
>> compatibility. Ideally this check should be done before enabling
>> IOMMUs. Currently its done after enabling IOMMUs. Also its causes
> 
> Why should it be done before enabling the IOMMUs? In other words, at
> some more detail here.

Sure. Basically IOMMU enable stage checks for SNP support. I will update it.

> 
>> issue if kvm_amd is builtin.
>>
>> Hence move SNP enable check before enabling IOMMUs.
>>
>> Fixes: 04d65a9dbb33 ("iommu/amd: Don't rely on external callers to enable IOMMU SNP support")
>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> 
> Ashish, as the submitter, this requires your Signed-off-by:.
> 
>> ---
>>  drivers/iommu/amd/init.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>> index c5cd92edada0..419a0bc8eeea 100644
>> --- a/drivers/iommu/amd/init.c
>> +++ b/drivers/iommu/amd/init.c
>> @@ -3256,13 +3256,14 @@ static int __init state_next(void)
>>  		}
>>  		break;
>>  	case IOMMU_ACPI_FINISHED:
>> +		/* SNP enable has to be called after early_amd_iommu_init() */
> 
> This comment doesn't really explain anything, so I think it should
> either be improved or just remove it.

Sure. I will remove it.

-Vasant
Kalra, Ashish Jan. 24, 2025, 9:46 p.m. UTC | #3 
Hello All,

On 1/22/2025 11:07 AM, Vasant Hegde wrote:
> Hi Tom,
> 
> 
> On 1/22/2025 8:52 PM, Tom Lendacky wrote:
>> On 1/21/25 19:00, Ashish Kalra wrote:
>>> From: Vasant Hegde <vasant.hegde@amd.com>
>>>
>>> iommu_snp_enable() checks for IOMMU feature support and page table
>>> compatibility. Ideally this check should be done before enabling
>>> IOMMUs. Currently its done after enabling IOMMUs. Also its causes
>>
>> Why should it be done before enabling the IOMMUs? In other words, at
>> some more detail here.
> 
> Sure. Basically IOMMU enable stage checks for SNP support. I will update it.
> 
>>
>>> issue if kvm_amd is builtin.
>>>
>>> Hence move SNP enable check before enabling IOMMUs.
>>>
>>> Fixes: 04d65a9dbb33 ("iommu/amd: Don't rely on external callers to enable IOMMU SNP support")
>>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>>
>> Ashish, as the submitter, this requires your Signed-off-by:.
>>
>>> ---
>>>  drivers/iommu/amd/init.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>>> index c5cd92edada0..419a0bc8eeea 100644
>>> --- a/drivers/iommu/amd/init.c
>>> +++ b/drivers/iommu/amd/init.c
>>> @@ -3256,13 +3256,14 @@ static int __init state_next(void)
>>>  		}
>>>  		break;
>>>  	case IOMMU_ACPI_FINISHED:
>>> +		/* SNP enable has to be called after early_amd_iommu_init() */
>>
>> This comment doesn't really explain anything, so I think it should
>> either be improved or just remove it.
> 
> Sure. I will remove it.

We have hit a blocker issue with this patch. 

With discussions with the AMD IOMMU team, here is the AMD IOMMU initialization flow:

IOMMU initialization happens in various stages.

1) Detect IOMMU Presence
   start_kernel() -> mm_core_init() -> mem_init() -> pci_iommu_alloc() -> amd_iommu_detect()
   At this stage memory subsystem is not initialized completely. So we just detect the IOMMU presence.

2) Interrupt Remapping
   During APIC init it checks for IOMMU interrupt remapping. At this stage, we initialize the IOMMU and
   enable the IOMMU.
   start_kernel() -> x86_late_time_init() -> apic_intr_mode_init() -> x86_64_probe_apic() -> 
		enable_IR_x2apic() -> irq_remapping_prepare() -> amd_iommu_prepare()

3) PCI init
   This is done using rootfs_initcall(pci_iommu_init);
   pci_iommu_init() -> amd_iommu_init()
   At this stage we enable the IOMMU interrupt, probe device etc. IOMMU is ready to use.

IOMMU SNP check
  Core IOMMU subsystem init is done during iommu_subsys_init() via subsys_initcall.
  This function does change the DMA mode depending on kernel config.
  Hence, SNP check should be done after subsys_initcall. That's why its done currently during IOMMU PCI init (IOMMU_PCI_INIT stage).
  And for that reason snp_rmptable_init() is currently invoked via device_initcall().
 
The summary is that we cannot move snp_rmptable_init() to subsys_initcall as core IOMMU subsystem gets initialized via subsys_initcall.

As discussed internally, we have 2 possible options to fix this: 

1 ) Similar to calling sp_mod_init() to explicitly initialize the PSP driver, we call snp_rmptable_init() from KVM_AMD if it's built-in. 
    So that we don't need changes to IOMMU driver ... as IOMMU driver does SNP check as part of rootfs_initcall()  (amd_iommu_init())
 
2) Rework it such that  
   Core IOMMU (iommu_subsys_init()) initialized (as currently) via subsys_initcall
   FIX: 
   And then we add a fix to invoke snp_rmptable_init() via a subsys_initcall_sync() (instead of device_initcall()).
   (again as core IOMMU subsystem init is called by subsys_initcall()) 
   --> snp_rmptable_init() will additionally need to call iommu_snp_enable() to check and enable SNP support on the IOMMU.

Issues with option (1): 

Here, snp_rmptable_init() is still invoked via device_initcall() for normal module loading case and i remember Sean had concerns of enabling
host SNP at device_initcall level generally as it is too late, though looking at AMD IOMMU driver initialization flow, i don't think there is
much of a choice here.

One other possibility is moving snp_rmptable_init() call to KVM initialization, but that has issues with PSP driver loading and
initializing before KVM (with module loading case) and that will cause PSP's SEV/SNP init to fail as SNP is not enabled yet.
 
But again that will work when SEV/SNP init will move to KVM module load time where PSP module will be simply be loaded and initialized
before KVM, but will not attempt to do SEV/SNP init.
 
This approach is quite fragile and will need to be tested and needs to work with multiple scenarios and cases as explained above, there is
a good chance of this breaking something.

And that's why option (2) is preferred. 

Issues with option (2): 

How to call iommu_snp_enable() from snp_rmptable_init() ?

We probably can't call iommu_snp_enable() explicitly from snp_rmptable_init() as i remember the last time we proposed this, it was rejected as
maintainers were not in favor of core kernel code calling driver functions, though the AMD IOMMU driver is always built-in ?

Therefore, probably the approach will be something like AMD_IOMMU driver registering some kind of callback interface and that callback is 
invoked via snp_rmptable_init() to check and enable SNP support on the IOMMU. 

Boris, it will be nice to have your feedback/thoughts on this approach/option? 

Looking fwd. to more feedback/thoughts/comments on the above.

Thanks,
Ashish
Sean Christopherson Jan. 25, 2025, 12:39 a.m. UTC | #4 
On Fri, Jan 24, 2025, Ashish Kalra wrote:
> With discussions with the AMD IOMMU team, here is the AMD IOMMU
> initialization flow:

..

> IOMMU SNP check
>   Core IOMMU subsystem init is done during iommu_subsys_init() via
>   subsys_initcall.  This function does change the DMA mode depending on
>   kernel config.  Hence, SNP check should be done after subsys_initcall.
>   That's why its done currently during IOMMU PCI init (IOMMU_PCI_INIT stage).
>   And for that reason snp_rmptable_init() is currently invoked via
>   device_initcall().
>  
> The summary is that we cannot move snp_rmptable_init() to subsys_initcall as
> core IOMMU subsystem gets initialized via subsys_initcall.

Just explicitly invoke RMP initialization during IOMMU SNP setup.  Pretending
there's no connection when snp_rmptable_init() checks amd_iommu_snp_en and has
a comment saying it needs to come after IOMMU SNP setup is ridiculous.

Compile tested only.

---
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 24 Jan 2025 16:25:58 -0800
Subject: [PATCH] x86/sev: iommu/amd: Explicitly init SNP's RMP table during
 IOMMU SNP setup

Explicitly initialize the RMP table during IOMMU SNP setup, as there is a
hard dependency on the IOMMU being configured first, and dancing around
the dependency with initcall shenanigans and a comment is all kinds of
stupid.

The RMP is blatantly not a device; initializing it via a device_initcall()
is confusing and "works" only because of dumb luck: due to kernel build
order, when the the PSP driver is built-in, its effective device_initcall()
just so happens to be invoked after snp_rmptable_init().

That all falls apart if the order is changed in any way.  E.g. if KVM
is built-in and attempts to access the RMP during its device_initcall(),
chaos ensues.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/sev.h |  1 +
 arch/x86/virt/svm/sev.c    | 25 ++++++++-----------------
 drivers/iommu/amd/init.c   |  7 ++++++-
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 91f08af31078..30da0fc15923 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -503,6 +503,7 @@ static inline void snp_kexec_begin(void) { }
 
 #ifdef CONFIG_KVM_AMD_SEV
 bool snp_probe_rmptable_info(void);
+int __init snp_rmptable_init(void);
 int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level);
 void snp_dump_hva_rmpentry(unsigned long address);
 int psmash(u64 pfn);
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 9a6a943d8e41..d932aa21340b 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -189,19 +189,19 @@ void __init snp_fixup_e820_tables(void)
  * described in the SNP_INIT_EX firmware command description in the SNP
  * firmware ABI spec.
  */
-static int __init snp_rmptable_init(void)
+int __init snp_rmptable_init(void)
 {
 	u64 max_rmp_pfn, calc_rmp_sz, rmptable_size, rmp_end, val;
 	void *rmptable_start;
 
-	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
-		return 0;
+	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
+		return -ENOSYS;
 
-	if (!amd_iommu_snp_en)
-		goto nosnp;
+	if (WARN_ON_ONCE(!amd_iommu_snp_en))
+		return -ENOSYS;
 
 	if (!probed_rmp_size)
-		goto nosnp;
+		return -ENOSYS;
 
 	rmp_end = probed_rmp_base + probed_rmp_size - 1;
 
@@ -218,13 +218,13 @@ static int __init snp_rmptable_init(void)
 	if (calc_rmp_sz > probed_rmp_size) {
 		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
 		       calc_rmp_sz, probed_rmp_size);
-		goto nosnp;
+		return -ENOSYS;
 	}
 
 	rmptable_start = memremap(probed_rmp_base, probed_rmp_size, MEMREMAP_WB);
 	if (!rmptable_start) {
 		pr_err("Failed to map RMP table\n");
-		goto nosnp;
+		return -ENOMEM;
 	}
 
 	/*
@@ -261,17 +261,8 @@ static int __init snp_rmptable_init(void)
 	crash_kexec_post_notifiers = true;
 
 	return 0;
-
-nosnp:
-	cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
-	return -ENOSYS;
 }
 
-/*
- * This must be called after the IOMMU has been initialized.
- */
-device_initcall(snp_rmptable_init);
-
 static struct rmpentry *get_rmpentry(u64 pfn)
 {
 	if (WARN_ON_ONCE(pfn > rmptable_max_pfn))
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 0e0a531042ac..d00530156a72 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3171,7 +3171,7 @@ static bool __init detect_ivrs(void)
 	return true;
 }
 
-static void iommu_snp_enable(void)
+static __init void iommu_snp_enable(void)
 {
 #ifdef CONFIG_KVM_AMD_SEV
 	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
@@ -3196,6 +3196,11 @@ static void iommu_snp_enable(void)
 		goto disable_snp;
 	}
 
+	if (snp_rmptable_init()) {
+		pr_warn("SNP: RMP initialization failed, SNP cannot be supported.\n");
+		goto disable_snp;
+	}
+
 	pr_info("IOMMU SNP support enabled.\n");
 	return;
 

base-commit: ac80076177131f6e3291737c851a6fe32cc03fd3
--
Kalra, Ashish Jan. 27, 2025, 8:43 p.m. UTC | #5 
Hello Sean,

On 1/24/2025 6:39 PM, Sean Christopherson wrote:
> On Fri, Jan 24, 2025, Ashish Kalra wrote:
>> With discussions with the AMD IOMMU team, here is the AMD IOMMU
>> initialization flow:
> 
> ..
> 
>> IOMMU SNP check
>>   Core IOMMU subsystem init is done during iommu_subsys_init() via
>>   subsys_initcall.  This function does change the DMA mode depending on
>>   kernel config.  Hence, SNP check should be done after subsys_initcall.
>>   That's why its done currently during IOMMU PCI init (IOMMU_PCI_INIT stage).
>>   And for that reason snp_rmptable_init() is currently invoked via
>>   device_initcall().
>>  
>> The summary is that we cannot move snp_rmptable_init() to subsys_initcall as
>> core IOMMU subsystem gets initialized via subsys_initcall.
> 
> Just explicitly invoke RMP initialization during IOMMU SNP setup.  Pretending
> there's no connection when snp_rmptable_init() checks amd_iommu_snp_en and has
> a comment saying it needs to come after IOMMU SNP setup is ridiculous.
> 

Thanks for the suggestion and the patch, i have tested it works for all cases
and scenarios. I will post the next version of the patch-set based on this
patch.

Ashish

> Compile tested only.
> 
> ---
> From: Sean Christopherson <seanjc@google.com>
> Date: Fri, 24 Jan 2025 16:25:58 -0800
> Subject: [PATCH] x86/sev: iommu/amd: Explicitly init SNP's RMP table during
>  IOMMU SNP setup
> 
> Explicitly initialize the RMP table during IOMMU SNP setup, as there is a
> hard dependency on the IOMMU being configured first, and dancing around
> the dependency with initcall shenanigans and a comment is all kinds of
> stupid.
> 
> The RMP is blatantly not a device; initializing it via a device_initcall()
> is confusing and "works" only because of dumb luck: due to kernel build
> order, when the the PSP driver is built-in, its effective device_initcall()
> just so happens to be invoked after snp_rmptable_init().
> 
> That all falls apart if the order is changed in any way.  E.g. if KVM
> is built-in and attempts to access the RMP during its device_initcall(),
> chaos ensues.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/sev.h |  1 +
>  arch/x86/virt/svm/sev.c    | 25 ++++++++-----------------
>  drivers/iommu/amd/init.c   |  7 ++++++-
>  3 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 91f08af31078..30da0fc15923 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -503,6 +503,7 @@ static inline void snp_kexec_begin(void) { }
>  
>  #ifdef CONFIG_KVM_AMD_SEV
>  bool snp_probe_rmptable_info(void);
> +int __init snp_rmptable_init(void);
>  int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level);
>  void snp_dump_hva_rmpentry(unsigned long address);
>  int psmash(u64 pfn);
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 9a6a943d8e41..d932aa21340b 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -189,19 +189,19 @@ void __init snp_fixup_e820_tables(void)
>   * described in the SNP_INIT_EX firmware command description in the SNP
>   * firmware ABI spec.
>   */
> -static int __init snp_rmptable_init(void)
> +int __init snp_rmptable_init(void)
>  {
>  	u64 max_rmp_pfn, calc_rmp_sz, rmptable_size, rmp_end, val;
>  	void *rmptable_start;
>  
> -	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> -		return 0;
> +	if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP)))
> +		return -ENOSYS;
>  
> -	if (!amd_iommu_snp_en)
> -		goto nosnp;
> +	if (WARN_ON_ONCE(!amd_iommu_snp_en))
> +		return -ENOSYS;
>  
>  	if (!probed_rmp_size)
> -		goto nosnp;
> +		return -ENOSYS;
>  
>  	rmp_end = probed_rmp_base + probed_rmp_size - 1;
>  
> @@ -218,13 +218,13 @@ static int __init snp_rmptable_init(void)
>  	if (calc_rmp_sz > probed_rmp_size) {
>  		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
>  		       calc_rmp_sz, probed_rmp_size);
> -		goto nosnp;
> +		return -ENOSYS;
>  	}
>  
>  	rmptable_start = memremap(probed_rmp_base, probed_rmp_size, MEMREMAP_WB);
>  	if (!rmptable_start) {
>  		pr_err("Failed to map RMP table\n");
> -		goto nosnp;
> +		return -ENOMEM;
>  	}
>  
>  	/*
> @@ -261,17 +261,8 @@ static int __init snp_rmptable_init(void)
>  	crash_kexec_post_notifiers = true;
>  
>  	return 0;
> -
> -nosnp:
> -	cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
> -	return -ENOSYS;
>  }
>  
> -/*
> - * This must be called after the IOMMU has been initialized.
> - */
> -device_initcall(snp_rmptable_init);
> -
>  static struct rmpentry *get_rmpentry(u64 pfn)
>  {
>  	if (WARN_ON_ONCE(pfn > rmptable_max_pfn))
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 0e0a531042ac..d00530156a72 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3171,7 +3171,7 @@ static bool __init detect_ivrs(void)
>  	return true;
>  }
>  
> -static void iommu_snp_enable(void)
> +static __init void iommu_snp_enable(void)
>  {
>  #ifdef CONFIG_KVM_AMD_SEV
>  	if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP))
> @@ -3196,6 +3196,11 @@ static void iommu_snp_enable(void)
>  		goto disable_snp;
>  	}
>  
> +	if (snp_rmptable_init()) {
> +		pr_warn("SNP: RMP initialization failed, SNP cannot be supported.\n");
> +		goto disable_snp;
> +	}
> +
>  	pr_info("IOMMU SNP support enabled.\n");
>  	return;
>  
> 
> base-commit: ac80076177131f6e3291737c851a6fe32cc03fd3
Sean Christopherson Jan. 27, 2025, 9:12 p.m. UTC | #6 
On Mon, Jan 27, 2025, Ashish Kalra wrote:
> Hello Sean,
> 
> On 1/24/2025 6:39 PM, Sean Christopherson wrote:
> > On Fri, Jan 24, 2025, Ashish Kalra wrote:
> >> With discussions with the AMD IOMMU team, here is the AMD IOMMU
> >> initialization flow:
> > 
> > ..
> > 
> >> IOMMU SNP check
> >>   Core IOMMU subsystem init is done during iommu_subsys_init() via
> >>   subsys_initcall.  This function does change the DMA mode depending on
> >>   kernel config.  Hence, SNP check should be done after subsys_initcall.
> >>   That's why its done currently during IOMMU PCI init (IOMMU_PCI_INIT stage).
> >>   And for that reason snp_rmptable_init() is currently invoked via
> >>   device_initcall().
> >>  
> >> The summary is that we cannot move snp_rmptable_init() to subsys_initcall as
> >> core IOMMU subsystem gets initialized via subsys_initcall.
> > 
> > Just explicitly invoke RMP initialization during IOMMU SNP setup.  Pretending
> > there's no connection when snp_rmptable_init() checks amd_iommu_snp_en and has
> > a comment saying it needs to come after IOMMU SNP setup is ridiculous.
> > 
> 
> Thanks for the suggestion and the patch, i have tested it works for all cases
> and scenarios. I will post the next version of the patch-set based on this
> patch.

One thing I didn't account for: if IOMMU initialization fails and iommu_snp_enable()
is never reached, CC_ATTR_HOST_SEV_SNP will be left set.

I don't see any great options.  Something like the below might work?  And maybe
keep a device_initcall() in arch/x86/virt/svm/sev.c that sanity checks that SNP
really is fully enabled?  Dunno, hopefully someone has a better idea.

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 0e0a531042ac..6d62ee8e0055 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3295,6 +3295,9 @@ static int __init iommu_go_to_state(enum iommu_init_state state)
                ret = state_next();
        }
 
+       if (ret && !amd_iommu_snp_en)
+               cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
+
        return ret;
 }
Vasant Hegde Jan. 29, 2025, 9:24 a.m. UTC | #7 
Hi Sean,


On 1/28/2025 2:42 AM, Sean Christopherson wrote:
> On Mon, Jan 27, 2025, Ashish Kalra wrote:
>> Hello Sean,
>>
>> On 1/24/2025 6:39 PM, Sean Christopherson wrote:
>>> On Fri, Jan 24, 2025, Ashish Kalra wrote:
>>>> With discussions with the AMD IOMMU team, here is the AMD IOMMU
>>>> initialization flow:
>>>
>>> ..
>>>
>>>> IOMMU SNP check
>>>>   Core IOMMU subsystem init is done during iommu_subsys_init() via
>>>>   subsys_initcall.  This function does change the DMA mode depending on
>>>>   kernel config.  Hence, SNP check should be done after subsys_initcall.
>>>>   That's why its done currently during IOMMU PCI init (IOMMU_PCI_INIT stage).
>>>>   And for that reason snp_rmptable_init() is currently invoked via
>>>>   device_initcall().
>>>>  
>>>> The summary is that we cannot move snp_rmptable_init() to subsys_initcall as
>>>> core IOMMU subsystem gets initialized via subsys_initcall.
>>>
>>> Just explicitly invoke RMP initialization during IOMMU SNP setup.  Pretending
>>> there's no connection when snp_rmptable_init() checks amd_iommu_snp_en and has
>>> a comment saying it needs to come after IOMMU SNP setup is ridiculous.
>>>
>>
>> Thanks for the suggestion and the patch, i have tested it works for all cases
>> and scenarios. I will post the next version of the patch-set based on this
>> patch.
> 
> One thing I didn't account for: if IOMMU initialization fails and iommu_snp_enable()
> is never reached, CC_ATTR_HOST_SEV_SNP will be left set.
> 
> I don't see any great options.  Something like the below might work?  And maybe

We did explore few other options. But I don't see any other better option.

Below code works fine.  But we still need to handle `iommu=off` or
`amd_iommu=off` kernel command line. Below change will take care of this
scenario. Does this looks OK?

----
commit 8e9296346e8f6a0831a5f6076c81a636bf044a41
Author: Vasant Hegde <vasant.hegde@amd.com>
Date:   Wed Jan 29 14:47:04 2025 +0530

    iommu/amd: SNP fix

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

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index c5cd92edada0..08802316411f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3426,18 +3426,24 @@ void __init amd_iommu_detect(void)
 	int ret;

 	if (no_iommu || (iommu_detected && !gart_iommu_aperture))
-		return;
+		goto disable_snp;

 	if (!amd_iommu_sme_check())
-		return;
+		goto disable_snp;

 	ret = iommu_go_to_state(IOMMU_IVRS_DETECTED);
 	if (ret)
-		return;
+		goto disable_snp;

 	amd_iommu_detected = true;
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
+	return;
+
+disable_snp:
+	/* Disable SNP if amd_iommu is not enabled */
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
 }

 /****************************************************************************




> keep a device_initcall() in arch/x86/virt/svm/sev.c that sanity checks that SNP
> really is fully enabled?  Dunno, hopefully someone has a better idea.

That will not solve the initial problem this series trying to solve (i. e.
kvm_amd as built and making sure SNP init happens before device_initcall() path).

I think with your patch and above changes it should work fine.

-Vasant


> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 0e0a531042ac..6d62ee8e0055 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3295,6 +3295,9 @@ static int __init iommu_go_to_state(enum iommu_init_state state)
>                 ret = state_next();
>         }
>  
> +       if (ret && !amd_iommu_snp_en)
> +               cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
> +
>         return ret;
>  }
diff mbox series

Patch

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index c5cd92edada0..419a0bc8eeea 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3256,13 +3256,14 @@  static int __init state_next(void)
 		}
 		break;
 	case IOMMU_ACPI_FINISHED:
+		/* SNP enable has to be called after early_amd_iommu_init() */
+		iommu_snp_enable();
 		early_enable_iommus();
 		x86_platform.iommu_shutdown = disable_iommus;
 		init_state = IOMMU_ENABLED;
 		break;
 	case IOMMU_ENABLED:
 		register_syscore_ops(&amd_iommu_syscore_ops);
-		iommu_snp_enable();
 		ret = amd_iommu_init_pci();
 		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_PCI_INIT;
 		break;