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?
> +
> /**
> * 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.
> 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.
> + }
> +
> + 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?
> + 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?
