Re: [PATCHv6 14/16] x86/smp: Add smp_ops.stop_this_cpu() callback

Re: [PATCHv6 14/16] x86/smp: Add smp_ops.stop_this_cpu() callback

[Huang, Kai]

On Wed, 2024-01-24 at 14:55 +0200, 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 <kirill.shutemov@linux.intel.com>

Acked-by: Kai Huang <kai.huang@intel.com>

> ---
>  arch/x86/include/asm/smp.h | 1 +
>  arch/x86/kernel/process.c  | 7 +++++++
>  arch/x86/kernel/reboot.c   | 6 ++++++
>  3 files changed, 14 insertions(+)
> 
> 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 ab49ade31b0d..00c1b957476d 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 0574d4ad6b41..0a75efe579c0 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -880,6 +880,12 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
>  	cpu_emergency_disable_virtualization();
>  
>  	atomic_dec(&waiting_for_crash_ipi);
> +
> +	if (smp_ops.stop_this_cpu) {
> +		smp_ops.stop_this_cpu();
> +		unreachable();
> +	}
> +
>  	/* Assume hlt works */
>  	halt();
>  	for (;;)

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 15/16] x86/mm: Introduce kernel_ident_mapping_free()

[Huang, Kai]

On Wed, 2024-01-24 at 14:55 +0200, 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 <kirill.shutemov@linux.intel.com>

FWIW:

Acked-by: Kai Huang <kai.huang@intel.com>

> ---
>  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)
>  {

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 16/16] x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method

[Huang, Kai]

On Wed, 2024-01-24 at 14:55 +0200, 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 <kirill.shutemov@linux.intel.com>
> Link: https://lore.kernel.org/all/13356251.uLZWGnKmhe@kreacher

FWIW:

Acked-by: Kai Huang <kai.huang@intel.com>

[...]

> +
> +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,
> +	};

Nit: Reverse Christmas-tree style

[...]

>  
> -#define ACPI_MP_WAKE_COMMAND_WAKEUP    1
> +#define ACPI_MP_WAKE_COMMAND_WAKEUP	1

Nit: Is this change intended?

> +#define ACPI_MP_WAKE_COMMAND_TEST	2
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 16/16] x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method

[kirill.shutemov]

On Fri, Jan 26, 2024 at 02:21:30PM +0000, Huang, Kai wrote:
> > +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,
> > +	};
> 
> Nit: Reverse Christmas-tree style
> 
> [...]

Okay, will fix if new version is required.

> >  
> > -#define ACPI_MP_WAKE_COMMAND_WAKEUP    1
> > +#define ACPI_MP_WAKE_COMMAND_WAKEUP	1
> 
> Nit: Is this change intended?

Yes. Changed to indentation with tabs.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 16/16] x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method

[Kuppuswamy Sathyanarayanan]

On 1/24/24 4:55 AM, 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 <kirill.shutemov@linux.intel.com>
> Link: https://lore.kernel.org/all/13356251.uLZWGnKmhe@kreacher
> ---

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  arch/x86/include/asm/acpi.h          |   2 +
>  arch/x86/kernel/acpi/Makefile        |   2 +-
>  arch/x86/kernel/acpi/madt_playdead.S |  28 ++++
>  arch/x86/kernel/acpi/madt_wakeup.c   | 184 ++++++++++++++++++++++++++-
>  include/acpi/actbl2.h                |  15 ++-
>  5 files changed, 227 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..4e498d28cdc8
> --- /dev/null
> +++ b/arch/x86/kernel/acpi/madt_playdead.S
> @@ -0,0 +1,28 @@
> +/* 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, %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)
Nit: IMO, since you are dumping failure error message (not timeout
message), you can use non zero acpi_mp_wake_mailbox->command
check. But it is up to you.
> +		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 e1a395af7591..2aedda70ef88 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -1120,8 +1120,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
>  
> @@ -1134,7 +1146,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) */
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 16/16] x86/acpi: Add support for CPU offlining for ACPI MADT wakeup method

[Kirill A. Shutemov]

On Fri, Jan 26, 2024 at 12:03:14PM -0800, Kuppuswamy Sathyanarayanan wrote:
> > +	/* Don't wait longer than a second. */
> > +	timeout = USEC_PER_SEC;
> > +	while (READ_ONCE(acpi_mp_wake_mailbox->command) && --timeout)
> > +		udelay(1);
> > +
> > +	if (!timeout)
> Nit: IMO, since you are dumping failure error message (not timeout
> message), you can use non zero acpi_mp_wake_mailbox->command
> check. But it is up to you.

