On 4/9/24 07:27, Dan Williams wrote:
Alexey Kardashevskiy wrote:
The PSP advertises the SEV-TIO support via the FEATURE_INFO command
support of which is advertised via SNP_PLATFORM_STATUS.
Add FEATURE_INFO and use it to detect the TIO support in the PSP.
If present, enable TIO in the SNP_INIT_EX call.
While at this, add new bits to sev_data_snp_init_ex() from SEV-SNP 1.55.
Note that this tests the PSP firmware support but not if the feature
is enabled in the BIOS.
While at this, add new sev_data_snp_shutdown_ex::x86_snp_shutdown
Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
---
include/linux/psp-sev.h | 31 ++++++++-
include/uapi/linux/psp-sev.h | 4 +-
drivers/crypto/ccp/sev-dev.c | 73 ++++++++++++++++++++
3 files changed, 104 insertions(+), 4 deletions(-)
Taking a peek to familiarize myself with that is required for TIO
enabling in the PSP driver...
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 52d5ee101d3a..1d63044f66be 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -107,6 +107,7 @@ enum sev_cmd {
SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX = 0x0CA,
SEV_CMD_SNP_COMMIT = 0x0CB,
SEV_CMD_SNP_VLEK_LOAD = 0x0CD,
+ SEV_CMD_SNP_FEATURE_INFO = 0x0CE,
SEV_CMD_MAX,
};
@@ -584,6 +585,25 @@ struct sev_data_snp_addr {
u64 address; /* In/Out */
} __packed;
+/**
+ * struct sev_data_snp_feature_info - SEV_CMD_SNP_FEATURE_INFO command params
+ *
+ * @len: length of this struct
+ * @ecx_in: subfunction index of CPUID Fn8000_0024
+ * @feature_info_paddr: physical address of a page with sev_snp_feature_info
+ */
+#define SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO 1
+
+struct sev_snp_feature_info {
+ u32 eax, ebx, ecx, edx; /* Out */
+} __packed;
+
+struct sev_data_snp_feature_info {
+ u32 length; /* In */
+ u32 ecx_in; /* In */
+ u64 feature_info_paddr; /* In */
+} __packed;
Why use CPU register names in C structures? I would hope the spec
renames these parameters to something meaninful?
This mimics the CPUID instruction and (my guess) x86 people are used to
"CPUID's ECX" == "Subfunction index". The spec (the one I mention below)
calls it precisely "ECX_IN".
+
/**
* struct sev_data_snp_launch_start - SNP_LAUNCH_START command params
*
@@ -745,10 +765,14 @@ struct sev_data_snp_guest_request {
Would be nice to have direct pointer to the spec and spec chapter
documented for these command structure fields.
For every command? Seems overkill. Any good example?
Although the file could have mentioned in the header that SNP_xxx are
from "SEV Secure Nested Paging Firmware ABI Specification" which google
easily finds, and search on that pdf for "SNP_INIT_EX" finds the
structure layout. Using the exact chapter numbers/titles means they
cannot change, or someone has to track the changes.
struct sev_data_snp_init_ex {
u32 init_rmp:1;
u32 list_paddr_en:1;
- u32 rsvd:30;
+ u32 rapl_dis:1;
+ u32 ciphertext_hiding_en:1;
+ u32 tio_en:1;
+ u32 rsvd:27;
u32 rsvd1;
u64 list_paddr;
- u8 rsvd2[48];
+ u16 max_snp_asid;
+ u8 rsvd2[46];
} __packed;
/**
@@ -787,7 +811,8 @@ struct sev_data_range_list {
struct sev_data_snp_shutdown_ex {
u32 len;
u32 iommu_snp_shutdown:1;
- u32 rsvd1:31;
+ u32 x86_snp_shutdown:1;
+ u32 rsvd1:30;
} __packed;
/**
[..]
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index f6eafde584d9..a49fe54b8dd8 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -223,6 +223,7 @@ static int sev_cmd_buffer_len(int cmd)
case SEV_CMD_SNP_GUEST_REQUEST: return sizeof(struct sev_data_snp_guest_request);
case SEV_CMD_SNP_CONFIG: return sizeof(struct sev_user_data_snp_config);
case SEV_CMD_SNP_COMMIT: return sizeof(struct sev_data_snp_commit);
+ case SEV_CMD_SNP_FEATURE_INFO: return sizeof(struct sev_data_snp_feature_info);
default: return 0;
}
@@ -1125,6 +1126,77 @@ static int snp_platform_status_locked(struct sev_device *sev,
return ret;
}
+static int snp_feature_info_locked(struct sev_device *sev, u32 ecx,
+ struct sev_snp_feature_info *fi, int *psp_ret)
+{
+ struct sev_data_snp_feature_info buf = {
+ .length = sizeof(buf),
+ .ecx_in = ecx,
+ };
+ struct page *status_page;
+ void *data;
+ int ret;
+
+ status_page = alloc_page(GFP_KERNEL_ACCOUNT);
+ if (!status_page)
+ return -ENOMEM;
+
+ data = page_address(status_page);
+
+ if (sev->snp_initialized && rmp_mark_pages_firmware(__pa(data), 1, true)) {
+ ret = -EFAULT;
+ goto cleanup;
Jonathan already mentioned this, but "goto cleanup" is so 2022.
This requires DEFINE_FREE() which yet another place to look at. When I
Then, no_free_ptr() just hurts to read (cold breath of c++). It is not
needed here but unavoidably will be in other places when I start using
__free(kfree). But alright, I'll switch.
+ }
+
+ buf.feature_info_paddr = __psp_pa(data);
+ ret = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &buf, psp_ret);
+
+ if (sev->snp_initialized && snp_reclaim_pages(__pa(data), 1, true))
+ ret = -EFAULT;
+
+ if (!ret)
+ memcpy(fi, data, sizeof(*fi));
+
+cleanup:
+ __free_pages(status_page, 0);
+ return ret;
+}
+
+static int snp_get_feature_info(struct sev_device *sev, u32 ecx, struct sev_snp_feature_info *fi)
Why not make this bool...
+{
+ struct sev_user_data_snp_status status = { 0 };
+ int psp_ret = 0, ret;
+
+ ret = snp_platform_status_locked(sev, &status, &psp_ret);
+ if (ret)
+ return ret;
+ if (ret != SEV_RET_SUCCESS)
+ return -EFAULT;
+ if (!status.feature_info)
+ return -ENOENT;
+
+ ret = snp_feature_info_locked(sev, ecx, fi, &psp_ret);
+ if (ret)
+ return ret;
+ if (ret != SEV_RET_SUCCESS)
+ return -EFAULT;
+
+ return 0;
+}
+
+static bool sev_tio_present(struct sev_device *sev)
+{
+ struct sev_snp_feature_info fi = { 0 };
+ bool present;
+
+ if (snp_get_feature_info(sev, 0, &fi))
...since the caller does not care?
sev_tio_present() does not but other users of snp_get_feature_info()
(one is coming sooner that TIO) might, WIP.
+ return false;
+
+ present = (fi.ebx & SNP_FEATURE_FN8000_0024_EBX_X00_SEVTIO) != 0;
+ dev_info(sev->dev, "SEV-TIO support is %s\n", present ? "present" : "not present");
+ return present;
+}
+
static int __sev_snp_init_locked(int *error)
{
struct psp_device *psp = psp_master;
@@ -1189,6 +1261,7 @@ static int __sev_snp_init_locked(int *error)
data.init_rmp = 1;
data.list_paddr_en = 1;
data.list_paddr = __psp_pa(snp_range_list);
+ data.tio_en = sev_tio_present(sev);
Where does this get saved for follow-on code to consume that TIO is
active?
Oh. It is not saved, whether TIO is actually active is determined by the
result of calling PSP's TIO_STATUS (which I should skip if tio_en ==
false in the first place). Thanks,
--
Alexey
