2022-02-18 17:17:16

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 00/32] TDX Guest: TDX core support

Hi All,

Intel's Trust Domain Extensions (TDX) protects confidential guest VMs
from the host and physical attacks by isolating the guest register
state and by encrypting the guest memory. In TDX, a special TDX module
sits between the host and the guest, and runs in a special mode and
manages the guest/host separation.

Please review and consider applying.

More details of TDX guests can be found in Documentation/x86/tdx.rst.

All dependencies of the patchset are in Linus' tree now.

SEV/TDX comparison:
-------------------

TDX has a lot of similarities to SEV. It enhances confidentiality
of guest memory and state (like registers) and includes a new exception
(#VE) for the same basic reasons as SEV-ES. Like SEV-SNP (not merged
yet), TDX limits the host's ability to make changes in the guest
physical address space.

TDX/VM comparison:
------------------

Some of the key differences between TD and regular VM is,

1. Multi CPU bring-up is done using the ACPI MADT wake-up table.
2. A new #VE exception handler is added. The TDX module injects #VE exception
to the guest TD in cases of instructions that need to be emulated, disallowed
MSR accesses, etc.
3. By default memory is marked as private, and TD will selectively share it with
VMM based on need.

You can find TDX related documents in the following link.

https://software.intel.com/content/www/br/pt/develop/articles/intel-trust-domain-extensions.html

Git tree:

https://github.com/intel/tdx.git guest-upstream

Previous version:

https://lore.kernel.org/r/[email protected]

Changes from v2:
- Move TDX-Guest-specific code under arch/x86/coco/
- Code shared between host and guest is under arch/x86/virt/
- Fix handling CR4.MCE for !CONFIG_X86_MCE
- A separate patch to clarify CR0.NE situation
- Use u8/u16/u32 for port I/O handler
- Rework TDCALL helpers:
+ consolidation between guest and host
+ clearer interface
+ A new tdx_module_call() panic() if TDCALL fails
- Rework MMIO handling to imporove readability
- New generic API to deal encryption masks
- Move tdx_early_init() before copy_bootdata() (again)
- Rework #VE handing to share more code with #GP handler
- Rework __set_memory_enc_pgtable() to provide proper abstruction for both
SME/SEV and TDX cases.
- Fix warning on build with X86_MEM_ENCRYPT=y
- ... and more
Changes from v1:
- Rebased to tip/master (94985da003a4).
- Address feedback from Borislav and Josh.
- Wire up KVM hypercalls. Needed to send IPI.
Andi Kleen (1):
x86/tdx: Handle early boot port I/O

Isaku Yamahata (1):
x86/tdx: ioapic: Add shared bit for IOAPIC base address

Kirill A. Shutemov (20):
x86/mm: Fix warning on build with X86_MEM_ENCRYPT=y
x86/coco: Add API to handle encryption mask
x86/tdx: Provide common base for SEAMCALL and TDCALL C wrappers
x86/tdx: Extend the confidential computing API to support TDX guests
x86/tdx: Exclude shared bit from __PHYSICAL_MASK
x86/traps: Add #VE support for TDX guest
x86/tdx: Add HLT support for TDX guests
x86/tdx: Add MSR support for TDX guests
x86/tdx: Handle CPUID via #VE
x86/tdx: Handle in-kernel MMIO
x86: Adjust types used in port I/O helpers
x86: Consolidate port I/O helpers
x86/boot: Allow to hook up alternative port I/O helpers
x86/boot/compressed: Support TDX guest port I/O at decompression time
x86/boot: Set CR0.NE early and keep it set during the boot
x86/tdx: Make pages shared in ioremap()
x86/mm/cpa: Generailize __set_memory_enc_pgtable()
x86/mm/cpa: Add support for TDX shared memory
x86/kvm: Use bounce buffers for TD guest
ACPICA: Avoid cache flush on TDX guest

Kuppuswamy Sathyanarayanan (8):
x86/tdx: Detect running as a TDX guest in early boot
x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper
functions
x86/tdx: Detect TDX at early kernel decompression time
x86/tdx: Add port I/O emulation
x86/tdx: Wire up KVM hypercalls
x86/acpi, x86/boot: Add multiprocessor wake-up support
x86/topology: Disable CPU online/offline control for TDX guests
Documentation/x86: Document TDX kernel architecture

Sean Christopherson (2):
x86/boot: Add a trampoline for booting APs via firmware handoff
x86/boot: Avoid #VE during boot for TDX platforms

Documentation/x86/index.rst | 1 +
Documentation/x86/tdx.rst | 194 ++++++++
arch/x86/Kbuild | 1 +
arch/x86/Kconfig | 15 +
arch/x86/boot/a20.c | 14 +-
arch/x86/boot/boot.h | 35 +-
arch/x86/boot/compressed/Makefile | 1 +
arch/x86/boot/compressed/head_64.S | 27 +-
arch/x86/boot/compressed/misc.c | 26 +-
arch/x86/boot/compressed/misc.h | 4 +-
arch/x86/boot/compressed/pgtable.h | 2 +-
arch/x86/boot/compressed/tdcall.S | 3 +
arch/x86/boot/compressed/tdx.c | 97 ++++
arch/x86/boot/compressed/tdx.h | 15 +
arch/x86/boot/cpuflags.c | 3 +-
arch/x86/boot/cpuflags.h | 1 +
arch/x86/boot/early_serial_console.c | 28 +-
arch/x86/boot/io.h | 28 ++
arch/x86/boot/main.c | 4 +
arch/x86/boot/pm.c | 10 +-
arch/x86/boot/tty.c | 4 +-
arch/x86/boot/video-vga.c | 6 +-
arch/x86/boot/video.h | 8 +-
arch/x86/coco/Makefile | 2 +
arch/x86/coco/tdcall.S | 197 ++++++++
arch/x86/coco/tdx.c | 601 +++++++++++++++++++++++
arch/x86/include/asm/acenv.h | 16 +-
arch/x86/include/asm/apic.h | 7 +
arch/x86/include/asm/coco.h | 26 +
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/include/asm/disabled-features.h | 8 +-
arch/x86/include/asm/idtentry.h | 4 +
arch/x86/include/asm/io.h | 42 +-
arch/x86/include/asm/kvm_para.h | 22 +
arch/x86/include/asm/mem_encrypt.h | 6 +-
arch/x86/include/asm/pgtable.h | 13 +-
arch/x86/include/asm/realmode.h | 1 +
arch/x86/include/asm/set_memory.h | 1 -
arch/x86/include/asm/shared/io.h | 34 ++
arch/x86/include/asm/shared/tdx.h | 37 ++
arch/x86/include/asm/tdx.h | 82 ++++
arch/x86/include/asm/x86_init.h | 7 +
arch/x86/kernel/acpi/boot.c | 118 +++++
arch/x86/kernel/apic/apic.c | 10 +
arch/x86/kernel/apic/io_apic.c | 15 +-
arch/x86/kernel/asm-offsets.c | 19 +
arch/x86/kernel/cc_platform.c | 78 ++-
arch/x86/kernel/cpu/mshyperv.c | 3 +
arch/x86/kernel/head64.c | 7 +
arch/x86/kernel/head_64.S | 28 +-
arch/x86/kernel/idt.c | 3 +
arch/x86/kernel/process.c | 4 +
arch/x86/kernel/smpboot.c | 12 +-
arch/x86/kernel/traps.c | 138 +++++-
arch/x86/mm/ioremap.c | 5 +
arch/x86/mm/mem_encrypt.c | 9 +-
arch/x86/mm/mem_encrypt_amd.c | 65 ++-
arch/x86/mm/mem_encrypt_identity.c | 8 +-
arch/x86/mm/pat/set_memory.c | 14 +-
arch/x86/realmode/rm/header.S | 1 +
arch/x86/realmode/rm/trampoline_64.S | 57 ++-
arch/x86/realmode/rm/trampoline_common.S | 12 +-
arch/x86/realmode/rm/wakemain.c | 14 +-
arch/x86/virt/tdxcall.S | 91 ++++
include/linux/cc_platform.h | 10 +
kernel/cpu.c | 7 +
66 files changed, 2151 insertions(+), 211 deletions(-)
create mode 100644 Documentation/x86/tdx.rst
create mode 100644 arch/x86/boot/compressed/tdcall.S
create mode 100644 arch/x86/boot/compressed/tdx.c
create mode 100644 arch/x86/boot/compressed/tdx.h
create mode 100644 arch/x86/boot/io.h
create mode 100644 arch/x86/coco/Makefile
create mode 100644 arch/x86/coco/tdcall.S
create mode 100644 arch/x86/coco/tdx.c
create mode 100644 arch/x86/include/asm/coco.h
create mode 100644 arch/x86/include/asm/shared/io.h
create mode 100644 arch/x86/include/asm/shared/tdx.h
create mode 100644 arch/x86/include/asm/tdx.h
create mode 100644 arch/x86/virt/tdxcall.S

--
2.34.1


2022-02-18 17:19:05

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 27/32] x86/mm/cpa: Generailize __set_memory_enc_pgtable()

TDX steals a bit from the physical address and uses it to indicate
whether the page is private to the guest (bit set 0) or unprotected
and shared with the VMM (bit set 1).

AMD SEV uses a similar scheme, repurposing a bit from the physical address
to indicate encrypted or decrypted pages.

The kernel already has the infrastructure to deal with encrypted/decrypted
pages for AMD SEV, but TDX requires few tweaks:

- TDX and SEV have different requirements to the cache and tlb
flushing.

- TDX has own routine to notify VMM about page encryption status change.

Modify __set_memory_enc_pgtable() and make it flexible enough to cover
both AMD SEV and Intel TDX. The AMD-specific behaviour is isolated in
callback under x86_platform.cc. TDX will provide own version of the
callbacks.

Signed-off-by: Kirill A. Shutemov <[email protected]>
Cc: Tom Lendacky <[email protected]>
---
arch/x86/include/asm/set_memory.h | 1 -
arch/x86/include/asm/x86_init.h | 7 ++++
arch/x86/mm/mem_encrypt_amd.c | 65 ++++++++++++++++++++-----------
arch/x86/mm/pat/set_memory.c | 10 ++---
4 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h
index ff0f2d90338a..ce8dd215f5b3 100644
--- a/arch/x86/include/asm/set_memory.h
+++ b/arch/x86/include/asm/set_memory.h
@@ -84,7 +84,6 @@ int set_pages_rw(struct page *page, int numpages);
int set_direct_map_invalid_noflush(struct page *page);
int set_direct_map_default_noflush(struct page *page);
bool kernel_page_present(struct page *page);
-void notify_range_enc_status_changed(unsigned long vaddr, int npages, bool enc);

extern int kernel_set_to_readonly;

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 22b7412c08f6..d38c253cc222 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -251,6 +251,12 @@ struct x86_hyper_runtime {
bool (*sev_es_hcall_finish)(struct ghcb *ghcb, struct pt_regs *regs);
};

+struct x86_cc_runtime {
+ int (*enc_status_changed)(unsigned long vaddr, int numpages, bool enc);
+ bool (*enc_tlb_flush_required)(bool enc);
+ bool (*enc_cache_flush_required)(void);
+};
+
/**
* struct x86_platform_ops - platform specific runtime functions
* @calibrate_cpu: calibrate CPU
@@ -287,6 +293,7 @@ struct x86_platform_ops {
struct x86_legacy_features legacy;
void (*set_legacy_features)(void);
struct x86_hyper_runtime hyper;
+ const struct x86_cc_runtime *cc;
};

struct x86_apic_ops {
diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
index 2b2d018ea345..3fe746be8d90 100644
--- a/arch/x86/mm/mem_encrypt_amd.c
+++ b/arch/x86/mm/mem_encrypt_amd.c
@@ -177,25 +177,6 @@ void __init sme_map_bootdata(char *real_mode_data)
__sme_early_map_unmap_mem(__va(cmdline_paddr), COMMAND_LINE_SIZE, true);
}

-void __init sme_early_init(void)
-{
- unsigned int i;
-
- if (!sme_me_mask)
- return;
-
- early_pmd_flags = __sme_set(early_pmd_flags);
-
- __supported_pte_mask = __sme_set(__supported_pte_mask);
-
- /* Update the protection map with memory encryption mask */
- for (i = 0; i < ARRAY_SIZE(protection_map); i++)
- protection_map[i] = pgprot_encrypted(protection_map[i]);
-
- if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
- swiotlb_force = SWIOTLB_FORCE;
-}
-
void __init sev_setup_arch(void)
{
phys_addr_t total_mem = memblock_phys_mem_size();
@@ -256,7 +237,7 @@ static unsigned long pg_level_to_pfn(int level, pte_t *kpte, pgprot_t *ret_prot)
return pfn;
}

-void notify_range_enc_status_changed(unsigned long vaddr, int npages, bool enc)
+static int amd_enc_status_changed(unsigned long vaddr, int npages, bool enc)
{
#ifdef CONFIG_PARAVIRT
unsigned long sz = npages << PAGE_SHIFT;
@@ -270,7 +251,7 @@ void notify_range_enc_status_changed(unsigned long vaddr, int npages, bool enc)
kpte = lookup_address(vaddr, &level);
if (!kpte || pte_none(*kpte)) {
WARN_ONCE(1, "kpte lookup for vaddr\n");
- return;
+ return 0;
}

pfn = pg_level_to_pfn(level, kpte, NULL);
@@ -285,6 +266,44 @@ void notify_range_enc_status_changed(unsigned long vaddr, int npages, bool enc)
vaddr = (vaddr & pmask) + psize;
}
#endif
+ return 0;
+}
+
+static bool amd_enc_tlb_flush_required(bool enc)
+{
+ return true;
+}
+
+static bool amd_enc_cache_flush_required(void)
+{
+ return this_cpu_has(X86_FEATURE_SME_COHERENT);
+}
+
+static const struct x86_cc_runtime amd_cc_runtime = {
+ .enc_status_changed = amd_enc_status_changed,
+ .enc_tlb_flush_required = amd_enc_tlb_flush_required,
+ .enc_cache_flush_required = amd_enc_cache_flush_required,
+};
+
+void __init sme_early_init(void)
+{
+ unsigned int i;
+
+ if (!sme_me_mask)
+ return;
+
+ early_pmd_flags = __sme_set(early_pmd_flags);
+
+ __supported_pte_mask = __sme_set(__supported_pte_mask);
+
+ /* Update the protection map with memory encryption mask */
+ for (i = 0; i < ARRAY_SIZE(protection_map); i++)
+ protection_map[i] = pgprot_encrypted(protection_map[i]);
+
+ if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
+ swiotlb_force = SWIOTLB_FORCE;
+
+ x86_platform.cc = &amd_cc_runtime;
}

static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
@@ -392,7 +411,7 @@ static int __init early_set_memory_enc_dec(unsigned long vaddr,

ret = 0;

- notify_range_enc_status_changed(start, PAGE_ALIGN(size) >> PAGE_SHIFT, enc);
+ x86_platform.cc->enc_status_changed(start, PAGE_ALIGN(size) >> PAGE_SHIFT, enc);
out:
__flush_tlb_all();
return ret;
@@ -410,7 +429,7 @@ int __init early_set_memory_encrypted(unsigned long vaddr, unsigned long size)

void __init early_set_mem_enc_dec_hypercall(unsigned long vaddr, int npages, bool enc)
{
- notify_range_enc_status_changed(vaddr, npages, enc);
+ x86_platform.cc->enc_status_changed(vaddr, npages, enc);
}

void __init mem_encrypt_free_decrypted_mem(void)
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index e79366b8a9da..65a27dec095f 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -2007,10 +2007,9 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
kmap_flush_unused();
vm_unmap_aliases();

- /*
- * Before changing the encryption attribute, we need to flush caches.
- */
- cpa_flush(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT));
+ /* Flush caches as needed before changing the encryption attribute. */
+ if (x86_platform.cc->enc_tlb_flush_required(enc))
+ cpa_flush(&cpa, x86_platform.cc->enc_cache_flush_required());

ret = __change_page_attr_set_clr(&cpa, 1);

@@ -2027,7 +2026,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
* Notify hypervisor that a given memory range is mapped encrypted
* or decrypted.
*/
- notify_range_enc_status_changed(addr, numpages, enc);
+ if (!ret)
+ ret = x86_platform.cc->enc_status_changed(addr, numpages, enc);

return ret;
}
--
2.34.1

2022-02-18 17:25:23

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

AMD SME/SEV uses a bit in the page table entries to indicate that the
page is encrypted and not accessible to the VMM.

TDX uses a similar approach, but the polarity of the mask is opposite to
AMD: if the bit is set the page is accessible to VMM.

Provide vendor-neutral API to deal with the mask. It will be extended to
cover TDX.

pgprot_decrypted() is used by drivers (i915, virtio_gpu, vfio).
cc_mkdec() called by pgprot_decrypted(). Export cc_mkdec().

HyperV doesn't use bits in page table entries, so the mask is 0 for both
encrypthion and decrypthion.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/coco.h | 26 +++++++++++++
arch/x86/include/asm/pgtable.h | 13 ++++---
arch/x86/kernel/cc_platform.c | 62 ++++++++++++++++++++++++------
arch/x86/kernel/cpu/mshyperv.c | 3 ++
arch/x86/mm/mem_encrypt_identity.c | 8 ++--
arch/x86/mm/pat/set_memory.c | 4 +-
6 files changed, 93 insertions(+), 23 deletions(-)
create mode 100644 arch/x86/include/asm/coco.h

diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
new file mode 100644
index 000000000000..802d87d08e31
--- /dev/null
+++ b/arch/x86/include/asm/coco.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_COCO_H
+#define _ASM_X86_COCO_H
+
+#include <asm/pgtable_types.h>
+
+enum cc_vendor {
+ CC_VENDOR_NONE,
+ CC_VENDOR_AMD,
+ CC_VENDOR_HYPERV,
+ CC_VENDOR_INTEL,
+};
+
+void cc_init(enum cc_vendor, u64 mask);
+
+#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
+u64 cc_get_mask(bool enc);
+u64 cc_mkenc(u64 val);
+u64 cc_mkdec(u64 val);
+#else
+#define cc_get_mask(enc) 0
+#define cc_mkenc(val) (val)
+#define cc_mkdec(val) (val)
+#endif
+
+#endif /* _ASM_X86_COCO_H */
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 8a9432fb3802..62ab07e24aef 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -15,17 +15,12 @@
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>
#include <asm/pkru.h>
#include <asm/fpu/api.h>
+#include <asm/coco.h>
#include <asm-generic/pgtable_uffd.h>
#include <linux/page_table_check.h>

@@ -38,6 +33,12 @@ 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
+ */
+#define pgprot_encrypted(prot) __pgprot(cc_mkenc(pgprot_val(prot)))
+#define pgprot_decrypted(prot) __pgprot(cc_mkdec(pgprot_val(prot)))
+
#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/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index 6a6ffcd978f6..93e6be7b7eca 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -9,18 +9,16 @@

#include <linux/export.h>
#include <linux/cc_platform.h>
-#include <linux/mem_encrypt.h>

-#include <asm/mshyperv.h>
+#include <asm/coco.h>
#include <asm/processor.h>

-static bool __maybe_unused intel_cc_platform_has(enum cc_attr attr)
+static u64 cc_mask;
+static enum cc_vendor cc_vendor;
+
+static bool intel_cc_platform_has(enum cc_attr attr)
{
-#ifdef CONFIG_INTEL_TDX_GUEST
- return false;
-#else
return false;
-#endif
}

/*
@@ -74,12 +72,52 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)

bool cc_platform_has(enum cc_attr attr)
{
- if (sme_me_mask)
+ switch (cc_vendor) {
+ case CC_VENDOR_AMD:
return amd_cc_platform_has(attr);
-
- if (hv_is_isolation_supported())
+ case CC_VENDOR_INTEL:
+ return intel_cc_platform_has(attr);
+ case CC_VENDOR_HYPERV:
return hyperv_cc_platform_has(attr);
-
- return false;
+ default:
+ return false;
+ }
}
EXPORT_SYMBOL_GPL(cc_platform_has);
+
+u64 cc_get_mask(bool enc)
+{
+ switch (cc_vendor) {
+ case CC_VENDOR_AMD:
+ return enc ? cc_mask : 0;
+ default:
+ return 0;
+ }
+}
+
+u64 cc_mkenc(u64 val)
+{
+ switch (cc_vendor) {
+ case CC_VENDOR_AMD:
+ return val | cc_mask;
+ default:
+ return val;
+ }
+}
+
+u64 cc_mkdec(u64 val)
+{
+ switch (cc_vendor) {
+ case CC_VENDOR_AMD:
+ return val & ~cc_mask;
+ default:
+ return val;
+ }
+}
+EXPORT_SYMBOL_GPL(cc_mkdec);
+
+__init void cc_init(enum cc_vendor vendor, u64 mask)
+{
+ cc_vendor = vendor;
+ cc_mask = mask;
+}
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 5a99f993e639..9af6be143998 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -33,6 +33,7 @@
#include <asm/nmi.h>
#include <clocksource/hyperv_timer.h>
#include <asm/numa.h>
+#include <asm/coco.h>

/* Is Linux running as the root partition? */
bool hv_root_partition;
@@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
*/
swiotlb_force = SWIOTLB_FORCE;
#endif
+ if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
+ cc_init(CC_VENDOR_HYPERV, 0);
}

if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) {
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index 3f0abb403340..fa758247ab57 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -44,6 +44,7 @@
#include <asm/setup.h>
#include <asm/sections.h>
#include <asm/cmdline.h>
+#include <asm/coco.h>

#include "mm_internal.h"

@@ -565,8 +566,7 @@ void __init sme_enable(struct boot_params *bp)
} else {
/* SEV state cannot be controlled by a command line option */
sme_me_mask = me_mask;
- physical_mask &= ~sme_me_mask;
- return;
+ goto out;
}

/*
@@ -600,6 +600,8 @@ void __init sme_enable(struct boot_params *bp)
sme_me_mask = 0;
else
sme_me_mask = active_by_default ? me_mask : 0;
-
+out:
physical_mask &= ~sme_me_mask;
+ if (sme_me_mask)
+ cc_init(CC_VENDOR_AMD, sme_me_mask);
}
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index b4072115c8ef..e79366b8a9da 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -1999,8 +1999,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
memset(&cpa, 0, sizeof(cpa));
cpa.vaddr = &addr;
cpa.numpages = numpages;
- cpa.mask_set = enc ? __pgprot(_PAGE_ENC) : __pgprot(0);
- cpa.mask_clr = enc ? __pgprot(0) : __pgprot(_PAGE_ENC);
+ cpa.mask_set = __pgprot(cc_get_mask(enc));
+ cpa.mask_clr = __pgprot(cc_get_mask(!enc));
cpa.pgd = init_mm.pgd;

/* Must avoid aliasing mappings in the highmem code */
--
2.34.1

2022-02-18 17:37:49

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 26/32] x86/tdx: Make pages shared in ioremap()

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.

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/mm/ioremap.c | 5 +++++
1 file changed, 5 insertions(+)

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

2022-02-18 17:57:07

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 32/32] Documentation/x86: Document TDX kernel architecture

From: Kuppuswamy Sathyanarayanan <[email protected]>

Document the TDX guest architecture details like #VE support,
shared memory, etc.

Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
Documentation/x86/index.rst | 1 +
Documentation/x86/tdx.rst | 194 ++++++++++++++++++++++++++++++++++++
2 files changed, 195 insertions(+)
create mode 100644 Documentation/x86/tdx.rst

diff --git a/Documentation/x86/index.rst b/Documentation/x86/index.rst
index f498f1d36cd3..382e53ca850a 100644
--- a/Documentation/x86/index.rst
+++ b/Documentation/x86/index.rst
@@ -24,6 +24,7 @@ x86-specific Documentation
intel-iommu
intel_txt
amd-memory-encryption
+ tdx
pti
mds
microcode
diff --git a/Documentation/x86/tdx.rst b/Documentation/x86/tdx.rst
new file mode 100644
index 000000000000..903c9cecccbd
--- /dev/null
+++ b/Documentation/x86/tdx.rst
@@ -0,0 +1,194 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====================================
+Intel Trust Domain Extensions (TDX)
+=====================================
+
+Intel's Trust Domain Extensions (TDX) protect confidential guest VMs
+from the host and physical attacks by isolating the guest register
+state and by encrypting the guest memory. In TDX, a special TDX module
+sits between the host and the guest, and runs in a special mode and
+manages the guest/host separation.
+
+Since the host cannot directly access guest registers or memory, much
+normal functionality of a hypervisor (such as trapping MMIO, some MSRs,
+some CPUIDs, and some other instructions) has to be moved into the
+guest. This is implemented using a Virtualization Exception (#VE) that
+is handled by the guest kernel. Some #VEs are handled inside the guest
+kernel, but some require the hypervisor (VMM) to be involved. The TD
+hypercall mechanism allows TD guests to call TDX module or hypervisor
+function.
+
+#VE Exceptions:
+===============
+
+In TDX guests, #VE Exceptions are delivered to TDX guests in following
+scenarios:
+
+* Execution of certain instructions (see list below)
+* Certain MSR accesses.
+* CPUID usage (only for certain leaves)
+* Shared memory access (including MMIO)
+
+#VE due to instruction execution
+---------------------------------
+
+Intel TDX dis-allows execution of certain instructions in non-root
+mode. Execution of these instructions would lead to #VE or #GP.
+
+Details are,
+
+List of instructions that can cause a #VE is,
+
+* String I/O (INS, OUTS), IN, OUT
+* HLT
+* MONITOR, MWAIT
+* WBINVD, INVD
+* VMCALL
+
+List of instructions that can cause a #GP is,
+
+* All VMX instructions: INVEPT, INVVPID, VMCLEAR, VMFUNC, VMLAUNCH,
+ VMPTRLD, VMPTRST, VMREAD, VMRESUME, VMWRITE, VMXOFF, VMXON
+* ENCLS, ENCLV
+* GETSEC
+* RSM
+* ENQCMD
+
+#VE due to MSR access
+----------------------
+
+In TDX guest, MSR access behavior can be categorized as,
+
+* Native supported (also called "context switched MSR")
+ No special handling is required for these MSRs in TDX guests.
+* #GP triggered
+ Dis-allowed MSR read/write would lead to #GP.
+* #VE triggered
+ All MSRs that are not natively supported or dis-allowed
+ (triggers #GP) will trigger #VE. To support access to
+ these MSRs, it needs to be emulated using TDCALL.
+
+Look Intel TDX Module Specification, sec "MSR Virtualization" for the complete
+list of MSRs that fall under the categories above.
+
+#VE due to CPUID instruction
+----------------------------
+
+In TDX guests, most of CPUID leaf/sub-leaf combinations are virtualized by
+the TDX module while some trigger #VE. Combinations of CPUID leaf/sub-leaf
+which triggers #VE are configured by the VMM during the TD initialization
+time (using TDH.MNG.INIT).
+
+#VE on Memory Accesses
+----------------------
+
+A TD guest is in control of whether its memory accesses are treated as
+private or shared. It selects the behavior with a bit in its page table
+entries.
+
+#VE on Shared Pages
+-------------------
+
+Access to shared mappings can cause a #VE. The hypervisor controls whether
+access of shared mapping causes a #VE, so the guest must be careful to only
+reference shared pages it can safely handle a #VE, avoid nested #VEs.
+
+Content of shared mapping is not trusted since shared memory is writable
+by the hypervisor. Shared mappings are never used for sensitive memory content
+like stacks or kernel text, only for I/O buffers and MMIO regions. The kernel
+will not encounter shared mappings in sensitive contexts like syscall entry
+or NMIs.
+
+#VE on Private Pages
+--------------------
+
+Some accesses to private mappings may cause #VEs. Before a mapping is
+accepted (AKA in the SEPT_PENDING state), a reference would cause a #VE.
+But, after acceptance, references typically succeed.
+
+The hypervisor can cause a private page reference to fail if it chooses
+to move an accepted page to a "blocked" state. However, if it does
+this, page access will not generate a #VE. It will, instead, cause a
+"TD Exit" where the hypervisor is required to handle the exception.
+
+Linux #VE handler
+-----------------
+
+Both user/kernel #VE exceptions are handled by the tdx_handle_virt_exception()
+handler. If successfully handled, the instruction pointer is incremented to
+complete the handling process. If failed to handle, it is treated as a regular
+exception and handled via fixup handlers.
+
+In TD guests, #VE nesting (a #VE triggered before handling the current one
+or AKA syscall gap issue) problem is handled by TDX module ensuring that
+interrupts, including NMIs, are blocked. The hardware blocks interrupts
+starting with #VE delivery until TDGETVEINFO is called.
+
+The kernel must avoid triggering #VE in entry paths: do not touch TD-shared
+memory, including MMIO regions, and do not use #VE triggering MSRs,
+instructions, or CPUID leaves that might generate #VE.
+
+MMIO handling:
+==============
+
+In non-TDX VMs, MMIO is usually implemented by giving a guest access to a
+mapping which will cause a VMEXIT on access, and then the VMM emulates the
+access. That's not possible in TDX guests because VMEXIT will expose the
+register state to the host. TDX guests don't trust the host and can't have
+their state exposed to the host.
+
+In TDX the MMIO regions are instead configured to trigger a #VE
+exception in the guest. The guest #VE handler then emulates the MMIO
+instructions inside the guest and converts them into a controlled TDCALL
+to the host, rather than completely exposing the state to the host.
+
+MMIO addresses on x86 are just special physical addresses. They can be
+accessed with any instruction that accesses memory. However, the
+introduced instruction decoding method is limited. It is only designed
+to decode instructions like those generated by io.h macros.
+
+MMIO access via other means (like structure overlays) may result in
+MMIO_DECODE_FAILED and an oops.
+
+Shared memory:
+==============
+
+Intel TDX doesn't allow the VMM to access guest private memory. Any
+memory that is required for communication with VMM must be shared
+explicitly by setting the bit in the page table entry. The shared bit
+can be enumerated with TDX_GET_INFO.
+
+After setting the shared bit, the conversion must be completed with
+MapGPA hypercall. The call informs the VMM about the conversion between
+private/shared mappings.
+
+set_memory_decrypted() converts a range of pages to shared.
+set_memory_encrypted() converts memory back to private.
+
+Device drivers are the primary user of shared memory, but there's no
+need in touching every driver. DMA buffers and ioremap()'ed regions are
+converted to shared automatically.
+
+TDX uses SWIOTLB for most DMA allocations. The SWIOTLB buffer is
+converted to shared on boot.
+
+For coherent DMA allocation, the DMA buffer gets converted on the
+allocation. Check force_dma_unencrypted() for details.
+
+References
+==========
+
+More details about TDX module (and its response for MSR, memory access,
+IO, CPUID etc) can be found at,
+
+https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
+
+More details about TDX hypercall and TDX module call ABI can be found
+at,
+
+https://www.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface-1.0-344426-002.pdf
+
+More details about TDVF requirements can be found at,
+
+https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.01.pdf
--
2.34.1

2022-02-18 17:58:08

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 14/32] x86: Adjust types used in port I/O helpers

Change port I/O helpers to use u8/u16/u32 instead of unsigned
char/short/int for values. Use u16 instead of int for port number.

It aligns the helpers with implementation in boot stub in preparation
for consolidation.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/io.h | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index f6d91ecb8026..638c1a2a82e0 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -258,37 +258,37 @@ static inline void slow_down_io(void)
#endif

#define BUILDIO(bwl, bw, type) \
-static inline void out##bwl(unsigned type value, int port) \
+static inline void out##bwl(type value, u16 port) \
{ \
asm volatile("out" #bwl " %" #bw "0, %w1" \
: : "a"(value), "Nd"(port)); \
} \
\
-static inline unsigned type in##bwl(int port) \
+static inline type in##bwl(u16 port) \
{ \
- unsigned type value; \
+ type value; \
asm volatile("in" #bwl " %w1, %" #bw "0" \
: "=a"(value) : "Nd"(port)); \
return value; \
} \
\
-static inline void out##bwl##_p(unsigned type value, int port) \
+static inline void out##bwl##_p(type value, u16 port) \
{ \
out##bwl(value, port); \
slow_down_io(); \
} \
\
-static inline unsigned type in##bwl##_p(int port) \
+static inline type in##bwl##_p(u16 port) \
{ \
- unsigned type value = in##bwl(port); \
+ type value = in##bwl(port); \
slow_down_io(); \
return value; \
} \
\
-static inline void outs##bwl(int port, const void *addr, unsigned long count) \
+static inline void outs##bwl(u16 port, const void *addr, unsigned long count) \
{ \
if (cc_platform_has(CC_ATTR_GUEST_UNROLL_STRING_IO)) { \
- unsigned type *value = (unsigned type *)addr; \
+ type *value = (type *)addr; \
while (count) { \
out##bwl(*value, port); \
value++; \
@@ -301,10 +301,10 @@ static inline void outs##bwl(int port, const void *addr, unsigned long count) \
} \
} \
\
-static inline void ins##bwl(int port, void *addr, unsigned long count) \
+static inline void ins##bwl(u16 port, void *addr, unsigned long count) \
{ \
if (cc_platform_has(CC_ATTR_GUEST_UNROLL_STRING_IO)) { \
- unsigned type *value = (unsigned type *)addr; \
+ type *value = (type *)addr; \
while (count) { \
*value = in##bwl(port); \
value++; \
@@ -317,9 +317,9 @@ static inline void ins##bwl(int port, void *addr, unsigned long count) \
} \
}

-BUILDIO(b, b, char)
-BUILDIO(w, w, short)
-BUILDIO(l, , int)
+BUILDIO(b, b, u8)
+BUILDIO(w, w, u16)
+BUILDIO(l, , u32)

#define inb inb
#define inw inw
--
2.34.1

2022-02-18 18:13:48

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 11/32] x86/tdx: Handle CPUID via #VE

In TDX guests, most CPUID leaf/sub-leaf combinations are virtualized
by the TDX module while some trigger #VE.

Implement the #VE handling for EXIT_REASON_CPUID by handing it through
the hypercall, which in turn lets the TDX module handle it by invoking
the host VMM.

More details on CPUID Virtualization can be found in the TDX module
specification, the section titled "CPUID Virtualization".

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/coco/tdx.c | 41 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index c0549dce2588..83cbc94b30d0 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -154,6 +154,36 @@ static bool write_msr(struct pt_regs *regs)
return !__tdx_hypercall(&args, 0);
}

+static bool handle_cpuid(struct pt_regs *regs)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = EXIT_REASON_CPUID,
+ .r12 = regs->ax,
+ .r13 = regs->cx,
+ };
+
+ /*
+ * Emulate the CPUID instruction via a hypercall. More info about
+ * ABI can be found in TDX Guest-Host-Communication Interface
+ * (GHCI), section titled "VP.VMCALL<Instruction.CPUID>".
+ */
+ if (__tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT))
+ return false;
+
+ /*
+ * As per TDX GHCI CPUID ABI, r12-r15 registers contain contents of
+ * EAX, EBX, ECX, EDX registers after the CPUID instruction execution.
+ * So copy the register contents back to pt_regs.
+ */
+ regs->ax = args.r12;
+ regs->bx = args.r13;
+ regs->cx = args.r14;
+ regs->dx = args.r15;
+
+ return true;
+}
+
void tdx_get_ve_info(struct ve_info *ve)
{
struct tdx_module_output out;
@@ -186,8 +216,13 @@ void tdx_get_ve_info(struct ve_info *ve)
*/
static bool virt_exception_user(struct pt_regs *regs, struct ve_info *ve)
{
- pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
- return false;
+ switch (ve->exit_reason) {
+ case EXIT_REASON_CPUID:
+ return handle_cpuid(regs);
+ default:
+ pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
+ return false;
+ }
}

/* Handle the kernel #VE */
@@ -200,6 +235,8 @@ static bool virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
return read_msr(regs);
case EXIT_REASON_MSR_WRITE:
return write_msr(regs);
+ case EXIT_REASON_CPUID:
+ return handle_cpuid(regs);
default:
pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
return false;
--
2.34.1

2022-02-18 18:17:38

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 09/32] x86/tdx: Add HLT support for TDX guests

The HLT instruction is a privileged instruction, executing it stops
instruction execution and places the processor in a HALT state. It
is used in kernel for cases like reboot, idle loop and exception fixup
handlers. For the idle case, interrupts will be enabled (using STI)
before the HLT instruction (this is also called safe_halt()).

To support the HLT instruction in TDX guests, it needs to be emulated
using TDVMCALL (hypercall to VMM). More details about it can be found
in Intel Trust Domain Extensions (Intel TDX) Guest-Host-Communication
Interface (GHCI) specification, section TDVMCALL[Instruction.HLT].

In TDX guests, executing HLT instruction will generate a #VE, which is
used to emulate the HLT instruction. But #VE based emulation will not
work for the safe_halt() flavor, because it requires STI instruction to
be executed just before the TDCALL. Since idle loop is the only user of
safe_halt() variant, handle it as a special case.

To avoid *safe_halt() call in the idle function, define the
tdx_guest_idle() and use it to override the "x86_idle" function pointer
for a valid TDX guest.

Alternative choices like PV ops have been considered for adding
safe_halt() support. But it was rejected because HLT paravirt calls
only exist under PARAVIRT_XXL, and enabling it in TDX guest just for
safe_halt() use case is not worth the cost.

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/coco/tdcall.S | 13 ++++++++
arch/x86/coco/tdx.c | 66 ++++++++++++++++++++++++++++++++++++--
arch/x86/include/asm/tdx.h | 4 +++
arch/x86/kernel/process.c | 4 +++
4 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/tdcall.S b/arch/x86/coco/tdcall.S
index c4dd9468e7d9..3c35a056974d 100644
--- a/arch/x86/coco/tdcall.S
+++ b/arch/x86/coco/tdcall.S
@@ -138,6 +138,19 @@ SYM_FUNC_START(__tdx_hypercall)

movl $TDVMCALL_EXPOSE_REGS_MASK, %ecx

+ /*
+ * For the idle loop STI needs to be called directly before the TDCALL
+ * that enters idle (EXIT_REASON_HLT case). STI instruction enables
+ * interrupts only one instruction later. If there is a window between
+ * STI and the instruction that emulates the HALT state, there is a
+ * chance for interrupts to happen in this window, which can delay the
+ * HLT operation indefinitely. Since this is the not the desired
+ * result, conditionally call STI before TDCALL.
+ */
+ testq $TDX_HCALL_ISSUE_STI, %rsi
+ jz .Lskip_sti
+ sti
+.Lskip_sti:
tdcall

/*
diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index de7a02e634c2..8adc759961f3 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -7,6 +7,7 @@
#include <linux/cpufeature.h>
#include <asm/coco.h>
#include <asm/tdx.h>
+#include <asm/vmx.h>

/* TDX module Call Leaf IDs */
#define TDX_GET_INFO 1
@@ -59,6 +60,62 @@ static void get_info(void)
td_info.attributes = out.rdx;
}

+static u64 __cpuidle __halt(const bool irq_disabled, const bool do_sti)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = EXIT_REASON_HLT,
+ .r12 = irq_disabled,
+ };
+
+ /*
+ * Emulate HLT operation via hypercall. More info about ABI
+ * can be found in TDX Guest-Host-Communication Interface
+ * (GHCI), section 3.8 TDG.VP.VMCALL<Instruction.HLT>.
+ *
+ * The VMM uses the "IRQ disabled" param to understand IRQ
+ * enabled status (RFLAGS.IF) of the TD guest and to determine
+ * whether or not it should schedule the halted vCPU if an
+ * IRQ becomes pending. E.g. if IRQs are disabled, the VMM
+ * can keep the vCPU in virtual HLT, even if an IRQ is
+ * pending, without hanging/breaking the guest.
+ */
+ return __tdx_hypercall(&args, do_sti ? TDX_HCALL_ISSUE_STI : 0);
+}
+
+static bool handle_halt(void)
+{
+ /*
+ * Since non safe halt is mainly used in CPU offlining
+ * and the guest will always stay in the halt state, don't
+ * call the STI instruction (set do_sti as false).
+ */
+ const bool irq_disabled = irqs_disabled();
+ const bool do_sti = false;
+
+ if (__halt(irq_disabled, do_sti))
+ return false;
+
+ return true;
+}
+
+void __cpuidle tdx_safe_halt(void)
+{
+ /*
+ * For do_sti=true case, __tdx_hypercall() function enables
+ * interrupts using the STI instruction before the TDCALL. So
+ * set irq_disabled as false.
+ */
+ const bool irq_disabled = false;
+ const bool do_sti = true;
+
+ /*
+ * Use WARN_ONCE() to report the failure.
+ */
+ if (__halt(irq_disabled, do_sti))
+ WARN_ONCE(1, "HLT instruction emulation failed\n");
+}
+
void tdx_get_ve_info(struct ve_info *ve)
{
struct tdx_module_output out;
@@ -98,8 +155,13 @@ static bool virt_exception_user(struct pt_regs *regs, struct ve_info *ve)
/* Handle the kernel #VE */
static bool virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
{
- pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
- return false;
+ switch (ve->exit_reason) {
+ case EXIT_REASON_HLT:
+ return handle_halt();
+ default:
+ pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
+ return false;
+ }
}

bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve)
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 34cf998ad534..e6e23ade53a6 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -13,6 +13,7 @@
#define TDX_HYPERCALL_STANDARD 0

#define TDX_HCALL_HAS_OUTPUT BIT(0)
+#define TDX_HCALL_ISSUE_STI BIT(1)

#define TDX_SEAMCALL_VMFAILINVALID 0x8000FF00FFFF0000ULL

@@ -79,9 +80,12 @@ void tdx_get_ve_info(struct ve_info *ve);

bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve);

+void tdx_safe_halt(void);
+
#else

static inline void tdx_early_init(void) { };
+static inline void tdx_safe_halt(void) { };

#endif /* CONFIG_INTEL_TDX_GUEST */

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 81d8ef036637..71aa12082370 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -46,6 +46,7 @@
#include <asm/proto.h>
#include <asm/frame.h>
#include <asm/unwind.h>
+#include <asm/tdx.h>

#include "process.h"

@@ -870,6 +871,9 @@ void select_idle_routine(const struct cpuinfo_x86 *c)
} else if (prefer_mwait_c1_over_halt(c)) {
pr_info("using mwait in idle threads\n");
x86_idle = mwait_idle;
+ } else if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
+ pr_info("using TDX aware idle routine\n");
+ x86_idle = tdx_safe_halt;
} else
x86_idle = default_idle;
}
--
2.34.1

2022-02-18 18:24:10

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 31/32] ACPICA: Avoid cache flush on TDX guest

ACPI_FLUSH_CPU_CACHE() flushes caches on entering sleep states. It is
required to prevent data loss.

While running inside TDX guest, the kernel can bypass cache flushing.
Changing sleep state in a virtual machine doesn't affect the host system
sleep state and cannot lead to data loss.

The approach can be generalized to all guest kernels, but, to be
cautious, let's limit it to TDX for now.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/acenv.h | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/acenv.h b/arch/x86/include/asm/acenv.h
index 9aff97f0de7f..d19deca6dd27 100644
--- a/arch/x86/include/asm/acenv.h
+++ b/arch/x86/include/asm/acenv.h
@@ -13,7 +13,21 @@

/* Asm macros */

-#define ACPI_FLUSH_CPU_CACHE() wbinvd()
+/*
+ * ACPI_FLUSH_CPU_CACHE() flushes caches on entering sleep states.
+ * It is required to prevent data loss.
+ *
+ * While running inside TDX guest, the kernel can bypass cache flushing.
+ * Changing sleep state in a virtual machine doesn't affect the host system
+ * sleep state and cannot lead to data loss.
+ *
+ * TODO: Is it safe to generalize this from TDX guests to all guest kernels?
+ */
+#define ACPI_FLUSH_CPU_CACHE() \
+do { \
+ if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) \
+ wbinvd(); \
+} while (0)

int __acpi_acquire_global_lock(unsigned int *lock);
int __acpi_release_global_lock(unsigned int *lock);
--
2.34.1

2022-02-18 18:35:26

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 30/32] x86/tdx: ioapic: Add shared bit for IOAPIC base address

From: Isaku Yamahata <[email protected]>

The kernel interacts with each bare-metal IOAPIC with a special
MMIO page. When running under KVM, the guest's IOAPICs are
emulated by KVM.

When running as a TDX guest, the guest needs to mark each IOAPIC
mapping as "shared" with the host. This ensures that TDX private
protections are not applied to the page, which allows the TDX host
emulation to work.

ioremap()-created mappings such as virtio will be marked as
shared by default. However, the IOAPIC code does not use ioremap() and
instead uses the fixmap mechanism.

Introduce a special fixmap helper just for the IOAPIC code. Ensure
that it marks IOAPIC pages as "shared". This replaces
set_fixmap_nocache() with __set_fixmap() since __set_fixmap()
allows custom 'prot' values.

AMD SEV gets IOAPIC pages shared because FIXMAP_PAGE_NOCACHE has _ENC
bit clear. TDX has to set bit to share the page with the host.

Signed-off-by: Isaku Yamahata <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/kernel/apic/io_apic.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index c1bb384935b0..d775f58a3c3e 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -49,6 +49,7 @@
#include <linux/slab.h>
#include <linux/memblock.h>
#include <linux/msi.h>
+#include <linux/cc_platform.h>

#include <asm/irqdomain.h>
#include <asm/io.h>
@@ -65,6 +66,7 @@
#include <asm/irq_remapping.h>
#include <asm/hw_irq.h>
#include <asm/apic.h>
+#include <asm/pgtable.h>

#define for_each_ioapic(idx) \
for ((idx) = 0; (idx) < nr_ioapics; (idx)++)
@@ -2677,6 +2679,15 @@ static struct resource * __init ioapic_setup_resources(void)
return res;
}

+static void io_apic_set_fixmap_nocache(enum fixed_addresses idx,
+ phys_addr_t phys)
+{
+ pgprot_t flags = FIXMAP_PAGE_NOCACHE;
+
+ flags = pgprot_decrypted(flags);
+ __set_fixmap(idx, phys, flags);
+}
+
void __init io_apic_init_mappings(void)
{
unsigned long ioapic_phys, idx = FIX_IO_APIC_BASE_0;
@@ -2709,7 +2720,7 @@ void __init io_apic_init_mappings(void)
__func__, PAGE_SIZE, PAGE_SIZE);
ioapic_phys = __pa(ioapic_phys);
}
- set_fixmap_nocache(idx, ioapic_phys);
+ io_apic_set_fixmap_nocache(idx, ioapic_phys);
apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08lx (%08lx)\n",
__fix_to_virt(idx) + (ioapic_phys & ~PAGE_MASK),
ioapic_phys);
@@ -2838,7 +2849,7 @@ int mp_register_ioapic(int id, u32 address, u32 gsi_base,
ioapics[idx].mp_config.flags = MPC_APIC_USABLE;
ioapics[idx].mp_config.apicaddr = address;

- set_fixmap_nocache(FIX_IO_APIC_BASE_0 + idx, address);
+ io_apic_set_fixmap_nocache(FIX_IO_APIC_BASE_0 + idx, address);
if (bad_ioapic_register(idx)) {
clear_fixmap(FIX_IO_APIC_BASE_0 + idx);
return -ENODEV;
--
2.34.1

2022-02-18 18:36:15

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 24/32] x86/boot: Avoid #VE during boot for TDX platforms

From: Sean Christopherson <[email protected]>

There are a few MSRs and control register bits that the kernel
normally needs to modify during boot. But, TDX disallows
modification of these registers to help provide consistent security
guarantees. Fortunately, TDX ensures that these are all in the correct
state before the kernel loads, which means the kernel does not need to
modify them.

The conditions to avoid are:

* Any writes to the EFER MSR
* Clearing CR3.MCE

This theoretically makes the guest boot more fragile. If, for instance,
EFER was set up incorrectly and a WRMSR was performed, it will trigger
early exception panic or a triple fault, if it's before early
exceptions are set up. However, this is likely to trip up the guest
BIOS long before control reaches the kernel. In any case, these kinds
of problems are unlikely to occur in production environments, and
developers have good debug tools to fix them quickly.

Change the common boot code to work on TDX and non-TDX systems.
This should have no functional effect on non-TDX systems.

Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/Kconfig | 1 +
arch/x86/boot/compressed/head_64.S | 20 ++++++++++++++++++--
arch/x86/boot/compressed/pgtable.h | 2 +-
arch/x86/kernel/head_64.S | 28 ++++++++++++++++++++++++++--
arch/x86/realmode/rm/trampoline_64.S | 13 ++++++++++++-
5 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 85591ecc8473..3701e13e319c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -886,6 +886,7 @@ config INTEL_TDX_GUEST
depends on X86_X2APIC
select ARCH_HAS_CC_PLATFORM
select DYNAMIC_PHYSICAL_MASK
+ select X86_MCE
help
Support running as a guest under Intel TDX. Without this support,
the guest kernel can not boot or run under TDX.
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index d0c3d33f3542..6d903b2fc544 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -643,12 +643,28 @@ SYM_CODE_START(trampoline_32bit_src)
movl $MSR_EFER, %ecx
rdmsr
btsl $_EFER_LME, %eax
+ /* Avoid writing EFER if no change was made (for TDX guest) */
+ jc 1f
wrmsr
- popl %edx
+1: popl %edx
popl %ecx

+#ifdef CONFIG_X86_MCE
+ /*
+ * Preserve CR4.MCE if the kernel will enable #MC support.
+ * Clearing MCE may fault in some environments (that also force #MC
+ * support). Any machine check that occurs before #MC support is fully
+ * configured will crash the system regardless of the CR4.MCE value set
+ * here.
+ */
+ movl %cr4, %eax
+ andl $X86_CR4_MCE, %eax
+#else
+ movl $0, %eax
+#endif
+
/* Enable PAE and LA57 (if required) paging modes */
- movl $X86_CR4_PAE, %eax
+ orl $X86_CR4_PAE, %eax
testl %edx, %edx
jz 1f
orl $X86_CR4_LA57, %eax
diff --git a/arch/x86/boot/compressed/pgtable.h b/arch/x86/boot/compressed/pgtable.h
index 6ff7e81b5628..cc9b2529a086 100644
--- a/arch/x86/boot/compressed/pgtable.h
+++ b/arch/x86/boot/compressed/pgtable.h
@@ -6,7 +6,7 @@
#define TRAMPOLINE_32BIT_PGTABLE_OFFSET 0

#define TRAMPOLINE_32BIT_CODE_OFFSET PAGE_SIZE
-#define TRAMPOLINE_32BIT_CODE_SIZE 0x70
+#define TRAMPOLINE_32BIT_CODE_SIZE 0x80

#define TRAMPOLINE_32BIT_STACK_END TRAMPOLINE_32BIT_SIZE

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 9c63fc5988cd..184b7468ea76 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -140,8 +140,22 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
addq $(init_top_pgt - __START_KERNEL_map), %rax
1:

+#ifdef CONFIG_X86_MCE
+ /*
+ * Preserve CR4.MCE if the kernel will enable #MC support.
+ * Clearing MCE may fault in some environments (that also force #MC
+ * support). Any machine check that occurs before #MC support is fully
+ * configured will crash the system regardless of the CR4.MCE value set
+ * here.
+ */
+ movq %cr4, %rcx
+ andl $X86_CR4_MCE, %ecx
+#else
+ movl $0, %ecx
+#endif
+
/* Enable PAE mode, PGE and LA57 */
- movl $(X86_CR4_PAE | X86_CR4_PGE), %ecx
+ orl $(X86_CR4_PAE | X86_CR4_PGE), %ecx
#ifdef CONFIG_X86_5LEVEL
testl $1, __pgtable_l5_enabled(%rip)
jz 1f
@@ -246,13 +260,23 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
/* Setup EFER (Extended Feature Enable Register) */
movl $MSR_EFER, %ecx
rdmsr
+ /*
+ * Preserve current value of EFER for comparison and to skip
+ * EFER writes if no change was made (for TDX guest)
+ */
+ movl %eax, %edx
btsl $_EFER_SCE, %eax /* Enable System Call */
btl $20,%edi /* No Execute supported? */
jnc 1f
btsl $_EFER_NX, %eax
btsq $_PAGE_BIT_NX,early_pmd_flags(%rip)
-1: wrmsr /* Make changes effective */

+ /* Avoid writing EFER if no change was made (for TDX guest) */
+1: cmpl %edx, %eax
+ je 1f
+ xor %edx, %edx
+ wrmsr /* Make changes effective */
+1:
/* Setup cr0 */
movl $CR0_STATE, %eax
/* Make changes effective */
diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
index d380f2d1fd23..e38d61d6562e 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -143,11 +143,22 @@ SYM_CODE_START(startup_32)
movl %eax, %cr3

# Set up EFER
+ movl $MSR_EFER, %ecx
+ rdmsr
+ /*
+ * Skip writing to EFER if the register already has desired
+ * value (to avoid #VE for the TDX guest).
+ */
+ cmp pa_tr_efer, %eax
+ jne .Lwrite_efer
+ cmp pa_tr_efer + 4, %edx
+ je .Ldone_efer
+.Lwrite_efer:
movl pa_tr_efer, %eax
movl pa_tr_efer + 4, %edx
- movl $MSR_EFER, %ecx
wrmsr

+.Ldone_efer:
# Enable paging and in turn activate Long Mode.
movl $CR0_STATE, %eax
movl %eax, %cr0
--
2.34.1

2022-02-18 18:38:37

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 15/32] x86: Consolidate port I/O helpers

There are two implementations of port I/O helpers: one in the kernel and
one in the boot stub.

Move the helpers required for both to <asm/shared/io.h> and use the one
implementation everywhere.

Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
---
arch/x86/boot/boot.h | 35 +-------------------------------
arch/x86/boot/compressed/misc.h | 2 +-
arch/x86/include/asm/io.h | 22 ++------------------
arch/x86/include/asm/shared/io.h | 34 +++++++++++++++++++++++++++++++
4 files changed, 38 insertions(+), 55 deletions(-)
create mode 100644 arch/x86/include/asm/shared/io.h

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 34c9dbb6a47d..22a474c5b3e8 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -23,6 +23,7 @@
#include <linux/edd.h>
#include <asm/setup.h>
#include <asm/asm.h>
+#include <asm/shared/io.h>
#include "bitops.h"
#include "ctype.h"
#include "cpuflags.h"
@@ -35,40 +36,6 @@ extern struct boot_params boot_params;

#define cpu_relax() asm volatile("rep; nop")

