On Tue, Dec 13, 2022 at 03:06:07PM -0800, Dave Hansen wrote:

On 12/9/22 05:25, Kirill A. Shutemov wrote:

quoted

The check for SEPT_VE_DISABLE happens early in the kernel boot where
earlyprintk is not yet functional. Kernel successfully detect broken
TD configuration and stops the kernel with panic(), but it cannot
communicate the reason to the user.

Linux TDX guests require that the SEPT_VE_DISABLE "attribute" be set.
If it is not set, the kernel is theoretically required to handle
exceptions anywhere that kernel memory is accessed, including places
like NMI handlers and in the syscall entry gap.

Rather than even try to handle these exceptions, the kernel refuses to
run if SEPT_VE_DISABLE is unset.

However, the SEPT_VE_DISABLE detection and refusal code happens very
early in boot, even before earlyprintk runs.  Calling panic() will
effectively just hang the system.

Instead, call a TDX-specific panic() function.  This makes a very simple
TDVMCALL which gets a short error string out to the hypervisor without
any console infrastructure.

--

Is that better?

Yes, thank you.

Also, are you sure we want to do this?  Is there any way to do this
inside of panic() itself to get panic() itself to call tdx_panic() and
get a short error message out to the hypervisor?

Getting *all* users of panic this magic ability would be a lot better
than giving it to one call-site of panic().

I'm all for making the panic() path as short and simple as possible, but
it would be nice if this fancy hypercall would get used in more than one
spot.

Well, I don't see an obvious way to integrate this into panic().

There is panic_notifier_list and it kinda/sorta works, see the patch
below.

But it breaks panic_notifier_list contract: the callback will never return
and no other callback will be able to do their stuff. panic_timeout is
also broken.

So ReportFatalError() is no good for the task. And I don't have anything
else :/

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 83ca9a7f0b75..81f9a964dc1f 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c

@@ -7,6 +7,7 @@
 #include <linux/cpufeature.h>
 #include <linux/export.h>
 #include <linux/io.h>
+#include <linux/panic_notifier.h>
 #include <asm/coco.h>
 #include <asm/tdx.h>
 #include <asm/vmx.h>

@@ -146,8 +147,10 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
 }
 EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
 
-static void __noreturn tdx_panic(const char *msg)
+static int tdx_panic(struct notifier_block *this,
+				 unsigned long event, void *ptr)
 {
+	const char *msg = ptr;
 	struct tdx_hypercall_args args = {
 		.r10 = TDX_HYPERCALL_STANDARD,
 		.r11 = TDVMCALL_REPORT_FATAL_ERROR,

@@ -219,7 +222,7 @@ static void tdx_parse_tdinfo(u64 *cc_mask)
 		if (td_attr & ATTR_DEBUG)
 			pr_warn("%s\n", msg);
 		else
-			tdx_panic(msg);
+			panic(msg);
 	}
 }
 

@@ -851,6 +854,10 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
 	return true;
 }
 
+static struct notifier_block panic_block = {
+	.notifier_call = tdx_panic,
+};
+
 void __init tdx_early_init(void)
 {
 	u64 cc_mask;

@@ -863,6 +870,7 @@ void __init tdx_early_init(void)
 
 	setup_force_cpu_cap(X86_FEATURE_TDX_GUEST);
 
+	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
 	cc_set_vendor(CC_VENDOR_INTEL);
 	tdx_parse_tdinfo(&cc_mask);
 	cc_set_mask(cc_mask);

-- 
  Kiryl Shutsemau / Kirill A. Shutemov