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