Re: [PATCH 2/4] x86/tdx: Use ReportFatalError to report missing SEPT_VE_DISABLE

Kirill A. Shutemov
2022-12-15T17:19:15Z

On Tue, Dec 13, 2022 at 03:06:07PM -0800, Dave Hansen wrote:
> On 12/9/22 05:25, Kirill A. Shutemov wrote:
> > 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