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

Borislav Petkov
2024-01-15T19:54:36Z

On Sat, Dec 30, 2023 at 10:19:41AM -0600, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@amd.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.