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

[Alexey Kardashevskiy <aik@amd.com>]

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@amd.com>
>> ---
>>   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