-/* Basic port I/O */
-static inline void outb(u8 v, u16 port)
-{
- asm volatile("outb %0,%1" : : "a" (v), "dN" (port));
-}
-static inline u8 inb(u16 port)
-{
- u8 v;
- asm volatile("inb %1,%0" : "=a" (v) : "dN" (port));
- return v;
-}
-
-static inline void outw(u16 v, u16 port)
-{
- asm volatile("outw %0,%1" : : "a" (v), "dN" (port));
-}
-static inline u16 inw(u16 port)
-{
- u16 v;
- asm volatile("inw %1,%0" : "=a" (v) : "dN" (port));
- return v;
-}
-
-static inline void outl(u32 v, u16 port)
-{
- asm volatile("outl %0,%1" : : "a" (v), "dN" (port));
-}
-static inline u32 inl(u16 port)
-{
- u32 v;
- asm volatile("inl %1,%0" : "=a" (v) : "dN" (port));
- return v;
-}
-
static inline void io_delay(void)
{
const u16 DELAY_PORT = 0x80;
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 0d8e275a9d96..8a253e85f990 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -22,11 +22,11 @@
#include <linux/linkage.h>
#include <linux/screen_info.h>
#include <linux/elf.h>
-#include <linux/io.h>
#include <asm/page.h>
#include <asm/boot.h>
#include <asm/bootparam.h>
#include <asm/desc_defs.h>
+#include <asm/shared/io.h>

#include "tdx.h"

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 638c1a2a82e0..a1eb218a49f8 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -44,6 +44,7 @@
#include <asm/page.h>
#include <asm/early_ioremap.h>
#include <asm/pgtable_types.h>
+#include <asm/shared/io.h>

#define build_mmio_read(name, size, type, reg, barrier) \
static inline type name(const volatile void __iomem *addr) \
@@ -258,20 +259,6 @@ static inline void slow_down_io(void)
#endif

#define BUILDIO(bwl, bw, type) \
-static inline void out##bwl(type value, u16 port) \
-{ \
- asm volatile("out" #bwl " %" #bw "0, %w1" \
- : : "a"(value), "Nd"(port)); \
-} \
- \
-static inline type in##bwl(u16 port) \
-{ \
- type value; \
- asm volatile("in" #bwl " %w1, %" #bw "0" \
- : "=a"(value) : "Nd"(port)); \
- return value; \
-} \
- \
static inline void out##bwl##_p(type value, u16 port) \
{ \
out##bwl(value, port); \
@@ -320,10 +307,8 @@ static inline void ins##bwl(u16 port, void *addr, unsigned long count) \
BUILDIO(b, b, u8)
BUILDIO(w, w, u16)
BUILDIO(l, , u32)
+#undef BUILDIO

-#define inb inb
-#define inw inw
-#define inl inl
#define inb_p inb_p
#define inw_p inw_p
#define inl_p inl_p
@@ -331,9 +316,6 @@ BUILDIO(l, , u32)
#define insw insw
#define insl insl

-#define outb outb
-#define outw outw
-#define outl outl
#define outb_p outb_p
#define outw_p outw_p
#define outl_p outl_p
diff --git a/arch/x86/include/asm/shared/io.h b/arch/x86/include/asm/shared/io.h
new file mode 100644
index 000000000000..6707cd555f0c
--- /dev/null
+++ b/arch/x86/include/asm/shared/io.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_SHARED_IO_H
+#define _ASM_X86_SHARED_IO_H
+
+#include <linux/types.h>
+
+#define BUILDIO(bwl, bw, type) \
+static inline void out##bwl(type value, u16 port) \
+{ \
+ asm volatile("out" #bwl " %" #bw "0, %w1" \
+ : : "a"(value), "Nd"(port)); \
+} \
+ \
+static inline type in##bwl(u16 port) \
+{ \
+ type value; \
+ asm volatile("in" #bwl " %w1, %" #bw "0" \
+ : "=a"(value) : "Nd"(port)); \
+ return value; \
+}
+
+BUILDIO(b, b, u8)
+BUILDIO(w, w, u16)
+BUILDIO(l, , u32)
+#undef BUILDIO
+
+#define inb inb
+#define inw inw
+#define inl inl
+#define outb outb
+#define outw outw
+#define outl outl
+
+#endif
--
2.34.1

2022-02-18 18:39:29

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 07/32] x86/tdx: Exclude shared bit from __PHYSICAL_MASK

In TDX guests, by default memory is protected from host access. If a
guest needs to communicate with the VMM (like the I/O use case), it uses
a single bit in the physical address to communicate the protected/shared
attribute of the given page.

In the x86 ARCH code, __PHYSICAL_MASK macro represents the width of the
physical address in the given architecture. It is used in creating
physical PAGE_MASK for address bits in the kernel. Since in TDX guest,
a single bit is used as metadata, it needs to be excluded from valid
physical address bits to avoid using incorrect addresses bits in the
kernel.

Enable DYNAMIC_PHYSICAL_MASK to support updating the __PHYSICAL_MASK.

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]>
Reviewed-by: Thomas Gleixner <[email protected]>
---
arch/x86/Kconfig | 1 +
arch/x86/coco/tdx.c | 8 ++++++++
2 files changed, 9 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 581aaff0ccee..85591ecc8473 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -885,6 +885,7 @@ config INTEL_TDX_GUEST
depends on X86_64 && CPU_SUP_INTEL
depends on X86_X2APIC
select ARCH_HAS_CC_PLATFORM
+ select DYNAMIC_PHYSICAL_MASK
help
Support running as a guest under Intel TDX. Without this support,
the guest kernel can not boot or run under TDX.
diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index 87e561c5b4e3..ea5f02a5d738 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -72,6 +72,14 @@ void __init tdx_early_init(void)

get_info();

+ /*
+ * All bits above GPA width are reserved and kernel treats shared bit
+ * as flag, not as part of physical address.
+ *
+ * Adjust physical mask to only cover valid GPA bits.
+ */
+ physical_mask &= GENMASK_ULL(td_info.gpa_width - 2, 0);
+
/*
* The highest bit of a guest physical address is the "sharing" bit.
* Set it for shared pages and clear it for private pages.
--
2.34.1

2022-02-18 21:00:36

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 17/32] x86/boot/compressed: Support TDX guest port I/O at decompression time

Port I/O instructions trigger #VE in the TDX environment. In response to
the exception, kernel emulates these instructions using hypercalls.

But during early boot, on the decompression stage, it is cumbersome to
deal with #VE. It is cleaner to go to hypercalls directly, bypassing #VE
handling.

Hook up TDX-specific port I/O helpers if booting in TDX environment.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/boot/compressed/Makefile | 2 +-
arch/x86/boot/compressed/tdcall.S | 3 ++
arch/x86/boot/compressed/tdx.c | 71 +++++++++++++++++++++++++++++++
arch/x86/include/asm/shared/tdx.h | 29 +++++++++++++
arch/x86/include/asm/tdx.h | 24 -----------
5 files changed, 104 insertions(+), 25 deletions(-)
create mode 100644 arch/x86/boot/compressed/tdcall.S

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 732f6b21ecbd..8fd0e6ae2e1f 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -101,7 +101,7 @@ ifdef CONFIG_X86_64
endif

vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
-vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o
+vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o $(obj)/tdcall.o

vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
efi-obj-$(CONFIG_EFI_STUB) = $(objtree)/drivers/firmware/efi/libstub/lib.a
diff --git a/arch/x86/boot/compressed/tdcall.S b/arch/x86/boot/compressed/tdcall.S
new file mode 100644
index 000000000000..59b80ab6b41c
--- /dev/null
+++ b/arch/x86/boot/compressed/tdcall.S
@@ -0,0 +1,3 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include "../../coco/tdcall.S"
diff --git a/arch/x86/boot/compressed/tdx.c b/arch/x86/boot/compressed/tdx.c
index 8a72ce2e880f..6e6f3d6d09b0 100644
--- a/arch/x86/boot/compressed/tdx.c
+++ b/arch/x86/boot/compressed/tdx.c
@@ -2,6 +2,10 @@

#include "../cpuflags.h"
#include "../string.h"
+#include "../io.h"
+
+#include <vdso/limits.h>
+#include <uapi/asm/vmx.h>

#include <asm/shared/tdx.h>

@@ -12,6 +16,66 @@ bool early_is_tdx_guest(void)
return tdx_guest_detected;
}

+static inline unsigned int tdx_io_in(int size, u16 port)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = EXIT_REASON_IO_INSTRUCTION,
+ .r12 = size,
+ .r13 = 0,
+ .r14 = port,
+ };
+
+ if (__tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT))
+ return UINT_MAX;
+
+ return args.r11;
+}
+
+static inline void tdx_io_out(int size, u16 port, u32 value)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = EXIT_REASON_IO_INSTRUCTION,
+ .r12 = size,
+ .r13 = 1,
+ .r14 = port,
+ .r15 = value,
+ };
+
+ __tdx_hypercall(&args, 0);
+}
+
+static inline u8 tdx_inb(u16 port)
+{
+ return tdx_io_in(1, port);
+}
+
+static inline u16 tdx_inw(u16 port)
+{
+ return tdx_io_in(2, port);
+}
+
+static inline u32 tdx_inl(u16 port)
+{
+ return tdx_io_in(4, port);
+}
+
+static inline void tdx_outb(u8 value, u16 port)
+{
+ tdx_io_out(1, port, value);
+}
+
+static inline void tdx_outw(u16 value, u16 port)
+{
+ tdx_io_out(2, port, value);
+}
+
+static inline void tdx_outl(u32 value, u16 port)
+{
+ tdx_io_out(4, port, value);
+}
+
void early_tdx_detect(void)
{
u32 eax, sig[3];
@@ -23,4 +87,11 @@ void early_tdx_detect(void)

/* Cache TDX guest feature status */
tdx_guest_detected = true;
+
+ pio_ops.inb = tdx_inb;
+ pio_ops.inw = tdx_inw;
+ pio_ops.inl = tdx_inl;
+ pio_ops.outb = tdx_outb;
+ pio_ops.outw = tdx_outw;
+ pio_ops.outl = tdx_outl;
}
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 8209ba9ffe1a..51bce6351124 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -2,7 +2,36 @@
#ifndef _ASM_X86_SHARED_TDX_H
#define _ASM_X86_SHARED_TDX_H

+#include <linux/bits.h>
+#include <linux/types.h>
+
+#define TDX_HYPERCALL_STANDARD 0
+
+#define TDX_HCALL_HAS_OUTPUT BIT(0)
+#define TDX_HCALL_ISSUE_STI BIT(1)
+
#define TDX_CPUID_LEAF_ID 0x21
#define TDX_IDENT "IntelTDX "

+#ifndef __ASSEMBLY__
+
+/*
+ * Used in __tdx_hypercall() to pass down and get back registers' values of
+ * the TDCALL instruction when requesting services from the VMM.
+ *
+ * This is a software only structure and not part of the TDX module/VMM ABI.
+ */
+struct tdx_hypercall_args {
+ u64 r10;
+ u64 r11;
+ u64 r12;
+ u64 r13;
+ u64 r14;
+ u64 r15;
+};
+
+/* Used to request services from the VMM */
+u64 __tdx_hypercall(struct tdx_hypercall_args *args, unsigned long flags);
+
+#endif /* !__ASSEMBLY__ */
#endif /* _ASM_X86_SHARED_TDX_H */
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index d4c28b9f2fbc..54803cb6ccf5 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -3,16 +3,10 @@
#ifndef _ASM_X86_TDX_H
#define _ASM_X86_TDX_H

-#include <linux/bits.h>
#include <linux/init.h>
#include <asm/ptrace.h>
#include <asm/shared/tdx.h>

-#define TDX_HYPERCALL_STANDARD 0
-
-#define TDX_HCALL_HAS_OUTPUT BIT(0)
-#define TDX_HCALL_ISSUE_STI BIT(1)
-
#define TDX_SEAMCALL_VMFAILINVALID 0x8000FF00FFFF0000ULL

#ifndef __ASSEMBLY__
@@ -32,21 +26,6 @@ struct tdx_module_output {
u64 r11;
};

-/*
- * Used in __tdx_hypercall() to pass down and get back registers' values of
- * the TDCALL instruction when requesting services from the VMM.
- *
- * This is a software only structure and not part of the TDX module/VMM ABI.
- */
-struct tdx_hypercall_args {
- u64 r10;
- u64 r11;
- u64 r12;
- u64 r13;
- u64 r14;
- u64 r15;
-};
-
/*
* Used by the #VE exception handler to gather the #VE exception
* info from the TDX module. This is a software only structure
@@ -71,9 +50,6 @@ void __init tdx_early_init(void);
u64 __tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
struct tdx_module_output *out);

-/* Used to request services from the VMM */
-u64 __tdx_hypercall(struct tdx_hypercall_args *args, unsigned long flags);
-
void tdx_get_ve_info(struct ve_info *ve);

bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve);
--
2.34.1

2022-02-18 21:26:16

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 04/32] x86/tdx: Provide common base for SEAMCALL and TDCALL C wrappers

Secure Arbitration Mode (SEAM) is an extension of VMX architecture. It
defines a new VMX root operation (SEAM VMX root) and a new VMX non-root
operation (SEAM VMX non-root) which are both isolated from the legacy
VMX operation where the host kernel runs.

A CPU-attested software module (called 'TDX module') runs in SEAM VMX
root to manage and protect VMs running in SEAM VMX non-root. SEAM VMX
root is also used to host another CPU-attested software module (called
'P-SEAMLDR') to load and update the TDX module.

Host kernel transits to either P-SEAMLDR or TDX module via the new
SEAMCALL instruction, which is essentially a VMExit from VMX root mode
to SEAM VMX root mode. SEAMCALLs are leaf functions defined by
P-SEAMLDR and TDX module around the new SEAMCALL instruction.

A guest kernel can also communicate with TDX module via TDCALL
instruction.

TDCALLs and SEAMCALLs use an ABI different from the x86-64 system-v ABI.
RAX is used to carry both the SEAMCALL leaf function number (input) and
the completion status (output). Additional GPRs (RCX, RDX, R8-R11) may
be further used as both input and output operands in individual leaf.

TDCALL and SEAMCALL share the same ABI and require the largely same
code to pass down arguments and retrieve results.

Define an assembly macro that can be used to implement C wrapper for
both TDCALL and SEAMCALL.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/tdx.h | 20 ++++++++
arch/x86/kernel/asm-offsets.c | 9 ++++
arch/x86/virt/tdxcall.S | 91 +++++++++++++++++++++++++++++++++++
3 files changed, 120 insertions(+)
create mode 100644 arch/x86/virt/tdxcall.S

diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index ba8042ce61c2..2f8cb1e53e77 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -8,6 +8,25 @@
#define TDX_CPUID_LEAF_ID 0x21
#define TDX_IDENT "IntelTDX "

+#define TDX_SEAMCALL_VMFAILINVALID 0x8000FF00FFFF0000ULL
+
+#ifndef __ASSEMBLY__
+
+/*
+ * Used to gather the output registers values of the TDCALL and SEAMCALL
+ * instructions when requesting services from the TDX module.
+ *
+ * This is a software only structure and not part of the TDX module/VMM ABI.
+ */
+struct tdx_module_output {
+ u64 rcx;
+ u64 rdx;
+ u64 r8;
+ u64 r9;
+ u64 r10;
+ u64 r11;
+};
+
#ifdef CONFIG_INTEL_TDX_GUEST

void __init tdx_early_init(void);
@@ -18,4 +37,5 @@ static inline void tdx_early_init(void) { };

#endif /* CONFIG_INTEL_TDX_GUEST */

+#endif /* !__ASSEMBLY__ */
#endif /* _ASM_X86_TDX_H */
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 9fb0a2f8b62a..7dca52f5cfc6 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -18,6 +18,7 @@
#include <asm/bootparam.h>
#include <asm/suspend.h>
#include <asm/tlbflush.h>
+#include <asm/tdx.h>

#ifdef CONFIG_XEN
#include <xen/interface/xen.h>
@@ -65,6 +66,14 @@ static void __used common(void)
OFFSET(XEN_vcpu_info_arch_cr2, vcpu_info, arch.cr2);
#endif

+ BLANK();
+ OFFSET(TDX_MODULE_rcx, tdx_module_output, rcx);
+ OFFSET(TDX_MODULE_rdx, tdx_module_output, rdx);
+ OFFSET(TDX_MODULE_r8, tdx_module_output, r8);
+ OFFSET(TDX_MODULE_r9, tdx_module_output, r9);
+ OFFSET(TDX_MODULE_r10, tdx_module_output, r10);
+ OFFSET(TDX_MODULE_r11, tdx_module_output, r11);
+
BLANK();
OFFSET(BP_scratch, boot_params, scratch);
OFFSET(BP_secure_boot, boot_params, secure_boot);
diff --git a/arch/x86/virt/tdxcall.S b/arch/x86/virt/tdxcall.S
new file mode 100644
index 000000000000..90569faedacc
--- /dev/null
+++ b/arch/x86/virt/tdxcall.S
@@ -0,0 +1,91 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <asm/asm-offsets.h>
+#include <asm/tdx.h>
+
+/*
+ * TDX guests use the TDCALL instruction to make requests to the
+ * TDX module and hypercalls to the VMM.
+ *
+ * TDX host user SEAMCALL instruction to make requests to TDX module.
+ *
+ * They are supported in Binutils >= 2.36.
+ */
+#define tdcall .byte 0x66,0x0f,0x01,0xcc
+#define seamcall .byte 0x66,0x0f,0x01,0xcf
+
+.macro TDX_MODULE_CALL host:req
+ /*
+ * R12 will be used as temporary storage for struct tdx_module_output
+ * pointer. Since R12-R15 registers are not used by TDCALL/SEAMCALL
+ * services supported by this function, it can be reused.
+ */
+
+ /* Callee saved, so preserve it */
+ push %r12
+
+ /*
+ * Push output pointer to stack.
+ * After the operation, it will be fetched into R12 register.
+ */
+ push %r9
+
+ /* Mangle function call ABI into TDCALL/SEAMCALL ABI: */
+ /* Move Leaf ID to RAX */
+ mov %rdi, %rax
+ /* Move input 4 to R9 */
+ mov %r8, %r9
+ /* Move input 3 to R8 */
+ mov %rcx, %r8
+ /* Move input 1 to RCX */
+ mov %rsi, %rcx
+ /* Leave input param 2 in RDX */
+
+ .if \host
+ seamcall
+ /*
+ * SEAMCALL instruction is essentially a VMExit from VMX root
+ * mode to SEAM VMX root mode. VMfailInvalid (CF=1) indicates
+ * that the targeted SEAM firmware is not loaded or disabled,
+ * or P-SEAMLDR is busy with another SEAMCALL. %rax is not
+ * changed in this case.
+ *
+ * Set %rax to TDX_SEAMCALL_VMFAILINVALID for VMfailInvalid.
+ * This value will never be used as actual SEAMCALL error code.
+ */
+ jnc .Lno_vmfailinvalid
+ mov $TDX_SEAMCALL_VMFAILINVALID, %rax
+.Lno_vmfailinvalid:
+ .else
+ tdcall
+ .endif
+
+ /*
+ * Fetch output pointer from stack to R12 (It is used
+ * as temporary storage)
+ */
+ pop %r12
+
+ /* Check for success: 0 - Successful, otherwise failed */
+ test %rax, %rax
+ jnz .Lno_output_struct
+
+ /*
+ * Since this function can be initiated without an output pointer,
+ * check if caller provided an output struct before storing
+ * output registers.
+ */
+ test %r12, %r12
+ jz .Lno_output_struct
+
+ /* Copy result registers to output struct: */
+ movq %rcx, TDX_MODULE_rcx(%r12)
+ movq %rdx, TDX_MODULE_rdx(%r12)
+ movq %r8, TDX_MODULE_r8(%r12)
+ movq %r9, TDX_MODULE_r9(%r12)
+ movq %r10, TDX_MODULE_r10(%r12)
+ movq %r11, TDX_MODULE_r11(%r12)
+
+.Lno_output_struct:
+ /* Restore the state of R12 register */
+ pop %r12
+.endm
--
2.34.1

2022-02-18 21:48:03

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 12/32] x86/tdx: Handle in-kernel MMIO

In non-TDX VMs, MMIO is implemented by providing the guest a mapping
which will cause a VMEXIT on access and then the VMM emulating the
instruction that caused the VMEXIT. That's not possible for TDX VM.

To emulate an instruction an emulator needs two things:

- R/W access to the register file to read/modify instruction arguments
and see RIP of the faulted instruction.

- Read access to memory where instruction is placed to see what to
emulate. In this case it is guest kernel text.

Both of them are not available to VMM in TDX environment:

- Register file is never exposed to VMM. When a TD exits to the module,
it saves registers into the state-save area allocated for that TD.
The module then scrubs these registers before returning execution
control to the VMM, to help prevent leakage of TD state.

- Memory is encrypted a TD-private key. The CPU disallows software
other than the TDX module and TDs from making memory accesses using
the private key.

In TDX the MMIO regions are instead configured to trigger a #VE
exception in the guest. The guest #VE handler then emulates the MMIO
instruction inside the guest and converts it into a controlled hypercall
to the host.

MMIO addresses can be used with any CPU instruction that accesses
memory. Address only MMIO accesses done via io.h helpers, such as
'readl()' or 'writeq()'.

readX()/writeX() helpers limit the range of instructions which can trigger
MMIO. It makes MMIO instruction emulation feasible. Raw access to a MMIO
region allows the compiler to generate whatever instruction it wants.
Supporting all possible instructions is a task of a different scope.

MMIO access with anything other than helpers from io.h may result in
MMIO_DECODE_FAILED and an oops.

AMD SEV has the same limitations to MMIO handling.

=== Potential alternative approaches ===

== Paravirtualizing all MMIO ==

An alternative to letting MMIO induce a #VE exception is to avoid
the #VE in the first place. Similar to the port I/O case, it is
theoretically possible to paravirtualize MMIO accesses.

Like the exception-based approach offered here, a fully paravirtualized
approach would be limited to MMIO users that leverage common
infrastructure like the io.h macros.

However, any paravirtual approach would be patching approximately
120k call sites. With a conservative overhead estimation of 5 bytes per
call site (CALL instruction), it leads to bloating code by 600k.

Many drivers will never be used in the TDX environment and the bloat
cannot be justified.

== Patching TDX drivers ==

Rather than touching the entire kernel, it might also be possible to
just go after drivers that use MMIO in TDX guests. Right now, that's
limited only to virtio and some x86-specific drivers.

All virtio MMIO appears to be done through a single function, which
makes virtio eminently easy to patch. This will be implemented in the
future, removing the bulk of MMIO #VEs.

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/coco/tdx.c | 110 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 110 insertions(+)

diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index 83cbc94b30d0..74ab7c5a767d 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -8,11 +8,17 @@
#include <asm/coco.h>
#include <asm/tdx.h>
#include <asm/vmx.h>
+#include <asm/insn.h>
+#include <asm/insn-eval.h>

/* TDX module Call Leaf IDs */
#define TDX_GET_INFO 1
#define TDX_GET_VEINFO 3

+/* MMIO direction */
+#define EPT_READ 0
+#define EPT_WRITE 1
+
static struct {
unsigned int gpa_width;
unsigned long attributes;
@@ -184,6 +190,108 @@ static bool handle_cpuid(struct pt_regs *regs)
return true;
}

+static bool mmio_read(int size, unsigned long addr, unsigned long *val)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = EXIT_REASON_EPT_VIOLATION,
+ .r12 = size,
+ .r13 = EPT_READ,
+ .r14 = addr,
+ .r15 = *val,
+ };
+
+ if (__tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT))
+ return false;
+ *val = args.r11;
+ return true;
+}
+
+static bool mmio_write(int size, unsigned long addr, unsigned long val)
+{
+ return !_tdx_hypercall(EXIT_REASON_EPT_VIOLATION, size, EPT_WRITE,
+ addr, val);
+}
+
+static bool handle_mmio(struct pt_regs *regs, struct ve_info *ve)
+{
+ char buffer[MAX_INSN_SIZE];
+ unsigned long *reg, val;
+ struct insn insn = {};
+ enum mmio_type mmio;
+ int size, extend_size;
+ u8 extend_val = 0;
+
+ if (copy_from_kernel_nofault(buffer, (void *)regs->ip, MAX_INSN_SIZE))
+ return false;
+
+ if (insn_decode(&insn, buffer, MAX_INSN_SIZE, INSN_MODE_64))
+ return false;
+
+ mmio = insn_decode_mmio(&insn, &size);
+ if (WARN_ON_ONCE(mmio == MMIO_DECODE_FAILED))
+ return false;
+
+ if (mmio != MMIO_WRITE_IMM && mmio != MMIO_MOVS) {
+ reg = insn_get_modrm_reg_ptr(&insn, regs);
+ if (!reg)
+ return false;
+ }
+
+ ve->instr_len = insn.length;
+
+ switch (mmio) {
+ case MMIO_WRITE:
+ memcpy(&val, reg, size);
+ return mmio_write(size, ve->gpa, val);
+ case MMIO_WRITE_IMM:
+ val = insn.immediate.value;
+ return mmio_write(size, ve->gpa, val);
+ case MMIO_READ:
+ case MMIO_READ_ZERO_EXTEND:
+ case MMIO_READ_SIGN_EXTEND:
+ break;
+ case MMIO_MOVS:
+ case MMIO_DECODE_FAILED:
+ return false;
+ default:
+ BUG();
+ }
+
+ /* Handle reads */
+ if (!mmio_read(size, ve->gpa, &val))
+ return false;
+
+ switch (mmio) {
+ case MMIO_READ:
+ /* Zero-extend for 32-bit operation */
+ extend_size = size == 4 ? sizeof(*reg) : 0;
+ break;
+ case MMIO_READ_ZERO_EXTEND:
+ /* Zero extend based on operand size */
+ extend_size = insn.opnd_bytes;
+ break;
+ case MMIO_READ_SIGN_EXTEND:
+ /* Sign extend based on operand size */
+ extend_size = insn.opnd_bytes;
+ if (size == 1 && val & BIT(7))
+ extend_val = 0xFF;
+ else if (size > 1 && val & BIT(15))
+ extend_val = 0xFF;
+ break;
+ case MMIO_MOVS:
+ case MMIO_DECODE_FAILED:
+ return false;
+ default:
+ BUG();
+ }
+
+ if (extend_size)
+ memset(reg, extend_val, extend_size);
+ memcpy(reg, &val, size);
+ return true;
+}
+
void tdx_get_ve_info(struct ve_info *ve)
{
struct tdx_module_output out;
@@ -237,6 +345,8 @@ static bool virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
return write_msr(regs);
case EXIT_REASON_CPUID:
return handle_cpuid(regs);
+ case EXIT_REASON_EPT_VIOLATION:
+ return handle_mmio(regs, ve);
default:
pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
return false;
--
2.34.1

2022-02-18 21:52:18

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 20/32] x86/tdx: Wire up KVM hypercalls

From: Kuppuswamy Sathyanarayanan <[email protected]>

KVM hypercalls use the VMCALL or VMMCALL instructions. Although the ABI
is similar, those instructions no longer function for TDX guests.

Make vendor-specific TDVMCALLs instead of VMCALL. This enables TDX
guests to run with KVM acting as the hypervisor.

Among other things, KVM hypercall is used to send IPIs.

Since the KVM driver can be built as a kernel module, export
tdx_kvm_hypercall() to make the symbols visible to kvm.ko.

Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
---
arch/x86/coco/tdx.c | 17 +++++++++++++++++
arch/x86/include/asm/kvm_para.h | 22 ++++++++++++++++++++++
arch/x86/include/asm/tdx.h | 11 +++++++++++
3 files changed, 50 insertions(+)

diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index d23d389124a1..f73983f3ffc4 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -48,6 +48,23 @@ static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15)
return __tdx_hypercall(&args, 0);
}

+#ifdef CONFIG_KVM_GUEST
+long tdx_kvm_hypercall(unsigned int nr, unsigned long p1, unsigned long p2,
+ unsigned long p3, unsigned long p4)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = nr,
+ .r11 = p1,
+ .r12 = p2,
+ .r13 = p3,
+ .r14 = p4,
+ };
+
+ return __tdx_hypercall(&args, 0);
+}
+EXPORT_SYMBOL_GPL(tdx_kvm_hypercall);
+#endif
+
static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
struct tdx_module_output *out)
{
diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index 56935ebb1dfe..57bc74e112f2 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -7,6 +7,8 @@
#include <linux/interrupt.h>
#include <uapi/asm/kvm_para.h>

+#include <asm/tdx.h>
+
#ifdef CONFIG_KVM_GUEST
bool kvm_check_and_clear_guest_paused(void);
#else
@@ -32,6 +34,10 @@ static inline bool kvm_check_and_clear_guest_paused(void)
static inline long kvm_hypercall0(unsigned int nr)
{
long ret;
+
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return tdx_kvm_hypercall(nr, 0, 0, 0, 0);
+
asm volatile(KVM_HYPERCALL
: "=a"(ret)
: "a"(nr)
@@ -42,6 +48,10 @@ static inline long kvm_hypercall0(unsigned int nr)
static inline long kvm_hypercall1(unsigned int nr, unsigned long p1)
{
long ret;
+
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return tdx_kvm_hypercall(nr, p1, 0, 0, 0);
+
asm volatile(KVM_HYPERCALL
: "=a"(ret)
: "a"(nr), "b"(p1)
@@ -53,6 +63,10 @@ static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
unsigned long p2)
{
long ret;
+
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return tdx_kvm_hypercall(nr, p1, p2, 0, 0);
+
asm volatile(KVM_HYPERCALL
: "=a"(ret)
: "a"(nr), "b"(p1), "c"(p2)
@@ -64,6 +78,10 @@ static inline long kvm_hypercall3(unsigned int nr, unsigned long p1,
unsigned long p2, unsigned long p3)
{
long ret;
+
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return tdx_kvm_hypercall(nr, p1, p2, p3, 0);
+
asm volatile(KVM_HYPERCALL
: "=a"(ret)
: "a"(nr), "b"(p1), "c"(p2), "d"(p3)
@@ -76,6 +94,10 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
unsigned long p4)
{
long ret;
+
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return tdx_kvm_hypercall(nr, p1, p2, p3, p4);
+
asm volatile(KVM_HYPERCALL
: "=a"(ret)
: "a"(nr), "b"(p1), "c"(p2), "d"(p3), "S"(p4)
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index ba0f8c2b185c..6a97d42b0de9 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -67,5 +67,16 @@ static inline bool tdx_early_handle_ve(struct pt_regs *regs) { return false; }

#endif /* CONFIG_INTEL_TDX_GUEST */

+#if defined(CONFIG_KVM_GUEST) && defined(CONFIG_INTEL_TDX_GUEST)
+long tdx_kvm_hypercall(unsigned int nr, unsigned long p1, unsigned long p2,
+ unsigned long p3, unsigned long p4);
+#else
+static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
+ unsigned long p2, unsigned long p3,
+ unsigned long p4)
+{
+ return -ENODEV;
+}
+#endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_X86_TDX_H */
--
2.34.1

2022-02-18 22:37:55

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 19/32] x86/tdx: Handle early boot port I/O

From: Andi Kleen <[email protected]>

TDX guests cannot do port I/O directly. The TDX module triggers a #VE
exception to let the guest kernel emulate port I/O by converting them
into TDCALLs to call the host.

But before IDT handlers are set up, port I/O cannot be emulated using
normal kernel #VE handlers. To support the #VE-based emulation during
this boot window, add a minimal early #VE handler support in early
exception handlers. This is similar to what AMD SEV does. This is
mainly to support earlyprintk's serial driver, as well as potentially
the VGA driver.

The early handler only supports I/O-related #VE exceptions. Unhandled or
failed exceptions will be handled via early_fixup_exceptions() (like
normal exception failures).

Signed-off-by: Andi Kleen <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
---
arch/x86/coco/tdx.c | 16 ++++++++++++++++
arch/x86/include/asm/tdx.h | 4 ++++
arch/x86/kernel/head64.c | 3 +++
3 files changed, 23 insertions(+)

diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index e3693d0494a7..d23d389124a1 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -344,6 +344,22 @@ static bool handle_io(struct pt_regs *regs, u32 exit_qual)
return ret;
}

+/*
+ * Early #VE exception handler. Only handles a subset of port I/O.
+ * Intended only for earlyprintk. If failed, return false.
+ */
+__init bool tdx_early_handle_ve(struct pt_regs *regs)
+{
+ struct ve_info ve;
+
+ tdx_get_ve_info(&ve);
+
+ if (ve.exit_reason != EXIT_REASON_IO_INSTRUCTION)
+ return false;
+
+ return handle_io(regs, ve.exit_qual);
+}
+
void tdx_get_ve_info(struct ve_info *ve)
{
struct tdx_module_output out;
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 54803cb6ccf5..ba0f8c2b185c 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -56,11 +56,15 @@ bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve);

void tdx_safe_halt(void);

+bool tdx_early_handle_ve(struct pt_regs *regs);
+
#else

static inline void tdx_early_init(void) { };
static inline void tdx_safe_halt(void) { };

+static inline bool tdx_early_handle_ve(struct pt_regs *regs) { return false; }
+
#endif /* CONFIG_INTEL_TDX_GUEST */

#endif /* !__ASSEMBLY__ */
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 6dff50c3edd6..ecbf50e5b8e0 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -417,6 +417,9 @@ void __init do_early_exception(struct pt_regs *regs, int trapnr)
trapnr == X86_TRAP_VC && handle_vc_boot_ghcb(regs))
return;

+ if (trapnr == X86_TRAP_VE && tdx_early_handle_ve(regs))
+ return;
+
early_fixup_exception(regs, trapnr);
}

--
2.34.1

2022-02-18 22:43:43

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 06/32] x86/tdx: Extend the confidential computing API to support TDX guests

Confidential Computing (CC) features (like string I/O unroll support,
memory encryption/decryption support, etc) are conditionally enabled
in the kernel using cc_platform_has() API. Since TDX guests also need
to use these CC features, extend cc_platform_has() API and add TDX
guest-specific CC attributes support.

Like AMD SME/SEV, TDX uses a bit in the page table entry to indicate
encryption status of the page, but the polarity of the mask is
opposite to AMD: if the bit is set the page is accessible to VMM.

Details about which bit in the page table entry to be used to indicate
shared/private state can be determined by using the TDINFO TDCALL.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/Kconfig | 1 +
arch/x86/coco/tdx.c | 44 +++++++++++++++++++++++++++++++++++
arch/x86/kernel/cc_platform.c | 6 +++++
3 files changed, 51 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ea4190c53db6..581aaff0ccee 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -884,6 +884,7 @@ config INTEL_TDX_GUEST
bool "Intel TDX (Trust Domain Extensions) - Guest Support"
depends on X86_64 && CPU_SUP_INTEL
depends on X86_X2APIC
+ select ARCH_HAS_CC_PLATFORM
help
Support running as a guest under Intel TDX. Without this support,
the guest kernel can not boot or run under TDX.
diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index 89367fa576c1..87e561c5b4e3 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -5,8 +5,17 @@
#define pr_fmt(fmt) "tdx: " fmt

#include <linux/cpufeature.h>
+#include <asm/coco.h>
#include <asm/tdx.h>

+/* TDX module Call Leaf IDs */
+#define TDX_GET_INFO 1
+
+static struct {
+ unsigned int gpa_width;
+ unsigned long attributes;
+} td_info __ro_after_init;
+
/*
* Wrapper for standard use of __tdx_hypercall with no output aside from
* return code.
@@ -25,9 +34,34 @@ static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15)
return __tdx_hypercall(&args, 0);
}

+static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+ struct tdx_module_output *out)
+{
+ if (__tdx_module_call(fn, rcx, rdx, r8, r9, out))
+ panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
+}
+
+static void get_info(void)
+{
+ struct tdx_module_output out;
+
+ /*
+ * TDINFO TDX module call is used to get the TD execution environment
+ * information like GPA width, number of available vcpus, debug mode
+ * information, etc. More details about the ABI can be found in TDX
+ * Guest-Host-Communication Interface (GHCI), section 2.4.2 TDCALL
+ * [TDG.VP.INFO].
+ */
+ tdx_module_call(TDX_GET_INFO, 0, 0, 0, 0, &out);
+
+ td_info.gpa_width = out.rcx & GENMASK(5, 0);
+ td_info.attributes = out.rdx;
+}
+
void __init tdx_early_init(void)
{
u32 eax, sig[3];
+ u64 mask;

cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2], &sig[1]);

@@ -36,5 +70,15 @@ void __init tdx_early_init(void)

setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);

+ get_info();
+
+ /*
+ * The highest bit of a guest physical address is the "sharing" bit.
+ * Set it for shared pages and clear it for private pages.
+ */
+ mask = BIT_ULL(td_info.gpa_width - 1);
+
+ cc_init(CC_VENDOR_INTEL, mask);
+
pr_info("Guest detected\n");
}
diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index 93e6be7b7eca..dba713c444ab 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -90,6 +90,8 @@ u64 cc_get_mask(bool enc)
switch (cc_vendor) {
case CC_VENDOR_AMD:
return enc ? cc_mask : 0;
+ case CC_VENDOR_INTEL:
+ return enc ? 0 : cc_mask;
default:
return 0;
}
@@ -100,6 +102,8 @@ u64 cc_mkenc(u64 val)
switch (cc_vendor) {
case CC_VENDOR_AMD:
return val | cc_mask;
+ case CC_VENDOR_INTEL:
+ return val & ~cc_mask;
default:
return val;
}
@@ -110,6 +114,8 @@ u64 cc_mkdec(u64 val)
switch (cc_vendor) {
case CC_VENDOR_AMD:
return val & ~cc_mask;
+ case CC_VENDOR_INTEL:
+ return val | cc_mask;
default:
return val;
}
--
2.34.1

2022-02-19 00:29:17

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 28/32] x86/mm/cpa: Add support for TDX shared memory

Intel TDX protects guest memory from VMM access. Any memory that is
required for communication with the VMM must be explicitly shared.

It is a two-step process: the guest sets the shared bit in the page
table entry and notifies VMM about the change. The notification happens
using MapGPA hypercall.

Conversion back to private memory requires clearing the shared bit,
notifying VMM with MapGPA hypercall following with accepting the memory
with AcceptPage hypercall.

Provide a TDX version of x86_platform.cc callbacks. It makes
__set_memory_enc_pgtable() work right in TDX guest.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/coco/tdx.c | 108 ++++++++++++++++++++++++++++++++++
arch/x86/kernel/cc_platform.c | 1 +
2 files changed, 109 insertions(+)

diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index f73983f3ffc4..5a833569acb8 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -10,10 +10,15 @@
#include <asm/vmx.h>
#include <asm/insn.h>
#include <asm/insn-eval.h>
+#include <asm/x86_init.h>

/* TDX module Call Leaf IDs */
#define TDX_GET_INFO 1
#define TDX_GET_VEINFO 3
+#define TDX_ACCEPT_PAGE 6
+
+/* TDX hypercall Leaf IDs */
+#define TDVMCALL_MAP_GPA 0x10001

/* MMIO direction */
#define EPT_READ 0
@@ -456,6 +461,107 @@ bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve)
return ret;
}

+static bool tdx_tlb_flush_required(bool enc)
+{
+ /*
+ * TDX guest is responsible for flushing caches on private->shared
+ * transition. VMM is responsible for flushing on shared->private.
+ */
+ return !enc;
+}
+
+static bool tdx_cache_flush_required(void)
+{
+ return true;
+}
+
+static bool accept_page(phys_addr_t gpa, enum pg_level pg_level)
+{
+ /*
+ * Pass the page physical address to the TDX module to accept the
+ * pending, private page.
+ *
+ * Bits 2:0 of GPA encode page size: 0 - 4K, 1 - 2M, 2 - 1G.
+ */
+ switch (pg_level) {
+ case PG_LEVEL_4K:
+ break;
+ case PG_LEVEL_2M:
+ gpa |= 1;
+ break;
+ case PG_LEVEL_1G:
+ gpa |= 2;
+ break;
+ default:
+ return false;
+ }
+
+ return !__tdx_module_call(TDX_ACCEPT_PAGE, gpa, 0, 0, 0, NULL);
+}
+
+/*
+ * Inform the VMM of the guest's intent for this physical page: shared with
+ * the VMM or private to the guest. The VMM is expected to change its mapping
+ * of the page in response.
+ */
+static int tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
+{
+ phys_addr_t start = __pa(vaddr);
+ phys_addr_t end = __pa(vaddr + numpages * PAGE_SIZE);
+
+ if (end <= start)
+ return -EINVAL;
+
+ if (!enc) {
+ start = cc_mkenc(start);
+ end = cc_mkenc(end);
+ }
+
+ /*
+ * Notify the VMM about page mapping conversion. More info about ABI
+ * can be found in TDX Guest-Host-Communication Interface (GHCI),
+ * section "TDG.VP.VMCALL<MapGPA>"
+ */
+ if (_tdx_hypercall(TDVMCALL_MAP_GPA, start, end - start, 0, 0))
+ return -EIO;
+
+ /* private->shared conversion requires only MapGPA call */
+ if (!enc)
+ return 0;
+
+ /*
+ * For shared->private conversion, accept the page using
+ * TDX_ACCEPT_PAGE TDX module call.
+ */
+ while (start < end) {
+ /* Try if 1G page accept is possible */
+ if (!(start & ~PUD_MASK) && end - start >= PUD_SIZE &&
+ accept_page(start, PG_LEVEL_1G)) {
+ start += PUD_SIZE;
+ continue;
+ }
+
+ /* Try if 2M page accept is possible */
+ if (!(start & ~PMD_MASK) && end - start >= PMD_SIZE &&
+ accept_page(start, PG_LEVEL_2M)) {
+ start += PMD_SIZE;
+ continue;
+ }
+
+ if (!accept_page(start, PG_LEVEL_4K))
+ return -EIO;
+ start += PAGE_SIZE;
+ }
+
+ return 0;
+}
+
+static const struct x86_cc_runtime tdx_cc_runtime = {
+ .enc_status_changed = tdx_enc_status_changed,
+ .enc_tlb_flush_required = tdx_tlb_flush_required,
+ .enc_cache_flush_required = tdx_cache_flush_required,
+};
+
void __init tdx_early_init(void)
{
u32 eax, sig[3];
@@ -486,5 +592,7 @@ void __init tdx_early_init(void)

cc_init(CC_VENDOR_INTEL, mask);

+ x86_platform.cc = &tdx_cc_runtime;
+
pr_info("Guest detected\n");
}
diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index 4a05b13e24f2..fac4d588d3b3 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -21,6 +21,7 @@ static bool intel_cc_platform_has(enum cc_attr attr)
switch (attr) {
case CC_ATTR_GUEST_UNROLL_STRING_IO:
case CC_ATTR_HOTPLUG_DISABLED:
+ case CC_ATTR_GUEST_MEM_ENCRYPT:
return true;
default:
return false;
--
2.34.1

2022-02-19 00:29:47

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 10/32] x86/tdx: Add MSR support for TDX guests

Use hypercall to emulate MSR read/write for the TDX platform.

There are two viable approaches for doing MSRs in a TD guest:

1. Execute the RDMSR/WRMSR instructions like most VMs and bare metal
do. Some will succeed, others will cause a #VE. All of those that
cause a #VE will be handled with a TDCALL.
2. Use paravirt infrastructure. The paravirt hook has to keep a list
of which MSRs would cause a #VE and use a TDCALL. All other MSRs
execute RDMSR/WRMSR instructions directly.

The second option can be ruled out because the list of MSRs was
challenging to maintain. That leaves option #1 as the only viable
solution for the minimal TDX support.

For performance-critical MSR writes (like TSC_DEADLINE), future patches
will replace the WRMSR/#VE sequence with the direct TDCALL.

RDMSR and WRMSR specification details can be found in
Guest-Host-Communication Interface (GHCI) for Intel Trust Domain
Extensions (Intel TDX) specification, sec titled "TDG.VP.
VMCALL<Instruction.RDMSR>" and "TDG.VP.VMCALL<Instruction.WRMSR>".

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/coco/tdx.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)

diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index 8adc759961f3..c0549dce2588 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -116,6 +116,44 @@ void __cpuidle tdx_safe_halt(void)
WARN_ONCE(1, "HLT instruction emulation failed\n");
}

+static bool read_msr(struct pt_regs *regs)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = EXIT_REASON_MSR_READ,
+ .r12 = regs->cx,
+ };
+
+ /*
+ * Emulate the MSR read via hypercall. More info about ABI
+ * can be found in TDX Guest-Host-Communication Interface
+ * (GHCI), section titled "TDG.VP.VMCALL<Instruction.RDMSR>".
+ */
+ if (__tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT))
+ return false;
+
+ regs->ax = lower_32_bits(args.r11);
+ regs->dx = upper_32_bits(args.r11);
+ return true;
+}
+
+static bool write_msr(struct pt_regs *regs)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = EXIT_REASON_MSR_WRITE,
+ .r12 = regs->cx,
+ .r13 = (u64)regs->dx << 32 | regs->ax,
+ };
+
+ /*
+ * Emulate the MSR write via hypercall. More info about ABI
+ * can be found in TDX Guest-Host-Communication Interface
+ * (GHCI) section titled "TDG.VP.VMCALL<Instruction.WRMSR>".
+ */
+ return !__tdx_hypercall(&args, 0);
+}
+
void tdx_get_ve_info(struct ve_info *ve)
{
struct tdx_module_output out;
@@ -158,6 +196,10 @@ static bool virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
switch (ve->exit_reason) {
case EXIT_REASON_HLT:
return handle_halt();
+ case EXIT_REASON_MSR_READ:
+ return read_msr(regs);
+ case EXIT_REASON_MSR_WRITE:
+ return write_msr(regs);
default:
pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
return false;
--
2.34.1

2022-02-19 00:59:57

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 03/32] x86/tdx: Detect running as a TDX guest in early boot

From: Kuppuswamy Sathyanarayanan <[email protected]>

cc_platform_has() API is used in the kernel to enable confidential
computing features. Since TDX guest is a confidential computing
platform, it also needs to use this API.

In preparation of extending cc_platform_has() API to support TDX guest,
use CPUID instruction to detect support for TDX guests in the early
boot code (via tdx_early_init()). Since copy_bootdata() is the first
user of cc_platform_has() API, detect the TDX guest status before it.

Since cc_plaform_has() API will be used frequently across the boot
code, instead of repeatedly detecting the TDX guest status using the
CPUID instruction, detect once and cache the result.

Define a synthetic feature flag (X86_FEATURE_TDX_GUEST) and set this
bit in a valid TDX guest platform.

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/Kbuild | 1 +
arch/x86/Kconfig | 12 ++++++++++++
arch/x86/coco/Makefile | 2 ++
arch/x86/coco/tdx.c | 22 ++++++++++++++++++++++
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/include/asm/disabled-features.h | 8 +++++++-
arch/x86/include/asm/tdx.h | 21 +++++++++++++++++++++
arch/x86/kernel/head64.c | 4 ++++
8 files changed, 70 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/coco/Makefile
create mode 100644 arch/x86/coco/tdx.c
create mode 100644 arch/x86/include/asm/tdx.h

diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
index f384cb1a4f7a..b4861abb4386 100644
--- a/arch/x86/Kbuild
+++ b/arch/x86/Kbuild
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
+obj-y += coco/
obj-y += entry/

obj-$(CONFIG_PERF_EVENTS) += events/
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 391c4cac8958..ea4190c53db6 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -880,6 +880,18 @@ config ACRN_GUEST
IOT with small footprint and real-time features. More details can be
found in https://projectacrn.org/.

+config INTEL_TDX_GUEST
+ bool "Intel TDX (Trust Domain Extensions) - Guest Support"
+ depends on X86_64 && CPU_SUP_INTEL
+ depends on X86_X2APIC
+ help
+ Support running as a guest under Intel TDX. Without this support,
+ the guest kernel can not boot or run under TDX.
+ TDX includes memory encryption and integrity capabilities
+ which protect the confidentiality and integrity of guest
+ memory contents and CPU state. TDX guests are protected from
+ potential attacks from the VMM.
+
endif #HYPERVISOR_GUEST

source "arch/x86/Kconfig.cpu"
diff --git a/arch/x86/coco/Makefile b/arch/x86/coco/Makefile
new file mode 100644
index 000000000000..b285d485a72e
--- /dev/null
+++ b/arch/x86/coco/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_INTEL_TDX_GUEST) += tdx.o
diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
new file mode 100644
index 000000000000..77c26c6b932d
--- /dev/null
+++ b/arch/x86/coco/tdx.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2021-2022 Intel Corporation */
+
+#undef pr_fmt
+#define pr_fmt(fmt) "tdx: " fmt
+
+#include <linux/cpufeature.h>
+#include <asm/tdx.h>
+
+void __init tdx_early_init(void)
+{
+ u32 eax, sig[3];
+
+ cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2], &sig[1]);
+
+ if (memcmp(TDX_IDENT, sig, 12))
+ return;
+
+ setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
+
+ pr_info("Guest detected\n");
+}
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 5cd22090e53d..cacc8dde854b 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -238,6 +238,7 @@
#define X86_FEATURE_VMW_VMMCALL ( 8*32+19) /* "" VMware prefers VMMCALL hypercall instruction */
#define X86_FEATURE_PVUNLOCK ( 8*32+20) /* "" PV unlock function */
#define X86_FEATURE_VCPUPREEMPT ( 8*32+21) /* "" PV vcpu_is_preempted function */
+#define X86_FEATURE_TDX_GUEST ( 8*32+22) /* Intel Trust Domain Extensions Guest */

/* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */
#define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/
diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
index 1231d63f836d..b37de8268c9a 100644
--- a/arch/x86/include/asm/disabled-features.h
+++ b/arch/x86/include/asm/disabled-features.h
@@ -68,6 +68,12 @@
# define DISABLE_SGX (1 << (X86_FEATURE_SGX & 31))
#endif

+#ifdef CONFIG_INTEL_TDX_GUEST
+# define DISABLE_TDX_GUEST 0
+#else
+# define DISABLE_TDX_GUEST (1 << (X86_FEATURE_TDX_GUEST & 31))
+#endif
+
/*
* Make sure to add features to the correct mask
*/
@@ -79,7 +85,7 @@
#define DISABLED_MASK5 0
#define DISABLED_MASK6 0
#define DISABLED_MASK7 (DISABLE_PTI)
-#define DISABLED_MASK8 0
+#define DISABLED_MASK8 (DISABLE_TDX_GUEST)
#define DISABLED_MASK9 (DISABLE_SMAP|DISABLE_SGX)
#define DISABLED_MASK10 0
#define DISABLED_MASK11 0
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
new file mode 100644
index 000000000000..ba8042ce61c2
--- /dev/null
+++ b/arch/x86/include/asm/tdx.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2021-2022 Intel Corporation */
+#ifndef _ASM_X86_TDX_H
+#define _ASM_X86_TDX_H
+
+#include <linux/init.h>
+
+#define TDX_CPUID_LEAF_ID 0x21
+#define TDX_IDENT "IntelTDX "
+
+#ifdef CONFIG_INTEL_TDX_GUEST
+
+void __init tdx_early_init(void);
+
+#else
+
+static inline void tdx_early_init(void) { };
+
+#endif /* CONFIG_INTEL_TDX_GUEST */
+
+#endif /* _ASM_X86_TDX_H */
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 4f5ecbbaae77..6dff50c3edd6 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -40,6 +40,7 @@
#include <asm/extable.h>
#include <asm/trapnr.h>
#include <asm/sev.h>
+#include <asm/tdx.h>

/*
* Manage page tables very early on.
@@ -514,6 +515,9 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)

idt_setup_early_handler();

+ /* Needed before cc_platform_has() can be used for TDX */
+ tdx_early_init();
+
copy_bootdata(__va(real_mode_data));

/*
--
2.34.1

2022-02-19 02:20:46

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On 2/18/22 08:16, Kirill A. Shutemov wrote:
> +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> +u64 cc_get_mask(bool enc);
> +u64 cc_mkenc(u64 val);
> +u64 cc_mkdec(u64 val);
> +#else
> +#define cc_get_mask(enc) 0
> +#define cc_mkenc(val) (val)
> +#define cc_mkdec(val) (val)
> +#endif

Is there a reason the stubs as #defines? Static inlines are preferred
for consistent type safety among other things.

It would also be nice to talk about the u64 type in the changelog. If I
remember right, there was a reason you didn't want to have a pgprot_t
here.

...
> @@ -74,12 +72,52 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)
>
> bool cc_platform_has(enum cc_attr attr)
> {
> - if (sme_me_mask)
> + switch (cc_vendor) {
> + case CC_VENDOR_AMD:
> return amd_cc_platform_has(attr);
> -
> - if (hv_is_isolation_supported())
> + case CC_VENDOR_INTEL:
> + return intel_cc_platform_has(attr);
> + case CC_VENDOR_HYPERV:
> return hyperv_cc_platform_has(attr);
> -
> - return false;
> + default:
> + return false;
> + }
> }
> EXPORT_SYMBOL_GPL(cc_platform_has);

This patch is bordering on doing too many different things. Adding the
CC_VENDOR_*'s in a separate patch probably would have been nice, for
instance.

This wasn't really broached at all in the changelog.

> +u64 cc_get_mask(bool enc)
> +{
> + switch (cc_vendor) {
> + case CC_VENDOR_AMD:
> + return enc ? cc_mask : 0;
> + default:
> + return 0;
> + }
> +}

I had to ask myself why you need all three of these functions. It's
obvious _after_ reading the whole patch, but it left me wanting more in
the changelog about it.

> +u64 cc_mkenc(u64 val)
> +{
> + switch (cc_vendor) {
> + case CC_VENDOR_AMD:
> + return val | cc_mask;
> + default:
> + return val;
> + }
> +}
> +
> +u64 cc_mkdec(u64 val)
> +{
> + switch (cc_vendor) {
> + case CC_VENDOR_AMD:
> + return val & ~cc_mask;
> + default:
> + return val;
> + }
> +}
> +EXPORT_SYMBOL_GPL(cc_mkdec);
> +
> +__init void cc_init(enum cc_vendor vendor, u64 mask)
> +{
> + cc_vendor = vendor;
> + cc_mask = mask;
> +}
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 5a99f993e639..9af6be143998 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -33,6 +33,7 @@
> #include <asm/nmi.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/numa.h>
> +#include <asm/coco.h>
>
> /* Is Linux running as the root partition? */
> bool hv_root_partition;
> @@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
> */
> swiotlb_force = SWIOTLB_FORCE;
> #endif
> + if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> + cc_init(CC_VENDOR_HYPERV, 0);
> }
>
> if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) {
> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index 3f0abb403340..fa758247ab57 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -44,6 +44,7 @@
> #include <asm/setup.h>
> #include <asm/sections.h>
> #include <asm/cmdline.h>
> +#include <asm/coco.h>
>
> #include "mm_internal.h"
>
> @@ -565,8 +566,7 @@ void __init sme_enable(struct boot_params *bp)
> } else {
> /* SEV state cannot be controlled by a command line option */
> sme_me_mask = me_mask;
> - physical_mask &= ~sme_me_mask;
> - return;
> + goto out;
> }
>
> /*
> @@ -600,6 +600,8 @@ void __init sme_enable(struct boot_params *bp)
> sme_me_mask = 0;
> else
> sme_me_mask = active_by_default ? me_mask : 0;
> -
> +out:
> physical_mask &= ~sme_me_mask;
> + if (sme_me_mask)
> + cc_init(CC_VENDOR_AMD, sme_me_mask);
> }

I don't think you need to mop it up here, but where does this leave
sme_me_mask?

> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index b4072115c8ef..e79366b8a9da 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -1999,8 +1999,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
> memset(&cpa, 0, sizeof(cpa));
> cpa.vaddr = &addr;
> cpa.numpages = numpages;
> - cpa.mask_set = enc ? __pgprot(_PAGE_ENC) : __pgprot(0);
> - cpa.mask_clr = enc ? __pgprot(0) : __pgprot(_PAGE_ENC);
> + cpa.mask_set = __pgprot(cc_get_mask(enc));
> + cpa.mask_clr = __pgprot(cc_get_mask(!enc));
> cpa.pgd = init_mm.pgd;
>
> /* Must avoid aliasing mappings in the highmem code */

This actually ended up looking pretty nice. It was a real mess along
the way, but this makes me think this is a _good_ solution.

2022-02-19 02:57:31

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 29/32] x86/kvm: Use bounce buffers for TD guest

Intel TDX doesn't allow VMM to directly access guest private memory.
Any memory that is required for communication with the VMM must be
shared explicitly. The same rule applies for any DMA to and from the
TDX guest. All DMA pages have to be marked as shared pages. A generic way
to achieve this without any changes to device drivers is to use the
SWIOTLB framework.

Force SWIOTLB on TD guest and make SWIOTLB buffer shared by generalizing
mem_encrypt_init() to cover TDX.

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/Kconfig | 2 +-
arch/x86/coco/tdx.c | 3 +++
arch/x86/kernel/cc_platform.c | 1 +
arch/x86/mm/mem_encrypt.c | 9 ++++++++-
4 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 3701e13e319c..fb2706f7f04a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -885,7 +885,7 @@ config INTEL_TDX_GUEST
depends on X86_64 && CPU_SUP_INTEL
depends on X86_X2APIC
select ARCH_HAS_CC_PLATFORM
- select DYNAMIC_PHYSICAL_MASK
+ select X86_MEM_ENCRYPT
select X86_MCE
help
Support running as a guest under Intel TDX. Without this support,
diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index 5a833569acb8..8a7826fe49e3 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -5,6 +5,7 @@
#define pr_fmt(fmt) "tdx: " fmt

#include <linux/cpufeature.h>
+#include <linux/swiotlb.h>
#include <asm/coco.h>
#include <asm/tdx.h>
#include <asm/vmx.h>
@@ -594,5 +595,7 @@ void __init tdx_early_init(void)

x86_platform.cc = &tdx_cc_runtime;

+ swiotlb_force = SWIOTLB_FORCE;
+
pr_info("Guest detected\n");
}
diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index fac4d588d3b3..12a34e80ef50 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -22,6 +22,7 @@ static bool intel_cc_platform_has(enum cc_attr attr)
case CC_ATTR_GUEST_UNROLL_STRING_IO:
case CC_ATTR_HOTPLUG_DISABLED:
case CC_ATTR_GUEST_MEM_ENCRYPT:
+ case CC_ATTR_MEM_ENCRYPT:
return true;
default:
return false;
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 50d209939c66..10ee40b5204b 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -42,7 +42,14 @@ bool force_dma_unencrypted(struct device *dev)

static void print_mem_encrypt_feature_info(void)
{
- pr_info("AMD Memory Encryption Features active:");
+ pr_info("Memory Encryption Features active:");
+
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
+ pr_cont(" Intel TDX\n");
+ return;
+ }
+
+ pr_cont("AMD ");

/* Secure Memory Encryption */
if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
--
2.34.1

2022-02-19 03:03:54

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 08/32] x86/traps: Add #VE support for TDX guest

Virtualization Exceptions (#VE) are delivered to TDX guests due to
specific guest actions which may happen in either user space or the
kernel:

* Specific instructions (WBINVD, for example)
* Specific MSR accesses
* Specific CPUID leaf accesses
* Access to unmapped pages (EPT violation)

In the settings that Linux will run in, virtualization exceptions are
never generated on accesses to normal, TD-private memory that has been
accepted.

Syscall entry code has a critical window where the kernel stack is not
yet set up. Any exception in this window leads to hard to debug issues
and can be exploited for privilege escalation. Exceptions in the NMI
entry code also cause issues. Returning from the exception handler with
IRET will re-enable NMIs and nested NMI will corrupt the NMI stack.

For these reasons, the kernel avoids #VEs during the syscall gap and
the NMI entry code. Entry code paths do not access TD-shared memory,
MMIO regions, use #VE triggering MSRs, instructions, or CPUID leaves
that might generate #VE. VMM can remove memory from TD at any point,
but access to unaccepted (or missing) private memory leads to VM
termination, not to #VE.

Similarly to page faults and breakpoints, #VEs are allowed in NMI
handlers once the kernel is ready to deal with nested NMIs.

During #VE delivery, all interrupts, including NMIs, are blocked until
TDGETVEINFO is called. It prevents #VE nesting until the kernel reads
the VE info.

If a guest kernel action which would normally cause a #VE occurs in
the interrupt-disabled region before TDGETVEINFO, a #DF (fault
exception) is delivered to the guest which will result in an oops.

Add basic infrastructure to handle any #VE which occurs in the kernel
or userspace. Later patches will add handling for specific #VE
scenarios.

For now, convert unhandled #VE's (everything, until later in this
series) so that they appear just like a #GP by calling the
ve_raise_fault() directly. The ve_raise_fault() function is similar
to #GP handler and is responsible for sending SIGSEGV to userspace
and CPU die and notifying debuggers and other die chain users.

Co-developed-by: Sean Christopherson <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
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/coco/tdx.c | 60 ++++++++++++++
arch/x86/include/asm/idtentry.h | 4 +
arch/x86/include/asm/tdx.h | 21 +++++
arch/x86/kernel/idt.c | 3 +
arch/x86/kernel/traps.c | 138 ++++++++++++++++++++++++++------
5 files changed, 203 insertions(+), 23 deletions(-)

diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index ea5f02a5d738..de7a02e634c2 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -10,6 +10,7 @@

/* TDX module Call Leaf IDs */
#define TDX_GET_INFO 1
+#define TDX_GET_VEINFO 3

static struct {
unsigned int gpa_width;
@@ -58,6 +59,65 @@ static void get_info(void)
td_info.attributes = out.rdx;
}

+void tdx_get_ve_info(struct ve_info *ve)
+{
+ struct tdx_module_output out;
+
+ /*
+ * Retrieve the #VE info from the TDX module, which also clears the "#VE
+ * valid" flag. This must be done before anything else as any #VE that
+ * occurs while the valid flag is set, i.e. before the previous #VE info
+ * was consumed, is morphed to a #DF by the TDX module. Note, the TDX
+ * module also treats virtual NMIs as inhibited if the #VE valid flag is
+ * set, e.g. so that NMI=>#VE will not result in a #DF.
+ */
+ tdx_module_call(TDX_GET_VEINFO, 0, 0, 0, 0, &out);
+
+ ve->exit_reason = out.rcx;
+ ve->exit_qual = out.rdx;
+ ve->gla = out.r8;
+ ve->gpa = out.r9;
+ ve->instr_len = lower_32_bits(out.r10);
+ ve->instr_info = upper_32_bits(out.r10);
+}
+
+/*
+ * Handle the user initiated #VE.
+ *
+ * For example, executing the CPUID instruction from user space
+ * is a valid case and hence the resulting #VE has to be handled.
+ *
+ * For dis-allowed or invalid #VE just return failure.
+ */
+static bool virt_exception_user(struct pt_regs *regs, struct ve_info *ve)
+{
+ pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
+ return false;
+}
+
+/* Handle the kernel #VE */
+static bool virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
+{
+ pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
+ return false;
+}
+
+bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve)
+{
+ bool ret;
+
+ if (user_mode(regs))
+ ret = virt_exception_user(regs, ve);
+ else
+ ret = virt_exception_kernel(regs, ve);
+
+ /* After successful #VE handling, move the IP */
+ if (ret)
+ regs->ip += ve->instr_len;
+
+ return ret;
+}
+
void __init tdx_early_init(void)
{
u32 eax, sig[3];
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index 1345088e9902..8ccc81d653b3 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -625,6 +625,10 @@ DECLARE_IDTENTRY_XENCB(X86_TRAP_OTHER, exc_xen_hypervisor_callback);
DECLARE_IDTENTRY_RAW(X86_TRAP_OTHER, exc_xen_unknown_trap);
#endif

+#ifdef CONFIG_INTEL_TDX_GUEST
+DECLARE_IDTENTRY(X86_TRAP_VE, exc_virtualization_exception);
+#endif
+
/* Device interrupts common/spurious */
DECLARE_IDTENTRY_IRQ(X86_TRAP_OTHER, common_interrupt);
#ifdef CONFIG_X86_LOCAL_APIC
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 557227e40da9..34cf998ad534 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -5,6 +5,7 @@

#include <linux/bits.h>
#include <linux/init.h>
+#include <asm/ptrace.h>

#define TDX_CPUID_LEAF_ID 0x21
#define TDX_IDENT "IntelTDX "
@@ -47,6 +48,22 @@ struct tdx_hypercall_args {
u64 r15;
};

+/*
+ * Used by the #VE exception handler to gather the #VE exception
+ * info from the TDX module. This is a software only structure
+ * and not part of the TDX module/VMM ABI.
+ */
+struct ve_info {
+ u64 exit_reason;
+ u64 exit_qual;
+ /* Guest Linear (virtual) Address */
+ u64 gla;
+ /* Guest Physical (virtual) Address */
+ u64 gpa;
+ u32 instr_len;
+ u32 instr_info;
+};
+
#ifdef CONFIG_INTEL_TDX_GUEST

void __init tdx_early_init(void);
@@ -58,6 +75,10 @@ u64 __tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
/* Used to request services from the VMM */
u64 __tdx_hypercall(struct tdx_hypercall_args *args, unsigned long flags);

+void tdx_get_ve_info(struct ve_info *ve);
+
+bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve);
+
#else

static inline void tdx_early_init(void) { };
diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
index df0fa695bb09..1da074123c16 100644
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -68,6 +68,9 @@ static const __initconst struct idt_data early_idts[] = {
*/
INTG(X86_TRAP_PF, asm_exc_page_fault),
#endif
+#ifdef CONFIG_INTEL_TDX_GUEST
+ INTG(X86_TRAP_VE, asm_exc_virtualization_exception),
+#endif
};

/*
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 7ef00dee35be..b2510af38158 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -62,6 +62,7 @@
#include <asm/insn.h>
#include <asm/insn-eval.h>
#include <asm/vdso.h>
+#include <asm/tdx.h>

#ifdef CONFIG_X86_64
#include <asm/x86_init.h>
@@ -611,13 +612,43 @@ static bool try_fixup_enqcmd_gp(void)
#endif
}

+static bool gp_try_fixup_and_notify(struct pt_regs *regs, int trapnr,
+ unsigned long error_code, const char *str)
+{
+ int ret;
+
+ if (fixup_exception(regs, trapnr, error_code, 0))
+ return true;
+
+ current->thread.error_code = error_code;
+ current->thread.trap_nr = trapnr;
+
+ /*
+ * To be potentially processing a kprobe fault and to trust the result
+ * from kprobe_running(), we have to be non-preemptible.
+ */
+ if (!preemptible() && kprobe_running() &&
+ kprobe_fault_handler(regs, trapnr))
+ return true;
+
+ ret = notify_die(DIE_GPF, str, regs, error_code, trapnr, SIGSEGV);
+ return ret == NOTIFY_STOP;
+}
+
+static void gp_user_force_sig_segv(struct pt_regs *regs, int trapnr,
+ unsigned long error_code, const char *str)
+{
+ current->thread.error_code = error_code;
+ current->thread.trap_nr = trapnr;
+ show_signal(current, SIGSEGV, "", str, regs, error_code);
+ force_sig(SIGSEGV);
+}
+
DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
{
char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;
enum kernel_gp_hint hint = GP_NO_HINT;
- struct task_struct *tsk;
unsigned long gp_addr;
- int ret;

if (user_mode(regs) && try_fixup_enqcmd_gp())
return;
@@ -636,40 +667,21 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
return;
}

- tsk = current;
-
if (user_mode(regs)) {
if (fixup_iopl_exception(regs))
goto exit;

- tsk->thread.error_code = error_code;
- tsk->thread.trap_nr = X86_TRAP_GP;
-
if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
goto exit;

- show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
- force_sig(SIGSEGV);
+ gp_user_force_sig_segv(regs, X86_TRAP_GP, error_code, desc);
goto exit;
}

if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
goto exit;

- tsk->thread.error_code = error_code;
- tsk->thread.trap_nr = X86_TRAP_GP;
-
- /*
- * To be potentially processing a kprobe fault and to trust the result
- * from kprobe_running(), we have to be non-preemptible.
- */
- if (!preemptible() &&
- kprobe_running() &&
- kprobe_fault_handler(regs, X86_TRAP_GP))
- goto exit;
-
- ret = notify_die(DIE_GPF, desc, regs, error_code, X86_TRAP_GP, SIGSEGV);
- if (ret == NOTIFY_STOP)
+ if (gp_try_fixup_and_notify(regs, X86_TRAP_GP, error_code, desc))
goto exit;

if (error_code)
@@ -1267,6 +1279,86 @@ DEFINE_IDTENTRY(exc_device_not_available)
}
}

+#ifdef CONFIG_INTEL_TDX_GUEST
+
+#define VE_FAULT_STR "VE fault"
+
+static void ve_raise_fault(struct pt_regs *regs, long error_code)
+{
+ if (user_mode(regs)) {
+ gp_user_force_sig_segv(regs, X86_TRAP_VE, error_code, VE_FAULT_STR);
+ return;
+ }
+
+ if (gp_try_fixup_and_notify(regs, X86_TRAP_VE, error_code, VE_FAULT_STR))
+ return;
+
+ die_addr(VE_FAULT_STR, regs, error_code, 0);
+}
+
+/*
+ * Virtualization Exceptions (#VE) are delivered to TDX guests due to
+ * specific guest actions which may happen in either user space or the
+ * kernel:
+ *
+ * * Specific instructions (WBINVD, for example)
+ * * Specific MSR accesses
+ * * Specific CPUID leaf accesses
+ * * Access to unmapped pages (EPT violation)
+ *
+ * In the settings that Linux will run in, virtualization exceptions are
+ * never generated on accesses to normal, TD-private memory that has been
+ * accepted.
+ *
+ * Syscall entry code has a critical window where the kernel stack is not
+ * yet set up. Any exception in this window leads to hard to debug issues
+ * and can be exploited for privilege escalation. Exceptions in the NMI
+ * entry code also cause issues. Returning from the exception handler with
+ * IRET will re-enable NMIs and nested NMI will corrupt the NMI stack.
+ *
+ * For these reasons, the kernel avoids #VEs during the syscall gap and
+ * the NMI entry code. Entry code paths do not access TD-shared memory,
+ * MMIO regions, use #VE triggering MSRs, instructions, or CPUID leaves
+ * that might generate #VE. VMM can remove memory from TD at any point,
+ * but access to unaccepted (or missing) private memory leads to VM
+ * termination, not to #VE.
+ *
+ * Similarly to page faults and breakpoints, #VEs are allowed in NMI
+ * handlers once the kernel is ready to deal with nested NMIs.
+ *
+ * During #VE delivery, all interrupts, including NMIs, are blocked until
+ * TDGETVEINFO is called. It prevents #VE nesting until the kernel reads
+ * the VE info.
+ *
+ * If a guest kernel action which would normally cause a #VE occurs in
+ * the interrupt-disabled region before TDGETVEINFO, a #DF (fault
+ * exception) is delivered to the guest which will result in an oops.
+ */
+DEFINE_IDTENTRY(exc_virtualization_exception)
+{
+ struct ve_info ve;
+
+ /*
+ * NMIs/Machine-checks/Interrupts will be in a disabled state
+ * till TDGETVEINFO TDCALL is executed. This ensures that VE
+ * info cannot be overwritten by a nested #VE.
+ */
+ tdx_get_ve_info(&ve);
+
+ cond_local_irq_enable(regs);
+
+ /*
+ * If tdx_handle_virt_exception() could not process
+ * it successfully, treat it as #GP(0) and handle it.
+ */
+ if (!tdx_handle_virt_exception(regs, &ve))
+ ve_raise_fault(regs, 0);
+
+ cond_local_irq_disable(regs);
+}
+
+#endif
+
#ifdef CONFIG_X86_32
DEFINE_IDTENTRY_SW(iret_error)
{
--
2.34.1

2022-02-19 03:15:33

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 22/32] x86/acpi, x86/boot: Add multiprocessor wake-up support

From: Kuppuswamy Sathyanarayanan <[email protected]>

TDX cannot use INIT/SIPI protocol to bring up secondary CPUs because it
requires assistance from untrusted VMM.

For platforms that do not support SIPI/INIT, ACPI defines a wakeup
model (using mailbox) via MADT multiprocessor wakeup structure. More
details about it can be found in ACPI specification v6.4, the section
titled "Multiprocessor Wakeup Structure". If a platform firmware
produces the multiprocessor wakeup structure, then OS may use this
new mailbox-based mechanism to wake up the APs.

Add ACPI MADT wake structure parsing support for x86 platform and if
MADT wake table is present, update apic->wakeup_secondary_cpu_64 with
new API which uses MADT wake mailbox to wake-up CPU.

Co-developed-by: Sean Christopherson <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/apic.h | 5 ++
arch/x86/kernel/acpi/boot.c | 118 ++++++++++++++++++++++++++++++++++++
arch/x86/kernel/apic/apic.c | 10 +++
3 files changed, 133 insertions(+)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 35006e151774..bd8ae0a7010a 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -490,6 +490,11 @@ static inline unsigned int read_apic_id(void)
return apic->get_apic_id(reg);
}

+#ifdef CONFIG_X86_64
+typedef int (*wakeup_cpu_handler)(int apicid, unsigned long start_eip);
+extern void acpi_wake_cpu_handler_update(wakeup_cpu_handler handler);
+#endif
+
extern int default_apic_id_valid(u32 apicid);
extern int default_acpi_madt_oem_check(char *, char *);
extern void default_setup_apic_routing(void);
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 5b6d1a95776f..99518eac2bbc 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -65,6 +65,15 @@ static u64 acpi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
static bool acpi_support_online_capable;
#endif

+#ifdef CONFIG_X86_64
+/* Physical address of the Multiprocessor Wakeup Structure mailbox */
+static u64 acpi_mp_wake_mailbox_paddr;
+/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
+static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox;
+/* Lock to protect mailbox (acpi_mp_wake_mailbox) from parallel access */
+static DEFINE_SPINLOCK(mailbox_lock);
+#endif
+
#ifdef CONFIG_X86_IO_APIC
/*
* Locks related to IOAPIC hotplug
@@ -336,6 +345,84 @@ acpi_parse_lapic_nmi(union acpi_subtable_headers * header, const unsigned long e
return 0;
}

+#ifdef CONFIG_X86_64
+/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
+static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
+{
+ static physid_mask_t apic_id_wakemap = PHYSID_MASK_NONE;
+ u8 timeout;
+
+ /* Remap mailbox memory only for the first call to acpi_wakeup_cpu() */
+ if (physids_empty(apic_id_wakemap)) {
+ acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
+ sizeof(*acpi_mp_wake_mailbox),
+ MEMREMAP_WB);
+ }
+
+ /*
+ * According to the ACPI specification r6.4, section titled
+ * "Multiprocessor Wakeup Structure" the mailbox-based wakeup
+ * mechanism cannot be used more than once for the same CPU.
+ * Skip wakeups if they are attempted more than once.
+ */
+ if (physid_isset(apicid, apic_id_wakemap)) {
+ pr_err("CPU already awake (APIC ID %x), skipping wakeup\n",
+ apicid);
+ return -EINVAL;
+ }
+
+ spin_lock(&mailbox_lock);
+
+ /*
+ * Mailbox memory is shared between firmware and OS. Firmware will
+ * listen on mailbox command address, and once it receives the wakeup
+ * command, CPU associated with the given apicid will be booted.
+ *
+ * The value of apic_id and wakeup_vector has to be set before updating
+ * the wakeup command. To let compiler preserve order of writes, use
+ * smp_store_release.
+ */
+ smp_store_release(&acpi_mp_wake_mailbox->apic_id, apicid);
+ smp_store_release(&acpi_mp_wake_mailbox->wakeup_vector, start_ip);
+ smp_store_release(&acpi_mp_wake_mailbox->command,
+ ACPI_MP_WAKE_COMMAND_WAKEUP);
+
+ /*
+ * After writing the wakeup command, wait for maximum timeout of 0xFF
+ * for firmware to reset the command address back zero to indicate
+ * the successful reception of command.
+ * NOTE: 0xFF as timeout value is decided based on our experiments.
+ *
+ * XXX: Change the timeout once ACPI specification comes up with
+ * standard maximum timeout value.
+ */
+ timeout = 0xFF;
+ while (READ_ONCE(acpi_mp_wake_mailbox->command) && --timeout)
+ cpu_relax();
+
+ /* If timed out (timeout == 0), return error */
+ if (!timeout) {
+ /*
+ * XXX: Is there a recovery path after timeout is hit?
+ * Spec is unclear. Reset command to 0 if timeout is hit.
+ */
+ acpi_mp_wake_mailbox->command = 0;
+ spin_unlock(&mailbox_lock);
+ return -EIO;
+ }
+
+ /*
+ * If the CPU wakeup process is successful, store the
+ * status in apic_id_wakemap to prevent re-wakeup
+ * requests.
+ */
+ physid_set(apicid, apic_id_wakemap);
+
+ spin_unlock(&mailbox_lock);
+
+ return 0;
+}
+#endif
#endif /*CONFIG_X86_LOCAL_APIC */

#ifdef CONFIG_X86_IO_APIC
@@ -1083,6 +1170,29 @@ static int __init acpi_parse_madt_lapic_entries(void)
}
return 0;
}
+
+#ifdef CONFIG_X86_64
+static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
+ const unsigned long end)
+{
+ struct acpi_madt_multiproc_wakeup *mp_wake;
+
+ if (!IS_ENABLED(CONFIG_SMP))
+ return -ENODEV;
+
+ mp_wake = (struct acpi_madt_multiproc_wakeup *)header;
+ if (BAD_MADT_ENTRY(mp_wake, end))
+ return -EINVAL;
+
+ acpi_table_print_madt_entry(&header->common);
+
+ acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
+
+ acpi_wake_cpu_handler_update(acpi_wakeup_cpu);
+
+ return 0;
+}
+#endif /* CONFIG_X86_64 */
#endif /* CONFIG_X86_LOCAL_APIC */

#ifdef CONFIG_X86_IO_APIC
@@ -1278,6 +1388,14 @@ static void __init acpi_process_madt(void)

smp_found_config = 1;
}
+
+#ifdef CONFIG_X86_64
+ /*
+ * Parse MADT MP Wake entry.
+ */
+ acpi_table_parse_madt(ACPI_MADT_TYPE_MULTIPROC_WAKEUP,
+ acpi_parse_mp_wake, 1);
+#endif
}
if (error == -EINVAL) {
/*
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b70344bf6600..3c8f2c797a98 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2551,6 +2551,16 @@ u32 x86_msi_msg_get_destid(struct msi_msg *msg, bool extid)
}
EXPORT_SYMBOL_GPL(x86_msi_msg_get_destid);

+#ifdef CONFIG_X86_64
+void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
+{
+ struct apic **drv;
+
+ for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
+ (*drv)->wakeup_secondary_cpu_64 = handler;
+}
+#endif
+
/*
* Override the generic EOI implementation with an optimized version.
* Only called during early boot when only one CPU is active and with
--
2.34.1

2022-02-19 07:32:29

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 21/32] x86/boot: Add a trampoline for booting APs via firmware handoff

From: Sean Christopherson <[email protected]>

Historically, x86 platforms have booted secondary processors (APs)
using INIT followed by the start up IPI (SIPI) messages. In regular
VMs, this boot sequence is supported by the VMM emulation. But such a
wakeup model is fatal for secure VMs like TDX in which VMM is an
untrusted entity. To address this issue, a new wakeup model was added
in ACPI v6.4, in which firmware (like TDX virtual BIOS) will help boot
the APs. More details about this wakeup model can be found in ACPI
specification v6.4, the section titled "Multiprocessor Wakeup Structure".

Since the existing trampoline code requires processors to boot in real
mode with 16-bit addressing, it will not work for this wakeup model
(because it boots the AP in 64-bit mode). To handle it, extend the
trampoline code to support 64-bit mode firmware handoff. Also, extend
IDT and GDT pointers to support 64-bit mode hand off.

There is no TDX-specific detection for this new boot method. The kernel
will rely on it as the sole boot method whenever the new ACPI structure
is present.

The ACPI table parser for the MADT multiprocessor wake up structure and
the wakeup method that uses this structure will be added by the following
patch in this series.

Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Dan Williams <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/apic.h | 2 ++
arch/x86/include/asm/realmode.h | 1 +
arch/x86/kernel/smpboot.c | 12 ++++++--
arch/x86/realmode/rm/header.S | 1 +
arch/x86/realmode/rm/trampoline_64.S | 38 ++++++++++++++++++++++++
arch/x86/realmode/rm/trampoline_common.S | 12 +++++++-
6 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 48067af94678..35006e151774 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -328,6 +328,8 @@ struct apic {

/* wakeup_secondary_cpu */
int (*wakeup_secondary_cpu)(int apicid, unsigned long start_eip);
+ /* wakeup secondary CPU using 64-bit wakeup point */
+ int (*wakeup_secondary_cpu_64)(int apicid, unsigned long start_eip);

void (*inquire_remote_apic)(int apicid);

diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index 331474b150f1..fd6f6e5b755a 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -25,6 +25,7 @@ struct real_mode_header {
u32 sev_es_trampoline_start;
#endif
#ifdef CONFIG_X86_64
+ u32 trampoline_start64;
u32 trampoline_pgd;
#endif
/* ACPI S3 wakeup */
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 617012f4619f..6269dd126dba 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1088,6 +1088,11 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,
unsigned long boot_error = 0;
unsigned long timeout;

+#ifdef CONFIG_X86_64
+ /* If 64-bit wakeup method exists, use the 64-bit mode trampoline IP */
+ if (apic->wakeup_secondary_cpu_64)
+ start_ip = real_mode_header->trampoline_start64;
+#endif
idle->thread.sp = (unsigned long)task_pt_regs(idle);
early_gdt_descr.address = (unsigned long)get_cpu_gdt_rw(cpu);
initial_code = (unsigned long)start_secondary;
@@ -1129,11 +1134,14 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle,

/*
* Wake up a CPU in difference cases:
- * - Use the method in the APIC driver if it's defined
+ * - Use a method from the APIC driver if one defined, with wakeup
+ * straight to 64-bit mode preferred over wakeup to RM.
* Otherwise,
* - Use an INIT boot APIC message for APs or NMI for BSP.
*/
- if (apic->wakeup_secondary_cpu)
+ if (apic->wakeup_secondary_cpu_64)
+ boot_error = apic->wakeup_secondary_cpu_64(apicid, start_ip);
+ else if (apic->wakeup_secondary_cpu)
boot_error = apic->wakeup_secondary_cpu(apicid, start_ip);
else
boot_error = wakeup_cpu_via_init_nmi(cpu, start_ip, apicid,
diff --git a/arch/x86/realmode/rm/header.S b/arch/x86/realmode/rm/header.S
index 8c1db5bf5d78..2eb62be6d256 100644
--- a/arch/x86/realmode/rm/header.S
+++ b/arch/x86/realmode/rm/header.S
@@ -24,6 +24,7 @@ SYM_DATA_START(real_mode_header)
.long pa_sev_es_trampoline_start
#endif
#ifdef CONFIG_X86_64
+ .long pa_trampoline_start64
.long pa_trampoline_pgd;
#endif
/* ACPI S3 wakeup */
diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
index cc8391f86cdb..ae112a91592f 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -161,6 +161,19 @@ SYM_CODE_START(startup_32)
ljmpl $__KERNEL_CS, $pa_startup_64
SYM_CODE_END(startup_32)

+SYM_CODE_START(pa_trampoline_compat)
+ /*
+ * In compatibility mode. Prep ESP and DX for startup_32, then disable
+ * paging and complete the switch to legacy 32-bit mode.
+ */
+ movl $rm_stack_end, %esp
+ movw $__KERNEL_DS, %dx
+
+ movl $X86_CR0_PE, %eax
+ movl %eax, %cr0
+ ljmpl $__KERNEL32_CS, $pa_startup_32
+SYM_CODE_END(pa_trampoline_compat)
+
.section ".text64","ax"
.code64
.balign 4
@@ -169,6 +182,20 @@ SYM_CODE_START(startup_64)
jmpq *tr_start(%rip)
SYM_CODE_END(startup_64)

+SYM_CODE_START(trampoline_start64)
+ /*
+ * APs start here on a direct transfer from 64-bit BIOS with identity
+ * mapped page tables. Load the kernel's GDT in order to gear down to
+ * 32-bit mode (to handle 4-level vs. 5-level paging), and to (re)load
+ * segment registers. Load the zero IDT so any fault triggers a
+ * shutdown instead of jumping back into BIOS.
+ */
+ lidt tr_idt(%rip)
+ lgdt tr_gdt64(%rip)
+
+ ljmpl *tr_compat(%rip)
+SYM_CODE_END(trampoline_start64)
+
.section ".rodata","a"
# Duplicate the global descriptor table
# so the kernel can live anywhere
@@ -182,6 +209,17 @@ SYM_DATA_START(tr_gdt)
.quad 0x00cf93000000ffff # __KERNEL_DS
SYM_DATA_END_LABEL(tr_gdt, SYM_L_LOCAL, tr_gdt_end)

+SYM_DATA_START(tr_gdt64)
+ .short tr_gdt_end - tr_gdt - 1 # gdt limit
+ .long pa_tr_gdt
+ .long 0
+SYM_DATA_END(tr_gdt64)
+
+SYM_DATA_START(tr_compat)
+ .long pa_trampoline_compat
+ .short __KERNEL32_CS
+SYM_DATA_END(tr_compat)
+
.bss
.balign PAGE_SIZE
SYM_DATA(trampoline_pgd, .space PAGE_SIZE)
diff --git a/arch/x86/realmode/rm/trampoline_common.S b/arch/x86/realmode/rm/trampoline_common.S
index 5033e640f957..4331c32c47f8 100644
--- a/arch/x86/realmode/rm/trampoline_common.S
+++ b/arch/x86/realmode/rm/trampoline_common.S
@@ -1,4 +1,14 @@
/* SPDX-License-Identifier: GPL-2.0 */
.section ".rodata","a"
.balign 16
-SYM_DATA_LOCAL(tr_idt, .fill 1, 6, 0)
+
+/*
+ * When a bootloader hands off to the kernel in 32-bit mode an
+ * IDT with a 2-byte limit and 4-byte base is needed. When a boot
+ * loader hands off to a kernel 64-bit mode the base address
+ * extends to 8-bytes. Reserve enough space for either scenario.
+ */
+SYM_DATA_START_LOCAL(tr_idt)
+ .short 0
+ .quad 0
+SYM_DATA_END(tr_idt)
--
2.34.1

2022-02-19 11:00:01

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 01/32] x86/mm: Fix warning on build with X86_MEM_ENCRYPT=y

So far, AMD_MEM_ENCRYPT is the only user of X86_MEM_ENCRYPT. TDX will be
the second. It will make mem_encrypt.c build without AMD_MEM_ENCRYPT,
which triggers a warning:

arch/x86/mm/mem_encrypt.c:69:13: warning: no previous prototype for
function 'mem_encrypt_init' [-Wmissing-prototypes]

Fix it by moving mem_encrypt_init() declaration outside of #ifdef
CONFIG_AMD_MEM_ENCRYPT.

Signed-off-by: Kirill A. Shutemov <[email protected]>
Fixes: 20f07a044a76 ("x86/sev: Move common memory encryption code to mem_encrypt.c")
Acked-by: David Rientjes <[email protected]>
---
arch/x86/include/asm/mem_encrypt.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
index e2c6f433ed10..88ceaf3648b3 100644
--- a/arch/x86/include/asm/mem_encrypt.h
+++ b/arch/x86/include/asm/mem_encrypt.h
@@ -49,9 +49,6 @@ void __init early_set_mem_enc_dec_hypercall(unsigned long vaddr, int npages,

void __init mem_encrypt_free_decrypted_mem(void);

-/* Architecture __weak replacement functions */
-void __init mem_encrypt_init(void);
-
void __init sev_es_init_vc_handling(void);

#define __bss_decrypted __section(".bss..decrypted")
@@ -89,6 +86,9 @@ static inline void mem_encrypt_free_decrypted_mem(void) { }

#endif /* CONFIG_AMD_MEM_ENCRYPT */

+/* Architecture __weak replacement functions */
+void __init mem_encrypt_init(void);
+
/*
* The __sme_pa() and __sme_pa_nodebug() macros are meant for use when
* writing to or comparing values from the cr3 register. Having the
--
2.34.1

2022-02-19 11:00:23

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 23/32] x86/boot: Set CR0.NE early and keep it set during the boot

TDX guest requires CR0.NE to be set. Clearing the bit triggers #GP(0).

If CR0.NE is 0, the MS-DOS compatibility mode for handling floating-point
exceptions is selected. In this mode, the software exception handler for
floating-point exceptions is invoked externally using the processor’s
FERR#, INTR, and IGNNE# pins.

Using FERR# and IGNNE# to handle floating-point exception is deprecated.
CR0.NE=0 also limits newer processors to operate with one logical
processor active.

Kernel uses CR0_STATE constant to initialize CR0. It has NE bit set.
But during early boot kernel has more ad-hoc approach to setting bit
in the register.

Make CR0 initialization consistent, deriving the initial value of CR0
from CR0_STATE.

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 7 ++++---
arch/x86/realmode/rm/trampoline_64.S | 8 ++++----
2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index fd9441f40457..d0c3d33f3542 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -289,7 +289,7 @@ SYM_FUNC_START(startup_32)
pushl %eax

/* Enter paged protected Mode, activating Long Mode */
- movl $(X86_CR0_PG | X86_CR0_PE), %eax /* Enable Paging and Protected mode */
+ movl $CR0_STATE, %eax
movl %eax, %cr0

/* Jump from 32bit compatibility mode into 64bit mode. */
@@ -662,8 +662,9 @@ SYM_CODE_START(trampoline_32bit_src)
pushl $__KERNEL_CS
pushl %eax

- /* Enable paging again */
- movl $(X86_CR0_PG | X86_CR0_PE), %eax
+ /* Enable paging again. */
+ movl %cr0, %eax
+ btsl $X86_CR0_PG_BIT, %eax
movl %eax, %cr0

lret
diff --git a/arch/x86/realmode/rm/trampoline_64.S b/arch/x86/realmode/rm/trampoline_64.S
index ae112a91592f..d380f2d1fd23 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -70,7 +70,7 @@ SYM_CODE_START(trampoline_start)
movw $__KERNEL_DS, %dx # Data segment descriptor

# Enable protected mode
- movl $X86_CR0_PE, %eax # protected mode (PE) bit
+ movl $(CR0_STATE & ~X86_CR0_PG), %eax
movl %eax, %cr0 # into protected mode

# flush prefetch and jump to startup_32
@@ -148,8 +148,8 @@ SYM_CODE_START(startup_32)
movl $MSR_EFER, %ecx
wrmsr

- # Enable paging and in turn activate Long Mode
- movl $(X86_CR0_PG | X86_CR0_WP | X86_CR0_PE), %eax
+ # Enable paging and in turn activate Long Mode.
+ movl $CR0_STATE, %eax
movl %eax, %cr0

/*
@@ -169,7 +169,7 @@ SYM_CODE_START(pa_trampoline_compat)
movl $rm_stack_end, %esp
movw $__KERNEL_DS, %dx

- movl $X86_CR0_PE, %eax
+ movl $(CR0_STATE & ~X86_CR0_PG), %eax
movl %eax, %cr0
ljmpl $__KERNEL32_CS, $pa_startup_32
SYM_CODE_END(pa_trampoline_compat)
--
2.34.1

2022-02-19 14:52:28

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 13/32] x86/tdx: Detect TDX at early kernel decompression time

From: Kuppuswamy Sathyanarayanan <[email protected]>

The early decompression code does port I/O for its console output. But,
handling the decompression-time port I/O demands a different approach
from normal runtime because the IDT required to support #VE based port
I/O emulation is not yet set up. Paravirtualizing I/O calls during
the decompression step is acceptable because the decompression code size is
small enough and hence patching it will not bloat the image size a lot.

To support port I/O in decompression code, TDX must be detected before
the decompression code might do port I/O. Detect whether the kernel runs
in a TDX guest.

Add an early_is_tdx_guest() interface to query the cached TDX guest
status in the decompression code.

The actual port I/O paravirtualization will come later in the series.

Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/boot/compressed/Makefile | 1 +
arch/x86/boot/compressed/misc.c | 8 ++++++++
arch/x86/boot/compressed/misc.h | 2 ++
arch/x86/boot/compressed/tdx.c | 26 ++++++++++++++++++++++++++
arch/x86/boot/compressed/tdx.h | 15 +++++++++++++++
arch/x86/boot/cpuflags.c | 3 +--
arch/x86/boot/cpuflags.h | 1 +
arch/x86/include/asm/shared/tdx.h | 8 ++++++++
arch/x86/include/asm/tdx.h | 4 +---
9 files changed, 63 insertions(+), 5 deletions(-)
create mode 100644 arch/x86/boot/compressed/tdx.c
create mode 100644 arch/x86/boot/compressed/tdx.h
create mode 100644 arch/x86/include/asm/shared/tdx.h

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 6115274fe10f..732f6b21ecbd 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -101,6 +101,7 @@ ifdef CONFIG_X86_64
endif

vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
+vmlinux-objs-$(CONFIG_INTEL_TDX_GUEST) += $(obj)/tdx.o

vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
efi-obj-$(CONFIG_EFI_STUB) = $(objtree)/drivers/firmware/efi/libstub/lib.a
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index a4339cb2d247..2b1169869b96 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -370,6 +370,14 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
lines = boot_params->screen_info.orig_video_lines;
cols = boot_params->screen_info.orig_video_cols;

+ /*
+ * Detect TDX guest environment.
+ *
+ * It has to be done before console_init() in order to use
+ * paravirtualized port I/O operations if needed.
+ */
+ early_tdx_detect();
+
console_init();

/*
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 16ed360b6692..0d8e275a9d96 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -28,6 +28,8 @@
#include <asm/bootparam.h>
#include <asm/desc_defs.h>

+#include "tdx.h"
+
#define BOOT_CTYPE_H
#include <linux/acpi.h>

diff --git a/arch/x86/boot/compressed/tdx.c b/arch/x86/boot/compressed/tdx.c
new file mode 100644
index 000000000000..8a72ce2e880f
--- /dev/null
+++ b/arch/x86/boot/compressed/tdx.c
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include "../cpuflags.h"
+#include "../string.h"
+
+#include <asm/shared/tdx.h>
+
+static bool tdx_guest_detected;
+
+bool early_is_tdx_guest(void)
+{
+ return tdx_guest_detected;
+}
+
+void early_tdx_detect(void)
+{
+ u32 eax, sig[3];
+
+ cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2], &sig[1]);
+
+ if (memcmp(TDX_IDENT, sig, 12))
+ return;
+
+ /* Cache TDX guest feature status */
+ tdx_guest_detected = true;
+}
diff --git a/arch/x86/boot/compressed/tdx.h b/arch/x86/boot/compressed/tdx.h
new file mode 100644
index 000000000000..a7bff6ae002e
--- /dev/null
+++ b/arch/x86/boot/compressed/tdx.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef BOOT_COMPRESSED_TDX_H
+#define BOOT_COMPRESSED_TDX_H
+
+#include <linux/types.h>
+
+#ifdef CONFIG_INTEL_TDX_GUEST
+void early_tdx_detect(void);
+bool early_is_tdx_guest(void);
+#else
+static inline void early_tdx_detect(void) { };
+static inline bool early_is_tdx_guest(void) { return false; }
+#endif
+
+#endif /* BOOT_COMPRESSED_TDX_H */
diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c
index a0b75f73dc63..a83d67ec627d 100644
--- a/arch/x86/boot/cpuflags.c
+++ b/arch/x86/boot/cpuflags.c
@@ -71,8 +71,7 @@ int has_eflag(unsigned long mask)
# define EBX_REG "=b"
#endif

-static inline void cpuid_count(u32 id, u32 count,
- u32 *a, u32 *b, u32 *c, u32 *d)
+void cpuid_count(u32 id, u32 count, u32 *a, u32 *b, u32 *c, u32 *d)
{
asm volatile(".ifnc %%ebx,%3 ; movl %%ebx,%3 ; .endif \n\t"
"cpuid \n\t"
diff --git a/arch/x86/boot/cpuflags.h b/arch/x86/boot/cpuflags.h
index 2e20814d3ce3..475b8fde90f7 100644
--- a/arch/x86/boot/cpuflags.h
+++ b/arch/x86/boot/cpuflags.h
@@ -17,5 +17,6 @@ extern u32 cpu_vendor[3];

int has_eflag(unsigned long mask);
void get_cpuflags(void);
+void cpuid_count(u32 id, u32 count, u32 *a, u32 *b, u32 *c, u32 *d);

#endif
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
new file mode 100644
index 000000000000..8209ba9ffe1a
--- /dev/null
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_SHARED_TDX_H
+#define _ASM_X86_SHARED_TDX_H
+
+#define TDX_CPUID_LEAF_ID 0x21
+#define TDX_IDENT "IntelTDX "
+
+#endif /* _ASM_X86_SHARED_TDX_H */
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index e6e23ade53a6..d4c28b9f2fbc 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -6,9 +6,7 @@
#include <linux/bits.h>
#include <linux/init.h>
#include <asm/ptrace.h>
-
-#define TDX_CPUID_LEAF_ID 0x21
-#define TDX_IDENT "IntelTDX "
+#include <asm/shared/tdx.h>

#define TDX_HYPERCALL_STANDARD 0

--
2.34.1

2022-02-19 15:29:10

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Sat, Feb 19, 2022 at 12:33:00AM +0300, Kirill A. Shutemov wrote:
> I'll update a log. Or do you want a separate patch for the vendor
> introduction?

Yes pls.

--
Regards/Gruss,
Boris.

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

2022-02-19 18:33:43

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3.1 2/32] x86/coco: Explicitly declare type of confidential computing platform

Kernel derives type of confidential computing platform from sme_me_mask
value and hv_is_isolation_supported(). This detection process will be
more complicated as more platforms get added.

Declare confidential computing vendor explicitly via cc_init().

Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/coco.h | 16 ++++++++++++++++
arch/x86/kernel/cc_platform.c | 29 +++++++++++++++++------------
arch/x86/kernel/cpu/mshyperv.c | 3 +++
arch/x86/mm/mem_encrypt_identity.c | 8 +++++---
4 files changed, 41 insertions(+), 15 deletions(-)
create mode 100644 arch/x86/include/asm/coco.h

diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
new file mode 100644
index 000000000000..6e770e0dd683
--- /dev/null
+++ b/arch/x86/include/asm/coco.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_COCO_H
+#define _ASM_X86_COCO_H
+
+#include <asm/pgtable_types.h>
+
+enum cc_vendor {
+ CC_VENDOR_NONE,
+ CC_VENDOR_AMD,
+ CC_VENDOR_HYPERV,
+ CC_VENDOR_INTEL,
+};
+
+void cc_init(enum cc_vendor);
+
+#endif /* _ASM_X86_COCO_H */
diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index 6a6ffcd978f6..891d3074a16e 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -9,18 +9,15 @@

#include <linux/export.h>
#include <linux/cc_platform.h>
-#include <linux/mem_encrypt.h>

-#include <asm/mshyperv.h>
+#include <asm/coco.h>
#include <asm/processor.h>

-static bool __maybe_unused intel_cc_platform_has(enum cc_attr attr)
+static enum cc_vendor cc_vendor;
+
+static bool intel_cc_platform_has(enum cc_attr attr)
{
-#ifdef CONFIG_INTEL_TDX_GUEST
- return false;
-#else
return false;
-#endif
}

/*
@@ -74,12 +71,20 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)

bool cc_platform_has(enum cc_attr attr)
{
- if (sme_me_mask)
+ switch (cc_vendor) {
+ case CC_VENDOR_AMD:
return amd_cc_platform_has(attr);
-
- if (hv_is_isolation_supported())
+ case CC_VENDOR_INTEL:
+ return intel_cc_platform_has(attr);
+ case CC_VENDOR_HYPERV:
return hyperv_cc_platform_has(attr);
-
- return false;
+ default:
+ return false;
+ }
}
EXPORT_SYMBOL_GPL(cc_platform_has);
+
+__init void cc_init(enum cc_vendor vendor)
+{
+ cc_vendor = vendor;
+}
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 5a99f993e639..d77cf3a31f07 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -33,6 +33,7 @@
#include <asm/nmi.h>
#include <clocksource/hyperv_timer.h>
#include <asm/numa.h>
+#include <asm/coco.h>

/* Is Linux running as the root partition? */
bool hv_root_partition;
@@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
*/
swiotlb_force = SWIOTLB_FORCE;
#endif
+ if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
+ cc_init(CC_VENDOR_HYPERV);
}

if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) {
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index 3f0abb403340..eb7fbd85b77e 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -44,6 +44,7 @@
#include <asm/setup.h>
#include <asm/sections.h>
#include <asm/cmdline.h>
+#include <asm/coco.h>

#include "mm_internal.h"

@@ -565,8 +566,7 @@ void __init sme_enable(struct boot_params *bp)
} else {
/* SEV state cannot be controlled by a command line option */
sme_me_mask = me_mask;
- physical_mask &= ~sme_me_mask;
- return;
+ goto out;
}

/*
@@ -600,6 +600,8 @@ void __init sme_enable(struct boot_params *bp)
sme_me_mask = 0;
else
sme_me_mask = active_by_default ? me_mask : 0;
-
+out:
physical_mask &= ~sme_me_mask;
+ if (sme_me_mask)
+ cc_init(CC_VENDOR_AMD);
}
--
2.34.1

2022-02-20 07:09:16

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Fri, Feb 18, 2022 at 12:36:02PM -0800, Dave Hansen wrote:
> On 2/18/22 08:16, Kirill A. Shutemov wrote:
> > +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
> > +u64 cc_get_mask(bool enc);
> > +u64 cc_mkenc(u64 val);
> > +u64 cc_mkdec(u64 val);
> > +#else
> > +#define cc_get_mask(enc) 0
> > +#define cc_mkenc(val) (val)
> > +#define cc_mkdec(val) (val)
> > +#endif
>
> Is there a reason the stubs as #defines? Static inlines are preferred
> for consistent type safety among other things.

I was slightly worried about 32-bit non-PAE that has phys_addr_t and
pgprotval_t 32-bit. I was not completely sure it will not cause any
issue due to type mismatch. Maybe it is ungrounded.

With CONFIG_ARCH_HAS_CC_PLATFORM=y, all relevant types are 64-bit.

> It would also be nice to talk about the u64 type in the changelog. If I
> remember right, there was a reason you didn't want to have a pgprot_t
> here.

With standalone <asm/coco.h> I think we can make it work with other type.
But I'm not sure what it has to be.

I found helpers useful for modifying pgprotval_t and phys_addr_t. I
considered u64 a common ground.

Should I change this to something else?

> ...
> > @@ -74,12 +72,52 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)
> >
> > bool cc_platform_has(enum cc_attr attr)
> > {
> > - if (sme_me_mask)
> > + switch (cc_vendor) {
> > + case CC_VENDOR_AMD:
> > return amd_cc_platform_has(attr);
> > -
> > - if (hv_is_isolation_supported())
> > + case CC_VENDOR_INTEL:
> > + return intel_cc_platform_has(attr);
> > + case CC_VENDOR_HYPERV:
> > return hyperv_cc_platform_has(attr);
> > -
> > - return false;
> > + default:
> > + return false;
> > + }
> > }
> > EXPORT_SYMBOL_GPL(cc_platform_has);
>
> This patch is bordering on doing too many different things. Adding the
> CC_VENDOR_*'s in a separate patch probably would have been nice, for
> instance.
>
> This wasn't really broached at all in the changelog.

I'll update a log. Or do you want a separate patch for the vendor
introduction?

> > +u64 cc_get_mask(bool enc)
> > +{
> > + switch (cc_vendor) {
> > + case CC_VENDOR_AMD:
> > + return enc ? cc_mask : 0;
> > + default:
> > + return 0;
> > + }
> > +}
>
> I had to ask myself why you need all three of these functions. It's
> obvious _after_ reading the whole patch, but it left me wanting more in
> the changelog about it.

Okay.

> > +u64 cc_mkenc(u64 val)
> > +{
> > + switch (cc_vendor) {
> > + case CC_VENDOR_AMD:
> > + return val | cc_mask;
> > + default:
> > + return val;
> > + }
> > +}
> > +
> > +u64 cc_mkdec(u64 val)
> > +{
> > + switch (cc_vendor) {
> > + case CC_VENDOR_AMD:
> > + return val & ~cc_mask;
> > + default:
> > + return val;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(cc_mkdec);
> > +
> > +__init void cc_init(enum cc_vendor vendor, u64 mask)
> > +{
> > + cc_vendor = vendor;
> > + cc_mask = mask;
> > +}
> > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > index 5a99f993e639..9af6be143998 100644
> > --- a/arch/x86/kernel/cpu/mshyperv.c
> > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > @@ -33,6 +33,7 @@
> > #include <asm/nmi.h>
> > #include <clocksource/hyperv_timer.h>
> > #include <asm/numa.h>
> > +#include <asm/coco.h>
> >
> > /* Is Linux running as the root partition? */
> > bool hv_root_partition;
> > @@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
> > */
> > swiotlb_force = SWIOTLB_FORCE;
> > #endif
> > + if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> > + cc_init(CC_VENDOR_HYPERV, 0);
> > }
> >
> > if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) {
> > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > index 3f0abb403340..fa758247ab57 100644
> > --- a/arch/x86/mm/mem_encrypt_identity.c
> > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > @@ -44,6 +44,7 @@
> > #include <asm/setup.h>
> > #include <asm/sections.h>
> > #include <asm/cmdline.h>
> > +#include <asm/coco.h>
> >
> > #include "mm_internal.h"
> >
> > @@ -565,8 +566,7 @@ void __init sme_enable(struct boot_params *bp)
> > } else {
> > /* SEV state cannot be controlled by a command line option */
> > sme_me_mask = me_mask;
> > - physical_mask &= ~sme_me_mask;
> > - return;
> > + goto out;
> > }
> >
> > /*
> > @@ -600,6 +600,8 @@ void __init sme_enable(struct boot_params *bp)
> > sme_me_mask = 0;
> > else
> > sme_me_mask = active_by_default ? me_mask : 0;
> > -
> > +out:
> > physical_mask &= ~sme_me_mask;
> > + if (sme_me_mask)
> > + cc_init(CC_VENDOR_AMD, sme_me_mask);
> > }
>
> I don't think you need to mop it up here, but where does this leave
> sme_me_mask?

I think sme_me_mask still can be useful to indicate that the code is only
relevant for AMD context.

> > diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> > index b4072115c8ef..e79366b8a9da 100644
> > --- a/arch/x86/mm/pat/set_memory.c
> > +++ b/arch/x86/mm/pat/set_memory.c
> > @@ -1999,8 +1999,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
> > memset(&cpa, 0, sizeof(cpa));
> > cpa.vaddr = &addr;
> > cpa.numpages = numpages;
> > - cpa.mask_set = enc ? __pgprot(_PAGE_ENC) : __pgprot(0);
> > - cpa.mask_clr = enc ? __pgprot(0) : __pgprot(_PAGE_ENC);
> > + cpa.mask_set = __pgprot(cc_get_mask(enc));
> > + cpa.mask_clr = __pgprot(cc_get_mask(!enc));
> > cpa.pgd = init_mm.pgd;
> >
> > /* Must avoid aliasing mappings in the highmem code */
>
> This actually ended up looking pretty nice. It was a real mess along
> the way, but this makes me think this is a _good_ solution.

--
Kirill A. Shutemov

2022-02-21 09:03:30

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv3 05/32] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions

From: Kuppuswamy Sathyanarayanan <[email protected]>

Guests communicate with VMMs with hypercalls. Historically, these
are implemented using instructions that are known to cause VMEXITs
like VMCALL, VMLAUNCH, etc. However, with TDX, VMEXITs no longer
expose the guest state to the host. This prevents the old hypercall
mechanisms from working. So, to communicate with VMM, TDX
specification defines a new instruction called TDCALL.

In a TDX based VM, since the VMM is an untrusted entity, an intermediary
layer -- TDX module -- facilitates secure communication between the host
and the guest. TDX module is loaded like a firmware into a special CPU
mode called SEAM. TDX guests communicate with the TDX module using the
TDCALL instruction.

A guest uses TDCALL to communicate with both the TDX module and VMM.
The value of the RAX register when executing the TDCALL instruction is
used to determine the TDCALL type. A variant of TDCALL used to communicate
with the VMM is called TDVMCALL.

Add generic interfaces to communicate with the TDX module and VMM
(using the TDCALL instruction).

__tdx_hypercall() - Used by the guest to request services from the
VMM (via TDVMCALL).
__tdx_module_call() - Used to communicate with the TDX module (via
TDCALL).

Also define an additional wrapper _tdx_hypercall(), which adds error
handling support for the TDCALL failure.

The __tdx_module_call() and __tdx_hypercall() helper functions are
implemented in assembly in a .S file. The TDCALL ABI requires
shuffling arguments in and out of registers, which proved to be
awkward with inline assembly.

Just like syscalls, not all TDVMCALL use cases need to use the same
number of argument registers. The implementation here picks the current
worst-case scenario for TDCALL (4 registers). For TDCALLs with fewer
than 4 arguments, there will end up being a few superfluous (cheap)
instructions. But, this approach maximizes code reuse.

For registers used by the TDCALL instruction, please check TDX GHCI
specification, the section titled "TDCALL instruction" and "TDG.VP.VMCALL
Interface".

Based on previous patch by Sean Christopherson.

Reviewed-by: Tony Luck <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/coco/Makefile | 2 +-
arch/x86/coco/tdcall.S | 184 ++++++++++++++++++++++++++++++++++
arch/x86/coco/tdx.c | 18 ++++
arch/x86/include/asm/tdx.h | 27 +++++
arch/x86/kernel/asm-offsets.c | 10 ++
5 files changed, 240 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/coco/tdcall.S

diff --git a/arch/x86/coco/Makefile b/arch/x86/coco/Makefile
index b285d485a72e..4f19518841fc 100644
--- a/arch/x86/coco/Makefile
+++ b/arch/x86/coco/Makefile
@@ -1,2 +1,2 @@
# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_INTEL_TDX_GUEST) += tdx.o
+obj-$(CONFIG_INTEL_TDX_GUEST) += tdx.o tdcall.o
diff --git a/arch/x86/coco/tdcall.S b/arch/x86/coco/tdcall.S
new file mode 100644
index 000000000000..c4dd9468e7d9
--- /dev/null
+++ b/arch/x86/coco/tdcall.S
@@ -0,0 +1,184 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <asm/asm-offsets.h>
+#include <asm/asm.h>
+#include <asm/frame.h>
+#include <asm/unwind_hints.h>
+
+#include <linux/linkage.h>
+#include <linux/bits.h>
+#include <linux/errno.h>
+
+#include "../virt/tdxcall.S"
+
+/*
+ * Bitmasks of exposed registers (with VMM).
+ */
+#define TDX_R10 BIT(10)
+#define TDX_R11 BIT(11)
+#define TDX_R12 BIT(12)
+#define TDX_R13 BIT(13)
+#define TDX_R14 BIT(14)
+#define TDX_R15 BIT(15)
+
+/*
+ * These registers are clobbered to hold arguments for each
+ * TDVMCALL. They are safe to expose to the VMM.
+ * Each bit in this mask represents a register ID. Bit field
+ * details can be found in TDX GHCI specification, section
+ * titled "TDCALL [TDG.VP.VMCALL] leaf".
+ */
+#define TDVMCALL_EXPOSE_REGS_MASK ( TDX_R10 | TDX_R11 | \
+ TDX_R12 | TDX_R13 | \
+ TDX_R14 | TDX_R15 )
+
+/*
+ * __tdx_module_call() - Used by TDX guests to request services from
+ * the TDX module (does not include VMM services).
+ *
+ * Transforms function call register arguments into the TDCALL
+ * register ABI. After TDCALL operation, TDX module output is saved
+ * in @out (if it is provided by the user)
+ *
+ *-------------------------------------------------------------------------
+ * TDCALL ABI:
+ *-------------------------------------------------------------------------
+ * Input Registers:
+ *
+ * RAX - TDCALL Leaf number.
+ * RCX,RDX,R8-R9 - TDCALL Leaf specific input registers.
+ *
+ * Output Registers:
+ *
+ * RAX - TDCALL instruction error code.
+ * RCX,RDX,R8-R11 - TDCALL Leaf specific output registers.
+ *
+ *-------------------------------------------------------------------------
+ *
+ * __tdx_module_call() function ABI:
+ *
+ * @fn (RDI) - TDCALL Leaf ID, moved to RAX
+ * @rcx (RSI) - Input parameter 1, moved to RCX
+ * @rdx (RDX) - Input parameter 2, moved to RDX
+ * @r8 (RCX) - Input parameter 3, moved to R8
+ * @r9 (R8) - Input parameter 4, moved to R9
+ *
+ * @out (R9) - struct tdx_module_output pointer
+ * stored temporarily in R12 (not
+ * shared with the TDX module). It
+ * can be NULL.
+ *
+ * Return status of TDCALL via RAX.
+ */
+SYM_FUNC_START(__tdx_module_call)
+ FRAME_BEGIN
+ TDX_MODULE_CALL host=0
+ FRAME_END
+ ret
+SYM_FUNC_END(__tdx_module_call)
+
+/*
+ * __tdx_hypercall() - Make hypercalls to a TDX VMM.
+ *
+ * Transforms values in function call argument struct tdx_hypercall_args @args
+ * into the TDCALL register ABI. After TDCALL operation, VMM output is saved
+ * back in @args.
+ *
+ *-------------------------------------------------------------------------
+ * TD VMCALL ABI:
+ *-------------------------------------------------------------------------
+ *
+ * Input Registers:
+ *
+ * RAX - TDCALL instruction leaf number (0 - TDG.VP.VMCALL)
+ * RCX - BITMAP which controls which part of TD Guest GPR
+ * is passed as-is to the VMM and back.
+ * R10 - Set 0 to indicate TDCALL follows standard TDX ABI
+ * specification. Non zero value indicates vendor
+ * specific ABI.
+ * R11 - VMCALL sub function number
+ * RBX, RBP, RDI, RSI - Used to pass VMCALL sub function specific arguments.
+ * R8-R9, R12-R15 - Same as above.
+ *
+ * Output Registers:
+ *
+ * RAX - TDCALL instruction status (Not related to hypercall
+ * output).
+ * R10 - Hypercall output error code.
+ * R11-R15 - Hypercall sub function specific output values.
+ *
+ *-------------------------------------------------------------------------
+ *
+ * __tdx_hypercall() function ABI:
+ *
+ * @args (RDI) - struct tdx_hypercall_args for input and output
+ * @flags (RSI) - TDX_HCALL_* flags
+ *
+ * On successful completion, return the hypercall error code.
+ */
+SYM_FUNC_START(__tdx_hypercall)
+ FRAME_BEGIN
+
+ /* Save callee-saved GPRs as mandated by the x86_64 ABI */
+ push %r15
+ push %r14
+ push %r13
+ push %r12
+
+ /* Mangle function call ABI into TDCALL ABI: */
+ /* Set TDCALL leaf ID (TDVMCALL (0)) in RAX */
+ xor %eax, %eax
+
+ /* Copy hypercall registers from arg struct: */
+ movq TDX_HYPERCALL_r10(%rdi), %r10
+ movq TDX_HYPERCALL_r11(%rdi), %r11
+ movq TDX_HYPERCALL_r12(%rdi), %r12
+ movq TDX_HYPERCALL_r13(%rdi), %r13
+ movq TDX_HYPERCALL_r14(%rdi), %r14
+ movq TDX_HYPERCALL_r15(%rdi), %r15
+
+ movl $TDVMCALL_EXPOSE_REGS_MASK, %ecx
+
+ tdcall
+
+ /*
+ * TDVMCALL leaf does not suppose to fail. If it fails something
+ * is horribly wrong with TDX module. Stop the world.
+ */
+ testq %rax, %rax
+ jne .Lpanic
+
+ /* TDVMCALL leaf return code is in R10 */
+ movq %r10, %rax
+
+ /* Copy hypercall result registers to arg struct if needed */
+ testq $TDX_HCALL_HAS_OUTPUT, %rsi
+ jz .Lout
+
+ movq %r10, TDX_HYPERCALL_r10(%rdi)
+ movq %r11, TDX_HYPERCALL_r11(%rdi)
+ movq %r12, TDX_HYPERCALL_r12(%rdi)
+ movq %r13, TDX_HYPERCALL_r13(%rdi)
+ movq %r14, TDX_HYPERCALL_r14(%rdi)
+ movq %r15, TDX_HYPERCALL_r15(%rdi)
+.Lout:
+ /*
+ * Zero out registers exposed to the VMM to avoid speculative execution
+ * with VMM-controlled values. This needs to include all registers
+ * present in TDVMCALL_EXPOSE_REGS_MASK (except R12-R15). R12-R15
+ * context will be restored.
+ */
+ xor %r10d, %r10d
+ xor %r11d, %r11d
+
+ /* Restore callee-saved GPRs as mandated by the x86_64 ABI */
+ pop %r12
+ pop %r13
+ pop %r14
+ pop %r15
+
+ FRAME_END
+
+ retq
+.Lpanic:
+ ud2
+SYM_FUNC_END(__tdx_hypercall)
diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index 77c26c6b932d..89367fa576c1 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -7,6 +7,24 @@
#include <linux/cpufeature.h>
#include <asm/tdx.h>

