Re: [PATCH] crypto: ccp - Have it depend on AMD_IOMMU

From: Borislav Petkov <bp@alien8.de>
To: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>,
	Michael Roth <michael.roth@amd.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] crypto: ccp - Have it depend on AMD_IOMMU
Date: Sun, 11 Feb 2024 21:18:31 +0100	[thread overview]
Message-ID: <20240211201831.GBZckrl84xVo-ILnOJ@fat_crate.local> (raw)
In-Reply-To: <5e4670e5-acc8-adf3-2a3d-eb02db7ed990@amd.com>

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette