The patchset adds bits and pieces to get kexec (and crashkernel) work on
TDX guest.
The last patch implements CPU offlining according to the approved ACPI
spec change poposal[1]. It unlocks kexec with all CPUs visible in the target
kernel. It requires BIOS-side enabling. If it missing we fallback to booting
2nd kernel with single CPU.
Please review. I would be glad for any feedback.
[1] https://lore.kernel.org/all/13356251.uLZWGnKmhe@kreacher
v5:
- Rename smp_ops.crash_play_dead to smp_ops.stop_this_cpu and use it in
stop_this_cpu();
- Split off enc_kexec_stop_conversion() from enc_kexec_unshare_mem();
- Introduce kernel_ident_mapping_free();
- Add explicit include for alternatives and stringify.
- Add barrier() after setting conversion_allowed to false;
- Mark cpu_hotplug_offline_disabled __ro_after_init;
- Print error if failed to hand over CPU to BIOS;
- Update comments and commit messages;
v4:
- Fix build for !KEXEC_CORE;
- Cleaner ATLERNATIVE use;
- Update commit messages and comments;
- Add Reviewed-bys;
v3:
- Rework acpi_mp_crash_stop_other_cpus() to avoid invoking hotplug state
machine;
- Free page tables if reset vector setup failed;
- Change asm_acpi_mp_play_dead() to pass reset vector and PGD as arguments;
- Mark acpi_mp_* variables as static and __ro_after_init;
- Use u32 for apicid;
- Disable CPU offlining if reset vector setup failed;
- Rename madt.S -> madt_playdead.S;
- Mark tdx_kexec_unshare_mem() as static;
- Rebase onto up-to-date tip/master;
- Whitespace fixes;
- Reorder patches;
- Add Reviewed-bys;
- Update comments and commit messages;
v2:
- Rework how unsharing hook ups into kexec codepath;
- Rework kvmclock_disable() fix based on Sean's;
- s/cpu_hotplug_not_supported()/cpu_hotplug_disable_offlining()/;
- use play_dead_common() to implement acpi_mp_play_dead();
- cond_resched() in tdx_shared_memory_show();
- s/target kernel/second kernel/;
- Update commit messages and comments;
Kirill A. Shutemov (16):
x86/acpi: Extract ACPI MADT wakeup code into a separate file
x86/apic: Mark acpi_mp_wake_* variables as __ro_after_init
cpu/hotplug: Add support for declaring CPU offlining not supported
cpu/hotplug, x86/acpi: Disable CPU offlining for ACPI MADT wakeup
x86/kvm: Do not try to disable kvmclock if it was not enabled
x86/kexec: Keep CR4.MCE set during kexec for TDX guest
x86/mm: Make x86_platform.guest.enc_status_change_*() return errno
x86/mm: Return correct level from lookup_address() if pte is none
x86/tdx: Account shared memory
x86/tdx: Convert shared memory back to private on kexec
x86/mm: Make e820_end_ram_pfn() cover E820_TYPE_ACPI ranges
x86/acpi: Rename fields in acpi_madt_multiproc_wakeup structure
x86/acpi: Do not attempt to bring up secondary CPUs in kexec case
x86/smp: Add smp_ops.stop_this_cpu() callback
x86/mm: Introduce kernel_ident_mapping_free()
x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method
arch/x86/Kconfig | 7 +
arch/x86/coco/core.c | 1 -
arch/x86/coco/tdx/tdx.c | 204 ++++++++++++++++++-
arch/x86/hyperv/ivm.c | 9 +-
arch/x86/include/asm/acpi.h | 7 +
arch/x86/include/asm/init.h | 3 +
arch/x86/include/asm/pgtable_types.h | 1 +
arch/x86/include/asm/smp.h | 1 +
arch/x86/include/asm/x86_init.h | 6 +-
arch/x86/kernel/acpi/Makefile | 11 +-
arch/x86/kernel/acpi/boot.c | 86 +-------
arch/x86/kernel/acpi/madt_playdead.S | 29 +++
arch/x86/kernel/acpi/madt_wakeup.c | 292 +++++++++++++++++++++++++++
arch/x86/kernel/crash.c | 6 +
arch/x86/kernel/e820.c | 9 +-
arch/x86/kernel/kvmclock.c | 12 +-
arch/x86/kernel/process.c | 20 +-
arch/x86/kernel/reboot.c | 25 ++-
arch/x86/kernel/relocate_kernel_64.S | 5 +
arch/x86/kernel/x86_init.c | 4 +-
arch/x86/mm/ident_map.c | 73 +++++++
arch/x86/mm/mem_encrypt_amd.c | 8 +-
arch/x86/mm/pat/set_memory.c | 17 +-
include/acpi/actbl2.h | 19 +-
include/linux/cc_platform.h | 10 -
include/linux/cpu.h | 2 +
kernel/cpu.c | 12 +-
27 files changed, 724 insertions(+), 155 deletions(-)
create mode 100644 arch/x86/kernel/acpi/madt_playdead.S
create mode 100644 arch/x86/kernel/acpi/madt_wakeup.c
--
2.41.0
The kernel will convert all shared memory back to private during kexec.
The direct mapping page tables will provide information on which memory
is shared.
It is extremely important to convert all shared memory. If a page is
missed, it will cause the second kernel to crash when it accesses it.
Keep track of the number of shared pages. This will allow for
cross-checking against the shared information in the direct mapping and
reporting if the shared bit is lost.
Include a debugfs interface that allows for the check to be performed at
any point.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/coco/tdx/tdx.c | 69 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 4397a6b2b04c..8a49484a2917 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -5,6 +5,7 @@
#define pr_fmt(fmt) "tdx: " fmt
#include <linux/cpufeature.h>
+#include <linux/debugfs.h>
#include <linux/export.h>
#include <linux/io.h>
#include <asm/coco.h>
@@ -38,6 +39,13 @@
#define TDREPORT_SUBTYPE_0 0
+static atomic_long_t nr_shared;
+
+static inline bool pte_decrypted(pte_t pte)
+{
+ return cc_mkdec(pte_val(pte)) == pte_val(pte);
+}
+
/* Called from __tdx_hypercall() for unrecoverable failure */
noinstr void __noreturn __tdx_hypercall_failed(void)
{
@@ -821,6 +829,11 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
return -EIO;
+ if (enc)
+ atomic_long_sub(numpages, &nr_shared);
+ else
+ atomic_long_add(numpages, &nr_shared);
+
return 0;
}
@@ -896,3 +909,59 @@ void __init tdx_early_init(void)
pr_info("Guest detected\n");
}
+
+#ifdef CONFIG_DEBUG_FS
+static int tdx_shared_memory_show(struct seq_file *m, void *p)
+{
+ unsigned long addr, end;
+ unsigned long found = 0;
+
+ addr = PAGE_OFFSET;
+ end = PAGE_OFFSET + get_max_mapped();
+
+ while (addr < end) {
+ unsigned long size;
+ unsigned int level;
+ pte_t *pte;
+
+ pte = lookup_address(addr, &level);
+ size = page_level_size(level);
+
+ if (pte && pte_decrypted(*pte))
+ found += size / PAGE_SIZE;
+
+ addr += size;
+
+ cond_resched();
+ }
+
+ seq_printf(m, "Number of shared pages in kernel page tables: %16lu\n",
+ found);
+ seq_printf(m, "Number of pages accounted as shared: %16ld\n",
+ atomic_long_read(&nr_shared));
+ return 0;
+}
+
+static int tdx_shared_memory_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, tdx_shared_memory_show, NULL);
+}
+
+static const struct file_operations tdx_shared_memory_fops = {
+ .open = tdx_shared_memory_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static __init int debug_tdx_shared_memory(void)
+{
+ if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return 0;
+
+ debugfs_create_file("tdx_shared_memory", 0400, arch_debugfs_dir,
+ NULL, &tdx_shared_memory_fops);
+ return 0;
+}
+fs_initcall(debug_tdx_shared_memory);
+#endif
--
2.41.0
TDX is going to have more than one reason to fail
enc_status_change_prepare().
Change the callback to return errno instead of assuming -EIO;
enc_status_change_finish() changed too to keep the interface symmetric.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/coco/tdx/tdx.c | 20 +++++++++++---------
arch/x86/hyperv/ivm.c | 9 +++------
arch/x86/include/asm/x86_init.h | 4 ++--
arch/x86/kernel/x86_init.c | 4 ++--
arch/x86/mm/mem_encrypt_amd.c | 8 ++++----
arch/x86/mm/pat/set_memory.c | 9 +++++----
6 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index cf1f13c82175..4397a6b2b04c 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -798,28 +798,30 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
return true;
}
-static bool tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
- bool enc)
+static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
+ bool enc)
{
/*
* Only handle shared->private conversion here.
* See the comment in tdx_early_init().
*/
- if (enc)
- return tdx_enc_status_changed(vaddr, numpages, enc);
- return true;
+ if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
+ return -EIO;
+
+ return 0;
}
-static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
+static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
bool enc)
{
/*
* Only handle private->shared conversion here.
* See the comment in tdx_early_init().
*/
- if (!enc)
- return tdx_enc_status_changed(vaddr, numpages, enc);
- return true;
+ if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
+ return -EIO;
+
+ return 0;
}
void __init tdx_early_init(void)
diff --git a/arch/x86/hyperv/ivm.c b/arch/x86/hyperv/ivm.c
index 02e55237d919..2e1be1afeebe 100644
--- a/arch/x86/hyperv/ivm.c
+++ b/arch/x86/hyperv/ivm.c
@@ -510,13 +510,12 @@ static int hv_mark_gpa_visibility(u16 count, const u64 pfn[],
* with host. This function works as wrap of hv_mark_gpa_visibility()
* with memory base and size.
*/
-static bool hv_vtom_set_host_visibility(unsigned long kbuffer, int pagecount, bool enc)
+static int hv_vtom_set_host_visibility(unsigned long kbuffer, int pagecount, bool enc)
{
enum hv_mem_host_visibility visibility = enc ?
VMBUS_PAGE_NOT_VISIBLE : VMBUS_PAGE_VISIBLE_READ_WRITE;
u64 *pfn_array;
int ret = 0;
- bool result = true;
int i, pfn;
pfn_array = kmalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
@@ -530,17 +529,15 @@ static bool hv_vtom_set_host_visibility(unsigned long kbuffer, int pagecount, bo
if (pfn == HV_MAX_MODIFY_GPA_REP_COUNT || i == pagecount - 1) {
ret = hv_mark_gpa_visibility(pfn, pfn_array,
visibility);
- if (ret) {
- result = false;
+ if (ret)
goto err_free_pfn_array;
- }
pfn = 0;
}
}
err_free_pfn_array:
kfree(pfn_array);
- return result;
+ return ret;
}
static bool hv_vtom_tlb_flush_required(bool private)
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index c878616a18b8..c9503fe2d13a 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -150,8 +150,8 @@ struct x86_init_acpi {
* @enc_cache_flush_required Returns true if a cache flush is needed before changing page encryption status
*/
struct x86_guest {
- bool (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc);
- bool (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
+ int (*enc_status_change_prepare)(unsigned long vaddr, int npages, bool enc);
+ int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
bool (*enc_tlb_flush_required)(bool enc);
bool (*enc_cache_flush_required)(void);
};
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index a37ebd3b4773..f0f54e109eb9 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -131,8 +131,8 @@ struct x86_cpuinit_ops x86_cpuinit = {
static void default_nmi_init(void) { };
-static bool enc_status_change_prepare_noop(unsigned long vaddr, int npages, bool enc) { return true; }
-static bool enc_status_change_finish_noop(unsigned long vaddr, int npages, bool enc) { return true; }
+static int enc_status_change_prepare_noop(unsigned long vaddr, int npages, bool enc) { return 0; }
+static int enc_status_change_finish_noop(unsigned long vaddr, int npages, bool enc) { return 0; }
static bool enc_tlb_flush_required_noop(bool enc) { return false; }
static bool enc_cache_flush_required_noop(void) { return false; }
static bool is_private_mmio_noop(u64 addr) {return false; }
diff --git a/arch/x86/mm/mem_encrypt_amd.c b/arch/x86/mm/mem_encrypt_amd.c
index 70b91de2e053..d314e577836d 100644
--- a/arch/x86/mm/mem_encrypt_amd.c
+++ b/arch/x86/mm/mem_encrypt_amd.c
@@ -283,7 +283,7 @@ static void enc_dec_hypercall(unsigned long vaddr, unsigned long size, bool enc)
#endif
}
-static bool amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc)
+static int amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool enc)
{
/*
* To maintain the security guarantees of SEV-SNP guests, make sure
@@ -292,11 +292,11 @@ static bool amd_enc_status_change_prepare(unsigned long vaddr, int npages, bool
if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && !enc)
snp_set_memory_shared(vaddr, npages);
- return true;
+ return 0;
}
/* Return true unconditionally: return value doesn't matter for the SEV side */
-static bool amd_enc_status_change_finish(unsigned long vaddr, int npages, bool enc)
+static int amd_enc_status_change_finish(unsigned long vaddr, int npages, bool enc)
{
/*
* After memory is mapped encrypted in the page table, validate it
@@ -308,7 +308,7 @@ static bool amd_enc_status_change_finish(unsigned long vaddr, int npages, bool e
if (!cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
enc_dec_hypercall(vaddr, npages << PAGE_SHIFT, enc);
- return true;
+ return 0;
}
static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc)
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index bda9f129835e..6fbf22d5fa56 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -2152,8 +2152,9 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
cpa_flush(&cpa, x86_platform.guest.enc_cache_flush_required());
/* Notify hypervisor that we are about to set/clr encryption attribute. */
- if (!x86_platform.guest.enc_status_change_prepare(addr, numpages, enc))
- return -EIO;
+ ret = x86_platform.guest.enc_status_change_prepare(addr, numpages, enc);
+ if (ret)
+ return ret;
ret = __change_page_attr_set_clr(&cpa, 1);
@@ -2168,8 +2169,8 @@ static int __set_memory_enc_pgtable(unsigned long addr, int numpages, bool enc)
/* Notify hypervisor that we have successfully set/clr encryption attribute. */
if (!ret) {
- if (!x86_platform.guest.enc_status_change_finish(addr, numpages, enc))
- ret = -EIO;
+ ret = x86_platform.guest.enc_status_change_finish(addr,
+ numpages, enc);
}
return ret;
--
2.41.0
lookup_address() only returns correct page table level for the entry if
the entry is not none.
Make the helper to always return correct 'level'. It allows to implement
iterator over kernel page tables using lookup_address().
Add one more entry into enum pg_level to indicate size of VA covered by
one PGD entry in 5-level paging mode.
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Rick Edgecombe <[email protected]>
---
arch/x86/include/asm/pgtable_types.h | 1 +
arch/x86/mm/pat/set_memory.c | 8 ++++----
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 0b748ee16b3d..3f648ffdfbe5 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -548,6 +548,7 @@ enum pg_level {
PG_LEVEL_2M,
PG_LEVEL_1G,
PG_LEVEL_512G,
+ PG_LEVEL_256T,
PG_LEVEL_NUM
};
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 6fbf22d5fa56..01f827eb8e80 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -666,32 +666,32 @@ pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long address,
pud_t *pud;
pmd_t *pmd;
- *level = PG_LEVEL_NONE;
+ *level = PG_LEVEL_256T;
if (pgd_none(*pgd))
return NULL;
+ *level = PG_LEVEL_512G;
p4d = p4d_offset(pgd, address);
if (p4d_none(*p4d))
return NULL;
- *level = PG_LEVEL_512G;
if (p4d_large(*p4d) || !p4d_present(*p4d))
return (pte_t *)p4d;
+ *level = PG_LEVEL_1G;
pud = pud_offset(p4d, address);
if (pud_none(*pud))
return NULL;
- *level = PG_LEVEL_1G;
if (pud_large(*pud) || !pud_present(*pud))
return (pte_t *)pud;
+ *level = PG_LEVEL_2M;
pmd = pmd_offset(pud, address);
if (pmd_none(*pmd))
return NULL;
- *level = PG_LEVEL_2M;
if (pmd_large(*pmd) || !pmd_present(*pmd))
return (pte_t *)pmd;
--
2.41.0
To prepare for the addition of support for MADT wakeup structure version
1, it is necessary to provide more appropriate names for the fields in
the structure.
The field 'mailbox_version' renamed as 'version'. This field signifies
the version of the structure and the related protocols, rather than the
version of the mailbox. This field has not been utilized in the code
thus far.
The field 'base_address' renamed as 'mailbox_address' to clarify the
kind of address it represents. In version 1, the structure includes the
reset vector address. Clear and distinct naming helps to prevent any
confusion.
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Kai Huang <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
arch/x86/kernel/acpi/madt_wakeup.c | 2 +-
include/acpi/actbl2.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index d222be8d7a07..004801b9b151 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -75,7 +75,7 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
acpi_table_print_madt_entry(&header->common);
- acpi_mp_wake_mailbox_paddr = mp_wake->base_address;
+ acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
cpu_hotplug_disable_offlining();
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 3751ae69432f..23b4cfb640fc 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -1109,9 +1109,9 @@ struct acpi_madt_generic_translator {
struct acpi_madt_multiproc_wakeup {
struct acpi_subtable_header header;
- u16 mailbox_version;
+ u16 version;
u32 reserved; /* reserved - must be zero */
- u64 base_address;
+ u64 mailbox_address;
};
#define ACPI_MULTIPROC_WAKEUP_MB_OS_SIZE 2032
--
2.41.0
ACPI MADT doesn't allow to offline a CPU after it was onlined. This
limits kexec: the second kernel won't be able to use more than one CPU.
To prevent a kexec kernel from onlining secondary CPUs invalidate the
mailbox address in the ACPI MADT wakeup structure which prevents a
kexec kernel to use it.
This is safe as the booting kernel has the mailbox address cached
already and acpi_wakeup_cpu() uses the cached value to bring up the
secondary CPUs.
Note: This is a Linux specific convention and not covered by the
ACPI specification.
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Kai Huang <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
arch/x86/kernel/acpi/madt_wakeup.c | 29 ++++++++++++++++++++++++++++-
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index 004801b9b151..30820f9de5af 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -14,6 +14,11 @@ static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox __ro_afte
static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
{
+ if (!acpi_mp_wake_mailbox_paddr) {
+ pr_warn_once("No MADT mailbox: cannot bringup secondary CPUs. Booting with kexec?\n");
+ return -EOPNOTSUPP;
+ }
+
/*
* Remap mailbox memory only for the first call to acpi_wakeup_cpu().
*
@@ -64,6 +69,28 @@ static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
return 0;
}
+static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake)
+{
+ cpu_hotplug_disable_offlining();
+
+ /*
+ * ACPI MADT doesn't allow to offline a CPU after it was onlined. This
+ * limits kexec: the second kernel won't be able to use more than one CPU.
+ *
+ * To prevent a kexec kernel from onlining secondary CPUs invalidate the
+ * mailbox address in the ACPI MADT wakeup structure which prevents a
+ * kexec kernel to use it.
+ *
+ * This is safe as the booting kernel has the mailbox address cached
+ * already and acpi_wakeup_cpu() uses the cached value to bring up the
+ * secondary CPUs.
+ *
+ * Note: This is a Linux specific convention and not covered by the
+ * ACPI specification.
+ */
+ mp_wake->mailbox_address = 0;
+}
+
int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
const unsigned long end)
{
@@ -77,7 +104,7 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
- cpu_hotplug_disable_offlining();
+ acpi_mp_disable_offlining(mp_wake);
apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
--
2.41.0
If the helper is defined, it is called instead of halt() to stop the CPU
at the end of stop_this_cpu() and on crash CPU shutdown.
ACPI MADT will use it to hand over the CPU to BIOS in order to be able
to wake it up again after kexec.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/smp.h | 1 +
arch/x86/kernel/process.c | 20 +++++++++++++-------
arch/x86/kernel/reboot.c | 12 ++++++++----
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 4fab2ed454f3..390d53fd34f9 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -38,6 +38,7 @@ struct smp_ops {
int (*cpu_disable)(void);
void (*cpu_die)(unsigned int cpu);
void (*play_dead)(void);
+ void (*stop_this_cpu)(void);
void (*send_call_func_ipi)(const struct cpumask *mask);
void (*send_call_func_single_ipi)(int cpu);
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index b6f4e8399fca..65c7971defd2 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -835,13 +835,19 @@ void __noreturn stop_this_cpu(void *dummy)
*/
cpumask_clear_cpu(cpu, &cpus_stop_mask);
- for (;;) {
- /*
- * Use native_halt() so that memory contents don't change
- * (stack usage and variables) after possibly issuing the
- * native_wbinvd() above.
- */
- native_halt();
+ if (smp_ops.stop_this_cpu) {
+ smp_ops.stop_this_cpu();
+ unreachable();
+ } else {
+
+ for (;;) {
+ /*
+ * Use native_halt() so that memory contents don't
+ * change (stack usage and variables) after possibly
+ * issuing the native_wbinvd() above.
+ */
+ native_halt();
+ }
}
}
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 16dde83df49a..738b3e810196 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -881,10 +881,14 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
cpu_emergency_disable_virtualization();
atomic_dec(&waiting_for_crash_ipi);
- /* Assume hlt works */
- halt();
- for (;;)
- cpu_relax();
+
+ if (smp_ops.stop_this_cpu) {
+ smp_ops.stop_this_cpu();
+ } else {
+ halt();
+ for (;;)
+ cpu_relax();
+ }
return NMI_HANDLED;
}
--
2.41.0
The helper complements kernel_ident_mapping_init(): it frees the
identity mapping that was previously allocated. It will be used in the
error path to free a partially allocated mapping or if the mapping is no
longer needed.
The caller provides a struct x86_mapping_info with the free_pgd_page()
callback hooked up and the pgd_t to free.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/include/asm/init.h | 3 ++
arch/x86/mm/ident_map.c | 73 +++++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+)
diff --git a/arch/x86/include/asm/init.h b/arch/x86/include/asm/init.h
index cc9ccf61b6bd..14d72727d7ee 100644
--- a/arch/x86/include/asm/init.h
+++ b/arch/x86/include/asm/init.h
@@ -6,6 +6,7 @@
struct x86_mapping_info {
void *(*alloc_pgt_page)(void *); /* allocate buf for page table */
+ void (*free_pgt_page)(void *, void *); /* free buf for page table */
void *context; /* context for alloc_pgt_page */
unsigned long page_flag; /* page flag for PMD or PUD entry */
unsigned long offset; /* ident mapping offset */
@@ -16,4 +17,6 @@ struct x86_mapping_info {
int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
unsigned long pstart, unsigned long pend);
+void kernel_ident_mapping_free(struct x86_mapping_info *info, pgd_t *pgd);
+
#endif /* _ASM_X86_INIT_H */
diff --git a/arch/x86/mm/ident_map.c b/arch/x86/mm/ident_map.c
index 968d7005f4a7..3996af7b4abf 100644
--- a/arch/x86/mm/ident_map.c
+++ b/arch/x86/mm/ident_map.c
@@ -4,6 +4,79 @@
* included by both the compressed kernel and the regular kernel.
*/
+static void free_pte(struct x86_mapping_info *info, pmd_t *pmd)
+{
+ pte_t *pte = pte_offset_kernel(pmd, 0);
+
+ info->free_pgt_page(pte, info->context);
+}
+
+static void free_pmd(struct x86_mapping_info *info, pud_t *pud)
+{
+ pmd_t *pmd = pmd_offset(pud, 0);
+ int i;
+
+ for (i = 0; i < PTRS_PER_PMD; i++) {
+ if (!pmd_present(pmd[i]))
+ continue;
+
+ if (pmd_leaf(pmd[i]))
+ continue;
+
+ free_pte(info, &pmd[i]);
+ }
+
+ info->free_pgt_page(pmd, info->context);
+}
+
+static void free_pud(struct x86_mapping_info *info, p4d_t *p4d)
+{
+ pud_t *pud = pud_offset(p4d, 0);
+ int i;
+
+ for (i = 0; i < PTRS_PER_PUD; i++) {
+ if (!pud_present(pud[i]))
+ continue;
+
+ if (pud_leaf(pud[i]))
+ continue;
+
+ free_pmd(info, &pud[i]);
+ }
+
+ info->free_pgt_page(pud, info->context);
+}
+
+static void free_p4d(struct x86_mapping_info *info, pgd_t *pgd)
+{
+ p4d_t *p4d = p4d_offset(pgd, 0);
+ int i;
+
+ for (i = 0; i < PTRS_PER_P4D; i++) {
+ if (!p4d_present(p4d[i]))
+ continue;
+
+ free_pud(info, &p4d[i]);
+ }
+
+ if (pgtable_l5_enabled())
+ info->free_pgt_page(pgd, info->context);
+}
+
+void kernel_ident_mapping_free(struct x86_mapping_info *info, pgd_t *pgd)
+{
+ int i;
+
+ for (i = 0; i < PTRS_PER_PGD; i++) {
+ if (!pgd_present(pgd[i]))
+ continue;
+
+ free_p4d(info, &pgd[i]);
+ }
+
+ info->free_pgt_page(pgd, info->context);
+}
+
static void ident_pmd_init(struct x86_mapping_info *info, pmd_t *pmd_page,
unsigned long addr, unsigned long end)
{
--
2.41.0
kvm_guest_cpu_offline() tries to disable kvmclock regardless if it is
present in the VM. It leads to write to a MSR that doesn't exist on some
configurations, namely in TDX guest:
unchecked MSR access error: WRMSR to 0x12 (tried to write 0x0000000000000000)
at rIP: 0xffffffff8110687c (kvmclock_disable+0x1c/0x30)
kvmclock enabling is gated by CLOCKSOURCE and CLOCKSOURCE2 KVM paravirt
features.
Do not disable kvmclock if it was not enabled.
Signed-off-by: Kirill A. Shutemov <[email protected]>
Fixes: c02027b5742b ("x86/kvm: Disable kvmclock on all CPUs on shutdown")
Reviewed-by: Sean Christopherson <[email protected]>
Reviewed-by: Vitaly Kuznetsov <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Wanpeng Li <[email protected]>
---
arch/x86/kernel/kvmclock.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index fb8f52149be9..f2fff625576d 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -24,8 +24,8 @@
static int kvmclock __initdata = 1;
static int kvmclock_vsyscall __initdata = 1;
-static int msr_kvm_system_time __ro_after_init = MSR_KVM_SYSTEM_TIME;
-static int msr_kvm_wall_clock __ro_after_init = MSR_KVM_WALL_CLOCK;
+static int msr_kvm_system_time __ro_after_init;
+static int msr_kvm_wall_clock __ro_after_init;
static u64 kvm_sched_clock_offset __ro_after_init;
static int __init parse_no_kvmclock(char *arg)
@@ -195,7 +195,8 @@ static void kvm_setup_secondary_clock(void)
void kvmclock_disable(void)
{
- native_write_msr(msr_kvm_system_time, 0, 0);
+ if (msr_kvm_system_time)
+ native_write_msr(msr_kvm_system_time, 0, 0);
}
static void __init kvmclock_init_mem(void)
@@ -294,7 +295,10 @@ void __init kvmclock_init(void)
if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE2)) {
msr_kvm_system_time = MSR_KVM_SYSTEM_TIME_NEW;
msr_kvm_wall_clock = MSR_KVM_WALL_CLOCK_NEW;
- } else if (!kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE)) {
+ } else if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE)) {
+ msr_kvm_system_time = MSR_KVM_SYSTEM_TIME;
+ msr_kvm_wall_clock = MSR_KVM_WALL_CLOCK;
+ } else {
return;
}
--
2.41.0
TDX guests allocate shared buffers to perform I/O. It is done by
allocating pages normally from the buddy allocator and converting them
to shared with set_memory_decrypted().
The second kernel has no idea what memory is converted this way. It only
sees E820_TYPE_RAM.
Accessing shared memory via private mapping is fatal. It leads to
unrecoverable TD exit.
On kexec walk direct mapping and convert all shared memory back to
private. It makes all RAM private again and second kernel may use it
normally.
The conversion occurs in two steps: stopping new conversions and
unsharing all memory. In the case of normal kexec, the stopping of
conversions takes place while scheduling is still functioning. This
allows for waiting until any ongoing conversions are finished. The
second step is carried out when all CPUs except one are inactive and
interrupts are disabled. This prevents any conflicts with code that may
access shared memory.
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Rick Edgecombe <[email protected]>
---
arch/x86/coco/tdx/tdx.c | 119 +++++++++++++++++++++++++++++++-
arch/x86/include/asm/x86_init.h | 2 +
arch/x86/kernel/crash.c | 6 ++
arch/x86/kernel/reboot.c | 13 ++++
4 files changed, 138 insertions(+), 2 deletions(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 8a49484a2917..5c64db168edd 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -6,8 +6,10 @@
#include <linux/cpufeature.h>
#include <linux/debugfs.h>
+#include <linux/delay.h>
#include <linux/export.h>
#include <linux/io.h>
+#include <linux/kexec.h>
#include <asm/coco.h>
#include <asm/tdx.h>
#include <asm/vmx.h>
@@ -15,6 +17,7 @@
#include <asm/insn.h>
#include <asm/insn-eval.h>
#include <asm/pgtable.h>
+#include <asm/set_memory.h>
/* MMIO direction */
#define EPT_READ 0
@@ -41,6 +44,9 @@
static atomic_long_t nr_shared;
+static atomic_t conversions_in_progress;
+static bool conversion_allowed = true;
+
static inline bool pte_decrypted(pte_t pte)
{
return cc_mkdec(pte_val(pte)) == pte_val(pte);
@@ -726,6 +732,14 @@ static bool tdx_tlb_flush_required(bool private)
static bool tdx_cache_flush_required(void)
{
+ /*
+ * Avoid issuing CLFLUSH on set_memory_decrypted() if conversions
+ * stopped. Otherwise it can race with unshare_all_memory() and trigger
+ * implicit conversion to shared.
+ */
+ if (!conversion_allowed)
+ return false;
+
/*
* AMD SME/SEV can avoid cache flushing if HW enforces cache coherence.
* TDX doesn't have such capability.
@@ -809,12 +823,25 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
bool enc)
{
+ atomic_inc(&conversions_in_progress);
+
+ /*
+ * Check after bumping conversions_in_progress to serialize
+ * against tdx_shutdown().
+ */
+ if (!conversion_allowed) {
+ atomic_dec(&conversions_in_progress);
+ return -EBUSY;
+ }
+
/*
* Only handle shared->private conversion here.
* See the comment in tdx_early_init().
*/
- if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
+ if (enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
+ atomic_dec(&conversions_in_progress);
return -EIO;
+ }
return 0;
}
@@ -826,17 +853,102 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
* Only handle private->shared conversion here.
* See the comment in tdx_early_init().
*/
- if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
+ if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
+ atomic_dec(&conversions_in_progress);
return -EIO;
+ }
if (enc)
atomic_long_sub(numpages, &nr_shared);
else
atomic_long_add(numpages, &nr_shared);
+ atomic_dec(&conversions_in_progress);
+
return 0;
}
+static void tdx_kexec_stop_conversion(bool crash)
+{
+ /* Stop new private<->shared conversions */
+ conversion_allowed = false;
+ barrier();
+
+ /*
+ * Crash kernel reaches here with interrupts disabled: can't wait for
+ * conversions to finish.
+ *
+ * If race happened, just report and proceed.
+ */
+ if (!crash) {
+ unsigned long timeout;
+
+ /*
+ * Wait for in-flight conversions to complete.
+ *
+ * Do not wait more than 30 seconds.
+ */
+ timeout = 30 * USEC_PER_SEC;
+ while (atomic_read(&conversions_in_progress) && timeout--)
+ udelay(1);
+ }
+
+ if (atomic_read(&conversions_in_progress))
+ pr_warn("Failed to finish shared<->private conversions\n");
+}
+
+static void tdx_kexec_unshare_mem(void)
+{
+ unsigned long addr, end;
+ long found = 0, shared;
+
+ /*
+ * Walk direct mapping and convert all shared memory back to private,
+ */
+
+ addr = PAGE_OFFSET;
+ end = PAGE_OFFSET + get_max_mapped();
+
+ while (addr < end) {
+ unsigned long size;
+ unsigned int level;
+ pte_t *pte;
+
+ pte = lookup_address(addr, &level);
+ size = page_level_size(level);
+
+ if (pte && pte_decrypted(*pte)) {
+ int pages = size / PAGE_SIZE;
+
+ /*
+ * Touching memory with shared bit set triggers implicit
+ * conversion to shared.
+ *
+ * Make sure nobody touches the shared range from
+ * now on.
+ */
+ set_pte(pte, __pte(0));
+
+ if (!tdx_enc_status_changed(addr, pages, true)) {
+ pr_err("Failed to unshare range %#lx-%#lx\n",
+ addr, addr + size);
+ }
+
+ found += pages;
+ }
+
+ addr += size;
+ }
+
+ __flush_tlb_all();
+
+ shared = atomic_long_read(&nr_shared);
+ if (shared != found) {
+ pr_err("shared page accounting is off\n");
+ pr_err("nr_shared = %ld, nr_found = %ld\n", shared, found);
+ }
+}
+
void __init tdx_early_init(void)
{
struct tdx_module_args args = {
@@ -896,6 +1008,9 @@ void __init tdx_early_init(void)
x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required;
x86_platform.guest.enc_tlb_flush_required = tdx_tlb_flush_required;
+ x86_platform.guest.enc_kexec_stop_conversion = tdx_kexec_stop_conversion;
+ x86_platform.guest.enc_kexec_unshare_mem = tdx_kexec_unshare_mem;
+
/*
* TDX intercepts the RDMSR to read the X2APIC ID in the parallel
* bringup low level code. That raises #VE which cannot be handled
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index c9503fe2d13a..3196ff20a29e 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -154,6 +154,8 @@ struct x86_guest {
int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
bool (*enc_tlb_flush_required)(bool enc);
bool (*enc_cache_flush_required)(void);
+ void (*enc_kexec_stop_conversion)(bool crash);
+ void (*enc_kexec_unshare_mem)(void);
};
/**
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index c92d88680dbf..b99bd28ad22f 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -40,6 +40,7 @@
#include <asm/intel_pt.h>
#include <asm/crash.h>
#include <asm/cmdline.h>
+#include <asm/tdx.h>
/* Used while preparing memory map entries for second kernel */
struct crash_memmap_data {
@@ -107,6 +108,11 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
crash_smp_send_stop();
+ if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
+ x86_platform.guest.enc_kexec_stop_conversion(true);
+ x86_platform.guest.enc_kexec_unshare_mem();
+ }
+
cpu_emergency_disable_virtualization();
/*
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 830425e6d38e..16dde83df49a 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -12,6 +12,7 @@
#include <linux/delay.h>
#include <linux/objtool.h>
#include <linux/pgtable.h>
+#include <linux/kexec.h>
#include <acpi/reboot.h>
#include <asm/io.h>
#include <asm/apic.h>
@@ -31,6 +32,7 @@
#include <asm/realmode.h>
#include <asm/x86_init.h>
#include <asm/efi.h>
+#include <asm/tdx.h>
/*
* Power off function, if any
@@ -716,6 +718,14 @@ static void native_machine_emergency_restart(void)
void native_machine_shutdown(void)
{
+ /*
+ * Call enc_kexec_stop_conversion() while all CPUs are still active and
+ * interrupts are enabled. This will allow all in-flight memory
+ * conversions to finish cleanly.
+ */
+ if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) && kexec_in_progress)
+ x86_platform.guest.enc_kexec_stop_conversion(false);
+
/* Stop the cpus and apics */
#ifdef CONFIG_X86_IO_APIC
/*
@@ -752,6 +762,9 @@ void native_machine_shutdown(void)
#ifdef CONFIG_X86_64
x86_platform.iommu_shutdown();
#endif
+
+ if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) && kexec_in_progress)
+ x86_platform.guest.enc_kexec_unshare_mem();
}
static void __machine_emergency_restart(int emergency)
--
2.41.0
e820__end_of_ram_pfn() is used to calculate max_pfn which, among other
things, guides where direct mapping ends. Any memory above max_pfn is
not going to be present in the direct mapping.
e820__end_of_ram_pfn() finds the end of the ram based on the highest
E820_TYPE_RAM range. But it doesn't includes E820_TYPE_ACPI ranges into
calculation.
Despite the name, E820_TYPE_ACPI covers not only ACPI data, but also EFI
tables and might be required by kernel to function properly.
Usually the problem is hidden because there is some E820_TYPE_RAM memory
above E820_TYPE_ACPI. But crashkernel only presents pre-allocated crash
memory as E820_TYPE_RAM on boot. If the preallocated range is small, it
can fit under the last E820_TYPE_ACPI range.
Modify e820__end_of_ram_pfn() and e820__end_of_low_ram_pfn() to cover
E820_TYPE_ACPI memory.
The problem was discovered during debugging kexec for TDX guest. TDX
guest uses E820_TYPE_ACPI to store the unaccepted memory bitmap and pass
it between the kernels on kexec.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
arch/x86/kernel/e820.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index fb8cf953380d..99c80680dc9e 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -827,7 +827,7 @@ u64 __init e820__memblock_alloc_reserved(u64 size, u64 align)
/*
* Find the highest page frame number we have available
*/
-static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type type)
+static unsigned long __init e820_end_ram_pfn(unsigned long limit_pfn)
{
int i;
unsigned long last_pfn = 0;
@@ -838,7 +838,8 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type
unsigned long start_pfn;
unsigned long end_pfn;
- if (entry->type != type)
+ if (entry->type != E820_TYPE_RAM &&
+ entry->type != E820_TYPE_ACPI)
continue;
start_pfn = entry->addr >> PAGE_SHIFT;
@@ -864,12 +865,12 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, enum e820_type
unsigned long __init e820__end_of_ram_pfn(void)
{
- return e820_end_pfn(MAX_ARCH_PFN, E820_TYPE_RAM);
+ return e820_end_ram_pfn(MAX_ARCH_PFN);
}
unsigned long __init e820__end_of_low_ram_pfn(void)
{
- return e820_end_pfn(1UL << (32 - PAGE_SHIFT), E820_TYPE_RAM);
+ return e820_end_ram_pfn(1UL << (32 - PAGE_SHIFT));
}
static void __init early_panic(char *msg)
--
2.41.0
MADT Multiprocessor Wakeup structure version 1 brings support of CPU
offlining: BIOS provides a reset vector where the CPU has to jump to
for offlining itself. The new TEST mailbox command can be used to test
whether the CPU offlined itself which means the BIOS has control over
the CPU and can online it again via the ACPI MADT wakeup method.
Add CPU offling support for the ACPI MADT wakeup method by implementing
custom cpu_die(), play_dead() and stop_this_cpu() SMP operations.
CPU offlining makes is possible to hand over secondary CPUs over kexec,
not limiting the second kernel to a single CPU.
The change conforms to the approved ACPI spec change proposal. See the
Link.
Signed-off-by: Kirill A. Shutemov <[email protected]>
Link: https://lore.kernel.org/all/13356251.uLZWGnKmhe@kreacher
---
arch/x86/include/asm/acpi.h | 2 +
arch/x86/kernel/acpi/Makefile | 2 +-
arch/x86/kernel/acpi/madt_playdead.S | 29 +++++
arch/x86/kernel/acpi/madt_wakeup.c | 184 ++++++++++++++++++++++++++-
include/acpi/actbl2.h | 15 ++-
5 files changed, 228 insertions(+), 4 deletions(-)
create mode 100644 arch/x86/kernel/acpi/madt_playdead.S
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 2625b915ae7f..021cafa214c2 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -81,6 +81,8 @@ union acpi_subtable_headers;
int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
const unsigned long end);
+void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
+
/*
* Check if the CPU can handle C2 and deeper
*/
diff --git a/arch/x86/kernel/acpi/Makefile b/arch/x86/kernel/acpi/Makefile
index 8c7329c88a75..37b1f28846de 100644
--- a/arch/x86/kernel/acpi/Makefile
+++ b/arch/x86/kernel/acpi/Makefile
@@ -4,7 +4,7 @@ obj-$(CONFIG_ACPI) += boot.o
obj-$(CONFIG_ACPI_SLEEP) += sleep.o wakeup_$(BITS).o
obj-$(CONFIG_ACPI_APEI) += apei.o
obj-$(CONFIG_ACPI_CPPC_LIB) += cppc.o
-obj-$(CONFIG_X86_ACPI_MADT_WAKEUP) += madt_wakeup.o
+obj-$(CONFIG_X86_ACPI_MADT_WAKEUP) += madt_wakeup.o madt_playdead.o
ifneq ($(CONFIG_ACPI_PROCESSOR),)
obj-y += cstate.o
diff --git a/arch/x86/kernel/acpi/madt_playdead.S b/arch/x86/kernel/acpi/madt_playdead.S
new file mode 100644
index 000000000000..e48049959513
--- /dev/null
+++ b/arch/x86/kernel/acpi/madt_playdead.S
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/linkage.h>
+#include <asm/nospec-branch.h>
+#include <asm/page_types.h>
+#include <asm/processor-flags.h>
+
+ .text
+ .align PAGE_SIZE
+
+/*
+ * asm_acpi_mp_play_dead() - Hand over control of the CPU to the BIOS
+ *
+ * rdi: Address of the ACPI MADT MPWK ResetVector
+ * rsi: PGD of the identity mapping
+ */
+SYM_FUNC_START(asm_acpi_mp_play_dead)
+ /* Turn off global entries. Following CR3 write will flush them. */
+ movq %cr4, %rdx
+ andq $~(X86_CR4_PGE), %rdx
+ movq %rdx, %cr4
+
+ /* Switch to identity mapping */
+ movq %rsi, %rax
+ movq %rax, %cr3
+
+ /* Jump to reset vector */
+ ANNOTATE_RETPOLINE_SAFE
+ jmp *%rdi
+SYM_FUNC_END(asm_acpi_mp_play_dead)
diff --git a/arch/x86/kernel/acpi/madt_wakeup.c b/arch/x86/kernel/acpi/madt_wakeup.c
index 30820f9de5af..9e984e2191ba 100644
--- a/arch/x86/kernel/acpi/madt_wakeup.c
+++ b/arch/x86/kernel/acpi/madt_wakeup.c
@@ -1,10 +1,19 @@
// SPDX-License-Identifier: GPL-2.0-or-later
#include <linux/acpi.h>
#include <linux/cpu.h>
+#include <linux/delay.h>
#include <linux/io.h>
+#include <linux/kexec.h>
+#include <linux/memblock.h>
+#include <linux/pgtable.h>
+#include <linux/sched/hotplug.h>
#include <asm/apic.h>
#include <asm/barrier.h>
+#include <asm/init.h>
+#include <asm/intel_pt.h>
+#include <asm/nmi.h>
#include <asm/processor.h>
+#include <asm/reboot.h>
/* Physical address of the Multiprocessor Wakeup Structure mailbox */
static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
@@ -12,6 +21,154 @@ static u64 acpi_mp_wake_mailbox_paddr __ro_after_init;
/* Virtual address of the Multiprocessor Wakeup Structure mailbox */
static struct acpi_madt_multiproc_wakeup_mailbox *acpi_mp_wake_mailbox __ro_after_init;
+static u64 acpi_mp_pgd __ro_after_init;
+static u64 acpi_mp_reset_vector_paddr __ro_after_init;
+
+static void acpi_mp_stop_this_cpu(void)
+{
+ asm_acpi_mp_play_dead(acpi_mp_reset_vector_paddr, acpi_mp_pgd);
+}
+
+static void acpi_mp_play_dead(void)
+{
+ play_dead_common();
+ asm_acpi_mp_play_dead(acpi_mp_reset_vector_paddr, acpi_mp_pgd);
+}
+
+static void acpi_mp_cpu_die(unsigned int cpu)
+{
+ u32 apicid = per_cpu(x86_cpu_to_apicid, cpu);
+ unsigned long timeout;
+
+ /*
+ * Use TEST mailbox command to prove that BIOS got control over
+ * the CPU before declaring it dead.
+ *
+ * BIOS has to clear 'command' field of the mailbox.
+ */
+ acpi_mp_wake_mailbox->apic_id = apicid;
+ smp_store_release(&acpi_mp_wake_mailbox->command,
+ ACPI_MP_WAKE_COMMAND_TEST);
+
+ /* Don't wait longer than a second. */
+ timeout = USEC_PER_SEC;
+ while (READ_ONCE(acpi_mp_wake_mailbox->command) && --timeout)
+ udelay(1);
+
+ if (!timeout)
+ pr_err("Failed to hand over CPU %d to BIOS\n", cpu);
+}
+
+/* The argument is required to match type of x86_mapping_info::alloc_pgt_page */
+static void __init *alloc_pgt_page(void *dummy)
+{
+ return memblock_alloc(PAGE_SIZE, PAGE_SIZE);
+}
+
+static void __init free_pgt_page(void *pgt, void *dummy)
+{
+ return memblock_free(pgt, PAGE_SIZE);
+}
+
+/*
+ * Make sure asm_acpi_mp_play_dead() is present in the identity mapping at
+ * the same place as in the kernel page tables. asm_acpi_mp_play_dead() switches
+ * to the identity mapping and the function has be present at the same spot in
+ * the virtual address space before and after switching page tables.
+ */
+static int __init init_transition_pgtable(pgd_t *pgd)
+{
+ pgprot_t prot = PAGE_KERNEL_EXEC_NOENC;
+ unsigned long vaddr, paddr;
+ p4d_t *p4d;
+ pud_t *pud;
+ pmd_t *pmd;
+ pte_t *pte;
+
+ vaddr = (unsigned long)asm_acpi_mp_play_dead;
+ pgd += pgd_index(vaddr);
+ if (!pgd_present(*pgd)) {
+ p4d = (p4d_t *)alloc_pgt_page(NULL);
+ if (!p4d)
+ return -ENOMEM;
+ set_pgd(pgd, __pgd(__pa(p4d) | _KERNPG_TABLE));
+ }
+ p4d = p4d_offset(pgd, vaddr);
+ if (!p4d_present(*p4d)) {
+ pud = (pud_t *)alloc_pgt_page(NULL);
+ if (!pud)
+ return -ENOMEM;
+ set_p4d(p4d, __p4d(__pa(pud) | _KERNPG_TABLE));
+ }
+ pud = pud_offset(p4d, vaddr);
+ if (!pud_present(*pud)) {
+ pmd = (pmd_t *)alloc_pgt_page(NULL);
+ if (!pmd)
+ return -ENOMEM;
+ set_pud(pud, __pud(__pa(pmd) | _KERNPG_TABLE));
+ }
+ pmd = pmd_offset(pud, vaddr);
+ if (!pmd_present(*pmd)) {
+ pte = (pte_t *)alloc_pgt_page(NULL);
+ if (!pte)
+ return -ENOMEM;
+ set_pmd(pmd, __pmd(__pa(pte) | _KERNPG_TABLE));
+ }
+ pte = pte_offset_kernel(pmd, vaddr);
+
+ paddr = __pa(vaddr);
+ set_pte(pte, pfn_pte(paddr >> PAGE_SHIFT, prot));
+
+ return 0;
+}
+
+static int __init acpi_mp_setup_reset(u64 reset_vector)
+{
+ pgd_t *pgd;
+ struct x86_mapping_info info = {
+ .alloc_pgt_page = alloc_pgt_page,
+ .free_pgt_page = free_pgt_page,
+ .page_flag = __PAGE_KERNEL_LARGE_EXEC,
+ .kernpg_flag = _KERNPG_TABLE_NOENC,
+ };
+
+ pgd = alloc_pgt_page(NULL);
+ if (!pgd)
+ return -ENOMEM;
+
+ for (int i = 0; i < nr_pfn_mapped; i++) {
+ unsigned long mstart, mend;
+
+ mstart = pfn_mapped[i].start << PAGE_SHIFT;
+ mend = pfn_mapped[i].end << PAGE_SHIFT;
+ if (kernel_ident_mapping_init(&info, pgd, mstart, mend)) {
+ kernel_ident_mapping_free(&info, pgd);
+ return -ENOMEM;
+ }
+ }
+
+ if (kernel_ident_mapping_init(&info, pgd,
+ PAGE_ALIGN_DOWN(reset_vector),
+ PAGE_ALIGN(reset_vector + 1))) {
+ kernel_ident_mapping_free(&info, pgd);
+ return -ENOMEM;
+ }
+
+ if (init_transition_pgtable(pgd)) {
+ kernel_ident_mapping_free(&info, pgd);
+ return -ENOMEM;
+ }
+
+ smp_ops.play_dead = acpi_mp_play_dead;
+ smp_ops.stop_this_cpu = acpi_mp_stop_this_cpu;
+ smp_ops.cpu_die = acpi_mp_cpu_die;
+
+ acpi_mp_reset_vector_paddr = reset_vector;
+ acpi_mp_pgd = __pa(pgd);
+
+ return 0;
+}
+
static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
{
if (!acpi_mp_wake_mailbox_paddr) {
@@ -97,14 +254,37 @@ int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
struct acpi_madt_multiproc_wakeup *mp_wake;
mp_wake = (struct acpi_madt_multiproc_wakeup *)header;
- if (BAD_MADT_ENTRY(mp_wake, end))
+
+ /*
+ * Cannot use the standard BAD_MADT_ENTRY() to sanity check the @mp_wake
+ * entry. 'sizeof (struct acpi_madt_multiproc_wakeup)' can be larger
+ * than the actual size of the MP wakeup entry in ACPI table because the
+ * 'reset_vector' is only available in the V1 MP wakeup structure.
+ */
+ if (!mp_wake)
+ return -EINVAL;
+ if (end - (unsigned long)mp_wake < ACPI_MADT_MP_WAKEUP_SIZE_V0)
+ return -EINVAL;
+ if (mp_wake->header.length < ACPI_MADT_MP_WAKEUP_SIZE_V0)
return -EINVAL;
acpi_table_print_madt_entry(&header->common);
acpi_mp_wake_mailbox_paddr = mp_wake->mailbox_address;
- acpi_mp_disable_offlining(mp_wake);
+ if (mp_wake->version >= ACPI_MADT_MP_WAKEUP_VERSION_V1 &&
+ mp_wake->header.length >= ACPI_MADT_MP_WAKEUP_SIZE_V1) {
+ if (acpi_mp_setup_reset(mp_wake->reset_vector)) {
+ pr_warn("Failed to setup MADT reset vector\n");
+ acpi_mp_disable_offlining(mp_wake);
+ }
+ } else {
+ /*
+ * CPU offlining requires version 1 of the ACPI MADT wakeup
+ * structure.
+ */
+ acpi_mp_disable_offlining(mp_wake);
+ }
apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 23b4cfb640fc..8348bf46a648 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -1112,8 +1112,20 @@ struct acpi_madt_multiproc_wakeup {
u16 version;
u32 reserved; /* reserved - must be zero */
u64 mailbox_address;
+ u64 reset_vector;
};
+/* Values for Version field above */
+
+enum acpi_madt_multiproc_wakeup_version {
+ ACPI_MADT_MP_WAKEUP_VERSION_NONE = 0,
+ ACPI_MADT_MP_WAKEUP_VERSION_V1 = 1,
+ ACPI_MADT_MP_WAKEUP_VERSION_RESERVED = 2, /* 2 and greater are reserved */
+};
+
+#define ACPI_MADT_MP_WAKEUP_SIZE_V0 16
+#define ACPI_MADT_MP_WAKEUP_SIZE_V1 24
+
#define ACPI_MULTIPROC_WAKEUP_MB_OS_SIZE 2032
#define ACPI_MULTIPROC_WAKEUP_MB_FIRMWARE_SIZE 2048
@@ -1126,7 +1138,8 @@ struct acpi_madt_multiproc_wakeup_mailbox {
u8 reserved_firmware[ACPI_MULTIPROC_WAKEUP_MB_FIRMWARE_SIZE]; /* reserved for firmware use */
};
-#define ACPI_MP_WAKE_COMMAND_WAKEUP 1
+#define ACPI_MP_WAKE_COMMAND_WAKEUP 1
+#define ACPI_MP_WAKE_COMMAND_TEST 2
/* 17: CPU Core Interrupt Controller (ACPI 6.5) */
--
2.41.0
If the helper is defined, it is called instead of halt() to stop the CPU
at the end of stop_this_cpu() and on crash CPU shutdown.
ACPI MADT will use it to hand over the CPU to BIOS in order to be able
to wake it up again after kexec.
Signed-off-by: Kirill A. Shutemov <[email protected]>
---
v5.1:
- Fix build for !SMP;
---
arch/x86/include/asm/smp.h | 1 +
arch/x86/kernel/process.c | 7 +++++++
arch/x86/kernel/reboot.c | 12 ++++++++----
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 4fab2ed454f3..390d53fd34f9 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -38,6 +38,7 @@ struct smp_ops {
int (*cpu_disable)(void);
void (*cpu_die)(unsigned int cpu);
void (*play_dead)(void);
+ void (*stop_this_cpu)(void);
void (*send_call_func_ipi)(const struct cpumask *mask);
void (*send_call_func_single_ipi)(int cpu);
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index b6f4e8399fca..ea4c812c7bf3 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -835,6 +835,13 @@ void __noreturn stop_this_cpu(void *dummy)
*/
cpumask_clear_cpu(cpu, &cpus_stop_mask);
+#ifdef CONFIG_SMP
+ if (smp_ops.stop_this_cpu) {
+ smp_ops.stop_this_cpu();
+ unreachable();
+ }
+#endif
+
for (;;) {
/*
* Use native_halt() so that memory contents don't change
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 16dde83df49a..738b3e810196 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -881,10 +881,14 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
cpu_emergency_disable_virtualization();
atomic_dec(&waiting_for_crash_ipi);
- /* Assume hlt works */
- halt();
- for (;;)
- cpu_relax();
+
+ if (smp_ops.stop_this_cpu) {
+ smp_ops.stop_this_cpu();
+ } else {
+ halt();
+ for (;;)
+ cpu_relax();
+ }
return NMI_HANDLED;
}
--
2.41.0
The break apart looks good.
On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> + while (addr < end) {
> + unsigned long size;
> + unsigned int level;
> + pte_t *pte;
> +
> + pte = lookup_address(addr, &level);
> + size = page_level_size(level);
> +
> + if (pte && pte_decrypted(*pte)) {
> + int pages = size / PAGE_SIZE;
> +
> + /*
> + * Touching memory with shared bit set
> triggers implicit
> + * conversion to shared.
> + *
> + * Make sure nobody touches the shared range
> from
> + * now on.
> + */
> + set_pte(pte, __pte(0));
Does this need a full shootdown at this point, at least in the crash
case? A local flush for the non-crash case might be nice too.
Also, probably a barrier() between set_pte() and
tdx_enc_status_changed().
> +
> + if (!tdx_enc_status_changed(addr, pages,
> true)) {
> + pr_err("Failed to unshare range %#lx-
> %#lx\n",
> + addr, addr + size);
> + }
On Fri, Jan 05, 2024 at 07:38:48PM +0000, Edgecombe, Rick P wrote:
> The break apart looks good.
>
> On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> > +???????while (addr < end) {
> > +???????????????unsigned long size;
> > +???????????????unsigned int level;
> > +???????????????pte_t *pte;
> > +
> > +???????????????pte = lookup_address(addr, &level);
> > +???????????????size = page_level_size(level);
> > +
> > +???????????????if (pte && pte_decrypted(*pte)) {
> > +???????????????????????int pages = size / PAGE_SIZE;
> > +
> > +???????????????????????/*
> > +??????????????????????? * Touching memory with shared bit set
> > triggers implicit
> > +??????????????????????? * conversion to shared.
> > +??????????????????????? *
> > +??????????????????????? * Make sure nobody touches the shared range
> > from
> > +??????????????????????? * now on.
> > +??????????????????????? */
> > +???????????????????????set_pte(pte, __pte(0));
>
> Does this need a full shootdown at this point, at least in the crash
> case? A local flush for the non-crash case might be nice too.
See __flush_tlb_all() after the loop.
> Also, probably a barrier() between set_pte() and
> tdx_enc_status_changed().
But why? There's no concurrency at this point. Interrupts are disabled and
only one CPU is active. Nobody can touch the memory relevant for the PTE.
--
Kiryl Shutsemau / Kirill A. Shutemov
On Sat, 2024-01-06 at 03:59 +0300, [email protected]
wrote:
> But why? There's no concurrency at this point. Interrupts are
> disabled and
> only one CPU is active. Nobody can touch the memory relevant for the
> PTE.
Oh, right, sorry. I had thought there could be other CPUs active at
that point.
On Mon, 2023-12-25 at 11:05 +0300, Kirill A. Shutemov wrote:
> If the helper is defined, it is called instead of halt() to stop the CPU
> at the end of stop_this_cpu() and on crash CPU shutdown.
>
> ACPI MADT will use it to hand over the CPU to BIOS in order to be able
> to wake it up again after kexec.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> ---
>
> v5.1:
> - Fix build for !SMP;
>
> ---
> arch/x86/include/asm/smp.h | 1 +
> arch/x86/kernel/process.c | 7 +++++++
> arch/x86/kernel/reboot.c | 12 ++++++++----
> 3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 4fab2ed454f3..390d53fd34f9 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -38,6 +38,7 @@ struct smp_ops {
> int (*cpu_disable)(void);
> void (*cpu_die)(unsigned int cpu);
> void (*play_dead)(void);
> + void (*stop_this_cpu)(void);
>
> void (*send_call_func_ipi)(const struct cpumask *mask);
> void (*send_call_func_single_ipi)(int cpu);
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index b6f4e8399fca..ea4c812c7bf3 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -835,6 +835,13 @@ void __noreturn stop_this_cpu(void *dummy)
> */
> cpumask_clear_cpu(cpu, &cpus_stop_mask);
>
> +#ifdef CONFIG_SMP
> + if (smp_ops.stop_this_cpu) {
> + smp_ops.stop_this_cpu();
> + unreachable();
> + }
> +#endif
If I read correctly this will result in stop_this_cpu() having different
behaviour for SMP and !SMP build for TDX guest. For example, AFAICT
machine_halt() also calls stop_this_cpu() on local cpu after it stops other
cpus. So for the local cpu, in SMP build it will calls into BIOS's reset vector
but in !SMP it will call native_halt().
> +
> for (;;) {
> /*
> * Use native_halt() so that memory contents don't change
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 16dde83df49a..738b3e810196 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -881,10 +881,14 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
> cpu_emergency_disable_virtualization();
>
> atomic_dec(&waiting_for_crash_ipi);
> - /* Assume hlt works */
> - halt();
> - for (;;)
> - cpu_relax();
> +
> + if (smp_ops.stop_this_cpu) {
> + smp_ops.stop_this_cpu();
Could you explain why unreachable() is called in stop_this_cpu() but not here?
> + } else {
> + halt();
> + for (;;)
> + cpu_relax();
> + }
Similar to stop_this_cpu(), if you also call unreachable() here, then I think
you can remove the 'else' here but directly calls halt() + cpu_relax() loop.
On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> The helper complements kernel_ident_mapping_init(): it frees the
> identity mapping that was previously allocated. It will be used in the
> error path to free a partially allocated mapping or if the mapping is no
> longer needed.
>
> The caller provides a struct x86_mapping_info with the free_pgd_page()
> callback hooked up and the pgd_t to free.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> ---
> arch/x86/include/asm/init.h | 3 ++
> arch/x86/mm/ident_map.c | 73 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 76 insertions(+)
>
> diff --git a/arch/x86/include/asm/init.h b/arch/x86/include/asm/init.h
> index cc9ccf61b6bd..14d72727d7ee 100644
> --- a/arch/x86/include/asm/init.h
> +++ b/arch/x86/include/asm/init.h
> @@ -6,6 +6,7 @@
>
> struct x86_mapping_info {
> void *(*alloc_pgt_page)(void *); /* allocate buf for page table */
> + void (*free_pgt_page)(void *, void *); /* free buf for page table */
> void *context; /* context for alloc_pgt_page */
> unsigned long page_flag; /* page flag for PMD or PUD entry */
> unsigned long offset; /* ident mapping offset */
> @@ -16,4 +17,6 @@ struct x86_mapping_info {
> int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
> unsigned long pstart, unsigned long pend);
>
> +void kernel_ident_mapping_free(struct x86_mapping_info *info, pgd_t *pgd);
Maybe range-based free function can provide more flexibility (e.g., you can
directly call the free function to cleanup in kernel_ident_mapping_init()
internally when something goes wrong), but I guess this is sufficient for
current use case (and perhaps the majority use cases).
Reviewed-by: Kai Huang <[email protected]>
On Mon, 2024-01-08 at 03:13 +0000, Huang, Kai wrote:
> On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> > The helper complements kernel_ident_mapping_init(): it frees the
> > identity mapping that was previously allocated. It will be used in the
> > error path to free a partially allocated mapping or if the mapping is no
> > longer needed.
> >
> > The caller provides a struct x86_mapping_info with the free_pgd_page()
> > callback hooked up and the pgd_t to free.
> >
> > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > ---
> > arch/x86/include/asm/init.h | 3 ++
> > arch/x86/mm/ident_map.c | 73 +++++++++++++++++++++++++++++++++++++
> > 2 files changed, 76 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/init.h b/arch/x86/include/asm/init.h
> > index cc9ccf61b6bd..14d72727d7ee 100644
> > --- a/arch/x86/include/asm/init.h
> > +++ b/arch/x86/include/asm/init.h
> > @@ -6,6 +6,7 @@
> >
> > struct x86_mapping_info {
> > void *(*alloc_pgt_page)(void *); /* allocate buf for page table */
> > + void (*free_pgt_page)(void *, void *); /* free buf for page table */
> > void *context; /* context for alloc_pgt_page */
> > unsigned long page_flag; /* page flag for PMD or PUD entry */
> > unsigned long offset; /* ident mapping offset */
> > @@ -16,4 +17,6 @@ struct x86_mapping_info {
> > int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
> > unsigned long pstart, unsigned long pend);
> >
> > +void kernel_ident_mapping_free(struct x86_mapping_info *info, pgd_t *pgd);
>
> Maybe range-based free function can provide more flexibility (e.g., you can
> directly call the free function to cleanup in kernel_ident_mapping_init()
> internally when something goes wrong), but I guess this is sufficient for
> current use case (and perhaps the majority use cases).
>
> Reviewed-by: Kai Huang <[email protected]>
>
Another argument of range-based free function is, theoretically you can build
the identical mapping table using different x86_mapping_info on different
ranges, thus it makes less sense to use one 'struct x86_mapping_info *info' to
free the entire page table, albeit in this implementation only the
'free_pgt_page()' callback is used.
On Mon, Jan 08, 2024 at 03:04:31AM +0000, Huang, Kai wrote:
> On Mon, 2023-12-25 at 11:05 +0300, Kirill A. Shutemov wrote:
> > If the helper is defined, it is called instead of halt() to stop the CPU
> > at the end of stop_this_cpu() and on crash CPU shutdown.
> >
> > ACPI MADT will use it to hand over the CPU to BIOS in order to be able
> > to wake it up again after kexec.
> >
> > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > ---
> >
> > v5.1:
> > - Fix build for !SMP;
> >
> > ---
> > arch/x86/include/asm/smp.h | 1 +
> > arch/x86/kernel/process.c | 7 +++++++
> > arch/x86/kernel/reboot.c | 12 ++++++++----
> > 3 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> > index 4fab2ed454f3..390d53fd34f9 100644
> > --- a/arch/x86/include/asm/smp.h
> > +++ b/arch/x86/include/asm/smp.h
> > @@ -38,6 +38,7 @@ struct smp_ops {
> > int (*cpu_disable)(void);
> > void (*cpu_die)(unsigned int cpu);
> > void (*play_dead)(void);
> > + void (*stop_this_cpu)(void);
> >
> > void (*send_call_func_ipi)(const struct cpumask *mask);
> > void (*send_call_func_single_ipi)(int cpu);
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index b6f4e8399fca..ea4c812c7bf3 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -835,6 +835,13 @@ void __noreturn stop_this_cpu(void *dummy)
> > */
> > cpumask_clear_cpu(cpu, &cpus_stop_mask);
> >
> > +#ifdef CONFIG_SMP
> > + if (smp_ops.stop_this_cpu) {
> > + smp_ops.stop_this_cpu();
> > + unreachable();
> > + }
> > +#endif
>
> If I read correctly this will result in stop_this_cpu() having different
> behaviour for SMP and !SMP build for TDX guest. For example, AFAICT
> machine_halt() also calls stop_this_cpu() on local cpu after it stops other
> cpus. So for the local cpu, in SMP build it will calls into BIOS's reset vector
> but in !SMP it will call native_halt().
It doesn't make a difference in practice: both halt and giving CPU to
BIOS will be unrecoverable operation. Both are equally acceptable for
machine_halt().
> > +
> > for (;;) {
> > /*
> > * Use native_halt() so that memory contents don't change
> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > index 16dde83df49a..738b3e810196 100644
> > --- a/arch/x86/kernel/reboot.c
> > +++ b/arch/x86/kernel/reboot.c
> > @@ -881,10 +881,14 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
> > cpu_emergency_disable_virtualization();
> >
> > atomic_dec(&waiting_for_crash_ipi);
> > - /* Assume hlt works */
> > - halt();
> > - for (;;)
> > - cpu_relax();
> > +
> > + if (smp_ops.stop_this_cpu) {
> > + smp_ops.stop_this_cpu();
>
> Could you explain why unreachable() is called in stop_this_cpu() but not here?
Compiler complained previously on stop_this_cpu() when it had halt() in
'else' case because the function is declared as __noreturn. I left
unreachable() after reworking it without 'else' to document the behaviour.
> > + } else {
> > + halt();
> > + for (;;)
> > + cpu_relax();
> > + }
>
> Similar to stop_this_cpu(), if you also call unreachable() here, then I think
> you can remove the 'else' here but directly calls halt() + cpu_relax() loop.
It doesn't make much difference to me. But okay, I will rework it the same
way in the next version.
--
Kiryl Shutsemau / Kirill A. Shutemov
On Mon, Jan 08, 2024 at 03:30:21AM +0000, Huang, Kai wrote:
> On Mon, 2024-01-08 at 03:13 +0000, Huang, Kai wrote:
> > On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> > > The helper complements kernel_ident_mapping_init(): it frees the
> > > identity mapping that was previously allocated. It will be used in the
> > > error path to free a partially allocated mapping or if the mapping is no
> > > longer needed.
> > >
> > > The caller provides a struct x86_mapping_info with the free_pgd_page()
> > > callback hooked up and the pgd_t to free.
> > >
> > > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > > ---
> > > arch/x86/include/asm/init.h | 3 ++
> > > arch/x86/mm/ident_map.c | 73 +++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 76 insertions(+)
> > >
> > > diff --git a/arch/x86/include/asm/init.h b/arch/x86/include/asm/init.h
> > > index cc9ccf61b6bd..14d72727d7ee 100644
> > > --- a/arch/x86/include/asm/init.h
> > > +++ b/arch/x86/include/asm/init.h
> > > @@ -6,6 +6,7 @@
> > >
> > > struct x86_mapping_info {
> > > void *(*alloc_pgt_page)(void *); /* allocate buf for page table */
> > > + void (*free_pgt_page)(void *, void *); /* free buf for page table */
> > > void *context; /* context for alloc_pgt_page */
> > > unsigned long page_flag; /* page flag for PMD or PUD entry */
> > > unsigned long offset; /* ident mapping offset */
> > > @@ -16,4 +17,6 @@ struct x86_mapping_info {
> > > int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
> > > unsigned long pstart, unsigned long pend);
> > >
> > > +void kernel_ident_mapping_free(struct x86_mapping_info *info, pgd_t *pgd);
> >
> > Maybe range-based free function can provide more flexibility (e.g., you can
> > directly call the free function to cleanup in kernel_ident_mapping_init()
> > internally when something goes wrong), but I guess this is sufficient for
> > current use case (and perhaps the majority use cases).
> >
> > Reviewed-by: Kai Huang <[email protected]>
> >
>
> Another argument of range-based free function is, theoretically you can build
> the identical mapping table using different x86_mapping_info on different
> ranges, thus it makes less sense to use one 'struct x86_mapping_info *info' to
> free the entire page table, albeit in this implementation only the
> 'free_pgt_page()' callback is used.
The interface can be changed if there will be need for such behaviour.
This kind of future-proofing rarely helpful.
--
Kiryl Shutsemau / Kirill A. Shutemov
On Mon, 2024-01-08 at 13:17 +0300, [email protected] wrote:
> On Mon, Jan 08, 2024 at 03:30:21AM +0000, Huang, Kai wrote:
> > On Mon, 2024-01-08 at 03:13 +0000, Huang, Kai wrote:
> > > On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> > > > The helper complements kernel_ident_mapping_init(): it frees the
> > > > identity mapping that was previously allocated. It will be used in the
> > > > error path to free a partially allocated mapping or if the mapping is no
> > > > longer needed.
> > > >
> > > > The caller provides a struct x86_mapping_info with the free_pgd_page()
> > > > callback hooked up and the pgd_t to free.
> > > >
> > > > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > > > ---
> > > > arch/x86/include/asm/init.h | 3 ++
> > > > arch/x86/mm/ident_map.c | 73 +++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 76 insertions(+)
> > > >
> > > > diff --git a/arch/x86/include/asm/init.h b/arch/x86/include/asm/init.h
> > > > index cc9ccf61b6bd..14d72727d7ee 100644
> > > > --- a/arch/x86/include/asm/init.h
> > > > +++ b/arch/x86/include/asm/init.h
> > > > @@ -6,6 +6,7 @@
> > > >
> > > > struct x86_mapping_info {
> > > > void *(*alloc_pgt_page)(void *); /* allocate buf for page table */
> > > > + void (*free_pgt_page)(void *, void *); /* free buf for page table */
> > > > void *context; /* context for alloc_pgt_page */
> > > > unsigned long page_flag; /* page flag for PMD or PUD entry */
> > > > unsigned long offset; /* ident mapping offset */
> > > > @@ -16,4 +17,6 @@ struct x86_mapping_info {
> > > > int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
> > > > unsigned long pstart, unsigned long pend);
> > > >
> > > > +void kernel_ident_mapping_free(struct x86_mapping_info *info, pgd_t *pgd);
> > >
> > > Maybe range-based free function can provide more flexibility (e.g., you can
> > > directly call the free function to cleanup in kernel_ident_mapping_init()
> > > internally when something goes wrong), but I guess this is sufficient for
> > > current use case (and perhaps the majority use cases).
> > >
> > > Reviewed-by: Kai Huang <[email protected]>
> > >
> >
> > Another argument of range-based free function is, theoretically you can build
> > the identical mapping table using different x86_mapping_info on different
> > ranges, thus it makes less sense to use one 'struct x86_mapping_info *info' to
> > free the entire page table, albeit in this implementation only the
> > 'free_pgt_page()' callback is used.
>
> The interface can be changed if there will be need for such behaviour.
> This kind of future-proofing rarely helpful.
>
Do you want to just pass the 'free_pgt_page' function pointer to
kernel_ident_mapping_free(), instead of 'struct x86_mapping_info *info'? As
mentioned above conceptually the page table can be built from multiple
x86_mapping_info for multiple ranges.
On Mon, Jan 08, 2024 at 01:13:18PM +0000, Huang, Kai wrote:
> On Mon, 2024-01-08 at 13:17 +0300, [email protected] wrote:
> > On Mon, Jan 08, 2024 at 03:30:21AM +0000, Huang, Kai wrote:
> > > On Mon, 2024-01-08 at 03:13 +0000, Huang, Kai wrote:
> > > > On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> > > > > The helper complements kernel_ident_mapping_init(): it frees the
> > > > > identity mapping that was previously allocated. It will be used in the
> > > > > error path to free a partially allocated mapping or if the mapping is no
> > > > > longer needed.
> > > > >
> > > > > The caller provides a struct x86_mapping_info with the free_pgd_page()
> > > > > callback hooked up and the pgd_t to free.
> > > > >
> > > > > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > > > > ---
> > > > > arch/x86/include/asm/init.h | 3 ++
> > > > > arch/x86/mm/ident_map.c | 73 +++++++++++++++++++++++++++++++++++++
> > > > > 2 files changed, 76 insertions(+)
> > > > >
> > > > > diff --git a/arch/x86/include/asm/init.h b/arch/x86/include/asm/init.h
> > > > > index cc9ccf61b6bd..14d72727d7ee 100644
> > > > > --- a/arch/x86/include/asm/init.h
> > > > > +++ b/arch/x86/include/asm/init.h
> > > > > @@ -6,6 +6,7 @@
> > > > >
> > > > > struct x86_mapping_info {
> > > > > void *(*alloc_pgt_page)(void *); /* allocate buf for page table */
> > > > > + void (*free_pgt_page)(void *, void *); /* free buf for page table */
> > > > > void *context; /* context for alloc_pgt_page */
> > > > > unsigned long page_flag; /* page flag for PMD or PUD entry */
> > > > > unsigned long offset; /* ident mapping offset */
> > > > > @@ -16,4 +17,6 @@ struct x86_mapping_info {
> > > > > int kernel_ident_mapping_init(struct x86_mapping_info *info, pgd_t *pgd_page,
> > > > > unsigned long pstart, unsigned long pend);
> > > > >
> > > > > +void kernel_ident_mapping_free(struct x86_mapping_info *info, pgd_t *pgd);
> > > >
> > > > Maybe range-based free function can provide more flexibility (e.g., you can
> > > > directly call the free function to cleanup in kernel_ident_mapping_init()
> > > > internally when something goes wrong), but I guess this is sufficient for
> > > > current use case (and perhaps the majority use cases).
> > > >
> > > > Reviewed-by: Kai Huang <[email protected]>
> > > >
> > >
> > > Another argument of range-based free function is, theoretically you can build
> > > the identical mapping table using different x86_mapping_info on different
> > > ranges, thus it makes less sense to use one 'struct x86_mapping_info *info' to
> > > free the entire page table, albeit in this implementation only the
> > > 'free_pgt_page()' callback is used.
> >
> > The interface can be changed if there will be need for such behaviour.
> > This kind of future-proofing rarely helpful.
> >
>
> Do you want to just pass the 'free_pgt_page' function pointer to
> kernel_ident_mapping_free(), instead of 'struct x86_mapping_info *info'? As
> mentioned above conceptually the page table can be built from multiple
> x86_mapping_info for multiple ranges.
I don't think we have such cases in kernel. Let's not overcomplicate
things. I see value in keeping interface symmetric.
We can always change things according to needs.
--
Kiryl Shutsemau / Kirill A. Shutemov
> > > @@ -835,6 +835,13 @@ void __noreturn stop_this_cpu(void *dummy)
> > > */
> > > cpumask_clear_cpu(cpu, &cpus_stop_mask);
> > >
> > > +#ifdef CONFIG_SMP
> > > + if (smp_ops.stop_this_cpu) {
> > > + smp_ops.stop_this_cpu();
> > > + unreachable();
> > > + }
> > > +#endif
> >
> > If I read correctly this will result in stop_this_cpu() having different
> > behaviour for SMP and !SMP build for TDX guest. For example, AFAICT
> > machine_halt() also calls stop_this_cpu() on local cpu after it stops other
> > cpus. So for the local cpu, in SMP build it will calls into BIOS's reset vector
> > but in !SMP it will call native_halt().
>
> It doesn't make a difference in practice: both halt and giving CPU to
> BIOS will be unrecoverable operation. Both are equally acceptable for
> machine_halt().
>
OK fair enough. :-)
On 23.12.23 г. 1:52 ч., Kirill A. Shutemov wrote:
> TDX guests allocate shared buffers to perform I/O. It is done by
> allocating pages normally from the buddy allocator and converting them
> to shared with set_memory_decrypted().
>
> The second kernel has no idea what memory is converted this way. It only
> sees E820_TYPE_RAM.
>
> Accessing shared memory via private mapping is fatal. It leads to
> unrecoverable TD exit.
>
> On kexec walk direct mapping and convert all shared memory back to
> private. It makes all RAM private again and second kernel may use it
> normally.
>
> The conversion occurs in two steps: stopping new conversions and
> unsharing all memory. In the case of normal kexec, the stopping of
> conversions takes place while scheduling is still functioning. This
> allows for waiting until any ongoing conversions are finished. The
> second step is carried out when all CPUs except one are inactive and
> interrupts are disabled. This prevents any conflicts with code that may
> access shared memory.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> Reviewed-by: Rick Edgecombe <[email protected]>
> ---
> arch/x86/coco/tdx/tdx.c | 119 +++++++++++++++++++++++++++++++-
> arch/x86/include/asm/x86_init.h | 2 +
> arch/x86/kernel/crash.c | 6 ++
> arch/x86/kernel/reboot.c | 13 ++++
> 4 files changed, 138 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 8a49484a2917..5c64db168edd 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -6,8 +6,10 @@
>
> #include <linux/cpufeature.h>
> #include <linux/debugfs.h>
> +#include <linux/delay.h>
> #include <linux/export.h>
> #include <linux/io.h>
> +#include <linux/kexec.h>
> #include <asm/coco.h>
> #include <asm/tdx.h>
> #include <asm/vmx.h>
> @@ -15,6 +17,7 @@
> #include <asm/insn.h>
> #include <asm/insn-eval.h>
> #include <asm/pgtable.h>
> +#include <asm/set_memory.h>
>
> /* MMIO direction */
> #define EPT_READ 0
> @@ -41,6 +44,9 @@
>
> static atomic_long_t nr_shared;
>
> +static atomic_t conversions_in_progress;
> +static bool conversion_allowed = true;
Given the usage model of this variable, shouldn't it be simply accessed
via READ/WRITE_ONCE macros?
> +
> static inline bool pte_decrypted(pte_t pte)
> {
> return cc_mkdec(pte_val(pte)) == pte_val(pte);
> @@ -726,6 +732,14 @@ static bool tdx_tlb_flush_required(bool private)
>
> static bool tdx_cache_flush_required(void)
> {
> + /*
> + * Avoid issuing CLFLUSH on set_memory_decrypted() if conversions
> + * stopped. Otherwise it can race with unshare_all_memory() and trigger
> + * implicit conversion to shared.
> + */
> + if (!conversion_allowed)
> + return false;
> +
> /*
> * AMD SME/SEV can avoid cache flushing if HW enforces cache coherence.
> * TDX doesn't have such capability.
> @@ -809,12 +823,25 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> bool enc)
> {
> + atomic_inc(&conversions_in_progress);
> +
> + /*
> + * Check after bumping conversions_in_progress to serialize
> + * against tdx_shutdown().
> + */
> + if (!conversion_allowed) {
> + atomic_dec(&conversions_in_progress);
> + return -EBUSY;
> + }
nit: Can you make the inc of conversions_in_progress be done here, this
eliminated the atomic_dec in case they aren't. Somewhat simplifies the
logic.
> +
> /*
> * Only handle shared->private conversion here.
> * See the comment in tdx_early_init().
> */
> - if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> + if (enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
> + atomic_dec(&conversions_in_progress);
> return -EIO;
> + }
>
> return 0;
> }
> @@ -826,17 +853,102 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> * Only handle private->shared conversion here.
> * See the comment in tdx_early_init().
> */
> - if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> + if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
> + atomic_dec(&conversions_in_progress);
> return -EIO;
> + }
>
> if (enc)
> atomic_long_sub(numpages, &nr_shared);
> else
> atomic_long_add(numpages, &nr_shared);
>
> + atomic_dec(&conversions_in_progress);
> +
> return 0;
> }
>
> +static void tdx_kexec_stop_conversion(bool crash)
> +{
> + /* Stop new private<->shared conversions */
> + conversion_allowed = false;
What's the logic behind this compiler barrier?
> + barrier();
> +
> + /*
> + * Crash kernel reaches here with interrupts disabled: can't wait for
> + * conversions to finish.
> + *
> + * If race happened, just report and proceed.
> + */
> + if (!crash) {
> + unsigned long timeout;
> +
> + /*
> + * Wait for in-flight conversions to complete.
> + *
> + * Do not wait more than 30 seconds.
> + */
> + timeout = 30 * USEC_PER_SEC;
> + while (atomic_read(&conversions_in_progress) && timeout--)
> + udelay(1);
> + }
> +
> + if (atomic_read(&conversions_in_progress))
> + pr_warn("Failed to finish shared<->private conversions\n");
> +}
> +
<snip>
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index c9503fe2d13a..3196ff20a29e 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -154,6 +154,8 @@ struct x86_guest {
> int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
> bool (*enc_tlb_flush_required)(bool enc);
> bool (*enc_cache_flush_required)(void);
> + void (*enc_kexec_stop_conversion)(bool crash);
> + void (*enc_kexec_unshare_mem)(void);
These are only being initialized in the TDX case, but called in all
cases when CC_ATTR_GUEST_MEM_ENCRYPT is true, which includes AMD. So it
would cause a crash, no ? Shouldn't you also introduce noop handlers
initialized in the default x86_platform struct in
arch/x86/kernel/x86_init.c ?
> };
>
> /**
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index c92d88680dbf..b99bd28ad22f 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -40,6 +40,7 @@
> #include <asm/intel_pt.h>
> #include <asm/crash.h>
> #include <asm/cmdline.h>
> +#include <asm/tdx.h>
>
> /* Used while preparing memory map entries for second kernel */
> struct crash_memmap_data {
> @@ -107,6 +108,11 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
>
> crash_smp_send_stop();
>
> + if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
> + x86_platform.guest.enc_kexec_stop_conversion(true);
> + x86_platform.guest.enc_kexec_unshare_mem();
> + }
> +
> cpu_emergency_disable_virtualization();
>
> /*
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 830425e6d38e..16dde83df49a 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -12,6 +12,7 @@
> #include <linux/delay.h>
> #include <linux/objtool.h>
> #include <linux/pgtable.h>
> +#include <linux/kexec.h>
> #include <acpi/reboot.h>
> #include <asm/io.h>
> #include <asm/apic.h>
> @@ -31,6 +32,7 @@
> #include <asm/realmode.h>
> #include <asm/x86_init.h>
> #include <asm/efi.h>
> +#include <asm/tdx.h>
>
> /*
> * Power off function, if any
> @@ -716,6 +718,14 @@ static void native_machine_emergency_restart(void)
>
> void native_machine_shutdown(void)
> {
> + /*
> + * Call enc_kexec_stop_conversion() while all CPUs are still active and
> + * interrupts are enabled. This will allow all in-flight memory
> + * conversions to finish cleanly.
> + */
> + if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) && kexec_in_progress)
> + x86_platform.guest.enc_kexec_stop_conversion(false);
> +
> /* Stop the cpus and apics */
> #ifdef CONFIG_X86_IO_APIC
> /*
> @@ -752,6 +762,9 @@ void native_machine_shutdown(void)
> #ifdef CONFIG_X86_64
> x86_platform.iommu_shutdown();
> #endif
> +
> + if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) && kexec_in_progress)
> + x86_platform.guest.enc_kexec_unshare_mem();
> }
>
> static void __machine_emergency_restart(int emergency)
On 23.12.23 г. 1:52 ч., Kirill A. Shutemov wrote:
> MADT Multiprocessor Wakeup structure version 1 brings support of CPU
> offlining: BIOS provides a reset vector where the CPU has to jump to
> for offlining itself. The new TEST mailbox command can be used to test
> whether the CPU offlined itself which means the BIOS has control over
> the CPU and can online it again via the ACPI MADT wakeup method.
>
> Add CPU offling support for the ACPI MADT wakeup method by implementing
> custom cpu_die(), play_dead() and stop_this_cpu() SMP operations.
>
> CPU offlining makes is possible to hand over secondary CPUs over kexec,
> not limiting the second kernel to a single CPU.
>
> The change conforms to the approved ACPI spec change proposal. See the
> Link.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> Link: https://lore.kernel.org/all/13356251.uLZWGnKmhe@kreacher
> ---
> arch/x86/include/asm/acpi.h | 2 +
> arch/x86/kernel/acpi/Makefile | 2 +-
> arch/x86/kernel/acpi/madt_playdead.S | 29 +++++
> arch/x86/kernel/acpi/madt_wakeup.c | 184 ++++++++++++++++++++++++++-
> include/acpi/actbl2.h | 15 ++-
> 5 files changed, 228 insertions(+), 4 deletions(-)
> create mode 100644 arch/x86/kernel/acpi/madt_playdead.S
>
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index 2625b915ae7f..021cafa214c2 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -81,6 +81,8 @@ union acpi_subtable_headers;
> int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> const unsigned long end);
>
> +void asm_acpi_mp_play_dead(u64 reset_vector, u64 pgd_pa);
> +
> /*
> * Check if the CPU can handle C2 and deeper
> */
> diff --git a/arch/x86/kernel/acpi/Makefile b/arch/x86/kernel/acpi/Makefile
> index 8c7329c88a75..37b1f28846de 100644
> --- a/arch/x86/kernel/acpi/Makefile
> +++ b/arch/x86/kernel/acpi/Makefile
> @@ -4,7 +4,7 @@ obj-$(CONFIG_ACPI) += boot.o
> obj-$(CONFIG_ACPI_SLEEP) += sleep.o wakeup_$(BITS).o
> obj-$(CONFIG_ACPI_APEI) += apei.o
> obj-$(CONFIG_ACPI_CPPC_LIB) += cppc.o
> -obj-$(CONFIG_X86_ACPI_MADT_WAKEUP) += madt_wakeup.o
> +obj-$(CONFIG_X86_ACPI_MADT_WAKEUP) += madt_wakeup.o madt_playdead.o
>
> ifneq ($(CONFIG_ACPI_PROCESSOR),)
> obj-y += cstate.o
> diff --git a/arch/x86/kernel/acpi/madt_playdead.S b/arch/x86/kernel/acpi/madt_playdead.S
> new file mode 100644
> index 000000000000..e48049959513
> --- /dev/null
> +++ b/arch/x86/kernel/acpi/madt_playdead.S
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <linux/linkage.h>
> +#include <asm/nospec-branch.h>
> +#include <asm/page_types.h>
> +#include <asm/processor-flags.h>
> +
> + .text
> + .align PAGE_SIZE
> +
> +/*
> + * asm_acpi_mp_play_dead() - Hand over control of the CPU to the BIOS
> + *
> + * rdi: Address of the ACPI MADT MPWK ResetVector
> + * rsi: PGD of the identity mapping
> + */
> +SYM_FUNC_START(asm_acpi_mp_play_dead)
> + /* Turn off global entries. Following CR3 write will flush them. */
> + movq %cr4, %rdx
> + andq $~(X86_CR4_PGE), %rdx
> + movq %rdx, %cr4
> +
> + /* Switch to identity mapping */
> + movq %rsi, %rax
> + movq %rax, %cr3
nit: Can't you move directly to cr3
> +
> + /* Jump to reset vector */
> + ANNOTATE_RETPOLINE_SAFE
> + jmp *%rdi
> +SYM_FUNC_END(asm_acpi_mp_play_dead)
<snip>
On Mon, Jan 15, 2024 at 03:19:43PM +0200, Nikolay Borisov wrote:
> > + /* Switch to identity mapping */
> > + movq %rsi, %rax
> > + movq %rax, %cr3
>
> nit: Can't you move directly to cr3
For some reason, I thought I couldn't. Will fix.
--
Kiryl Shutsemau / Kirill A. Shutemov
On Mon, Jan 15, 2024 at 12:53:42PM +0200, Nikolay Borisov wrote:
>
>
> On 23.12.23 г. 1:52 ч., Kirill A. Shutemov wrote:
> > TDX guests allocate shared buffers to perform I/O. It is done by
> > allocating pages normally from the buddy allocator and converting them
> > to shared with set_memory_decrypted().
> >
> > The second kernel has no idea what memory is converted this way. It only
> > sees E820_TYPE_RAM.
> >
> > Accessing shared memory via private mapping is fatal. It leads to
> > unrecoverable TD exit.
> >
> > On kexec walk direct mapping and convert all shared memory back to
> > private. It makes all RAM private again and second kernel may use it
> > normally.
> >
> > The conversion occurs in two steps: stopping new conversions and
> > unsharing all memory. In the case of normal kexec, the stopping of
> > conversions takes place while scheduling is still functioning. This
> > allows for waiting until any ongoing conversions are finished. The
> > second step is carried out when all CPUs except one are inactive and
> > interrupts are disabled. This prevents any conflicts with code that may
> > access shared memory.
> >
> > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > Reviewed-by: Rick Edgecombe <[email protected]>
> > ---
> > arch/x86/coco/tdx/tdx.c | 119 +++++++++++++++++++++++++++++++-
> > arch/x86/include/asm/x86_init.h | 2 +
> > arch/x86/kernel/crash.c | 6 ++
> > arch/x86/kernel/reboot.c | 13 ++++
> > 4 files changed, 138 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> > index 8a49484a2917..5c64db168edd 100644
> > --- a/arch/x86/coco/tdx/tdx.c
> > +++ b/arch/x86/coco/tdx/tdx.c
> > @@ -6,8 +6,10 @@
> > #include <linux/cpufeature.h>
> > #include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > #include <linux/export.h>
> > #include <linux/io.h>
> > +#include <linux/kexec.h>
> > #include <asm/coco.h>
> > #include <asm/tdx.h>
> > #include <asm/vmx.h>
> > @@ -15,6 +17,7 @@
> > #include <asm/insn.h>
> > #include <asm/insn-eval.h>
> > #include <asm/pgtable.h>
> > +#include <asm/set_memory.h>
> > /* MMIO direction */
> > #define EPT_READ 0
> > @@ -41,6 +44,9 @@
> > static atomic_long_t nr_shared;
> > +static atomic_t conversions_in_progress;
> > +static bool conversion_allowed = true;
>
> Given the usage model of this variable, shouldn't it be simply accessed via
> READ/WRITE_ONCE macros?
What do you see it changing?
> > +
> > static inline bool pte_decrypted(pte_t pte)
> > {
> > return cc_mkdec(pte_val(pte)) == pte_val(pte);
> > @@ -726,6 +732,14 @@ static bool tdx_tlb_flush_required(bool private)
> > static bool tdx_cache_flush_required(void)
> > {
> > + /*
> > + * Avoid issuing CLFLUSH on set_memory_decrypted() if conversions
> > + * stopped. Otherwise it can race with unshare_all_memory() and trigger
> > + * implicit conversion to shared.
> > + */
> > + if (!conversion_allowed)
> > + return false;
> > +
> > /*
> > * AMD SME/SEV can avoid cache flushing if HW enforces cache coherence.
> > * TDX doesn't have such capability.
> > @@ -809,12 +823,25 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> > static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> > bool enc)
> > {
> > + atomic_inc(&conversions_in_progress);
> > +
> > + /*
> > + * Check after bumping conversions_in_progress to serialize
> > + * against tdx_shutdown().
> > + */
> > + if (!conversion_allowed) {
> > + atomic_dec(&conversions_in_progress);
> > + return -EBUSY;
> > + }
>
> nit: Can you make the inc of conversions_in_progress be done here, this
> eliminated the atomic_dec in case they aren't. Somewhat simplifies the
> logic.
Okay, fair enough. Will change.
> > +
> > /*
> > * Only handle shared->private conversion here.
> > * See the comment in tdx_early_init().
> > */
> > - if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> > + if (enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
> > + atomic_dec(&conversions_in_progress);
> > return -EIO;
> > + }
> > return 0;
> > }
> > @@ -826,17 +853,102 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> > * Only handle private->shared conversion here.
> > * See the comment in tdx_early_init().
> > */
> > - if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> > + if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
> > + atomic_dec(&conversions_in_progress);
> > return -EIO;
> > + }
> > if (enc)
> > atomic_long_sub(numpages, &nr_shared);
> > else
> > atomic_long_add(numpages, &nr_shared);
> > + atomic_dec(&conversions_in_progress);
> > +
> > return 0;
> > }
> > +static void tdx_kexec_stop_conversion(bool crash)
> > +{
> > + /* Stop new private<->shared conversions */
> > + conversion_allowed = false;
>
> What's the logic behind this compiler barrier?
Disallow compiler to push the assignment past atomic_read() loop below.
Not sure if anything else prevents such reorder without the barrier.
And I don't think WRITE_ONCE() will do the trick. It only prevents
multiple writes, but doesn't prevent reorders agains accesses
non-READ_ONCE()/WRITE_ONCE() accesses.
> > + barrier();
> > +
> > + /*
> > + * Crash kernel reaches here with interrupts disabled: can't wait for
> > + * conversions to finish.
> > + *
> > + * If race happened, just report and proceed.
> > + */
> > + if (!crash) {
> > + unsigned long timeout;
> > +
> > + /*
> > + * Wait for in-flight conversions to complete.
> > + *
> > + * Do not wait more than 30 seconds.
> > + */
> > + timeout = 30 * USEC_PER_SEC;
> > + while (atomic_read(&conversions_in_progress) && timeout--)
> > + udelay(1);
> > + }
> > +
> > + if (atomic_read(&conversions_in_progress))
> > + pr_warn("Failed to finish shared<->private conversions\n");
> > +}
> > +
>
> <snip>
>
> > diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> > index c9503fe2d13a..3196ff20a29e 100644
> > --- a/arch/x86/include/asm/x86_init.h
> > +++ b/arch/x86/include/asm/x86_init.h
> > @@ -154,6 +154,8 @@ struct x86_guest {
> > int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
> > bool (*enc_tlb_flush_required)(bool enc);
> > bool (*enc_cache_flush_required)(void);
> > + void (*enc_kexec_stop_conversion)(bool crash);
> > + void (*enc_kexec_unshare_mem)(void);
>
> These are only being initialized in the TDX case, but called in all cases
> when CC_ATTR_GUEST_MEM_ENCRYPT is true, which includes AMD. So it would
> cause a crash, no ? Shouldn't you also introduce noop handlers initialized
> in the default x86_platform struct in arch/x86/kernel/x86_init.c ?
kexec on AMD will not work without them, I think. But noops makes sense
anyway. Will fix.
--
Kiryl Shutsemau / Kirill A. Shutemov
On 16.01.24 г. 9:28 ч., Kirill A. Shutemov wrote:
<snip>
>>> @@ -41,6 +44,9 @@
>>> static atomic_long_t nr_shared;
>>> +static atomic_t conversions_in_progress;
>>> +static bool conversion_allowed = true;
>>
>> Given the usage model of this variable, shouldn't it be simply accessed via
>> READ/WRITE_ONCE macros?
>
> What do you see it changing?
Serving as documentation that you are accessing a shared variable
without an explicit lock (unless I'm missing something).
conversion_allowed can be read by multiple threads, no ? And it's
written by a single thread?
>
<snip>
>>> +static void tdx_kexec_stop_conversion(bool crash)
>>> +{
>>> + /* Stop new private<->shared conversions */
>>> + conversion_allowed = false;
>>
>> What's the logic behind this compiler barrier?
>
> Disallow compiler to push the assignment past atomic_read() loop below.
> Not sure if anything else prevents such reorder without the barrier.
>
> And I don't think WRITE_ONCE() will do the trick. It only prevents
> multiple writes, but doesn't prevent reorders agains accesses
> non-READ_ONCE()/WRITE_ONCE() accesses.
>
>>> + barrier();
>>> +
>>> + /*
>>> + * Crash kernel reaches here with interrupts disabled: can't wait for
>>> + * conversions to finish.
>>> + *
>>> + * If race happened, just report and proceed.
>>> + */
>>> + if (!crash) {
>>> + unsigned long timeout;
>>> +
>>> + /*
>>> + * Wait for in-flight conversions to complete.
>>> + *
>>> + * Do not wait more than 30 seconds.
>>> + */
>>> + timeout = 30 * USEC_PER_SEC;
>>> + while (atomic_read(&conversions_in_progress) && timeout--)
>>> + udelay(1);
>>> + }
>>> +
>>> + if (atomic_read(&conversions_in_progress))
>>> + pr_warn("Failed to finish shared<->private conversions\n");
>>> +}
>>> +
>>
>> <snip>
>>
>>> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
>>> index c9503fe2d13a..3196ff20a29e 100644
>>> --- a/arch/x86/include/asm/x86_init.h
>>> +++ b/arch/x86/include/asm/x86_init.h
>>> @@ -154,6 +154,8 @@ struct x86_guest {
>>> int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool enc);
>>> bool (*enc_tlb_flush_required)(bool enc);
>>> bool (*enc_cache_flush_required)(void);
>>> + void (*enc_kexec_stop_conversion)(bool crash);
>>> + void (*enc_kexec_unshare_mem)(void);
>>
>> These are only being initialized in the TDX case, but called in all cases
>> when CC_ATTR_GUEST_MEM_ENCRYPT is true, which includes AMD. So it would
>> cause a crash, no ? Shouldn't you also introduce noop handlers initialized
>> in the default x86_platform struct in arch/x86/kernel/x86_init.c ?
>
> kexec on AMD will not work without them, I think. But noops makes sense
> anyway. Will fix.
I'm not disputing whether those are needed for AMD or not, that way I
see it you make those callbacks mandatory in the case of
CC_ATTR_GUEST_MEM_ENCRYPT being present, yet only implement them for
TDX. So in the case of AMD they will be NULL and so AMD with kexec
enabled (albeit erroneously) will crash, no ?
>
On Tue, Jan 16, 2024 at 10:01:47AM +0200, Nikolay Borisov wrote:
>
>
> On 16.01.24 г. 9:28 ч., Kirill A. Shutemov wrote:
>
> <snip>
>
> > > > @@ -41,6 +44,9 @@
> > > > static atomic_long_t nr_shared;
> > > > +static atomic_t conversions_in_progress;
> > > > +static bool conversion_allowed = true;
> > >
> > > Given the usage model of this variable, shouldn't it be simply accessed via
> > > READ/WRITE_ONCE macros?
> >
> > What do you see it changing?
>
>
> Serving as documentation that you are accessing a shared variable without an
> explicit lock (unless I'm missing something). conversion_allowed can be read
> by multiple threads, no ? And it's written by a single thread?
I don't think READ_ONCE()/WRITE_ONCE() have documentation sense you imply.
I would argue adding them will add more confusion as they serve no purpose
in this context: issuing multiple loads/stores for the variable have no
impact on outcome.
> > kexec on AMD will not work without them, I think. But noops makes sense
> > anyway. Will fix.
>
> I'm not disputing whether those are needed for AMD or not, that way I see it
> you make those callbacks mandatory in the case of CC_ATTR_GUEST_MEM_ENCRYPT
> being present, yet only implement them for TDX. So in the case of AMD they
> will be NULL and so AMD with kexec enabled (albeit erroneously) will crash,
> no ?
As I said, I will fix it.
--
Kiryl Shutsemau / Kirill A. Shutemov
On Tue, Jan 16, 2024 at 10:28:22AM +0300, Kirill A. Shutemov wrote:
> > > @@ -809,12 +823,25 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> > > static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> > > bool enc)
> > > {
> > > + atomic_inc(&conversions_in_progress);
> > > +
> > > + /*
> > > + * Check after bumping conversions_in_progress to serialize
> > > + * against tdx_shutdown().
s/tdx_shutdown/tdx_kexec_stop_conversion/
> > > + */
> > > + if (!conversion_allowed) {
> > > + atomic_dec(&conversions_in_progress);
> > > + return -EBUSY;
> > > + }
> >
> > nit: Can you make the inc of conversions_in_progress be done here, this
> > eliminated the atomic_dec in case they aren't. Somewhat simplifies the
> > logic.
>
> Okay, fair enough. Will change.
Actually, no, it will breaks serialization.
Consider following scenario (includes change you proposed):
CPU0 CPU1
tdx_enc_status_change_prepare()
if (!conversion_allowed) // false
tdx_kexec_stop_conversion()
conversion_allowed = false;
<...>
<return as no conversion in flight>
<continue with conversion>
Incrementing conversion_in_progress before checking conversion_allowed
will make tdx_kexec_stop_conversion() to spin until CPU0 is done with
conversion.
--
Kiryl Shutsemau / Kirill A. Shutemov