diff mbox series

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

Message ID 20250228170720.144739-3-sgarzare@redhat.com (mailing list archive)
State New
Headers show
Series Enlightened vTPM support for SVSM on SEV-SNP | expand

Commit Message

Stefano Garzarella Feb. 28, 2025, 5:07 p.m. UTC 
Add two new functions to probe and send commands to the SVSM vTPM.
They leverage the two calls defined by the AMD SVSM specification
for the vTPM protocol: SVSM_VTPM_QUERY and SVSM_VTPM_CMD.

Expose these functions to be used by other modules such as a tpm
driver.

Co-developed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Co-developed-by: Claudio Carvalho <cclaudio@linux.ibm.com>
Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
James, Claudio are you fine with the Cdb, Sob?
The code is pretty much similar to what was in the initial RFC, but I
changed the context for that I reset the author but added C-o-b.
Please, let me know if this is okay or if I need to do anything
else (reset the author, etc.)
---
 arch/x86/include/asm/sev.h |  3 +++
 arch/x86/coco/sev/core.c   | 47 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

Comments

Borislav Petkov March 10, 2025, 11:30 a.m. UTC | #1 
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?

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

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);
Stefano Garzarella March 10, 2025, 12:46 p.m. UTC | #2 
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
>
Tom Lendacky March 10, 2025, 1:27 p.m. UTC | #3 
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
>>
>
Borislav Petkov March 10, 2025, 1:51 p.m. UTC | #4 
On Mon, Mar 10, 2025 at 08:27:37AM -0500, Tom Lendacky wrote:
> I don't think anything needs to be checked or printed.

Yes.

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

I still don't get the need to print anything. Why?
Tom Lendacky March 10, 2025, 1:56 p.m. UTC | #5 
On 3/10/25 08:51, Borislav Petkov wrote:
> On Mon, Mar 10, 2025 at 08:27:37AM -0500, Tom Lendacky wrote:
>> I don't think anything needs to be checked or printed.
> 
> Yes.
> 
>> 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.
> 
> I still don't get the need to print anything. Why?

It isn't needed. It's similar to "device" information/capabilities.
Maybe pr_debug() then? But I'm also fine with not printing anything.

Thanks,
Tom

>
Stefano Garzarella March 10, 2025, 1:59 p.m. UTC | #6 
On Mon, Mar 10, 2025 at 02:51:33PM +0100, Borislav Petkov wrote:
>On Mon, Mar 10, 2025 at 08:27:37AM -0500, Tom Lendacky wrote:
>> I don't think anything needs to be checked or printed.
>
>Yes.

Ack, I removed the check and the print.

@Boris I also removed `ret` to continue the slimming, so the end
result should be this:

bool snp_svsm_vtpm_probe(void)
{
	struct svsm_call call = {};

	/* The vTPM device is available only if a SVSM is present */
	if (!snp_vmpl)
		return false;

	call.caa = svsm_get_caa();
	call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);

	if (svsm_perform_call_protocol(&call))
		return false;

	/* Check platform commands contains TPM_SEND_COMMAND - platform command 8 */
	return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
}

Quite nice, thanks for the review!
Stefano
Borislav Petkov March 10, 2025, 2:02 p.m. UTC | #7 
On Mon, Mar 10, 2025 at 08:56:53AM -0500, Tom Lendacky wrote:
> It isn't needed. It's similar to "device" information/capabilities.
> Maybe pr_debug() then? But I'm also fine with not printing anything.

Yap, nothing it is then.

If the need arises, then we can debate :)
Borislav Petkov March 10, 2025, 2:04 p.m. UTC | #8 
On Mon, Mar 10, 2025 at 02:59:44PM +0100, Stefano Garzarella wrote:
> On Mon, Mar 10, 2025 at 02:51:33PM +0100, Borislav Petkov wrote:
> > On Mon, Mar 10, 2025 at 08:27:37AM -0500, Tom Lendacky wrote:
> > > I don't think anything needs to be checked or printed.
> > 
> > Yes.
> 
> Ack, I removed the check and the print.
> 
> @Boris I also removed `ret` to continue the slimming, so the end
> result should be this:
> 
> bool snp_svsm_vtpm_probe(void)
> {
> 	struct svsm_call call = {};
> 
> 	/* The vTPM device is available only if a SVSM is present */
> 	if (!snp_vmpl)
> 		return false;
> 
> 	call.caa = svsm_get_caa();
> 	call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
> 
> 	if (svsm_perform_call_protocol(&call))
> 		return false;
> 
> 	/* Check platform commands contains TPM_SEND_COMMAND - platform command 8 */
> 	return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
> }
> 
> Quite nice, thanks for the review!

Thanks, looks clean to me. :)
diff mbox series

Patch

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index f6ebf4492606..e379bcdddf07 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -485,6 +485,9 @@  void snp_msg_free(struct snp_msg_desc *mdesc);
 int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
 			   struct snp_guest_request_ioctl *rio);
 
+bool snp_svsm_vtpm_probe(void);
+int snp_svsm_vtpm_send_command(u8 *buffer);
+
 void __init snp_secure_tsc_prepare(void);
 void __init snp_secure_tsc_init(void);
 
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 82492efc5d94..4158e447d645 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2628,6 +2628,53 @@  static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_dat
 	return ret;
 }
 
+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 */
+	if (!snp_vmpl)
+		return false;
+
+	call.caa = svsm_get_caa();
+	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;
+
+	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);
+}
+EXPORT_SYMBOL_GPL(snp_svsm_vtpm_send_command);
+
 static struct platform_device sev_guest_device = {
 	.name		= "sev-guest",
 	.id		= -1,