Re: [RFC PATCH 06/21] crypto: ccp: Enable SEV-TIO feature in the PSP when supported

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 23 Aug 2024 23:21:20 +1000
Alexey Kardashevskiy <aik@amd.com> 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@amd.com>
I was curious so had a read.
Some minor comments inline.

Jonathan

> ---
>  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(-)
> 
> 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
> + */

Comment seems to have drifted away from the structure.

> +#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;
> +

>  /**
> @@ -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;

Has docs that want updating I think.

> +	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)

> +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;
> +	}
> +
> +	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;
		goto cleanup
	}

	memcpy(fi, data, sizeof(*fi));

> +
> +	if (!ret)
> +		memcpy(fi, data, sizeof(*fi));

rather than this is more consistent and hence easier to review.

> +
> +cleanup:
> +	__free_pages(status_page, 0);

	free_page(status_page);

Maybe worth a DEFINE_FREE() to let you do early returns and make this
even nicer to read.



> +	return ret;
> +}
> +
> +static int snp_get_feature_info(struct sev_device *sev, u32 ecx, struct sev_snp_feature_info *fi)
> +{
> +	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)

	won't get here as ret definitely == 0
given you checked it was just above.

> +		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;
and another.

	return snp_feature_info_locked(...


> +
> +	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))
> +		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");

Probably too noisy for final driver but fine for RFC I guess.

> +	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);
>  		cmd = SEV_CMD_SNP_INIT_EX;
>  	} else {
>  		cmd = SEV_CMD_SNP_INIT;