On 6/4/2025 11:49 PM, Alexey Kardashevskiy wrote:
> On 20/5/25 09:56, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@xxxxxxx>
>>
>> The FEATURE_INFO command provides host and guests a programmatic means
>> to learn about the supported features of the currently loaded firmware.
>> FEATURE_INFO command leverages the same mechanism as the CPUID instruction.
>> Instead of using the CPUID instruction to retrieve Fn8000_0024,
>> software can use FEATURE_INFO.
>>
>> The hypervisor may provide Fn8000_0024 values to the guest via the CPUID
>> page in SNP_LAUNCH_UPDATE. As with all CPUID output recorded in that page,
>> the hypervisor can filter Fn8000_0024. The firmware will examine
>> Fn8000_0024 and apply its CPUID policy.
>>
>> Switch to using SNP platform status instead of SEV platform status if
>> SNP is enabled and cache SNP platform status and feature information
>> from CPUID 0x8000_0024, sub-function 0, in the sev_device structure.
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
>> ---
>> drivers/crypto/ccp/sev-dev.c | 81 ++++++++++++++++++++++++++++++++++++
>> drivers/crypto/ccp/sev-dev.h | 3 ++
>> include/linux/psp-sev.h | 29 +++++++++++++
>> 3 files changed, 113 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 3451bada884e..b642f1183b8b 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -233,6 +233,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;
>> }
>> @@ -1073,6 +1074,69 @@ static void snp_set_hsave_pa(void *arg)
>> wrmsrq(MSR_VM_HSAVE_PA, 0);
>> }
>> +static int snp_get_platform_data(struct sev_user_data_status *status, int *error)
>> +{
>> + struct sev_data_snp_feature_info snp_feat_info;
>> + struct sev_device *sev = psp_master->sev_data;
>> + struct snp_feature_info *feat_info;
>> + struct sev_data_snp_addr buf;
>> + struct page *page;
>> + int rc;
>> +
>> + /*
>> + * The output buffer must be firmware page if SEV-SNP is
>> + * initialized.
>> + */
>> + if (sev->snp_initialized)
>> + return -EINVAL;
>> +
>> + buf.address = __psp_pa(&sev->snp_plat_status);
>> + rc = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
>> +
>> + if (rc) {
>> + dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
>> + rc, *error);
>> + return rc;
>> + }
>> +
>> + status->api_major = sev->snp_plat_status.api_major;
>> + status->api_minor = sev->snp_plat_status.api_minor;
>> + status->build = sev->snp_plat_status.build_id;
>> + status->state = sev->snp_plat_status.state;
>
> These 4 lines should be ....
>
>> +
>> + /*
>> + * Do feature discovery of the currently loaded firmware,
>> + * and cache feature information from CPUID 0x8000_0024,
>> + * sub-function 0.
>> + */
>> + if (sev->snp_plat_status.feature_info) {
>
> Could do:
>
> if (!sev->snp_plat_status.feature_info)
> return 0;
>
> and reduce indentation below.
>
>> + /*
>> + * Use dynamically allocated structure for the SNP_FEATURE_INFO
>> + * command to handle any alignment and page boundary check
>> + * requirements.
>> + */
>> + page = alloc_page(GFP_KERNEL);
>> + if (!page)
>> + return -ENOMEM;
>> + feat_info = page_address(page);
>> + snp_feat_info.length = sizeof(snp_feat_info);
>> + snp_feat_info.ecx_in = 0;
>> + snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
>> +
>> + rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
>> +
>> + if (!rc)
>> + sev->feat_info = *feat_info;
>> + else
>> + dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
>> + rc, *error);
>> +
>> + __free_page(page);
>> + }
>> +
>> + return rc;
>> +}
>> +
>> static int snp_filter_reserved_mem_regions(struct resource *rs, void *arg)
>> {
>> struct sev_data_range_list *range_list = arg;
>> @@ -1597,6 +1661,23 @@ static int sev_get_api_version(void)
>> struct sev_user_data_status status;
>> int error = 0, ret;
>> + /*
>> + * Use SNP platform status if SNP is enabled and cache
>> + * SNP platform status and SNP feature information.
>> + */
>> + if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
>> + ret = snp_get_platform_data(&status, &error);
>> + if (ret) {
>> + dev_err(sev->dev,
>> + "SEV-SNP: failed to get status. Error: %#x\n", error);
>
> Drop this as snp_get_platform_data() does that too (and even better as it prints @ret too).
>
>> + return 1;
>> + }
>
> .... here, and drop the @status parameter. Which in fact is pointless anyway as ....
>
> Let snp_get_platform_data() do just SNP stuff.
>
Yes.
>
>> + }
>> +
>> + /*
>> + * Fallback to SEV platform status if SNP is not enabled> + * or SNP platform status fails.
>> + */
>> ret = sev_platform_status(&status, &error);
>
> ... this sev_platform_status() is called anyway (not a fallback as the comment above says) and will override whatever snp_get_platform_data() wrote to @status.
>
Yes, as Tom suggested will be moving to caching both SEV and SNP platform status information.
>
>> if (ret) {
>> dev_err(sev->dev,
>> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
>> index 3e4e5574e88a..1c1a51e52d2b 100644
>> --- a/drivers/crypto/ccp/sev-dev.h
>> +++ b/drivers/crypto/ccp/sev-dev.h
>> @@ -57,6 +57,9 @@ struct sev_device {
>> bool cmd_buf_backup_active;
>> bool snp_initialized;
>> +
>> + struct sev_user_data_snp_status snp_plat_status;
>> + struct snp_feature_info feat_info;
>
>
> "snp_feat_info_0" as 1) it is for SNP like some other fields 2) there is subfunction=1 already defined so need some distinction.
>
Ok.
Thanks,
Ashish
>
>> };
>> int sev_dev_init(struct psp_device *psp);
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index 0b3a36bdaa90..0149d4a6aceb 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,
>> };
>> @@ -812,6 +813,34 @@ struct sev_data_snp_commit {
>> u32 len;
>> } __packed;
>> +/**
>> + * struct sev_data_snp_feature_info - SEV_SNP_FEATURE_INFO structure
>> + *
>> + * @length: len of the command buffer read by the PSP
>> + * @ecx_in: subfunction index
>> + * @feature_info_paddr : SPA of the FEATURE_INFO structure
>> + */
>> +struct sev_data_snp_feature_info {
>> + u32 length;
>> + u32 ecx_in;
>> + u64 feature_info_paddr;
>> +} __packed;
>> +
>> +/**
>> + * struct feature_info - FEATURE_INFO structure
>> + *
>> + * @eax: output of SNP_FEATURE_INFO command
>> + * @ebx: output of SNP_FEATURE_INFO command
>> + * @ecx: output of SNP_FEATURE_INFO command
>> + * #edx: output of SNP_FEATURE_INFO command
>> + */
>> +struct snp_feature_info {
>> + u32 eax;
>> + u32 ebx;
>> + u32 ecx;
>> + u32 edx;
>> +} __packed;
>
> (not insisting) You could even define this as "struct snp_feature_info_0" with all the bits from "Table 5. Contents of Each Subfunction of Fn8000_0024". Thanks,
>
>
>> +
>> #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>> /**
>