+/*
+ * Wrapper for standard use of __tdx_hypercall with no output aside from
+ * return code.
+ */
+static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15)
+{
+ struct tdx_hypercall_args args = {
+ .r10 = TDX_HYPERCALL_STANDARD,
+ .r11 = fn,
+ .r12 = r12,
+ .r13 = r13,
+ .r14 = r14,
+ .r15 = r15,
+ };
+
+ return __tdx_hypercall(&args, 0);
+}
+
void __init tdx_early_init(void)
{
u32 eax, sig[3];
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 2f8cb1e53e77..557227e40da9 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -3,11 +3,16 @@
#ifndef _ASM_X86_TDX_H
#define _ASM_X86_TDX_H

+#include <linux/bits.h>
#include <linux/init.h>

#define TDX_CPUID_LEAF_ID 0x21
#define TDX_IDENT "IntelTDX "

+#define TDX_HYPERCALL_STANDARD 0
+
+#define TDX_HCALL_HAS_OUTPUT BIT(0)
+
#define TDX_SEAMCALL_VMFAILINVALID 0x8000FF00FFFF0000ULL

#ifndef __ASSEMBLY__
@@ -27,10 +32,32 @@ struct tdx_module_output {
u64 r11;
};

+/*
+ * Used in __tdx_hypercall() to pass down and get back registers' values of
+ * the TDCALL instruction when requesting services from the VMM.
+ *
+ * This is a software only structure and not part of the TDX module/VMM ABI.
+ */
+struct tdx_hypercall_args {
+ u64 r10;
+ u64 r11;
+ u64 r12;
+ u64 r13;
+ u64 r14;
+ u64 r15;
+};
+
#ifdef CONFIG_INTEL_TDX_GUEST

void __init tdx_early_init(void);

+/* Used to communicate with the TDX module */
+u64 __tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+ struct tdx_module_output *out);
+
+/* Used to request services from the VMM */
+u64 __tdx_hypercall(struct tdx_hypercall_args *args, unsigned long flags);
+
#else

static inline void tdx_early_init(void) { };
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 7dca52f5cfc6..0b465e7d0a2f 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -74,6 +74,16 @@ static void __used common(void)
OFFSET(TDX_MODULE_r10, tdx_module_output, r10);
OFFSET(TDX_MODULE_r11, tdx_module_output, r11);

+#ifdef CONFIG_INTEL_TDX_GUEST
+ BLANK();
+ OFFSET(TDX_HYPERCALL_r10, tdx_hypercall_args, r10);
+ OFFSET(TDX_HYPERCALL_r11, tdx_hypercall_args, r11);
+ OFFSET(TDX_HYPERCALL_r12, tdx_hypercall_args, r12);
+ OFFSET(TDX_HYPERCALL_r13, tdx_hypercall_args, r13);
+ OFFSET(TDX_HYPERCALL_r14, tdx_hypercall_args, r14);
+ OFFSET(TDX_HYPERCALL_r15, tdx_hypercall_args, r15);
+#endif
+
BLANK();
OFFSET(BP_scratch, boot_params, scratch);
OFFSET(BP_secure_boot, boot_params, secure_boot);
--
2.34.1

2022-02-21 11:53:04

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [PATCHv3 13/32] x86/tdx: Detect TDX at early kernel decompression time

On Fri, Feb 18, 2022 at 07:16:59PM +0300, Kirill A. Shutemov wrote:
...
>
> +
> +void early_tdx_detect(void)
> +{
> + u32 eax, sig[3];
> +
> + cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2], &sig[1]);
> +
> + if (memcmp(TDX_IDENT, sig, 12))
> + return;

Maybe worth to guard ourself, like

BUILD_BUG_ON(sizeof(sig) != (sizeof(TDX_IDENT)-1));
if (memcmp(TDX_IDENT, sig, sizeof(sig))
return;

2022-02-21 13:16:12

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3.1 2/32] x86/coco: Explicitly declare type of confidential computing platform

On Mon, Feb 21, 2022 at 12:07:15PM +0100, Borislav Petkov wrote:
> On Sat, Feb 19, 2022 at 03:13:04AM +0300, Kirill A. Shutemov wrote:
> > diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
> > index 6a6ffcd978f6..891d3074a16e 100644
> > --- a/arch/x86/kernel/cc_platform.c
> > +++ b/arch/x86/kernel/cc_platform.c
> > @@ -9,18 +9,15 @@
> >
> > #include <linux/export.h>
> > #include <linux/cc_platform.h>
> > -#include <linux/mem_encrypt.h>
> >
> > -#include <asm/mshyperv.h>
> > +#include <asm/coco.h>
> > #include <asm/processor.h>
> >
> > -static bool __maybe_unused intel_cc_platform_has(enum cc_attr attr)
> > +static enum cc_vendor cc_vendor;
>
> static enum cc_vendor vendor __ro_after_init;

Hm. Isn't 'vendor' too generic? It may lead to name conflict in the
future.

What is wrong with cc_vendor here? I noticed that you don't like name of
a variable to match type name. Why?

> > @@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
> > */
> > swiotlb_force = SWIOTLB_FORCE;
> > #endif
> > + if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> > + cc_init(CC_VENDOR_HYPERV);
>
> Isn't that supposed to test HV_ISOLATION_TYPE_SNP instead?

Currently cc_platform_has() relies on hv_is_isolation_supported() which
checks for !HV_ISOLATION_TYPE_NONE. This is direct transfer to the new
scheme. It might be wrong, but it is not regression.

> I mean, I have no clue what HV_ISOLATION_TYPE_VBS is. It is not used
> anywhere in the tree either.
>
> a6c76bb08dc7 ("x86/hyperv: Load/save the Isolation Configuration leaf")
> calls it "'VBS' (software-based isolation)" - whatever that means - so
> I'm not sure that is going to need the cc-facilities.
>
> For stuff like that you need to use get_maintainers.pl and Cc them
> folks:
>
> $ git log -p -1 | ./scripts/get_maintainer.pl | grep -i hyper
> "K. Y. Srinivasan" <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Haiyang Zhang <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Stephen Hemminger <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> Wei Liu <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS,commit_signer:1/4=25%)
> Dexuan Cui <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS)
> [email protected] (open list:Hyper-V/Azure CORE AND DRIVERS)
>
> /me adds the ML to Cc.

+Tianyu, who brought HyperV cc_platform_has().

Speaking about HyperV, moving to scheme with cc_init() revealed that
HyperV never selected ARCH_HAS_CC_PLATFORM. Now it leads to build failure
if AMD memory encryption is not enabled:

ld: arch/x86/kernel/cpu/mshyperv.o: in function `ms_hyperv_init_platform':
mshyperv.c:(.init.text+0x297): undefined reference to `cc_init'

Maybe something like this:

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 0747a8f1fcee..574ea80601e9 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -8,6 +8,7 @@ config HYPERV
|| (ARM64 && !CPU_BIG_ENDIAN))
select PARAVIRT
select X86_HV_CALLBACK_VECTOR if X86
+ select ARCH_HAS_CC_PLATFORM if X86
select VMAP_PFN
help
Select this option to run Linux as a Hyper-V client operating

Again, it is pre-existing issue. It only escalated to build failure.

> > if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) {
> > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > index 3f0abb403340..eb7fbd85b77e 100644
> > --- a/arch/x86/mm/mem_encrypt_identity.c
> > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > @@ -44,6 +44,7 @@
> > #include <asm/setup.h>
> > #include <asm/sections.h>
> > #include <asm/cmdline.h>
> > +#include <asm/coco.h>
> >
> > #include "mm_internal.h"
> >
> > @@ -565,8 +566,7 @@ void __init sme_enable(struct boot_params *bp)
> > } else {
> > /* SEV state cannot be controlled by a command line option */
> > sme_me_mask = me_mask;
> > - physical_mask &= ~sme_me_mask;
> > - return;
> > + goto out;
> > }
> >
> > /*
> > @@ -600,6 +600,8 @@ void __init sme_enable(struct boot_params *bp)
> > sme_me_mask = 0;
> > else
> > sme_me_mask = active_by_default ? me_mask : 0;
> > -
> > +out:
> > physical_mask &= ~sme_me_mask;
> > + if (sme_me_mask)
> > + cc_init(CC_VENDOR_AMD);
> > }
>
> I guess.
>
> Adding SEV folks to Cc too.
>
> Please use get_maintainer.pl - you should know that - you're not some
> newbie who started doing kernel work two weeks ago.

Sorry, will do.

--
Kirill A. Shutemov

2022-02-21 21:40:01

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCHv3.1 2/32] x86/coco: Explicitly declare type of confidential computing platform

On Mon, Feb 21, 2022 at 02:44:51PM +0300, Kirill A. Shutemov wrote:
> Hm. Isn't 'vendor' too generic? It may lead to name conflict in the
> future.

It's a static variable visible only in this unit.

> What is wrong with cc_vendor here? I noticed that you don't like name of
> a variable to match type name. Why?

Because when I look at the name I don't know whether it is the type or a
variable of that type. Sure, sure, it depends on the context but let's
make it as non-ambiguous as possible.

> Currently cc_platform_has() relies on hv_is_isolation_supported() which
> checks for !HV_ISOLATION_TYPE_NONE. This is direct transfer to the new
> scheme. It might be wrong, but it is not regression.

I didn't say it is a regression - I'm just wondering why.

Thx.

--
Regards/Gruss,
Boris.

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

2022-02-21 22:29:36

by Wei Liu

[permalink] [raw]
Subject: Re: [PATCHv3.1 2/32] x86/coco: Explicitly declare type of confidential computing platform

On Mon, Feb 21, 2022 at 12:07:15PM +0100, Borislav Petkov wrote:
> On Sat, Feb 19, 2022 at 03:13:04AM +0300, Kirill A. Shutemov wrote:
[...]
> > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> > index 5a99f993e639..d77cf3a31f07 100644
> > --- a/arch/x86/kernel/cpu/mshyperv.c
> > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > @@ -33,6 +33,7 @@
> > #include <asm/nmi.h>
> > #include <clocksource/hyperv_timer.h>
> > #include <asm/numa.h>
> > +#include <asm/coco.h>
> >
> > /* Is Linux running as the root partition? */
> > bool hv_root_partition;
> > @@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
> > */
> > swiotlb_force = SWIOTLB_FORCE;
> > #endif
> > + if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> > + cc_init(CC_VENDOR_HYPERV);
>
> Isn't that supposed to test HV_ISOLATION_TYPE_SNP instead?
>
> I mean, I have no clue what HV_ISOLATION_TYPE_VBS is. It is not used
> anywhere in the tree either.
>
> a6c76bb08dc7 ("x86/hyperv: Load/save the Isolation Configuration leaf")
> calls it "'VBS' (software-based isolation)" - whatever that means - so
> I'm not sure that is going to need the cc-facilities.
>

Hi Boris and Kirill, I only see VBS mentioned here so I don't have much
context, but VBS likely means virtualization-based security. There is a
public document for it.

https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/oem-vbs

Whether it needs a new isolation type or not, I am not sure. Perhaps
Tianyu can provide more context.

Thanks,
Wei.

2022-02-22 04:05:45

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Mon, Feb 21, 2022 at 08:49:11PM +0100, Borislav Petkov wrote:
> On Mon, Feb 21, 2022 at 11:28:16AM -0800, Dave Hansen wrote:
> > Why don't we just have:
> >
> > pgprot_t cc_mkenc(pgprot prot)
> > pgprot_t cc_mkenc(pgprot prot)
> >
> > and *no* pgprot_{en,de}crypted()?
>
> Yes, and can the above cc_get_mask() thing be:
>
> cpa.mask_set = enc ? cc_mkenc(0) : cc_mkdec(0);
> cpa.mask_clr = enc ? cc_mkdec(0) : cc_mkenc(0);
>
> since we're going to feed it pgprot things?

Well, it actually going to be

cpa.mask_set = enc ? cc_mkenc(__pgprot(0)) : cc_mkdec(__pgprot(0));
cpa.mask_clr = enc ? cc_mkdec(__pgprot(0)) : cc_mkenc(__pgprot(0));

as '0' is not a valid pgprot_t.

Still wonna go this path?

--
Kirill A. Shutemov

2022-02-22 04:13:23

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Mon, Feb 21, 2022 at 11:28:16AM -0800, Dave Hansen wrote:
> I'm just a bit confused why *this* was chosen as the cc_whatever() hook.
> Just like the mask function, it has one spot where it gets used:
>
> +#define pgprot_encrypted(prot) __pgprot(cc_mkenc(pgprot_val(prot)))
> +#define pgprot_decrypted(prot) __pgprot(cc_mkdec(pgprot_val(prot)))
>
> So, why bother having another level of abstraction?
>
> Why don't we just have:
>
> pgprot_t cc_mkenc(pgprot prot)
> pgprot_t cc_mkenc(pgprot prot)
>
> and *no* pgprot_{en,de}crypted()?

Okay. Let me try this.

> >>> +out:
> >>> physical_mask &= ~sme_me_mask;
> >>> + if (sme_me_mask)
> >>> + cc_init(CC_VENDOR_AMD, sme_me_mask);
> >>> }
> >>
> >> I don't think you need to mop it up here, but where does this leave
> >> sme_me_mask?
> >
> > I think sme_me_mask still can be useful to indicate that the code is only
> > relevant for AMD context.
>
> Shouldn't we be able to tell that because something is in an
> AMD-specific file, function or #ifdef?

Sure. But for some code it is not immidiately obvious that it is
AMD-specific. Like from file name alone, mem_encrypt_identity.c doesn't
look like it is only AMD thing.

Anyway, I think getting rid of sme_me_mask is out of scope for the
patchset.

> Is there ever a time where sme_me_mask is populated by cc_mask is not?

Yes. Decompression code. (I know it doesn't affect bottom line much).

> This seems like it is just making a copy of sme_me_mask.
>
> sme_me_mask does look quite AMD-specialized, like its assembly
> manipulation. Even if it's just a copy of cc_mask, it would be nice to
> call that out so the relationship is crystal clear.

--
Kirill A. Shutemov

2022-02-22 04:38:23

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3 13/32] x86/tdx: Detect TDX at early kernel decompression time

On Mon, Feb 21, 2022 at 02:37:29PM +0300, Cyrill Gorcunov wrote:
> On Fri, Feb 18, 2022 at 07:16:59PM +0300, Kirill A. Shutemov wrote:
> ...
> >
> > +
> > +void early_tdx_detect(void)
> > +{
> > + u32 eax, sig[3];
> > +
> > + cpuid_count(TDX_CPUID_LEAF_ID, 0, &eax, &sig[0], &sig[2], &sig[1]);
> > +
> > + if (memcmp(TDX_IDENT, sig, 12))
> > + return;
>
> Maybe worth to guard ourself, like
>
> BUILD_BUG_ON(sizeof(sig) != (sizeof(TDX_IDENT)-1));
> if (memcmp(TDX_IDENT, sig, sizeof(sig))
> return;

Sure. Will do.

--
Kirill A. Shutemov

2022-02-22 04:51:19

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCHv3.1 2/32] x86/coco: Explicitly declare type of confidential computing platform

On Mon, Feb 21, 2022 at 01:52:58PM +0000, Wei Liu wrote:
> Hi Boris and Kirill, I only see VBS mentioned here so I don't have much
> context, but VBS likely means virtualization-based security. There is a
> public document for it.
>
> https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/oem-vbs
>
> Whether it needs a new isolation type or not, I am not sure. Perhaps
> Tianyu can provide more context.

Right, this came in with

c789b90a6904 ("x86/hyper-v: Add hyperv Isolation VM check in the cc_platform_has()")

which says

Hyper-V provides Isolation VM for confidential computing support and
guest memory is encrypted in it. Places checking cc_platform_has()
with GUEST_MEM_ENCRYPT attr should return "True" in Isolation VM.

I'm guessing this was done because you "need to adjust the SWIOTLB size
just like SEV guests."

So my question is, does this VBS thing do guest memory encryption or
does it only use hw virt features?

Because you guys have HV_ISOLATION_TYPE_SNP already. And so, the check

hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE;

includes VBS because VBS is only interested in the SWIOTLB buffer size
adjustment and not the rest of the cc_* stuff. Or?

But let's see what Tianyu says.

Thx.

--
Regards/Gruss,
Boris.

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

2022-02-22 05:20:42

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCHv3.1 2/32] x86/coco: Explicitly declare type of confidential computing platform

On Sat, Feb 19, 2022 at 03:13:04AM +0300, Kirill A. Shutemov wrote:
> Kernel derives type of confidential computing platform from sme_me_mask
> value and hv_is_isolation_supported(). This detection process will be
> more complicated as more platforms get added.
>
> Declare confidential computing vendor explicitly via cc_init().
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> ---
> arch/x86/include/asm/coco.h | 16 ++++++++++++++++
> arch/x86/kernel/cc_platform.c | 29 +++++++++++++++++------------
> arch/x86/kernel/cpu/mshyperv.c | 3 +++
> arch/x86/mm/mem_encrypt_identity.c | 8 +++++---
> 4 files changed, 41 insertions(+), 15 deletions(-)
> create mode 100644 arch/x86/include/asm/coco.h
>
> diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
> new file mode 100644
> index 000000000000..6e770e0dd683
> --- /dev/null
> +++ b/arch/x86/include/asm/coco.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_COCO_H
> +#define _ASM_X86_COCO_H
> +
> +#include <asm/pgtable_types.h>
> +
> +enum cc_vendor {
> + CC_VENDOR_NONE,
> + CC_VENDOR_AMD,
> + CC_VENDOR_HYPERV,
> + CC_VENDOR_INTEL,
> +};
> +
> +void cc_init(enum cc_vendor);
> +
> +#endif /* _ASM_X86_COCO_H */
> diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
> index 6a6ffcd978f6..891d3074a16e 100644
> --- a/arch/x86/kernel/cc_platform.c
> +++ b/arch/x86/kernel/cc_platform.c
> @@ -9,18 +9,15 @@
>
> #include <linux/export.h>
> #include <linux/cc_platform.h>
> -#include <linux/mem_encrypt.h>
>
> -#include <asm/mshyperv.h>
> +#include <asm/coco.h>
> #include <asm/processor.h>
>
> -static bool __maybe_unused intel_cc_platform_has(enum cc_attr attr)
> +static enum cc_vendor cc_vendor;

static enum cc_vendor vendor __ro_after_init;

> +
> +static bool intel_cc_platform_has(enum cc_attr attr)
> {
> -#ifdef CONFIG_INTEL_TDX_GUEST
> - return false;
> -#else
> return false;
> -#endif
> }
>
> /*
> @@ -74,12 +71,20 @@ static bool hyperv_cc_platform_has(enum cc_attr attr)
>
> bool cc_platform_has(enum cc_attr attr)
> {
> - if (sme_me_mask)
> + switch (cc_vendor) {
> + case CC_VENDOR_AMD:
> return amd_cc_platform_has(attr);
> -
> - if (hv_is_isolation_supported())
> + case CC_VENDOR_INTEL:
> + return intel_cc_platform_has(attr);
> + case CC_VENDOR_HYPERV:
> return hyperv_cc_platform_has(attr);
> -
> - return false;
> + default:
> + return false;
> + }
> }
> EXPORT_SYMBOL_GPL(cc_platform_has);
> +
> +__init void cc_init(enum cc_vendor vendor)
> +{
> + cc_vendor = vendor;
> +}
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 5a99f993e639..d77cf3a31f07 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -33,6 +33,7 @@
> #include <asm/nmi.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/numa.h>
> +#include <asm/coco.h>
>
> /* Is Linux running as the root partition? */
> bool hv_root_partition;
> @@ -344,6 +345,8 @@ static void __init ms_hyperv_init_platform(void)
> */
> swiotlb_force = SWIOTLB_FORCE;
> #endif
> + if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
> + cc_init(CC_VENDOR_HYPERV);

Isn't that supposed to test HV_ISOLATION_TYPE_SNP instead?

I mean, I have no clue what HV_ISOLATION_TYPE_VBS is. It is not used
anywhere in the tree either.

a6c76bb08dc7 ("x86/hyperv: Load/save the Isolation Configuration leaf")
calls it "'VBS' (software-based isolation)" - whatever that means - so
I'm not sure that is going to need the cc-facilities.

For stuff like that you need to use get_maintainers.pl and Cc them
folks:

$ git log -p -1 | ./scripts/get_maintainer.pl | grep -i hyper
"K. Y. Srinivasan" <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS)
Haiyang Zhang <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS)
Stephen Hemminger <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS)
Wei Liu <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS,commit_signer:1/4=25%)
Dexuan Cui <[email protected]> (supporter:Hyper-V/Azure CORE AND DRIVERS)
[email protected] (open list:Hyper-V/Azure CORE AND DRIVERS)

/me adds the ML to Cc.

> if (hv_max_functions_eax >= HYPERV_CPUID_NESTED_FEATURES) {
> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index 3f0abb403340..eb7fbd85b77e 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -44,6 +44,7 @@
> #include <asm/setup.h>
> #include <asm/sections.h>
> #include <asm/cmdline.h>
> +#include <asm/coco.h>
>
> #include "mm_internal.h"
>
> @@ -565,8 +566,7 @@ void __init sme_enable(struct boot_params *bp)
> } else {
> /* SEV state cannot be controlled by a command line option */
> sme_me_mask = me_mask;
> - physical_mask &= ~sme_me_mask;
> - return;
> + goto out;
> }
>
> /*
> @@ -600,6 +600,8 @@ void __init sme_enable(struct boot_params *bp)
> sme_me_mask = 0;
> else
> sme_me_mask = active_by_default ? me_mask : 0;
> -
> +out:
> physical_mask &= ~sme_me_mask;
> + if (sme_me_mask)
> + cc_init(CC_VENDOR_AMD);
> }

I guess.

Adding SEV folks to Cc too.

Please use get_maintainer.pl - you should know that - you're not some
newbie who started doing kernel work two weeks ago.

--
Regards/Gruss,
Boris.

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

2022-02-22 05:28:40

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Tue, Feb 22, 2022 at 01:21:49AM +0300, Kirill A. Shutemov wrote:
> Well, it actually going to be
>
> cpa.mask_set = enc ? cc_mkenc(__pgprot(0)) : cc_mkdec(__pgprot(0));
> cpa.mask_clr = enc ? cc_mkdec(__pgprot(0)) : cc_mkenc(__pgprot(0));
>
> as '0' is not a valid pgprot_t.
>
> Still wonna go this path?

Why "still"? What's wrong with that?

--
Regards/Gruss,
Boris.

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

2022-02-22 05:29:45

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On 2/18/22 13:33, Kirill A. Shutemov wrote:
> On Fri, Feb 18, 2022 at 12:36:02PM -0800, Dave Hansen wrote:
>> On 2/18/22 08:16, Kirill A. Shutemov wrote:
>>> +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
>>> +u64 cc_get_mask(bool enc);
>>> +u64 cc_mkenc(u64 val);
>>> +u64 cc_mkdec(u64 val);
>>> +#else
>>> +#define cc_get_mask(enc) 0
>>> +#define cc_mkenc(val) (val)
>>> +#define cc_mkdec(val) (val)
>>> +#endif
>>
>> Is there a reason the stubs as #defines? Static inlines are preferred
>> for consistent type safety among other things.
>
> I was slightly worried about 32-bit non-PAE that has phys_addr_t and
> pgprotval_t 32-bit. I was not completely sure it will not cause any
> issue due to type mismatch. Maybe it is ungrounded.
>
> With CONFIG_ARCH_HAS_CC_PLATFORM=y, all relevant types are 64-bit.
>
>> It would also be nice to talk about the u64 type in the changelog. If I
>> remember right, there was a reason you didn't want to have a pgprot_t
>> here.
>
> With standalone <asm/coco.h> I think we can make it work with other type.
> But I'm not sure what it has to be.
>
> I found helpers useful for modifying pgprotval_t and phys_addr_t. I
> considered u64 a common ground.
>
> Should I change this to something else?

cc_get_mask() is only used once and is assigned to a pgprot_t variable.
I expect it to return a pgprot_t.

...
>>> +u64 cc_mkenc(u64 val)
>>> +{
>>> + switch (cc_vendor) {
>>> + case CC_VENDOR_AMD:
>>> + return val | cc_mask;
>>> + default:
>>> + return val;
>>> + }
>>> +}
>>> +
>>> +u64 cc_mkdec(u64 val)
>>> +{
>>> + switch (cc_vendor) {
>>> + case CC_VENDOR_AMD:
>>> + return val & ~cc_mask;
>>> + default:
>>> + return val;
>>> + }
>>> +}
>>> +EXPORT_SYMBOL_GPL(cc_mkdec);

I'm just a bit confused why *this* was chosen as the cc_whatever() hook.
Just like the mask function, it has one spot where it gets used:

+#define pgprot_encrypted(prot) __pgprot(cc_mkenc(pgprot_val(prot)))
+#define pgprot_decrypted(prot) __pgprot(cc_mkdec(pgprot_val(prot)))

So, why bother having another level of abstraction?

Why don't we just have:

pgprot_t cc_mkenc(pgprot prot)
pgprot_t cc_mkenc(pgprot prot)

and *no* pgprot_{en,de}crypted()?

...
>>> +out:
>>> physical_mask &= ~sme_me_mask;
>>> + if (sme_me_mask)
>>> + cc_init(CC_VENDOR_AMD, sme_me_mask);
>>> }
>>
>> I don't think you need to mop it up here, but where does this leave
>> sme_me_mask?
>
> I think sme_me_mask still can be useful to indicate that the code is only
> relevant for AMD context.

Shouldn't we be able to tell that because something is in an
AMD-specific file, function or #ifdef?

Is there ever a time where sme_me_mask is populated by cc_mask is not?
This seems like it is just making a copy of sme_me_mask.

sme_me_mask does look quite AMD-specialized, like its assembly
manipulation. Even if it's just a copy of cc_mask, it would be nice to
call that out so the relationship is crystal clear.

2022-02-22 05:40:26

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Mon, Feb 21, 2022 at 11:56:00PM +0100, Borislav Petkov wrote:
> On Tue, Feb 22, 2022 at 01:21:49AM +0300, Kirill A. Shutemov wrote:
> > Well, it actually going to be
> >
> > cpa.mask_set = enc ? cc_mkenc(__pgprot(0)) : cc_mkdec(__pgprot(0));
> > cpa.mask_clr = enc ? cc_mkdec(__pgprot(0)) : cc_mkenc(__pgprot(0));
> >
> > as '0' is not a valid pgprot_t.
> >
> > Still wonna go this path?
>
> Why "still"? What's wrong with that?

IMO, it makes these statement substantially uglier.

--
Kirill A. Shutemov

2022-02-22 05:43:53

by Brijesh Singh

[permalink] [raw]
Subject: Re: [PATCHv3 27/32] x86/mm/cpa: Generailize __set_memory_enc_pgtable()



On 2/18/22 10:17, Kirill A. Shutemov wrote:
...
>
> - /*
> - * Before changing the encryption attribute, we need to flush caches.
> - */
> - cpa_flush(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT));
> + /* Flush caches as needed before changing the encryption attribute. */
> + if (x86_platform.cc->enc_tlb_flush_required(enc))
> + cpa_flush(&cpa, x86_platform.cc->enc_cache_flush_required());
>
> ret = __change_page_attr_set_clr(&cpa, 1);
>
> @@ -2027,7 +2026,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
> * Notify hypervisor that a given memory range is mapped encrypted
> * or decrypted.
> */
> - notify_range_enc_status_changed(addr, numpages, enc);
> + if (!ret)
> + ret = x86_platform.cc->enc_status_changed(addr, numpages, enc);
>

Boris has given similar comment on SNP series. This area of code have a
very similar requirement. Boris proposed it here [1] and I followed up
[2] with SNP specific comment. It will be good to get your feedback and
do a generic implementation outside of the SNP/TDX series.

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2]
https://lore.kernel.org/linux-mm/[email protected]/T/

thanks

2022-02-22 05:44:46

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Mon, Feb 21, 2022 at 11:28:16AM -0800, Dave Hansen wrote:
> Why don't we just have:
>
> pgprot_t cc_mkenc(pgprot prot)
> pgprot_t cc_mkenc(pgprot prot)
>
> and *no* pgprot_{en,de}crypted()?

Yes, and can the above cc_get_mask() thing be:

cpa.mask_set = enc ? cc_mkenc(0) : cc_mkdec(0);
cpa.mask_clr = enc ? cc_mkdec(0) : cc_mkenc(0);

since we're going to feed it pgprot things?

And then you don't need a separate cc_get_mask() thing or whatever
- there's only those two mkenc and mkdec things converting pgprots
back'n'forth.

> sme_me_mask does look quite AMD-specialized, like its assembly
> manipulation. Even if it's just a copy of cc_mask, it would be nice to
> call that out so the relationship is crystal clear.

Yes, the ultimate goal is to have cc_mask be shared by both AMD and
Intel. We'll phase out sme_me_mask on the AMD side gradually while Intel
can use cc_mask directly.

Methinks.

Thx.

--
Regards/Gruss,
Boris.

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

2022-02-22 07:30:30

by Dingji Li

[permalink] [raw]
Subject: Re: [PATCHv3 08/32] x86/traps: Add #VE support for TDX guest

Hi all,

I hope it is appropriate to ask these questions here:

I'm wondering if there are any performance comparisons available between TDX guests and VMX guests. The #VE processing adds non-trivial overhead to various VM exits, but how does it affect the performance of real-world applications? Existing patches have listed alternative methods to avoid the #VE in the first place, but there are trade-offs (e.g., bloated code, reduced generality). Besides, how much does the time spent in the TDX module affect VM exits / applications? (I guess the TDX module has a low overhead when compared to the #VE processing, but there is no public data.) Maybe some performance data can help make better trade-offs?

Thanks!

--
Best Regards,
Dingji Li


> On Feb 19, 2022, at 12:16 AM, Kirill A. Shutemov <[email protected]> wrote:
>
> Virtualization Exceptions (#VE) are delivered to TDX guests due to
> specific guest actions which may happen in either user space or the
> kernel:
>
> * Specific instructions (WBINVD, for example)
> * Specific MSR accesses
> * Specific CPUID leaf accesses
> * Access to unmapped pages (EPT violation)
>
> In the settings that Linux will run in, virtualization exceptions are
> never generated on accesses to normal, TD-private memory that has been
> accepted.
>
> Syscall entry code has a critical window where the kernel stack is not
> yet set up. Any exception in this window leads to hard to debug issues
> and can be exploited for privilege escalation. Exceptions in the NMI
> entry code also cause issues. Returning from the exception handler with
> IRET will re-enable NMIs and nested NMI will corrupt the NMI stack.
>
> For these reasons, the kernel avoids #VEs during the syscall gap and
> the NMI entry code. Entry code paths do not access TD-shared memory,
> MMIO regions, use #VE triggering MSRs, instructions, or CPUID leaves
> that might generate #VE. VMM can remove memory from TD at any point,
> but access to unaccepted (or missing) private memory leads to VM
> termination, not to #VE.
>
> Similarly to page faults and breakpoints, #VEs are allowed in NMI
> handlers once the kernel is ready to deal with nested NMIs.
>
> During #VE delivery, all interrupts, including NMIs, are blocked until
> TDGETVEINFO is called. It prevents #VE nesting until the kernel reads
> the VE info.
>
> If a guest kernel action which would normally cause a #VE occurs in
> the interrupt-disabled region before TDGETVEINFO, a #DF (fault
> exception) is delivered to the guest which will result in an oops.
>
> Add basic infrastructure to handle any #VE which occurs in the kernel
> or userspace. Later patches will add handling for specific #VE
> scenarios.
>
> For now, convert unhandled #VE's (everything, until later in this
> series) so that they appear just like a #GP by calling the
> ve_raise_fault() directly. The ve_raise_fault() function is similar
> to #GP handler and is responsible for sending SIGSEGV to userspace
> and CPU die and notifying debuggers and other die chain users.
>
> Co-developed-by: Sean Christopherson <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> 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/coco/tdx.c | 60 ++++++++++++++
> arch/x86/include/asm/idtentry.h | 4 +
> arch/x86/include/asm/tdx.h | 21 +++++
> arch/x86/kernel/idt.c | 3 +
> arch/x86/kernel/traps.c | 138 ++++++++++++++++++++++++++------
> 5 files changed, 203 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
> index ea5f02a5d738..de7a02e634c2 100644
> --- a/arch/x86/coco/tdx.c
> +++ b/arch/x86/coco/tdx.c
> @@ -10,6 +10,7 @@
>
> /* TDX module Call Leaf IDs */
> #define TDX_GET_INFO 1
> +#define TDX_GET_VEINFO 3
>
> static struct {
> unsigned int gpa_width;
> @@ -58,6 +59,65 @@ static void get_info(void)
> td_info.attributes = out.rdx;
> }
>
> +void tdx_get_ve_info(struct ve_info *ve)
> +{
> + struct tdx_module_output out;
> +
> + /*
> + * Retrieve the #VE info from the TDX module, which also clears the "#VE
> + * valid" flag. This must be done before anything else as any #VE that
> + * occurs while the valid flag is set, i.e. before the previous #VE info
> + * was consumed, is morphed to a #DF by the TDX module. Note, the TDX
> + * module also treats virtual NMIs as inhibited if the #VE valid flag is
> + * set, e.g. so that NMI=>#VE will not result in a #DF.
> + */
> + tdx_module_call(TDX_GET_VEINFO, 0, 0, 0, 0, &out);
> +
> + ve->exit_reason = out.rcx;
> + ve->exit_qual = out.rdx;
> + ve->gla = out.r8;
> + ve->gpa = out.r9;
> + ve->instr_len = lower_32_bits(out.r10);
> + ve->instr_info = upper_32_bits(out.r10);
> +}
> +
> +/*
> + * Handle the user initiated #VE.
> + *
> + * For example, executing the CPUID instruction from user space
> + * is a valid case and hence the resulting #VE has to be handled.
> + *
> + * For dis-allowed or invalid #VE just return failure.
> + */
> +static bool virt_exception_user(struct pt_regs *regs, struct ve_info *ve)
> +{
> + pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
> + return false;
> +}
> +
> +/* Handle the kernel #VE */
> +static bool virt_exception_kernel(struct pt_regs *regs, struct ve_info *ve)
> +{
> + pr_warn("Unexpected #VE: %lld\n", ve->exit_reason);
> + return false;
> +}
> +
> +bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve)
> +{
> + bool ret;
> +
> + if (user_mode(regs))
> + ret = virt_exception_user(regs, ve);
> + else
> + ret = virt_exception_kernel(regs, ve);
> +
> + /* After successful #VE handling, move the IP */
> + if (ret)
> + regs->ip += ve->instr_len;
> +
> + return ret;
> +}
> +
> void __init tdx_early_init(void)
> {
> u32 eax, sig[3];
> diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
> index 1345088e9902..8ccc81d653b3 100644
> --- a/arch/x86/include/asm/idtentry.h
> +++ b/arch/x86/include/asm/idtentry.h
> @@ -625,6 +625,10 @@ DECLARE_IDTENTRY_XENCB(X86_TRAP_OTHER, exc_xen_hypervisor_callback);
> DECLARE_IDTENTRY_RAW(X86_TRAP_OTHER, exc_xen_unknown_trap);
> #endif
>
> +#ifdef CONFIG_INTEL_TDX_GUEST
> +DECLARE_IDTENTRY(X86_TRAP_VE, exc_virtualization_exception);
> +#endif
> +
> /* Device interrupts common/spurious */
> DECLARE_IDTENTRY_IRQ(X86_TRAP_OTHER, common_interrupt);
> #ifdef CONFIG_X86_LOCAL_APIC
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 557227e40da9..34cf998ad534 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -5,6 +5,7 @@
>
> #include <linux/bits.h>
> #include <linux/init.h>
> +#include <asm/ptrace.h>
>
> #define TDX_CPUID_LEAF_ID 0x21
> #define TDX_IDENT "IntelTDX "
> @@ -47,6 +48,22 @@ struct tdx_hypercall_args {
> u64 r15;
> };
>
> +/*
> + * Used by the #VE exception handler to gather the #VE exception
> + * info from the TDX module. This is a software only structure
> + * and not part of the TDX module/VMM ABI.
> + */
> +struct ve_info {
> + u64 exit_reason;
> + u64 exit_qual;
> + /* Guest Linear (virtual) Address */
> + u64 gla;
> + /* Guest Physical (virtual) Address */
> + u64 gpa;
> + u32 instr_len;
> + u32 instr_info;
> +};
> +
> #ifdef CONFIG_INTEL_TDX_GUEST
>
> void __init tdx_early_init(void);
> @@ -58,6 +75,10 @@ u64 __tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
> /* Used to request services from the VMM */
> u64 __tdx_hypercall(struct tdx_hypercall_args *args, unsigned long flags);
>
> +void tdx_get_ve_info(struct ve_info *ve);
> +
> +bool tdx_handle_virt_exception(struct pt_regs *regs, struct ve_info *ve);
> +
> #else
>
> static inline void tdx_early_init(void) { };
> diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
> index df0fa695bb09..1da074123c16 100644
> --- a/arch/x86/kernel/idt.c
> +++ b/arch/x86/kernel/idt.c
> @@ -68,6 +68,9 @@ static const __initconst struct idt_data early_idts[] = {
> */
> INTG(X86_TRAP_PF, asm_exc_page_fault),
> #endif
> +#ifdef CONFIG_INTEL_TDX_GUEST
> + INTG(X86_TRAP_VE, asm_exc_virtualization_exception),
> +#endif
> };
>
> /*
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 7ef00dee35be..b2510af38158 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -62,6 +62,7 @@
> #include <asm/insn.h>
> #include <asm/insn-eval.h>
> #include <asm/vdso.h>
> +#include <asm/tdx.h>
>
> #ifdef CONFIG_X86_64
> #include <asm/x86_init.h>
> @@ -611,13 +612,43 @@ static bool try_fixup_enqcmd_gp(void)
> #endif
> }
>
> +static bool gp_try_fixup_and_notify(struct pt_regs *regs, int trapnr,
> + unsigned long error_code, const char *str)
> +{
> + int ret;
> +
> + if (fixup_exception(regs, trapnr, error_code, 0))
> + return true;
> +
> + current->thread.error_code = error_code;
> + current->thread.trap_nr = trapnr;
> +
> + /*
> + * To be potentially processing a kprobe fault and to trust the result
> + * from kprobe_running(), we have to be non-preemptible.
> + */
> + if (!preemptible() && kprobe_running() &&
> + kprobe_fault_handler(regs, trapnr))
> + return true;
> +
> + ret = notify_die(DIE_GPF, str, regs, error_code, trapnr, SIGSEGV);
> + return ret == NOTIFY_STOP;
> +}
> +
> +static void gp_user_force_sig_segv(struct pt_regs *regs, int trapnr,
> + unsigned long error_code, const char *str)
> +{
> + current->thread.error_code = error_code;
> + current->thread.trap_nr = trapnr;
> + show_signal(current, SIGSEGV, "", str, regs, error_code);
> + force_sig(SIGSEGV);
> +}
> +
> DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
> {
> char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;
> enum kernel_gp_hint hint = GP_NO_HINT;
> - struct task_struct *tsk;
> unsigned long gp_addr;
> - int ret;
>
> if (user_mode(regs) && try_fixup_enqcmd_gp())
> return;
> @@ -636,40 +667,21 @@ DEFINE_IDTENTRY_ERRORCODE(exc_general_protection)
> return;
> }
>
> - tsk = current;
> -
> if (user_mode(regs)) {
> if (fixup_iopl_exception(regs))
> goto exit;
>
> - tsk->thread.error_code = error_code;
> - tsk->thread.trap_nr = X86_TRAP_GP;
> -
> if (fixup_vdso_exception(regs, X86_TRAP_GP, error_code, 0))
> goto exit;
>
> - show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
> - force_sig(SIGSEGV);
> + gp_user_force_sig_segv(regs, X86_TRAP_GP, error_code, desc);
> goto exit;
> }
>
> if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
> goto exit;
>
> - tsk->thread.error_code = error_code;
> - tsk->thread.trap_nr = X86_TRAP_GP;
> -
> - /*
> - * To be potentially processing a kprobe fault and to trust the result
> - * from kprobe_running(), we have to be non-preemptible.
> - */
> - if (!preemptible() &&
> - kprobe_running() &&
> - kprobe_fault_handler(regs, X86_TRAP_GP))
> - goto exit;
> -
> - ret = notify_die(DIE_GPF, desc, regs, error_code, X86_TRAP_GP, SIGSEGV);
> - if (ret == NOTIFY_STOP)
> + if (gp_try_fixup_and_notify(regs, X86_TRAP_GP, error_code, desc))
> goto exit;
>
> if (error_code)
> @@ -1267,6 +1279,86 @@ DEFINE_IDTENTRY(exc_device_not_available)
> }
> }
>
> +#ifdef CONFIG_INTEL_TDX_GUEST
> +
> +#define VE_FAULT_STR "VE fault"
> +
> +static void ve_raise_fault(struct pt_regs *regs, long error_code)
> +{
> + if (user_mode(regs)) {
> + gp_user_force_sig_segv(regs, X86_TRAP_VE, error_code, VE_FAULT_STR);
> + return;
> + }
> +
> + if (gp_try_fixup_and_notify(regs, X86_TRAP_VE, error_code, VE_FAULT_STR))
> + return;
> +
> + die_addr(VE_FAULT_STR, regs, error_code, 0);
> +}
> +
> +/*
> + * Virtualization Exceptions (#VE) are delivered to TDX guests due to
> + * specific guest actions which may happen in either user space or the
> + * kernel:
> + *
> + * * Specific instructions (WBINVD, for example)
> + * * Specific MSR accesses
> + * * Specific CPUID leaf accesses
> + * * Access to unmapped pages (EPT violation)
> + *
> + * In the settings that Linux will run in, virtualization exceptions are
> + * never generated on accesses to normal, TD-private memory that has been
> + * accepted.
> + *
> + * Syscall entry code has a critical window where the kernel stack is not
> + * yet set up. Any exception in this window leads to hard to debug issues
> + * and can be exploited for privilege escalation. Exceptions in the NMI
> + * entry code also cause issues. Returning from the exception handler with
> + * IRET will re-enable NMIs and nested NMI will corrupt the NMI stack.
> + *
> + * For these reasons, the kernel avoids #VEs during the syscall gap and
> + * the NMI entry code. Entry code paths do not access TD-shared memory,
> + * MMIO regions, use #VE triggering MSRs, instructions, or CPUID leaves
> + * that might generate #VE. VMM can remove memory from TD at any point,
> + * but access to unaccepted (or missing) private memory leads to VM
> + * termination, not to #VE.
> + *
> + * Similarly to page faults and breakpoints, #VEs are allowed in NMI
> + * handlers once the kernel is ready to deal with nested NMIs.
> + *
> + * During #VE delivery, all interrupts, including NMIs, are blocked until
> + * TDGETVEINFO is called. It prevents #VE nesting until the kernel reads
> + * the VE info.
> + *
> + * If a guest kernel action which would normally cause a #VE occurs in
> + * the interrupt-disabled region before TDGETVEINFO, a #DF (fault
> + * exception) is delivered to the guest which will result in an oops.
> + */
> +DEFINE_IDTENTRY(exc_virtualization_exception)
> +{
> + struct ve_info ve;
> +
> + /*
> + * NMIs/Machine-checks/Interrupts will be in a disabled state
> + * till TDGETVEINFO TDCALL is executed. This ensures that VE
> + * info cannot be overwritten by a nested #VE.
> + */
> + tdx_get_ve_info(&ve);
> +
> + cond_local_irq_enable(regs);
> +
> + /*
> + * If tdx_handle_virt_exception() could not process
> + * it successfully, treat it as #GP(0) and handle it.
> + */
> + if (!tdx_handle_virt_exception(regs, &ve))
> + ve_raise_fault(regs, 0);
> +
> + cond_local_irq_disable(regs);
> +}
> +
> +#endif
> +
> #ifdef CONFIG_X86_32
> DEFINE_IDTENTRY_SW(iret_error)
> {
> --
> 2.34.1
>
>

2022-02-22 12:08:50

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Tue, Feb 22, 2022 at 01:28:56AM +0300, Kirill A. Shutemov wrote:
> On Mon, Feb 21, 2022 at 11:28:16AM -0800, Dave Hansen wrote:
> > I'm just a bit confused why *this* was chosen as the cc_whatever() hook.
> > Just like the mask function, it has one spot where it gets used:
> >
> > +#define pgprot_encrypted(prot) __pgprot(cc_mkenc(pgprot_val(prot)))
> > +#define pgprot_decrypted(prot) __pgprot(cc_mkdec(pgprot_val(prot)))
> >
> > So, why bother having another level of abstraction?
> >
> > Why don't we just have:
> >
> > pgprot_t cc_mkenc(pgprot prot)
> > pgprot_t cc_mkenc(pgprot prot)
> >
> > and *no* pgprot_{en,de}crypted()?
>
> Okay. Let me try this.

Below it how it would look like.

I don't like it, especially, tdx_enc_status_changed() case. There are more
cases like this in attestation code.

I would rather make cc_mkenc()/cc_mkdec() to operate on u64 (or
phys_addr_t?) while pgprot_encrypted()/pgprot_decrypted() cover pgprot_t.
It also makes set_memory cleaner:

cpa.mask_set = __pgprot(enc ? cc_mkenc(0) : cc_mkdec(0));
cpa.mask_clr = __pgprot(enc ? cc_mkdec(0) : cc_mkenc(0));

Opinions?

diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
index fb32c271fe34..bf5ba7d5b736 100644
--- a/arch/x86/coco/tdx.c
+++ b/arch/x86/coco/tdx.c
@@ -514,8 +514,8 @@ static int tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
return -EINVAL;

if (!enc) {
- start = cc_mkenc(start);
- end = cc_mkenc(end);
+ start |= pgprot_val(cc_mkenc(__pgprot(0)));
+ end |= pgprot_val(cc_mkenc(__pgprot(0)));
}

/*
diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index 802d87d08e31..fba1dd2ef319 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -14,13 +14,18 @@ enum cc_vendor {
void cc_init(enum cc_vendor, u64 mask);

#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
-u64 cc_get_mask(bool enc);
-u64 cc_mkenc(u64 val);
-u64 cc_mkdec(u64 val);
+pgprot_t cc_mkenc(pgprot_t prot);
+pgprot_t cc_mkdec(pgprot_t prot);
#else
-#define cc_get_mask(enc) 0
-#define cc_mkenc(val) (val)
-#define cc_mkdec(val) (val)
+static inline pgprot_t cc_mkenc(pgprot_t prot)
+{
+ return prot;
+}
+
+static inline pgprot_t cc_mkdec(pgprot_t prot)
+{
+ return prot;
+}
#endif

#endif /* _ASM_X86_COCO_H */
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 62ab07e24aef..8cba0d258e49 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -36,8 +36,8 @@ void ptdump_walk_user_pgd_level_checkwx(void);
/*
* Macros to add or remove encryption attribute
*/
-#define pgprot_encrypted(prot) __pgprot(cc_mkenc(pgprot_val(prot)))
-#define pgprot_decrypted(prot) __pgprot(cc_mkdec(pgprot_val(prot)))
+#define pgprot_encrypted(prot) cc_mkenc(prot)
+#define pgprot_decrypted(prot) cc_mkdec(prot)

#ifdef CONFIG_DEBUG_WX
#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
index 12a34e80ef50..d13a034e9a21 100644
--- a/arch/x86/kernel/cc_platform.c
+++ b/arch/x86/kernel/cc_platform.c
@@ -12,6 +12,7 @@

#include <asm/coco.h>
#include <asm/processor.h>
+#include <asm/pgtable_types.h>

static u64 cc_mask;
static enum cc_vendor cc_vendor;
@@ -93,39 +94,27 @@ bool cc_platform_has(enum cc_attr attr)
}
EXPORT_SYMBOL_GPL(cc_platform_has);

-u64 cc_get_mask(bool enc)
+pgprot_t cc_mkenc(pgprot_t prot)
{
switch (cc_vendor) {
case CC_VENDOR_AMD:
- return enc ? cc_mask : 0;
+ return __pgprot(pgprot_val(prot) | cc_mask);
case CC_VENDOR_INTEL:
- return enc ? 0 : cc_mask;
+ return __pgprot(pgprot_val(prot) & ~cc_mask);
default:
- return 0;
+ return prot;
}
}

-u64 cc_mkenc(u64 val)
+pgprot_t cc_mkdec(pgprot_t prot)
{
switch (cc_vendor) {
case CC_VENDOR_AMD:
- return val | cc_mask;
+ return __pgprot(pgprot_val(prot) & ~cc_mask);
case CC_VENDOR_INTEL:
- return val & ~cc_mask;
+ return __pgprot(pgprot_val(prot) | cc_mask);
default:
- return val;
- }
-}
-
-u64 cc_mkdec(u64 val)
-{
- switch (cc_vendor) {
- case CC_VENDOR_AMD:
- return val & ~cc_mask;
- case CC_VENDOR_INTEL:
- return val | cc_mask;
- default:
- return val;
+ return prot;
}
}
EXPORT_SYMBOL_GPL(cc_mkdec);
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 65a27dec095f..1ef63ea377dd 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -1999,8 +1999,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
memset(&cpa, 0, sizeof(cpa));
cpa.vaddr = &addr;
cpa.numpages = numpages;
- cpa.mask_set = __pgprot(cc_get_mask(enc));
- cpa.mask_clr = __pgprot(cc_get_mask(!enc));
+ cpa.mask_set = enc ? cc_mkenc(__pgprot(0)) : cc_mkdec(__pgprot(0));
+ cpa.mask_clr = enc ? cc_mkdec(__pgprot(0)) : cc_mkenc(__pgprot(0));
cpa.pgd = init_mm.pgd;

/* Must avoid aliasing mappings in the highmem code */
--
Kirill A. Shutemov

2022-02-22 12:59:42

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3 08/32] x86/traps: Add #VE support for TDX guest

On Tue, Feb 22, 2022 at 03:19:47PM +0800, Dingji Li wrote:
> Hi all,
>
> I hope it is appropriate to ask these questions here:
>
> I'm wondering if there are any performance comparisons available between
> TDX guests and VMX guests. The #VE processing adds non-trivial overhead
> to various VM exits, but how does it affect the performance of
> real-world applications? Existing patches have listed alternative
> methods to avoid the #VE in the first place, but there are trade-offs
> (e.g., bloated code, reduced generality). Besides, how much does the
> time spent in the TDX module affect VM exits / applications? (I guess
> the TDX module has a low overhead when compared to the #VE processing,
> but there is no public data.) Maybe some performance data can help make
> better trade-offs?

This is basic enabling of TDX guest support. The goal is to make TDX guest
functional. Yes, #VE handling adds non-trivial overhead and we have plan
to migrate it: there are patches in the queue that help to avoid bulk of
#VE, like replacing #VE-based MMIO with direct hypercalls. TDX will still
have performance penalty over plain VMX no matter what, but we aim to
minimize it.

I don't have any performance numbers to share at the moment.

--
Kirill A. Shutemov

2022-02-22 13:53:01

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Tue, Feb 22, 2022 at 02:03:12PM +0300, Kirill A. Shutemov wrote:
> I would rather make cc_mkenc()/cc_mkdec() to operate on u64 (or
> phys_addr_t?) while pgprot_encrypted()/pgprot_decrypted() cover pgprot_t.
> It also makes set_memory cleaner:
>
> cpa.mask_set = __pgprot(enc ? cc_mkenc(0) : cc_mkdec(0));
> cpa.mask_clr = __pgprot(enc ? cc_mkdec(0) : cc_mkenc(0));
>
> Opinions?

Right, do I see it correctly that the cc_mk{enc,dec}() things should
take a u64 as an argument and return a pgprot_t, and that would be the
most optimal way for all the use cases?

--
Regards/Gruss,
Boris.

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

2022-02-22 14:18:34

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Tue, Feb 22, 2022 at 02:18:31AM +0300, Kirill A. Shutemov wrote:
> On Mon, Feb 21, 2022 at 11:56:00PM +0100, Borislav Petkov wrote:
> > On Tue, Feb 22, 2022 at 01:21:49AM +0300, Kirill A. Shutemov wrote:
> > > Well, it actually going to be
> > >
> > > cpa.mask_set = enc ? cc_mkenc(__pgprot(0)) : cc_mkdec(__pgprot(0));
> > > cpa.mask_clr = enc ? cc_mkdec(__pgprot(0)) : cc_mkenc(__pgprot(0));
> > >
> > > as '0' is not a valid pgprot_t.
> > >
> > > Still wonna go this path?
> >
> > Why "still"? What's wrong with that?
>
> IMO, it makes these statement substantially uglier.

As opposed to what you had:

+ cpa.mask_set = __pgprot(cc_get_mask(enc));
+ cpa.mask_clr = __pgprot(cc_get_mask(!enc));

?

Sorry, but cc_get_mask() - first, the name is very misleading - and then
a function argument saying what mask to return is more confusing.

The fact that each vendor chose alternating representations of what an
encrypted page means needs to be abstracted away - the API should not
ask the user of the function what mask she wants. Your functions need
to return an encrypted mask or a decripted mask, not "hey, what kind of
mask do you want".

If you want to make it even simpler, you can hide the pgprot creation
inside the function even - I'm looking at how pgprot_nx() is defined:

cpa.mask_set = enc ? pgprot_enc(0) : pgprot_dec(0);
cpa.mask_clr = enc ? pgprot_dec(0) : pgprot_enc(0);

Or, if you think this is still not readable enough, you carve it out
into a separate function:

cpa_set_masks(struct cpa_data *cpa, bool enc);

and go to town there, do comments, do pgprot conversion per-hand,
whatever.

*BUT* it all depends on what your full requirements for those functions
are for how these masks are going to be used throughout the tree. So I'm
guessing a usage analysis will give you the proper design.

Thx.

--
Regards/Gruss,
Boris.

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

2022-02-22 18:00:58

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv3 02/32] x86/coco: Add API to handle encryption mask

On Tue, Feb 22, 2022 at 02:37:27PM +0100, Borislav Petkov wrote:
> On Tue, Feb 22, 2022 at 02:03:12PM +0300, Kirill A. Shutemov wrote:
> > I would rather make cc_mkenc()/cc_mkdec() to operate on u64 (or
> > phys_addr_t?) while pgprot_encrypted()/pgprot_decrypted() cover pgprot_t.
> > It also makes set_memory cleaner:
> >
> > cpa.mask_set = __pgprot(enc ? cc_mkenc(0) : cc_mkdec(0));
> > cpa.mask_clr = __pgprot(enc ? cc_mkdec(0) : cc_mkenc(0));
> >
> > Opinions?
>
> Right, do I see it correctly that the cc_mk{enc,dec}() things should
> take a u64 as an argument and return a pgprot_t, and that would be the
> most optimal way for all the use cases?

No, not really. With u64-in-u64-out in tdx_enc_status_changed() we have

if (!enc) {
start |= cc_mkdec(0);
end |= cc_mkdec(0);
}

to iterate over the range of physical addresses with shared bit set.
With u64-in-pgprot_t-out we will have do add pgprot_val() there.

We will have more cases like this in attestation code when we need to do
hypercall on a shared page.

--
Kirill A. Shutemov