In TDX guests, guest memory is protected from host access. If a guest
performs I/O, it needs to explicitly share the I/O memory with the host.
Make all ioremap()ed pages that are not backed by normal memory
(IORES_DESC_NONE or IORES_DESC_RESERVED) mapped as shared.
Since TDX memory encryption support is similar to AMD SEV architecture,
reuse the infrastructure from AMD SEV code.
Add tdx_shared_mask() interface to get the TDX guest shared bitmask.
pgprot_decrypted() is used by drivers (i915, virtio_gpu, vfio). Export
both pgprot_encrypted() and pgprot_decrypted().
Co-developed-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/pgtable.h | 19 +++++++++++++------
arch/x86/include/asm/tdx.h | 4 ++++
arch/x86/kernel/cc_platform.c | 23 +++++++++++++++++++++++
arch/x86/kernel/tdx.c | 9 +++++++++
arch/x86/mm/ioremap.c | 5 +++++
5 files changed, 54 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 8a9432fb3802..40e22db48319 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -15,12 +15,6 @@
cachemode2protval(_PAGE_CACHE_MODE_UC_MINUS))) \
: (prot))
-/*
- * Macros to add or remove encryption attribute
- */
-#define pgprot_encrypted(prot) __pgprot(__sme_set(pgprot_val(prot)))
-#define pgprot_decrypted(prot) __pgprot(__sme_clr(pgprot_val(prot)))
-
#ifndef __ASSEMBLY__
#include <linux/spinlock.h>
#include <asm/x86_init.h>
@@ -38,6 +32,19 @@ void ptdump_walk_pgd_level_debugfs(struct seq_file *m, struct mm_struct *mm,
void ptdump_walk_pgd_level_checkwx(void);
void ptdump_walk_user_pgd_level_checkwx(void);
+/*
+ * Macros to add or remove encryption attribute
+ */
+#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
+pgprot_t pgprot_encrypted(pgprot_t prot);
+pgprot_t pgprot_decrypted(pgprot_t prot);
+#define pgprot_encrypted(prot) pgprot_encrypted(prot)
+#define pgprot_decrypted(prot) pgprot_decrypted(prot)
+#else
+#define pgprot_encrypted(prot) (prot)
+#define pgprot_decrypted(prot) (prot)
+#endif
+
#ifdef CONFIG_DEBUG_WX
#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
#define debug_checkwx_user() ptdump_walk_user_pgd_level_checkwx()
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 4bcaadf21dc6..c6a279e67dff 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -55,6 +55,8 @@ void tdx_safe_halt(void);
bool tdx_early_handle_ve(struct pt_regs *regs);
+phys_addr_t tdx_shared_mask(void);
+
#else
static inline void tdx_early_init(void) { };
@@ -63,6 +65,8 @@ static inline void tdx_safe_halt(void) { };
static inline bool tdx_early_handle_ve(struct pt_regs *regs) { return false; }
+static inline phys_addr_t tdx_shared_mask(void) { return 0; }
+
#endif /* CONFIG_INTEL_TDX_GUEST */
#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_INTEL_TDX_GUEST)
diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index dcb31d6a7554..be8722ad4792 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -12,6 +12,7 @@
#include <linux/mem_encrypt.h>
#include <asm/mshyperv.h>
+#include <asm/pgtable.h>
#include <asm/processor.h>
#include <asm/tdx.h>
@@ -90,3 +91,25 @@ bool cc_platform_has(enum cc_attr attr)
return false;
}
EXPORT_SYMBOL_GPL(cc_platform_has);
+
+pgprot_t pgprot_encrypted(pgprot_t prot)
+{
+ if (sme_me_mask)
+ return __pgprot(__sme_set(pgprot_val(prot)));
+ else if (is_tdx_guest())
+ return __pgprot(pgprot_val(prot) & ~tdx_shared_mask());
+
+ return prot;
+}
+EXPORT_SYMBOL_GPL(pgprot_encrypted);
+
+pgprot_t pgprot_decrypted(pgprot_t prot)
+{
+ if (sme_me_mask)
+ return __pgprot(__sme_clr(pgprot_val(prot)));
+ else if (is_tdx_guest())
+ return __pgprot(pgprot_val(prot) | tdx_shared_mask());
+
+ return prot;
+}
+EXPORT_SYMBOL_GPL(pgprot_decrypted);
diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
index beeaf61934bc..3bf6621eae7d 100644
--- a/arch/x86/kernel/tdx.c
+++ b/arch/x86/kernel/tdx.c
@@ -66,6 +66,15 @@ long tdx_kvm_hypercall(unsigned int nr, unsigned long p1, unsigned long p2,
EXPORT_SYMBOL_GPL(tdx_kvm_hypercall);
#endif
+/*
+ * The highest bit of a guest physical address is the "sharing" bit.
+ * Set it for shared pages and clear it for private pages.
+ */
+phys_addr_t tdx_shared_mask(void)
+{
+ return BIT_ULL(td_info.gpa_width - 1);
+}
+
static void tdx_get_info(void)
{
struct tdx_module_output out;
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 026031b3b782..a5d4ec1afca2 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -242,10 +242,15 @@ __ioremap_caller(resource_size_t phys_addr, unsigned long size,
* If the page being mapped is in memory and SEV is active then
* make sure the memory encryption attribute is enabled in the
* resulting mapping.
+ * In TDX guests, memory is marked private by default. If encryption
+ * is not requested (using encrypted), explicitly set decrypt
+ * attribute in all IOREMAPPED memory.
*/
prot = PAGE_KERNEL_IO;
if ((io_desc.flags & IORES_MAP_ENCRYPTED) || encrypted)
prot = pgprot_encrypted(prot);
+ else
+ prot = pgprot_decrypted(prot);
switch (pcm) {
case _PAGE_CACHE_MODE_UC:
--
2.34.1
On Mon, Jan 24 2022 at 18:02, Kirill A. Shutemov wrote:
> In TDX guests, guest memory is protected from host access. If a guest
> performs I/O, it needs to explicitly share the I/O memory with the host.
>
> Make all ioremap()ed pages that are not backed by normal memory
> (IORES_DESC_NONE or IORES_DESC_RESERVED) mapped as shared.
>
> Since TDX memory encryption support is similar to AMD SEV architecture,
> reuse the infrastructure from AMD SEV code.
>
> Add tdx_shared_mask() interface to get the TDX guest shared bitmask.
>
> pgprot_decrypted() is used by drivers (i915, virtio_gpu, vfio). Export
> both pgprot_encrypted() and pgprot_decrypted().
How so?
# git grep pgprot_encrypted
arch/x86/include/asm/pgtable.h:#define pgprot_encrypted(prot) __pgprot(__sme_set(pgprot_val(prot)))
arch/x86/mm/ioremap.c: prot = pgprot_encrypted(prot);
arch/x86/mm/ioremap.c: return encrypted_prot ? pgprot_encrypted(prot)
arch/x86/mm/mem_encrypt_amd.c: protection_map[i] = pgprot_encrypted(protection_map[i]);
arch/x86/mm/pat/set_memory.c: cpa.mask_clr = pgprot_encrypted(cpa.mask_clr);
arch/x86/platform/efi/quirks.c: pgprot_val(pgprot_encrypted(FIXMAP_PAGE_NORMAL)));
fs/proc/vmcore.c: prot = pgprot_encrypted(prot);
include/linux/pgtable.h:#ifndef pgprot_encrypted
include/linux/pgtable.h:#define pgprot_encrypted(prot) (prot)
I cannot find any of the above mentioned subsystems in this grep
output. Neither does this patch add any users which require those
exports.
Thanks,
tglx
On Wed, Feb 02 2022 at 22:27, Kirill A. Shutemov wrote:
> On Wed, Feb 02, 2022 at 01:25:28AM +0100, Thomas Gleixner wrote:
>> On Mon, Jan 24 2022 at 18:02, Kirill A. Shutemov wrote:
>> I cannot find any of the above mentioned subsystems in this grep
>> output. Neither does this patch add any users which require those
>> exports.
>
> Try to grep pgprot_decrypted().
Bah.
> I guess we can get away not exporting pgprot_encrypted(), but this
> asymmetry bothers me :)
Well, no. We export only stuff which is needed. Exporting just because
is a NONO.
Thanks,
tglx
On Wed, Feb 02, 2022 at 01:25:28AM +0100, Thomas Gleixner wrote:
> On Mon, Jan 24 2022 at 18:02, Kirill A. Shutemov wrote:
>
> > In TDX guests, guest memory is protected from host access. If a guest
> > performs I/O, it needs to explicitly share the I/O memory with the host.
> >
> > Make all ioremap()ed pages that are not backed by normal memory
> > (IORES_DESC_NONE or IORES_DESC_RESERVED) mapped as shared.
> >
> > Since TDX memory encryption support is similar to AMD SEV architecture,
> > reuse the infrastructure from AMD SEV code.
> >
> > Add tdx_shared_mask() interface to get the TDX guest shared bitmask.
> >
> > pgprot_decrypted() is used by drivers (i915, virtio_gpu, vfio). Export
> > both pgprot_encrypted() and pgprot_decrypted().
>
> How so?
>
> # git grep pgprot_encrypted
> arch/x86/include/asm/pgtable.h:#define pgprot_encrypted(prot) __pgprot(__sme_set(pgprot_val(prot)))
> arch/x86/mm/ioremap.c: prot = pgprot_encrypted(prot);
> arch/x86/mm/ioremap.c: return encrypted_prot ? pgprot_encrypted(prot)
> arch/x86/mm/mem_encrypt_amd.c: protection_map[i] = pgprot_encrypted(protection_map[i]);
> arch/x86/mm/pat/set_memory.c: cpa.mask_clr = pgprot_encrypted(cpa.mask_clr);
> arch/x86/platform/efi/quirks.c: pgprot_val(pgprot_encrypted(FIXMAP_PAGE_NORMAL)));
> fs/proc/vmcore.c: prot = pgprot_encrypted(prot);
> include/linux/pgtable.h:#ifndef pgprot_encrypted
> include/linux/pgtable.h:#define pgprot_encrypted(prot) (prot)
>
> I cannot find any of the above mentioned subsystems in this grep
> output. Neither does this patch add any users which require those
> exports.
Try to grep pgprot_decrypted().
I guess we can get away not exporting pgprot_encrypted(), but this
asymmetry bothers me :)
--
Kirill A. Shutemov
On 2/7/22 08:27, Borislav Petkov wrote:
> On Mon, Jan 24, 2022 at 06:02:08PM +0300, Kirill A. Shutemov wrote:
>> -/*
>> - * Macros to add or remove encryption attribute
>> - */
>> -#define pgprot_encrypted(prot) __pgprot(__sme_set(pgprot_val(prot)))
>> -#define pgprot_decrypted(prot) __pgprot(__sme_clr(pgprot_val(prot)))
> Why can't you simply define
>
> cc_set() and cc_clear()
>
> helpers which either call the __sme variants or __tdx variants, the
> latter you can define the same way, respectively, as the __sme ones.
I think your basic point here is valid: Let's have a single function to
take a pgprot and turn it into an "encrypted" or "decrypted" pgprot.
But, we can't do it with functions like cc_set() and cc_clear() because
the polarity is different:
> +pgprot_t pgprot_encrypted(pgprot_t prot)
> +{
> + if (sme_me_mask)
> + return __pgprot(__sme_set(pgprot_val(prot)));
> + else if (is_tdx_guest())
> + return __pgprot(pgprot_val(prot) & ~tdx_shared_mask());
> +
> + return prot;
> +}
For "encrypted", SME sets bits and TDX clears bits.
For "decrypted", SME clears bits and TDX sets bits.
We can surely *do* this with cc_something() helpers. It's just not as
easy as making cc_set/cc_clear().
On Mon, Jan 24, 2022 at 06:02:08PM +0300, Kirill A. Shutemov wrote:
> -/*
> - * Macros to add or remove encryption attribute
> - */
> -#define pgprot_encrypted(prot) __pgprot(__sme_set(pgprot_val(prot)))
> -#define pgprot_decrypted(prot) __pgprot(__sme_clr(pgprot_val(prot)))
Why can't you simply define
cc_set() and cc_clear()
helpers which either call the __sme variants or __tdx variants, the
latter you can define the same way, respectively, as the __sme ones.
And then you do:
#define pgprot_encrypted(prot) __pgprot(cc_set(pgprot_val(prot)))
#define pgprot_decrypted(prot) __pgprot(cc_clear(pgprot_val(prot)))
And just so that it works as early as possible, you can define a global
tdx_shared_mask or so which gets initialized the moment you have
td_info.gpa_width.
And then you don't need to export anything or other ifdefferies - you
just make sure you have that mask defined as early as needed.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Feb 07, 2022 at 08:57:39AM -0800, Dave Hansen wrote:
> We can surely *do* this with cc_something() helpers. It's just not as
> easy as making cc_set/cc_clear().
Sure, that's easy: cc_pgprot_{enc,dec}() or so.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Feb 07, 2022 at 06:28:04PM +0100, Borislav Petkov wrote:
> On Mon, Feb 07, 2022 at 08:57:39AM -0800, Dave Hansen wrote:
> > We can surely *do* this with cc_something() helpers. It's just not as
> > easy as making cc_set/cc_clear().
>
> Sure, that's easy: cc_pgprot_{enc,dec}() or so.
So, I've ended up with this in <asm/pgtable.h>
/*
* Macros to add or remove encryption attribute
*/
#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
pgprotval_t cc_enc(pgprotval_t protval);
pgprotval_t cc_dec(pgprotval_t protval);
#define pgprot_encrypted(prot) __pgprot(cc_enc(pgprot_val(prot)))
#define pgprot_decrypted(prot) __pgprot(cc_dec(pgprot_val(prot)))
#else
#define pgprot_encrypted(prot) (prot)
#define pgprot_decrypted(prot) (prot)
#endif
And cc_platform.c:
pgprotval_t cc_enc(pgprotval_t protval)
{
if (sme_me_mask)
return __sme_set(protval);
else if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
return protval & ~tdx_shared_mask();
else
return protval;
}
pgprotval_t cc_dec(pgprotval_t protval)
{
if (sme_me_mask)
return __sme_clr(protval);
else if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
return protval | tdx_shared_mask();
else
return protval;
}
EXPORT_SYMBOL_GPL(cc_dec);
--
Kirill A. Shutemov
On 2/14/22 16:09, Kirill A. Shutemov wrote:
> On Mon, Feb 07, 2022 at 06:28:04PM +0100, Borislav Petkov wrote:
>> On Mon, Feb 07, 2022 at 08:57:39AM -0800, Dave Hansen wrote:
>>> We can surely *do* this with cc_something() helpers. It's just not as
>>> easy as making cc_set/cc_clear().
>>
>> Sure, that's easy: cc_pgprot_{enc,dec}() or so.
>
> So, I've ended up with this in <asm/pgtable.h>
>
> /*
> * Macros to add or remove encryption attribute
> */
> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> pgprotval_t cc_enc(pgprotval_t protval);
> pgprotval_t cc_dec(pgprotval_t protval);
> #define pgprot_encrypted(prot) __pgprot(cc_enc(pgprot_val(prot)))
> #define pgprot_decrypted(prot) __pgprot(cc_dec(pgprot_val(prot)))
> #else
> #define pgprot_encrypted(prot) (prot)
> #define pgprot_decrypted(prot) (prot)
> #endif
A couple of things. I think cc_pgprot_enc() and cc_pgprot_dec() would be
more descriptive/better names to use here.
Also, can they be defined in include/linux/cc_platform.h (with two
versions based on CONFIG_ARCH_HAS_CC_PLATFORM) and have that included
here? Or is there some header file include issues when trying to include
it? That would clean this block up into just two lines.
Thanks,
Tom
>
> And cc_platform.c:
>
> pgprotval_t cc_enc(pgprotval_t protval)
> {
> if (sme_me_mask)
> return __sme_set(protval);
> else if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> return protval & ~tdx_shared_mask();
> else
> return protval;
> }
>
> pgprotval_t cc_dec(pgprotval_t protval)
> {
> if (sme_me_mask)
> return __sme_clr(protval);
> else if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> return protval | tdx_shared_mask();
> else
> return protval;
> }
> EXPORT_SYMBOL_GPL(cc_dec);
On 2/15/22 09:41, Kirill A. Shutemov wrote:
> On Tue, Feb 15, 2022 at 08:49:34AM -0600, Tom Lendacky wrote:
>> On 2/14/22 16:09, Kirill A. Shutemov wrote:
>>> On Mon, Feb 07, 2022 at 06:28:04PM +0100, Borislav Petkov wrote:
>>>> On Mon, Feb 07, 2022 at 08:57:39AM -0800, Dave Hansen wrote:
>>>>> We can surely *do* this with cc_something() helpers. It's just not as
>>>>> easy as making cc_set/cc_clear().
>>>>
>>>> Sure, that's easy: cc_pgprot_{enc,dec}() or so.
>>>
>>> So, I've ended up with this in <asm/pgtable.h>
>>>
>>> /*
>>> * Macros to add or remove encryption attribute
>>> */
>>> #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
>>> pgprotval_t cc_enc(pgprotval_t protval);
>>> pgprotval_t cc_dec(pgprotval_t protval);
>>> #define pgprot_encrypted(prot) __pgprot(cc_enc(pgprot_val(prot)))
>>> #define pgprot_decrypted(prot) __pgprot(cc_dec(pgprot_val(prot)))
>>> #else
>>> #define pgprot_encrypted(prot) (prot)
>>> #define pgprot_decrypted(prot) (prot)
>>> #endif
>>
>> A couple of things. I think cc_pgprot_enc() and cc_pgprot_dec() would be
>> more descriptive/better names to use here.
>>
>> Also, can they be defined in include/linux/cc_platform.h (with two versions
>> based on CONFIG_ARCH_HAS_CC_PLATFORM) and have that included here? Or is
>> there some header file include issues when trying to include it? That would
>> clean this block up into just two lines.
>
> Well, pgprotval_t is x86-specific. It cannot be used in generic headers.
Ah, right.
> We can use u64 here instead. It is wider than pgprotval_t on 2-level
> paging x86, but should work.
Hmm..., yeah. Maybe unsigned long? CONFIG_ARCH_HAS_CC_PLATFORM is X86_64
only, so 2-level paging wouldn't be applicable when an unsigned long is
64-bits?
I'll let the maintainers weigh in on that.
>
> But with u64 as type, I'm not sure 'pgprot' in the name is jutified.
Maybe cc_mask_{enc,dec}()? It just sounds like cc_{enc,dec}() is actually
performing encryption or decryption and can be confusing.
Thanks,
Tom
>
> Hm?
>
On 2/15/22 08:27, Kirill A. Shutemov wrote:
>>> But with u64 as type, I'm not sure 'pgprot' in the name is jutified.
>> Maybe cc_mask_{enc,dec}()? It just sounds like cc_{enc,dec}() is actually
>> performing encryption or decryption and can be confusing.
> cc_{enc,dec}_mask() sounds better to me.
The pte_mk*() functions might be a good naming model here. Some of them
clear bits and some set them, but they all "make" a PTE.
On Tue, Feb 15, 2022 at 01:09:26AM +0300, Kirill A. Shutemov wrote:
> pgprotval_t cc_enc(pgprotval_t protval)
> {
> if (sme_me_mask)
> return __sme_set(protval);
> else if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> return protval & ~tdx_shared_mask();
^^^^^^^^^^^^^^^^^^^
LGTM.
Btw, what about sticking the mask tdx_shared_mask() returns into a
proper u64 variable and using it everywhere, just like sme_me_mask?
We could unify it later into a common encryption mask, see thread
starting here:
https://lore.kernel.org/r/[email protected]
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Feb 15, 2022 at 08:49:34AM -0600, Tom Lendacky wrote:
> On 2/14/22 16:09, Kirill A. Shutemov wrote:
> > On Mon, Feb 07, 2022 at 06:28:04PM +0100, Borislav Petkov wrote:
> > > On Mon, Feb 07, 2022 at 08:57:39AM -0800, Dave Hansen wrote:
> > > > We can surely *do* this with cc_something() helpers. It's just not as
> > > > easy as making cc_set/cc_clear().
> > >
> > > Sure, that's easy: cc_pgprot_{enc,dec}() or so.
> >
> > So, I've ended up with this in <asm/pgtable.h>
> >
> > /*
> > * Macros to add or remove encryption attribute
> > */
> > #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> > pgprotval_t cc_enc(pgprotval_t protval);
> > pgprotval_t cc_dec(pgprotval_t protval);
> > #define pgprot_encrypted(prot) __pgprot(cc_enc(pgprot_val(prot)))
> > #define pgprot_decrypted(prot) __pgprot(cc_dec(pgprot_val(prot)))
> > #else
> > #define pgprot_encrypted(prot) (prot)
> > #define pgprot_decrypted(prot) (prot)
> > #endif
>
> A couple of things. I think cc_pgprot_enc() and cc_pgprot_dec() would be
> more descriptive/better names to use here.
>
> Also, can they be defined in include/linux/cc_platform.h (with two versions
> based on CONFIG_ARCH_HAS_CC_PLATFORM) and have that included here? Or is
> there some header file include issues when trying to include it? That would
> clean this block up into just two lines.
Well, pgprotval_t is x86-specific. It cannot be used in generic headers.
We can use u64 here instead. It is wider than pgprotval_t on 2-level
paging x86, but should work.
But with u64 as type, I'm not sure 'pgprot' in the name is jutified.
Hm?
--
Kirill A. Shutemov
On Tue, Feb 15, 2022 at 09:55:16AM -0600, Tom Lendacky wrote:
> On 2/15/22 09:41, Kirill A. Shutemov wrote:
> > On Tue, Feb 15, 2022 at 08:49:34AM -0600, Tom Lendacky wrote:
> > > On 2/14/22 16:09, Kirill A. Shutemov wrote:
> > > > On Mon, Feb 07, 2022 at 06:28:04PM +0100, Borislav Petkov wrote:
> > > > > On Mon, Feb 07, 2022 at 08:57:39AM -0800, Dave Hansen wrote:
> > > > > > We can surely *do* this with cc_something() helpers. It's just not as
> > > > > > easy as making cc_set/cc_clear().
> > > > >
> > > > > Sure, that's easy: cc_pgprot_{enc,dec}() or so.
> > > >
> > > > So, I've ended up with this in <asm/pgtable.h>
> > > >
> > > > /*
> > > > * Macros to add or remove encryption attribute
> > > > */
> > > > #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> > > > pgprotval_t cc_enc(pgprotval_t protval);
> > > > pgprotval_t cc_dec(pgprotval_t protval);
> > > > #define pgprot_encrypted(prot) __pgprot(cc_enc(pgprot_val(prot)))
> > > > #define pgprot_decrypted(prot) __pgprot(cc_dec(pgprot_val(prot)))
> > > > #else
> > > > #define pgprot_encrypted(prot) (prot)
> > > > #define pgprot_decrypted(prot) (prot)
> > > > #endif
> > >
> > > A couple of things. I think cc_pgprot_enc() and cc_pgprot_dec() would be
> > > more descriptive/better names to use here.
> > >
> > > Also, can they be defined in include/linux/cc_platform.h (with two versions
> > > based on CONFIG_ARCH_HAS_CC_PLATFORM) and have that included here? Or is
> > > there some header file include issues when trying to include it? That would
> > > clean this block up into just two lines.
> >
> > Well, pgprotval_t is x86-specific. It cannot be used in generic headers.
>
> Ah, right.
>
> > We can use u64 here instead. It is wider than pgprotval_t on 2-level
> > paging x86, but should work.
>
> Hmm..., yeah. Maybe unsigned long? CONFIG_ARCH_HAS_CC_PLATFORM is X86_64
> only, so 2-level paging wouldn't be applicable when an unsigned long is
> 64-bits?
Hm. So for !CONFIG_ARCH_HAS_CC_PLATFORM it has to be define, if we would
try static inline dummy instead it will break x86 PAE as upper bit get
trancated when passed via helper.
I donno.
> I'll let the maintainers weigh in on that.
>
> >
> > But with u64 as type, I'm not sure 'pgprot' in the name is jutified.
>
> Maybe cc_mask_{enc,dec}()? It just sounds like cc_{enc,dec}() is actually
> performing encryption or decryption and can be confusing.
cc_{enc,dec}_mask() sounds better to me.
--
Kirill A. Shutemov
On Tue, Feb 15, 2022 at 08:33:21PM +0300, Kirill A. Shutemov wrote:
> Like cc_mkencrypted()/cc_mkdecrypted()? I donno. Looks strange.
cc_mkenc/cc_mkdec probably.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Feb 16, 2022 at 10:58:34AM +0100, Borislav Petkov wrote:
> On Tue, Feb 15, 2022 at 08:33:21PM +0300, Kirill A. Shutemov wrote:
> > Like cc_mkencrypted()/cc_mkdecrypted()? I donno. Looks strange.
>
> cc_mkenc/cc_mkdec probably.
Okay, what about this:
static u64 cc_mask;
pgprotval_t cc_mkenc(pgprotval_t protval)
{
if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
return protval & ~cc_mask;
else
return protval | cc_mask;
}
pgprotval_t cc_mkdec(pgprotval_t protval)
{
if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
return protval | cc_mask;
else
return protval & ~cc_mask;
}
EXPORT_SYMBOL_GPL(cc_mkdec);
__init void cc_init(void)
{
if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
cc_mask = tdx_shared_mask();
else
cc_mask = sme_me_mask;
}
I did not introduce explicit vendor variable but opted for
X86_FEATURE_TDX_GUEST to check vendor. There's no X86_FEATURE counter part
on AMD side presumably because it get used too early to be functional.
TDX needs cc_platform_has() later and X86_FEATURE infrastructure is
already functional there (and we can benefit from static branch).
cc_init() got called from sme_enable() for AMD and from tdx_early_init()
for TDX.
I also reworked cc_platform_has() to use combination of
X86_FEATURE_TDX_GUEST and cc_mask to route to right helper:
bool cc_platform_has(enum cc_attr attr)
{
if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
return intel_cc_platform_has(attr);
else if (cc_mask)
return amd_cc_platform_has(attr);
else if (hv_is_isolation_supported())
return hyperv_cc_platform_has(attr);
else
return false;
}
Any opinions?
--
Kirill A. Shutemov
On Wed, Feb 16, 2022 at 06:37:03PM +0300, Kirill A. Shutemov wrote:
> bool cc_platform_has(enum cc_attr attr)
> {
> if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> return intel_cc_platform_has(attr);
> else if (cc_mask)
> return amd_cc_platform_has(attr);
It is exactly stuff like that I'd like to avoid because that is
dependent on the order the test happens.
It would be a lot more robust if this did:
switch (cc_vendor) {
case INTEL: return intel_cc_platform_has(attr);
case AMD: return amd_cc_platform_has(attr);
case HYPERV: return hyperv_cc_platform_has(attr);
default: return false;
}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette