diff mbox series

crypto: ccp - Have it depend on AMD_IOMMU

Message ID 20240207204721.6189-1-bp@alien8.de
State New
Headers
Series crypto: ccp - Have it depend on AMD_IOMMU |

Commit Message

Borislav Petkov Feb. 7, 2024, 8:47 p.m. UTC 
  From: "Borislav Petkov (AMD)" <bp@alien8.de>

sev-dev.c calls code in the AMD IOMMU now but that can't really work if
latter is not enabled in Kconfig:

  ld: vmlinux.o: in function `__sev_firmware_shutdown.isra.0':
  sev-dev.c:(.text+0x2501f0e): undefined reference to `amd_iommu_snp_disable'
  ld: vmlinux.o: in function `snp_rmptable_init':
  sev.c:(.init.text+0x26260): undefined reference to `amd_iommu_snp_en'
  make[2]: *** [scripts/Makefile.vmlinux:37: vmlinux] Error 1
  make: *** [Makefile:240: __sub-make] Error 2

Fix those deps.

Fixes: f366a8dac1b8 ("iommu/amd: Clean up RMP entries for IOMMU pages during SNP shutdown")
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 drivers/crypto/ccp/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tom Lendacky Feb. 9, 2024, 5:51 p.m. UTC | #1 
On 2/7/24 14:47, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> sev-dev.c calls code in the AMD IOMMU now but that can't really work if
> latter is not enabled in Kconfig:
> 
>    ld: vmlinux.o: in function `__sev_firmware_shutdown.isra.0':
>    sev-dev.c:(.text+0x2501f0e): undefined reference to `amd_iommu_snp_disable'
>    ld: vmlinux.o: in function `snp_rmptable_init':
>    sev.c:(.init.text+0x26260): undefined reference to `amd_iommu_snp_en'
>    make[2]: *** [scripts/Makefile.vmlinux:37: vmlinux] Error 1
>    make: *** [Makefile:240: __sub-make] Error 2
> 
> Fix those deps.
> 
> Fixes: f366a8dac1b8 ("iommu/amd: Clean up RMP entries for IOMMU pages during SNP shutdown")
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
>   drivers/crypto/ccp/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
> index 32268e239bf1..f394e45e11ab 100644
> --- a/drivers/crypto/ccp/Kconfig
> +++ b/drivers/crypto/ccp/Kconfig
> @@ -38,7 +38,7 @@ config CRYPTO_DEV_CCP_CRYPTO
>   config CRYPTO_DEV_SP_PSP
>   	bool "Platform Security Processor (PSP) device"
>   	default y
> -	depends on CRYPTO_DEV_CCP_DD && X86_64
> +	depends on CRYPTO_DEV_CCP_DD && X86_64 && AMD_IOMMU

Or should the ifdef around amd_iommu_snp_disable() in 
include/linux/amd-iommu.h instead be:

#if defined(CONFIG_AMD_IOMMU) && defined(CONFIG_KVM_AMD_SEV)

and amd_iommu_snp_en be protected in arch/x86/include/asm/iommu.h as:

#ifdef CONFIG_AMD_IOMMU
extern bool amd_iommu_snp_en;
#else
#define amd_iommu_snp_en false
#endif

I think this would be better in case these should be referenced elsewhere 
in the future.

Thanks,
Tom

>   	help
>   	 Provide support for the AMD Platform Security Processor (PSP).
>   	 The PSP is a dedicated processor that provides support for key
Borislav Petkov Feb. 11, 2024, 8:18 p.m. UTC | #2 
On Fri, Feb 09, 2024 at 11:51:37AM -0600, Tom Lendacky wrote:
> Or should the ifdef around amd_iommu_snp_disable() in
> include/linux/amd-iommu.h instead be:
> 
> #if defined(CONFIG_AMD_IOMMU) && defined(CONFIG_KVM_AMD_SEV)

Pretty much, except the thunk should say that it is the thunk that gets
called:

#if defined(CONFIG_AMD_IOMMU) && defined(CONFIG_KVM_AMD_SEV)
int amd_iommu_snp_disable(void);
#else
static inline int amd_iommu_snp_disable(void) { return -ENODEV; }
#endif

and return -ENODEV to denote that so that the caller doesn't wonder
what's going on.

> I think this would be better in case these should be referenced
> elsewhere in the future.

However, looking at all that code, its design still looks iffy to me.

iommu_page_make_shared() and amd_iommu_snp_disable() is functionality
which the *IOMMU* provides in order to deal with SEV* guests. So that
functionality should be behind

CONFIG_AMD_IOMMU_SEV

or so, on which all its users depend or select. I'd prefer depend tho.

Putting IOMMU facilities around a KVM Kconfig symbol CONFIG_KVM_AMD_SEV
is simply bad design. There are providers of functionality which is
behind Kconfig symbols and users which depend on those. The current
thing is a hack IMNSVHO.

Thx.
diff mbox series

Patch

diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
index 32268e239bf1..f394e45e11ab 100644
--- a/drivers/crypto/ccp/Kconfig
+++ b/drivers/crypto/ccp/Kconfig
@@ -38,7 +38,7 @@  config CRYPTO_DEV_CCP_CRYPTO
 config CRYPTO_DEV_SP_PSP
 	bool "Platform Security Processor (PSP) device"
 	default y
-	depends on CRYPTO_DEV_CCP_DD && X86_64
+	depends on CRYPTO_DEV_CCP_DD && X86_64 && AMD_IOMMU
 	help
 	 Provide support for the AMD Platform Security Processor (PSP).
 	 The PSP is a dedicated processor that provides support for key