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

[Stefano Garzarella]

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.

> > +
> > +	/* 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