Re: [PATCH v4 2/5] crypto: ccp: Add support for SNP_FEATURE_INFO command

["Kalra, Ashish" <ashish.kalra@amd.com>]

On 6/4/2025 11:49 PM, Alexey Kardashevskiy wrote:
> On 20/5/25 09:56, Ashish Kalra wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> 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@amd.com>
>> ---
>>   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
>>     /**
>