Re: [PATCH v10 14/50] crypto: ccp: Add support to initialize the AMD-SP for SEV-SNP
From: Kalra, Ashish
Date: Wed Nov 29 2023 - 21:14:07 EST
Hello Boris,
+static int ___sev_platform_init_locked(int *error, bool probe)
{
- int rc = 0, psp_ret = SEV_RET_NO_FW_CALL;
+ int rc, psp_ret = SEV_RET_NO_FW_CALL;
struct psp_device *psp = psp_master;
struct sev_device *sev;
@@ -480,6 +493,34 @@ static int __sev_platform_init_locked(int *error)
if (sev->state == SEV_STATE_INIT)
return 0;
+ /*
+ * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
+ * so perform SEV-SNP initialization at probe time.
+ */
+ rc = __sev_snp_init_locked(error);
+ if (rc && rc != -ENODEV) {
+ /*
+ * Don't abort the probe if SNP INIT failed,
+ * continue to initialize the legacy SEV firmware.
+ */
+ dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n", rc, *error);
+ }
+
+ /* Delay SEV/SEV-ES support initialization */
+ if (probe && !psp_init_on_probe)
+ return 0;
+
+ if (!sev_es_tmr) {
+ /* Obtain the TMR memory area for SEV-ES use */
+ sev_es_tmr = sev_fw_alloc(SEV_ES_TMR_SIZE);
+ if (sev_es_tmr)
+ /* Must flush the cache before giving it to the firmware */
+ clflush_cache_range(sev_es_tmr, SEV_ES_TMR_SIZE);
+ else
+ dev_warn(sev->dev,
+ "SEV: TMR allocation failed, SEV-ES support unavailable\n");
+ }
+
if (sev_init_ex_buffer) {
rc = sev_read_init_ex_file();
if (rc)
@@ -522,6 +563,11 @@ static int __sev_platform_init_locked(int *error)
return 0;
}
+static int __sev_platform_init_locked(int *error)
+{
+ return ___sev_platform_init_locked(error, false);
+}
Uff, this is silly. And it makes the code hard to follow and that meat
of the platform init functionality in the ___-prefixed function a mess.
And the problem is that that "probe" functionality is replicated from
the one place where it is actually needed - sev_pci_init() which calls
that new sev_platform_init_on_probe() function - to everything that
calls __sev_platform_init_locked() for which you've added a wrapper.
What you should do, instead, is split the code around
__sev_snp_init_locked() in a separate function which does only that and
is called something like __sev_platform_init_snp_locked() or so which
does that unconditional work. And then you define:
_sev_platform_init_locked(int *error, bool probe)
note the *one* '_' - i.e., first layer:
_sev_platform_init_locked(int *error, bool probe):
{
__sev_platform_init_snp_locked(error);
if (!probe)
return 0;
if (psp_init_on_probe)
__sev_platform_init_locked(error);
...
}
and you do the probing in that function only so that it doesn't get lost
in the bunch of things __sev_platform_init_locked() does.
And then you call _sev_platform_init_locked() everywhere and no need for
a second sev_platform_init_on_probe().
It surely seems hard to follow up, so i am anyway going to clean it up by:
Adding the "probe" parameter to sev_platform_init() where the parameter
being true indicates that we only want to do SNP initialization on
probe, the same parameter will get passed on to
__sev_platform_init_locked().
So eventually there won't be a second sev_platform_init_on_probe() and
also there is no need for a ___sev_platform_init_locked().
We will only have sev_platform_init() and _sev_platform_init_locked().
+
+static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
+{
+ struct sev_data_range_list *range_list = arg;
+ struct sev_data_range *range = &range_list->ranges[range_list->num_elements];
+ size_t size;
+
+ if ((range_list->num_elements * sizeof(struct sev_data_range) +
+ sizeof(struct sev_data_range_list)) > PAGE_SIZE)
+ return -E2BIG;
Why? A comment would be helpful like with the rest this patch adds.
Ok.
+ switch (rs->desc) {
+ case E820_TYPE_RESERVED:
+ case E820_TYPE_PMEM:
+ case E820_TYPE_ACPI:
+ range->base = rs->start & PAGE_MASK;
+ size = (rs->end + 1) - rs->start;
+ range->page_count = size >> PAGE_SHIFT;
+ range_list->num_elements++;
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
+static int __sev_snp_init_locked(int *error)
+{
+ struct psp_device *psp = psp_master;
+ struct sev_data_snp_init_ex data;
+ struct sev_device *sev;
+ int rc = 0;
+
+ if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+ return -ENODEV;
+
+ if (!psp || !psp->sev_data)
+ return -ENODEV;
Only caller checks this already.
Ok.
+ sev = psp->sev_data;
+
+ if (sev->snp_initialized)
Do we really need this silly boolean or is there a way to query the
platform whether SNP has been initialized?
Yes it makes sense to have it as any platform specific way to query
whether the SNP has been initialized will be much more expensive then
simply checking this boolean.
+ return 0;
+
+ if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
+ dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
+ SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
+ return 0;
+ }
+
+ /*
+ * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h
+ * across all cores.
+ */
+ 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
+ * components that occasionally write to memory (e.g. logging to UEFI
+ * reserved regions) to not fail due to RMP initialization and SNP enablement.
+ */
+ if (sev_version_greater_or_equal(SNP_MIN_API_MAJOR, 52)) {
Is there a generic way to probe SNP_INIT_EX presence in the firmware or
are FW version numbers the only way?
It is not only the presence of SNP_INIT_EX but this check is more
specific to passing the HV_Fixed pages list to SNP_INIT_EX and that is
only supported with SNP FW versions 1.52 and beyond, so the FW version
check is the only way.
+ /*
+ * Firmware checks that the pages containing the ranges enumerated
+ * in the RANGES structure are either in the Default page state or in the
"default"
+ * firmware page state.
+ */
+ snp_range_list = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!snp_range_list) {
+ dev_err(sev->dev,
+ "SEV: SNP_INIT_EX range list memory allocation failed\n");
+ return -ENOMEM;
+ }
+
+ /*
+ * Retrieve all reserved memory regions setup by UEFI from the e820 memory map
+ * to be setup as HV-fixed pages.
+ */
+
^ Superfluous newline.
+ rc = walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_MEM, 0, ~0,
+ snp_range_list, snp_filter_reserved_mem_regions);
+ if (rc) {
+ dev_err(sev->dev,
+ "SEV: SNP_INIT_EX walk_iomem_res_desc failed rc = %d\n", rc);
+ return rc;
+ }
+
+ memset(&data, 0, sizeof(data));
+ data.init_rmp = 1;
+ data.list_paddr_en = 1;
+ data.list_paddr = __psp_pa(snp_range_list);
+
+ /*
+ * Before invoking SNP_INIT_EX with INIT_RMP=1, make sure that
+ * all dirty cache lines containing the RMP are flushed.
+ *
+ * NOTE: that includes writes via RMPUPDATE instructions, which
+ * are also cacheable writes.
+ */
+ wbinvd_on_all_cpus();
+
+ rc = __sev_do_cmd_locked(SEV_CMD_SNP_INIT_EX, &data, error);
+ if (rc)
+ return rc;
+ } else {
+ /*
+ * SNP_INIT is equivalent to SNP_INIT_EX with INIT_RMP=1, so
+ * just as with that case, make sure all dirty cache lines
+ * containing the RMP are flushed.
+ */
+ wbinvd_on_all_cpus();
+
+ rc = __sev_do_cmd_locked(SEV_CMD_SNP_INIT, NULL, error);
+ if (rc)
+ return rc;
+ }
So instead of duplicating the code here at the end of the if-else
branching, you can do:
void *arg = &data;
if () {
...
cmd = SEV_CMD_SNP_INIT_EX;
} else {
cmd = SEV_CMD_SNP_INIT;
arg = NULL;
}
wbinvd_on_all_cpus();
rc = __sev_do_cmd_locked(cmd, arg, error);
if (rc)
return rc;
Yes, makes sense, will fix it.
+ /* Prepare for first SNP guest launch after INIT */
+ wbinvd_on_all_cpus();
Why is that WBINVD needed?
As the comment above mentions, WBINVD + DF_FLUSH is needed before the
first guest launch.
+ rc = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, error);
+ if (rc)
+ return rc;
+
+ sev->snp_initialized = true;
+ dev_dbg(sev->dev, "SEV-SNP firmware initialized\n");
+
+ return rc;
+}
+
+static int __sev_snp_shutdown_locked(int *error)
+{
+ struct sev_device *sev = psp_master->sev_data;
+ struct sev_data_snp_shutdown_ex data;
+ int ret;
+
+ if (!sev->snp_initialized)
+ return 0;
+
+ memset(&data, 0, sizeof(data));
+ data.length = sizeof(data);
+ data.iommu_snp_shutdown = 1;
+
+ wbinvd_on_all_cpus();
+
+retry:
+ ret = __sev_do_cmd_locked(SEV_CMD_SNP_SHUTDOWN_EX, &data, error);
+ /* SHUTDOWN may require DF_FLUSH */
+ if (*error == SEV_RET_DFFLUSH_REQUIRED) {
+ ret = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, NULL);
+ if (ret) {
+ dev_err(sev->dev, "SEV-SNP DF_FLUSH failed\n");
+ return ret;
When you return here, sev->snp_initialized is still true but, in
reality, it probably is in some half-broken state after issuing those
commands you it is not really initialized anymore.
Yes, this needs to be fixed.
+ }
+ goto retry;
This needs an upper limit from which to break out and not potentially
endless-loop.
Ok.
+ }
+ if (ret) {
+ dev_err(sev->dev, "SEV-SNP firmware shutdown failed\n");
+ return ret;
+ }
+
+ sev->snp_initialized = false;
+ dev_dbg(sev->dev, "SEV-SNP firmware shutdown\n");
+
+ return ret;
+}
+
+static int sev_snp_shutdown(int *error)
+{
+ int rc;
+
+ mutex_lock(&sev_cmd_mutex);
+ rc = __sev_snp_shutdown_locked(error);
Why is this "locked" version even there if it is called only here?
IOW, put all the logic in here - no need for
__sev_snp_shutdown_locked().
In the latest code base, _sev_snp_shutdown_locked() is called from
__sev_firmware_shutdown().
Thanks,
Ashish
+ mutex_unlock(&sev_cmd_mutex);
+
+ return rc;
+}
...