I think my version is pretty idiomatic. The same pattern used in other
places. For instance, test_nmi_ipi().
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 09/16] x86/mm: Adding callbacks to prepare encrypted memory for kexec

[Nikolay Borisov]

On 24.01.24 г. 14:55 ч., Kirill A. Shutemov wrote:
> AMD SEV and Intel TDX guests allocate shared buffers for performing I/O.
> This is done by allocating pages normally from the buddy allocator and
> then converting them to shared using set_memory_decrypted().
> 
> On kexec, the second kernel is unaware of which memory has been
> converted in this manner. It only sees E820_TYPE_RAM. Accessing shared
> memory as private is fatal.
> 
> Therefore, the memory state must be reset to its original state before
> starting the new kernel with kexec.
> 
> The process of converting shared memory back to private occurs in two
> steps:
> 
> - enc_kexec_stop_conversion() stops new conversions.
> 
> - enc_kexec_unshare_mem() unshares all existing shared memory, reverting
>    it back to private.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 10/16] x86/tdx: Convert shared memory back to private on kexec

[Kalra, Ashish]

Hello Kirill,

On 1/24/2024 6:55 AM, 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 <kirill.shutemov@linux.intel.com>
> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>   arch/x86/coco/tdx/tdx.c | 124 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 122 insertions(+), 2 deletions(-)
<snip>
> +static void tdx_kexec_stop_conversion(bool crash)
> +{
> +	/* Stop new private<->shared conversions */
> +	conversion_allowed = false;
> +
> +	/*
> +	 * Make sure conversion_allowed is cleared before checking
> +	 * conversions_in_progress.
> +	 */
> +	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);
> +	}
> +}
In case of SNP and crash/kdump case, we need to prevent the boot_ghcb 
being converted to shared (in snp_kexec_unshare_mem()) as the boot_ghcb 
is required to handle all I/O for disabling IO-APIC/lapic, hpet, etc., 
as the enc_kexec_unshare_mem() callback is invoked before the apics, 
hpet, etc. are disabled.

Is there any reason why enc_kexec_unshare_mem() callback is invoked in 
crash case before the IO-APIC/lapic, hpet, etc. are shutdown/disabled ?

In case of kexec, enc_kexec_unshare_mem() callback is invoked after the 
IO-APIC/lapic, hpet, iommu, etc. have already been disabled/shutdown, 
hence, this callback can transition all guest shared memory (including 
boot_ghcb) back to private.

Thanks, Ashish


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 10/16] x86/tdx: Convert shared memory back to private on kexec

[Kirill A. Shutemov]

On Mon, Jan 29, 2024 at 04:24:09AM -0600, Kalra, Ashish wrote:
> In case of SNP and crash/kdump case, we need to prevent the boot_ghcb being
> converted to shared (in snp_kexec_unshare_mem()) as the boot_ghcb is
> required to handle all I/O for disabling IO-APIC/lapic, hpet, etc., as the
> enc_kexec_unshare_mem() callback is invoked before the apics, hpet, etc. are
> disabled.
> 
> Is there any reason why enc_kexec_unshare_mem() callback is invoked in crash
> case before the IO-APIC/lapic, hpet, etc. are shutdown/disabled ?

Not really. Either way works for TDX. I've tested the patch below. Is it
what you want?

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 6585a5f2c2ba..3001f4857ed7 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -107,11 +107,6 @@ 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();
 
 	/*
@@ -129,6 +124,12 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
 #ifdef CONFIG_HPET_TIMER
 	hpet_disable();
 #endif
+
+	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
+		x86_platform.guest.enc_kexec_stop_conversion(true);
+		x86_platform.guest.enc_kexec_unshare_mem();
+	}
+
 	crash_save_cpu(regs, safe_smp_processor_id());
 }
 
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 10/16] x86/tdx: Convert shared memory back to private on kexec

[Kalra, Ashish]

Hello Kirill,

On 1/29/2024 4:36 AM, Kirill A. Shutemov wrote:
> On Mon, Jan 29, 2024 at 04:24:09AM -0600, Kalra, Ashish wrote:
>> In case of SNP and crash/kdump case, we need to prevent the boot_ghcb being
>> converted to shared (in snp_kexec_unshare_mem()) as the boot_ghcb is
>> required to handle all I/O for disabling IO-APIC/lapic, hpet, etc., as the
>> enc_kexec_unshare_mem() callback is invoked before the apics, hpet, etc. are
>> disabled.
>>
>> Is there any reason why enc_kexec_unshare_mem() callback is invoked in crash
>> case before the IO-APIC/lapic, hpet, etc. are shutdown/disabled ?
> Not really. Either way works for TDX. I've tested the patch below. Is it
> what you want?
>
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index 6585a5f2c2ba..3001f4857ed7 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -107,11 +107,6 @@ 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();
>   
>   	/*
> @@ -129,6 +124,12 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
>   #ifdef CONFIG_HPET_TIMER
>   	hpet_disable();
>   #endif
> +
> +	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
> +		x86_platform.guest.enc_kexec_stop_conversion(true);
> +		x86_platform.guest.enc_kexec_unshare_mem();
> +	}
> +
>   	crash_save_cpu(regs, safe_smp_processor_id());
>   }
>   

Yes, this will work for SNP.

Thanks, Ashish


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 10/16] x86/tdx: Convert shared memory back to private on kexec

[Kirill A. Shutemov]

On Mon, Jan 29, 2024 at 07:09:37AM -0600, Kalra, Ashish wrote:
> Hello Kirill,
> 
> On 1/29/2024 4:36 AM, Kirill A. Shutemov wrote:
> > On Mon, Jan 29, 2024 at 04:24:09AM -0600, Kalra, Ashish wrote:
> > > In case of SNP and crash/kdump case, we need to prevent the boot_ghcb being
> > > converted to shared (in snp_kexec_unshare_mem()) as the boot_ghcb is
> > > required to handle all I/O for disabling IO-APIC/lapic, hpet, etc., as the
> > > enc_kexec_unshare_mem() callback is invoked before the apics, hpet, etc. are
> > > disabled.
> > > 
> > > Is there any reason why enc_kexec_unshare_mem() callback is invoked in crash
> > > case before the IO-APIC/lapic, hpet, etc. are shutdown/disabled ?
> > Not really. Either way works for TDX. I've tested the patch below. Is it
> > what you want?
> > 
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index 6585a5f2c2ba..3001f4857ed7 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -107,11 +107,6 @@ 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();
> >   	/*
> > @@ -129,6 +124,12 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
> >   #ifdef CONFIG_HPET_TIMER
> >   	hpet_disable();
> >   #endif
> > +
> > +	if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
> > +		x86_platform.guest.enc_kexec_stop_conversion(true);
> > +		x86_platform.guest.enc_kexec_unshare_mem();
> > +	}
> > +
> >   	crash_save_cpu(regs, safe_smp_processor_id());
> >   }
> 
> Yes, this will work for SNP.

Okay, good. Will include the change into the next version of the patchset.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 00/16] x86/tdx: Add kexec support

[Paolo Bonzini]

On 1/24/24 13:55, Kirill A. Shutemov wrote:
> 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.

Hi Kirill,

I have a very basic question: is there a reason why this series does not 
revert commit cb8eb06d50fc, "x86/virt/tdx: Disable TDX host support when 
kexec is enabled"?

Thanks,

Paolo


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 00/16] x86/tdx: Add kexec support

[Kirill A. Shutemov]

On Tue, Jan 30, 2024 at 02:43:15PM +0100, Paolo Bonzini wrote:
> On 1/24/24 13:55, Kirill A. Shutemov wrote:
> > 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.
> 
> Hi Kirill,
> 
> I have a very basic question: is there a reason why this series does not
> revert commit cb8eb06d50fc, "x86/virt/tdx: Disable TDX host support when
> kexec is enabled"?

My patchset enables kexec for TDX guest. The commit you refer blocks kexec
for host. TDX host and guest have totally different problems with handling
kexec. Kai looks on how to get host kexec functional.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 00/16] x86/tdx: Add kexec support

[Paolo Bonzini]

On Tue, Jan 30, 2024 at 3:34 PM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Tue, Jan 30, 2024 at 02:43:15PM +0100, Paolo Bonzini wrote:
> > On 1/24/24 13:55, Kirill A. Shutemov wrote:
> > > 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.
> >
> > Hi Kirill,
> >
> > I have a very basic question: is there a reason why this series does not
> > revert commit cb8eb06d50fc, "x86/virt/tdx: Disable TDX host support when
> > kexec is enabled"?
>
> My patchset enables kexec for TDX guest. The commit you refer blocks kexec
> for host. TDX host and guest have totally different problems with handling
> kexec. Kai looks on how to get host kexec functional.

Yeah, that was right there in the cover letter (and I should have
gotten a clue from the many references to CC_* constants...). Somebody
pointed me to this series as "the TDX kexec series from Intel" and I
had some tunnel vision issues. Sorry for the noise!

But since I have your attention, do you have a pointer to the
corresponding edk2 series? Thanks,

Paolo


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 00/16] x86/tdx: Add kexec support

[Kirill A. Shutemov]

On Tue, Jan 30, 2024 at 03:59:34PM +0100, Paolo Bonzini wrote:
> On Tue, Jan 30, 2024 at 3:34 PM Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >
> > On Tue, Jan 30, 2024 at 02:43:15PM +0100, Paolo Bonzini wrote:
> > > On 1/24/24 13:55, Kirill A. Shutemov wrote:
> > > > 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.
> > >
> > > Hi Kirill,
> > >
> > > I have a very basic question: is there a reason why this series does not
> > > revert commit cb8eb06d50fc, "x86/virt/tdx: Disable TDX host support when
> > > kexec is enabled"?
> >
> > My patchset enables kexec for TDX guest. The commit you refer blocks kexec
> > for host. TDX host and guest have totally different problems with handling
> > kexec. Kai looks on how to get host kexec functional.
> 
> Yeah, that was right there in the cover letter (and I should have
> gotten a clue from the many references to CC_* constants...). Somebody
> pointed me to this series as "the TDX kexec series from Intel" and I
> had some tunnel vision issues. Sorry for the noise!
> 
> But since I have your attention, do you have a pointer to the
> corresponding edk2 series?

Relevant code can be found here:

https://github.com/tianocore/edk2-staging/commits/tdvf-kexec/

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCHv6 00/16] x86/tdx: Add kexec support

[Huang, Kai]

> Hi Kirill,
> 
> I have a very basic question: is there a reason why this series does not revert
> commit cb8eb06d50fc, "x86/virt/tdx: Disable TDX host support when kexec is
> enabled"?
> 

Hi Paolo,

(Sorry I am replying using Outlook)

This series is for TDX guest, but not TDX host.

For TDX host kexec support I am working on a series to address.  It's in Intel internal review but I plan to send it out soon.

Things got a little bit late behind original schedule because currently I am in travel for Chinese New Year and sometimes not convenient to get access to Linux machine or even network.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 00/16] x86/tdx: Add kexec support

[Nikolay Borisov]

On 30.01.24 г. 15:43 ч., Paolo Bonzini wrote:
> On 1/24/24 13:55, Kirill A. Shutemov wrote:
>> 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.
> 
> Hi Kirill,
> 
> I have a very basic question: is there a reason why this series does not 
> revert commit cb8eb06d50fc, "x86/virt/tdx: Disable TDX host support when 
> kexec is enabled"?

While on the topic, Paolo do you think it's  better to have a runtime 
disable of kexec rather than at compile time:

[RFC PATCH] x86/virt/tdx: Disable KEXEC in the presence of TDX

20240118160118.1899299-1-nik.borisov@suse.com

I'm trying to get traction for this patch.


> 
> Thanks,
> 
> Paolo
> 
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 00/16] x86/tdx: Add kexec support

[Baoquan He]

On 01/31/24 at 09:31am, Nikolay Borisov wrote:
> 
> 
> On 30.01.24 г. 15:43 ч., Paolo Bonzini wrote:
> > On 1/24/24 13:55, Kirill A. Shutemov wrote:
> > > 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.
> > 
> > Hi Kirill,
> > 
> > I have a very basic question: is there a reason why this series does not
> > revert commit cb8eb06d50fc, "x86/virt/tdx: Disable TDX host support when
> > kexec is enabled"?
> 
> While on the topic, Paolo do you think it's  better to have a runtime
> disable of kexec rather than at compile time:
> 
> [RFC PATCH] x86/virt/tdx: Disable KEXEC in the presence of TDX
> 
> 20240118160118.1899299-1-nik.borisov@suse.com

Runtime disabling kexec looks better than at cmpile time, esp for
distros. While from above patch, making using of kexec_load_disabled to 
achive the runtime disabling may not be so good. Because we have a front
door to enable it through:

/proc/sys/kernel/kexec_load_disabled

If there's a flag or status to check if TDX host is enabled, and does
the checking in kexec_load_permitted(), that could be better. Anyway, I
saw Huang, Kai has posted the tdx host support patchset.

> 
> I'm trying to get traction for this patch.
> 
> 
> > 
> > Thanks,
> > 
> > Paolo
> > 
> > 
> 


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 00/16] x86/tdx: Add kexec support

[Nikolay Borisov]

On 31.01.24 г. 14:47 ч., Baoquan He wrote:
> On 01/31/24 at 09:31am, Nikolay Borisov wrote:
>>
>>
>> On 30.01.24 г. 15:43 ч., Paolo Bonzini wrote:
>>> On 1/24/24 13:55, Kirill A. Shutemov wrote:
>>>> 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.
>>>
>>> Hi Kirill,
>>>
>>> I have a very basic question: is there a reason why this series does not
>>> revert commit cb8eb06d50fc, "x86/virt/tdx: Disable TDX host support when
>>> kexec is enabled"?
>>
>> While on the topic, Paolo do you think it's  better to have a runtime
>> disable of kexec rather than at compile time:
>>
>> [RFC PATCH] x86/virt/tdx: Disable KEXEC in the presence of TDX
>>
>> 20240118160118.1899299-1-nik.borisov@suse.com
> 
> Runtime disabling kexec looks better than at cmpile time, esp for
> distros. While from above patch, making using of kexec_load_disabled to
> achive the runtime disabling may not be so good. Because we have a front
> door to enable it through:
> 
> /proc/sys/kernel/kexec_load_disabled

AFAIU it can't be enabled via this sysctl because the handler for it 
expects only 1 to be written to it:

      2                 .proc_handler   = proc_dointvec_minmax, 

      1                 .extra1         = SYSCTL_ONE, 

   994                  .extra2         = SYSCTL_ONE,

> 
> If there's a flag or status to check if TDX host is enabled, and does
> the checking in kexec_load_permitted(), that could be better. Anyway, I
> saw Huang, Kai has posted the tdx host support patchset.
> 
>>
>> I'm trying to get traction for this patch.
>>
>>
>>>
>>> Thanks,
>>>
>>> Paolo
>>>
>>>
>>
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCHv6 00/16] x86/tdx: Add kexec support

[Huang, Kai]

> > Runtime disabling kexec looks better than at cmpile time, esp for
> > distros. While from above patch, making using of kexec_load_disabled
> > to achive the runtime disabling may not be so good. Because we have a
> > front door to enable it through:
> >
> > /proc/sys/kernel/kexec_load_disabled
> 
> AFAIU it can't be enabled via this sysctl because the handler for it expects
> only 1 to be written to it:
> 
>       2                 .proc_handler   = proc_dointvec_minmax,
> 
>       1                 .extra1         = SYSCTL_ONE,
> 
>    994                  .extra2         = SYSCTL_ONE,
> 

This is also my understanding.  

The documentation also says once it is turned to disable we cannot turn back again:

kexec_load_disable
===================

A toggle indicating if the syscalls ``kexec_load`` and
``kexec_file_load`` have been disabled.
This value defaults to 0 (false: ``kexec_*load`` enabled), but can be
set to 1 (true: ``kexec_*load`` disabled).
Once true, kexec can no longer be used, and the toggle cannot be set
back to false.
......
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCHv6 00/16] x86/tdx: Add kexec support

[Baoquan He]

On 01/31/24 at 01:07pm, Huang, Kai wrote:
> > > Runtime disabling kexec looks better than at cmpile time, esp for
> > > distros. While from above patch, making using of kexec_load_disabled
> > > to achive the runtime disabling may not be so good. Because we have a
> > > front door to enable it through:
> > >
> > > /proc/sys/kernel/kexec_load_disabled
> > 
> > AFAIU it can't be enabled via this sysctl because the handler for it expects
> > only 1 to be written to it:
> > 
> >       2                 .proc_handler   = proc_dointvec_minmax,
> > 
> >       1                 .extra1         = SYSCTL_ONE,
> > 
> >    994                  .extra2         = SYSCTL_ONE,
> > 
> 
> This is also my understanding.  
> 
> The documentation also says once it is turned to disable we cannot turn back again:
> 
> kexec_load_disable
> ===================
> 
> A toggle indicating if the syscalls ``kexec_load`` and
> ``kexec_file_load`` have been disabled.
> This value defaults to 0 (false: ``kexec_*load`` enabled), but can be
> set to 1 (true: ``kexec_*load`` disabled).
> Once true, kexec can no longer be used, and the toggle cannot be set
> back to false.

you are quite right, I have never noticed this, thanks.

Then then mentioned patch looks good to me.


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec