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

From: Sathyanarayanan Kuppuswamy  <sathyanarayanan.kuppuswamy@linux.intel.com>
To: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	Borislav Petkov <bp@alien8.de>, Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Elena Reshetova <elena.reshetova@intel.com>,
	x86@kernel.org, linux-coco@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/4] x86/tdx: Use ReportFatalError to report missing SEPT_VE_DISABLE
Date: Fri, 9 Dec 2022 12:51:07 -0800	[thread overview]
Message-ID: <2e305bb5-9595-3531-6134-24344ff5c797@linux.intel.com> (raw)
In-Reply-To: <20221209170647.r32yjyc3hsqtnffo@box.shutemov.name>

On 12/9/22 9:06 AM, Kirill A. Shutemov wrote:
> On Fri, Dec 09, 2022 at 07:42:56AM -0800, Sathyanarayanan Kuppuswamy wrote:
>>
>>
>> On 12/9/22 5:25 AM, 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.
>>>
>>> Use TDG.VP.VMCALL<ReportFatalError> to report the error. The hypercall
>>> can encode message up to 64 bytes in eight registers.
>>>
>>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> ---
>>>  arch/x86/coco/tdx/tdx.c | 38 +++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>>> index cfd4c95b9f04..8ad04d101270 100644
>>> --- a/arch/x86/coco/tdx/tdx.c
>>> +++ b/arch/x86/coco/tdx/tdx.c
>>> @@ -22,6 +22,7 @@
>>>  
>>>  /* TDX hypercall Leaf IDs */
>>>  #define TDVMCALL_MAP_GPA		0x10001
>>> +#define TDVMCALL_REPORT_FATAL_ERROR	0x10003
>>>  
>>>  /* MMIO direction */
>>>  #define EPT_READ	0
>>> @@ -140,6 +141,41 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>>>  }
>>>  EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
>>>  
>>> +static void __noreturn tdx_panic(const char *msg)
>>> +{
>>> +	struct tdx_hypercall_args args = {
>>> +		.r10 = TDX_HYPERCALL_STANDARD,
>>> +		.r11 = TDVMCALL_REPORT_FATAL_ERROR,
>>> +		.r12 = 0, /* Error code: 0 is Panic */
>>> +	};
>>> +	union {
>>> +		/* Define register order according to the GHCI */
>>> +		struct { u64 r14, r15, rbx, rdi, rsi, r8, r9, rdx; };
>>> +
>>> +		char str[64];
>>> +	} message;
>>> +
>>> +	/* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
>>> +	strncpy(message.str, msg, 64);
>>> +
>>> +	args.r8  = message.r8;
>>> +	args.r9  = message.r9;
>>> +	args.r14 = message.r14;
>>> +	args.r15 = message.r15;
>>> +	args.rdi = message.rdi;
>>> +	args.rsi = message.rsi;
>>> +	args.rbx = message.rbx;
>>> +	args.rdx = message.rdx;
>>> +
>>> +	/*
>>> +	 * Keep calling the hypercall in case VMM did not terminated
>>> +	 * the TD as it must.
>>> +	 */
>>> +	while (1) {
>>> +		__tdx_hypercall(&args, 0);
>>> +	}
>>
>> Instead of an infinite loop, I'm wondering if the guest should panic after
>> retrying for few times.
> 
> Hm. What difference would it make?

IIUC, the goal of this patch is to report the fatal error to VMM and panic.
But, if VMM does not terminate the guest as we expect, rather than trying 
continuously, isn't it better to panic ourselves? That way the behavior
will be similar to what we have currently.

> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer