linux-kernel - RE: [PATCH v8 1/2] x86/tdx: Retry TDVMCALL_MAP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CO1PR21MB133257BE30F61447E12F83ABBF5DA@CO1PR21MB1332.namprd21.prod.outlook.com>
Date:   Wed, 21 Jun 2023 00:28:17 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     Dave Hansen <dave.hansen@...el.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "arnd@...db.de" <arnd@...db.de>, "bp@...en8.de" <bp@...en8.de>,
        "brijesh.singh@....com" <brijesh.singh@....com>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "jane.chu@...cle.com" <jane.chu@...cle.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        KY Srinivasan <kys@...rosoft.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "luto@...nel.org" <luto@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "seanjc@...gle.com" <seanjc@...gle.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        "rick.p.edgecombe@...el.com" <rick.p.edgecombe@...el.com>
Subject: RE: [PATCH v8 1/2] x86/tdx: Retry TDVMCALL_MAP_GPA() when needed

> From: Dave Hansen <dave.hansen@...el.com>
> Sent: Tuesday, June 20, 2023 4:34 PM
> >  ...
> > -	if (_tdx_hypercall(TDVMCALL_MAP_GPA, start, end - start, 0, 0))
> > +	while (retry_count < max_retries_per_page) {
> > +		memset(&args, 0, sizeof(args));
> > +		args.r10 = TDX_HYPERCALL_STANDARD;
> > +		args.r11 = TDVMCALL_MAP_GPA;
> > +		args.r12 = start;
> > +		args.r13 = end - start;
> > +
> 
> What's wrong with:
> 
> 	while (retry_count < max_retries_per_page) {
> 		struct tdx_hypercall_args args = {
> 			.r10 = TDX_HYPERCALL_STANDARD,
> 			.r11 = TDVMCALL_MAP_GPA,
> 			.r12 = start,
> 			.r13 = end - start };
> 
> ?
> 
> Or maybe with the brackets slightly differently arranged.
> 
> Why'd you declare all the variables outside the while() loop anyway?

Thanks for the suggestion of making the code compact!
I'll apply the below diff, and post v9 tomorrow (trying to
not post too frequently...)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 08eac8f46c11..1cb7e9ee3a68 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -710,9 +710,8 @@ static bool tdx_cache_flush_required(void)
  */
 static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
 {
+	/* Retrying the hypercall a second time should succeed; use 3 just in case */
 	const int max_retries_per_page = 3;
-	struct tdx_hypercall_args args;
-	u64 map_fail_paddr, ret;
 	int retry_count = 0;
 
 	if (!enc) {
@@ -722,13 +721,14 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
 	}
 
 	while (retry_count < max_retries_per_page) {
-		memset(&args, 0, sizeof(args));
-		args.r10 = TDX_HYPERCALL_STANDARD;
-		args.r11 = TDVMCALL_MAP_GPA;
-		args.r12 = start;
-		args.r13 = end - start;
-
-		ret = __tdx_hypercall_ret(&args);
+		struct tdx_hypercall_args args = {
+			.r10 = TDX_HYPERCALL_STANDARD,
+			.r11 = TDVMCALL_MAP_GPA,
+			.r12 = start,
+			.r13 = end - start };
+
+		u64 map_fail_paddr;
+		u64 ret = __tdx_hypercall_ret(&args);
 		if (ret != TDVMCALL_STATUS_RETRY)
 			return !ret;
 		/*


The function now looks like this:

/*
 * Notify the VMM about page mapping conversion. More info about ABI
 * can be found in TDX Guest-Host-Communication Interface (GHCI),
 * section "TDG.VP.VMCALL<MapGPA>".
 */
static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
{
	/* Retrying the hypercall a second time should succeed; use 3 just in case */
	const int max_retries_per_page = 3;
	int retry_count = 0;

	if (!enc) {
		/* Set the shared (decrypted) bits: */
		start |= cc_mkdec(0);
		end   |= cc_mkdec(0);
	}

	while (retry_count < max_retries_per_page) {
		struct tdx_hypercall_args args = {
			.r10 = TDX_HYPERCALL_STANDARD,
			.r11 = TDVMCALL_MAP_GPA,
			.r12 = start,
			.r13 = end - start };

		u64 map_fail_paddr;
		u64 ret = __tdx_hypercall_ret(&args);
		if (ret != TDVMCALL_STATUS_RETRY)
			return !ret;
		/*
		 * The guest must retry the operation for the pages in the
		 * region starting at the GPA specified in R11. R11 comes
		 * from the untrusted VMM. Sanity check it.
		 */
		map_fail_paddr = args.r11;
		if (map_fail_paddr < start || map_fail_paddr >= end)
			return false;

		/* "Consume" a retry without forward progress */
		if (map_fail_paddr == start) {
			retry_count++;
			continue;
		}

		start = map_fail_paddr;
		retry_count = 0;
	}

	return false;
}