Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions

[Tom Lendacky <thomas.lendacky@amd.com>]

On 3/10/25 07:46, Stefano Garzarella wrote:
> On Mon, Mar 10, 2025 at 12:30:06PM +0100, Borislav Petkov wrote:
>> On Fri, Feb 28, 2025 at 06:07:16PM +0100, Stefano Garzarella wrote:
>>> +bool snp_svsm_vtpm_probe(void)
>>> +{
>>> +    struct svsm_call call = {};
>>> +    u64 send_cmd_mask = 0;
>>> +    u64 platform_cmds;
>>> +    u64 features;
>>> +    int ret;
>>> +
>>> +    /* The vTPM device is available only if we have a SVSM */
>>
>> s/if we have a SVSM/if an SVSM is present/
>>
>>> +    if (!snp_vmpl)
>>> +        return false;
>>> +
>>> +    call.caa = svsm_get_caa();
>>> +    call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>>> +
>>> +    ret = svsm_perform_call_protocol(&call);
>>> +
>>
>>
>> ^ Superfluous newline.
>>
>>> +    if (ret != SVSM_SUCCESS)
>>> +        return false;
>>> +
>>> +    features = call.rdx_out;
>>> +    platform_cmds = call.rcx_out;
>>> +
>>> +    /* No feature supported, it should be zero */
>>> +    if (features)
>>> +        pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>>> +            features);
>>
>> So
>>
>>     return false;
>>
>> here?
> 
> In v1 we had that, but Tom Lendacky suggested to remove it:
> https://lore.kernel.org/linux-integrity/4valfkw7wtx3fpdv2qbymzggcu7mp4mhkd65j5q7zncs2dzorc@jjjevuwfchgl/
> 
> IIUC the features are supposed to be additive, so Tom's point was to
> avoid that in the future SVSM will supports new features and this driver
> stops working, when it could, just without using the new features.
> 
> I added a warning just to be aware of new features, but I can remove it.

I don't think anything needs to be checked or printed. If you want to do
anything, just issue a pr_info() with the features value (and maybe the
platform_cmds value, too). Issuing a pr_warn() here would be like
issuing a pr_warn() for a new CPUID value that the current kernel
doesn't know about.

Thanks,
Tom

> 
>>
>>> +
>>> +    /* TPM_SEND_COMMAND - platform command 8 */
>>> +    send_cmd_mask = 1 << 8;
>>
>>     BIT_ULL(8);
>>
>>> +
>>> +    return (platform_cmds & send_cmd_mask) == send_cmd_mask;
>>> +}
>>> +EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
>>> +
>>> +int snp_svsm_vtpm_send_command(u8 *buffer)
>>> +{
>>> +    struct svsm_call call = {};
>>> +
>>> +    call.caa = svsm_get_caa();
>>> +    call.rax = SVSM_VTPM_CALL(SVSM_VTPM_CMD);
>>> +    call.rcx = __pa(buffer);
>>> +
>>> +    return svsm_perform_call_protocol(&call);
>>> +}
>>
>> In any case, you can zap all those local vars, use comments instead
>> and slim
>> down the function, diff ontop:
> 
> Thanks for the diff, I'll apply it except, for now, the return in the
> feature check which is still not clear to me (I think I get Tom's point,
> but I would like confirmation from both of you).
> 
> Thanks,
> Stefano
> 
>>
>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index 3902af4b1385..6d7e97c1f567 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -2631,12 +2631,9 @@ static int snp_issue_guest_request(struct
>> snp_guest_req *req, struct snp_req_dat
>> bool snp_svsm_vtpm_probe(void)
>> {
>>     struct svsm_call call = {};
>> -    u64 send_cmd_mask = 0;
>> -    u64 platform_cmds;
>> -    u64 features;
>>     int ret;
>>
>> -    /* The vTPM device is available only if we have a SVSM */
>> +    /* The vTPM device is available only if a SVSM is present */
>>     if (!snp_vmpl)
>>         return false;
>>
>> @@ -2644,22 +2641,17 @@ bool snp_svsm_vtpm_probe(void)
>>     call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>>
>>     ret = svsm_perform_call_protocol(&call);
>> -
>>     if (ret != SVSM_SUCCESS)
>>         return false;
>>
>> -    features = call.rdx_out;
>> -    platform_cmds = call.rcx_out;
>> -
>>     /* No feature supported, it should be zero */
>> -    if (features)
>> -        pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>> -            features);
>> -
>> -    /* TPM_SEND_COMMAND - platform command 8 */
>> -    send_cmd_mask = 1 << 8;
>> +    if (call.rdx_out) {
>> +        pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>> call.rdx_out);
>> +        return false;
>> +    }
>>
>> -    return (platform_cmds & send_cmd_mask) == send_cmd_mask;
>> +    /* Check platform commands is TPM_SEND_COMMAND - platform command
>> 8 */
>> +    return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
>> }
>> EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
>>
>>
>> -- 
>> Regards/Gruss,
>>    Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette
>>
>