linux-kernel - Re: [PATCH v1 13/26] crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP

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] [thread-next>] [day] [month] [year] [list]

Date: Mon, 15 Jan 2024 20:53:46 +0100
From: Borislav Petkov <bp@...en8.de>
To: Michael Roth <michael.roth@....com>
Cc: x86@...nel.org, kvm@...r.kernel.org, linux-coco@...ts.linux.dev,
	linux-mm@...ck.org, linux-crypto@...r.kernel.org,
	linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
	jroedel@...e.de, thomas.lendacky@....com, hpa@...or.com,
	ardb@...nel.org, pbonzini@...hat.com, seanjc@...gle.com,
	vkuznets@...hat.com, jmattson@...gle.com, luto@...nel.org,
	dave.hansen@...ux.intel.com, slp@...hat.com, pgonda@...gle.com,
	peterz@...radead.org, srinivas.pandruvada@...ux.intel.com,
	rientjes@...gle.com, tobin@....com, vbabka@...e.cz,
	kirill@...temov.name, ak@...ux.intel.com, tony.luck@...el.com,
	sathyanarayanan.kuppuswamy@...ux.intel.com, alpergun@...gle.com,
	jarkko@...nel.org, ashish.kalra@....com, nikunj.dadhania@....com,
	pankaj.gupta@....com,
	"liam.merwick@...cle.com Brijesh Singh" <brijesh.singh@....com>,
	Jarkko Sakkinen <jarkko@...fian.com>
Subject: Re: [PATCH v1 13/26] crypto: ccp: Add support to initialize the
 AMD-SP for SEV-SNP

On Sat, Dec 30, 2023 at 10:19:41AM -0600, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@....com>
> 
> Before SNP VMs can be launched, the platform must be appropriately
> configured and initialized. Platform initialization is accomplished via
> the SNP_INIT command. Make sure to do a WBINVD and issue DF_FLUSH
> command to prepare for the first SNP guest launch after INIT.
							  ^^^^^^
Which "INIT"?

Sounds like after hipervisor's init...

> During the execution of SNP_INIT command, the firmware configures
> and enables SNP security policy enforcement in many system components.
> Some system components write to regions of memory reserved by early
> x86 firmware (e.g. UEFI). Other system components write to regions
> provided by the operation system, hypervisor, or x86 firmware.
> Such system components can only write to HV-fixed pages or Default
> pages. They will error when attempting to write to other page states

"... to pages in other page states... "

> after SNP_INIT enables their SNP enforcement.

And yes, this version looks much better. Some text cleanups ontop:

---
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 85634d4f8cfe..7942ec730525 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -549,24 +549,22 @@ static int __sev_snp_init_locked(int *error)
 		return 0;
 	}
 
-	/*
-	 * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h
-	 * across all cores.
-	 */
+	/* SNP_INIT requires MSR_VM_HSAVE_PA to be cleared on all CPUs. */
 	on_each_cpu(snp_set_hsave_pa, NULL, 1);
 
 	/*
-	 * Starting in SNP firmware v1.52, the SNP_INIT_EX command takes a list of
-	 * system physical address ranges to convert into the HV-fixed page states
-	 * during the RMP initialization.  For instance, the memory that UEFI
-	 * reserves should be included in the range list. This allows system
+	 * Starting in SNP firmware v1.52, the SNP_INIT_EX command takes a list
+	 * of system physical address ranges to convert into HV-fixed page
+	 * states during the RMP initialization.  For instance, the memory that
+	 * UEFI reserves should be included in the that list. This allows system
 	 * components that occasionally write to memory (e.g. logging to UEFI
-	 * reserved regions) to not fail due to RMP initialization and SNP enablement.
+	 * reserved regions) to not fail due to RMP initialization and SNP
+	 * enablement.
 	 */
 	if (sev_version_greater_or_equal(SNP_MIN_API_MAJOR, 52)) {
 		/*
 		 * Firmware checks that the pages containing the ranges enumerated
-		 * in the RANGES structure are either in the Default page state or in the
+		 * in the RANGES structure are either in the default page state or in the
 		 * firmware page state.
 		 */
 		snp_range_list = kzalloc(PAGE_SIZE, GFP_KERNEL);
@@ -577,7 +575,7 @@ static int __sev_snp_init_locked(int *error)
 		}
 
 		/*
-		 * Retrieve all reserved memory regions setup by UEFI from the e820 memory map
+		 * Retrieve all reserved memory regions from the e820 memory map
 		 * to be setup as HV-fixed pages.
 		 */
 		rc = walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_MEM, 0, ~0,
@@ -599,14 +597,13 @@ static int __sev_snp_init_locked(int *error)
 	}
 
 	/*
-	 * The following sequence must be issued before launching the
-	 * first SNP guest to ensure all dirty cache lines are flushed,
-	 * including from updates to the RMP table itself via RMPUPDATE
-	 * instructions:
+	 * The following sequence must be issued before launching the first SNP
+	 * guest to ensure all dirty cache lines are flushed, including from
+	 * updates to the RMP table itself via the RMPUPDATE instruction:
 	 *
-	 * - WBINDV on all running CPUs
+	 * - WBINVD on all running CPUs
 	 * - SEV_CMD_SNP_INIT[_EX] firmware command
-	 * - WBINDV on all running CPUs
+	 * - WBINVD on all running CPUs
 	 * - SEV_CMD_SNP_DF_FLUSH firmware command
 	 */
 	wbinvd_on_all_cpus();



-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette