Re: [PATCH Part1 v5 34/38] x86/sev: Add snp_msg_seqno() helper

[Brijesh Singh <brijesh.singh@amd.com>]

On 8/27/21 1:41 PM, Borislav Petkov wrote:
> On Fri, Aug 20, 2021 at 10:19:29AM -0500, Brijesh Singh wrote:
>> The SNP guest request message header contains a message count. The
>> message count is used while building the IV. The PSP firmware increments
>> the message count by 1, and expects that next message will be using the
>> incremented count. The snp_msg_seqno() helper will be used by driver to
>> get the message sequence counter used in the request message header,
>> and it will be automatically incremented after the request is successful.
>> The incremented value is saved in the secrets page so that the kexec'ed
>> kernel knows from where to begin.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   arch/x86/kernel/sev.c     | 79 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/sev-guest.h | 37 ++++++++++++++++++
>>   2 files changed, 116 insertions(+)
>>
>> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
>> index 319a40fc57ce..f42cd5a8e7bb 100644
>> --- a/arch/x86/kernel/sev.c
>> +++ b/arch/x86/kernel/sev.c
>> @@ -51,6 +51,8 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
>>    */
>>   static struct ghcb __initdata *boot_ghcb;
>>   
> 
> Explain what that is in a comment above it.
> 
>> +static u64 snp_secrets_phys;
> 
> snp_secrets_pa;
> 
> is the usual convention when a variable is supposed to contain a
> physical address.
> 

Noted.

>> +
>>   /* #VC handler runtime per-CPU data */
>>   struct sev_es_runtime_data {
>>   	struct ghcb ghcb_page;
>> @@ -2030,6 +2032,80 @@ bool __init handle_vc_boot_ghcb(struct pt_regs *regs)
>>   		halt();
>>   }
>>   
>> +static struct snp_secrets_page_layout *snp_map_secrets_page(void)
>> +{
>> +	u16 __iomem *secrets;
>> +
>> +	if (!snp_secrets_phys || !sev_feature_enabled(SEV_SNP))
>> +		return NULL;
>> +
>> +	secrets = ioremap_encrypted(snp_secrets_phys, PAGE_SIZE);
>> +	if (!secrets)
>> +		return NULL;
>> +
>> +	return (struct snp_secrets_page_layout *)secrets;
>> +}
> 
> Or simply:
> 
> static struct snp_secrets_page_layout *map_secrets_page(void)
> {
>          if (!snp_secrets_phys || !sev_feature_enabled(SEV_SNP))
>                  return NULL;
>                  
>          return ioremap_encrypted(snp_secrets_phys, PAGE_SIZE);
> }
> 
> ?
> 

Yes that also works.

>> +
>> +static inline u64 snp_read_msg_seqno(void)
> 
> Drop that "snp_" prefix from all those static function names. This one
> is even inline, which means its name doesn't matter at all.
> 
>> +{
>> +	struct snp_secrets_page_layout *layout;
>> +	u64 count;
>> +
>> +	layout = snp_map_secrets_page();
>> +	if (!layout)
>> +		return 0;
>> +
>> +	/* Read the current message sequence counter from secrets pages */
>> +	count = readl(&layout->os_area.msg_seqno_0);
>> +
>> +	iounmap(layout);
>> +
>> +	/* The sequence counter must begin with 1 */
> 
> That sounds weird. Why? 0 is special?

The SNP firmware spec says that counter must begin with the 1.

> 
>> +	if (!count)
>> +		return 1;
>> +
>> +	return count + 1;
>> +}
>> +
>> +u64 snp_msg_seqno(void)
> 
> Function name needs a verb. I.e.,
> 
> 	 snp_get_msg_seqno()
> 
Ok.

>> +{
>> +	u64 count = snp_read_msg_seqno();
>> +
>> +	if (unlikely(!count))
> 
> That looks like a left-over from a previous version as it can't happen.
> 
> Or are you handling the case where the u64 count will wraparound to 0?
> 
> But "The sequence counter must begin with 1" so that read function above
> needs more love.
> 

Yes, I will cleanup a bit more.

>> +		return 0;
> 
> 
>> +
>> +	/*
>> +	 * The message sequence counter for the SNP guest request is a
>> +	 * 64-bit value but the version 2 of GHCB specification defines a
>> +	 * 32-bit storage for the it.
>> +	 */
>> +	if (count >= UINT_MAX)
>> +		return 0;
> 
> Huh, WTF? So when the internal counter goes over u32, this function will
> return 0 only? More weird.
>

During the GHCB writing the seqno use to be 32-bit value and hence the 
GHCB spec choose the 32-bit value but recently the SNP firmware changed 
it from the 32 to 64. So, now we are left with the option of limiting 
the sequence number to 32-bit. If we go beyond 32-bit then all we can do 
is fail the call. If we pass the value of zero then FW will fail the call.



>> +
>> +	return count;
>> +}
>> +EXPORT_SYMBOL_GPL(snp_msg_seqno);
>> +
>> +static void snp_gen_msg_seqno(void)
> 
> That's not "gen" - that's "inc" what this function does. IOW,
> 
> 	snp_inc_msg_seqno
> 

I agree. I will update it.

>> +{
>> +	struct snp_secrets_page_layout *layout;
>> +	u64 count;
>> +
>> +	layout = snp_map_secrets_page();
>> +	if (!layout)
>> +		return;
>> +
>> +	/*
>> +	 * The counter is also incremented by the PSP, so increment it by 2
>> +	 * and save in secrets page.
>> +	 */
>> +	count = readl(&layout->os_area.msg_seqno_0);
>> +	count += 2;
>> +
>> +	writel(count, &layout->os_area.msg_seqno_0);
>> +	iounmap(layout);
> 
> Why does this need to constantly map and unmap the secrets page? Why
> don't you map it once on init and unmap it on exit?
> 

Yes, I can remove that with:

secrets_va = (__force void *)ioremap_encrypted(pa...)

And then use secrets_va instead of doing readl/writel.

>> +}
>> +
>>   int snp_issue_guest_request(int type, struct snp_guest_request_data *input, unsigned long *fw_err)
>>   {
>>   	struct ghcb_state state;
>> @@ -2077,6 +2153,9 @@ int snp_issue_guest_request(int type, struct snp_guest_request_data *input, unsi
>>   		ret = -EIO;
>>   	}
>>   
>> +	/* The command was successful, increment the sequence counter */
>> +	snp_gen_msg_seqno();
>> +
>>   e_put:
>>   	__sev_put_ghcb(&state);
>>   e_restore_irq:
>> diff --git a/include/linux/sev-guest.h b/include/linux/sev-guest.h
>> index 24dd17507789..16b6af24fda7 100644
>> --- a/include/linux/sev-guest.h
>> +++ b/include/linux/sev-guest.h
>> @@ -20,6 +20,41 @@ enum vmgexit_type {
>>   	GUEST_REQUEST_MAX
>>   };
>>   
>> +/*
>> + * The secrets page contains 96-bytes of reserved field that can be used by
>> + * the guest OS. The guest OS uses the area to save the message sequence
>> + * number for each VMPCK.
>> + *
>> + * See the GHCB spec section Secret page layout for the format for this area.
>> + */
>> +struct secrets_os_area {
>> +	u32 msg_seqno_0;
>> +	u32 msg_seqno_1;
>> +	u32 msg_seqno_2;
>> +	u32 msg_seqno_3;
>> +	u64 ap_jump_table_pa;
>> +	u8 rsvd[40];
>> +	u8 guest_usage[32];
>> +} __packed;
> 
> So those are differently named there:
> 
> struct secrets_page_os_area {
> 	uint32 vmpl0_message_seq_num;
> 	uint32 vmpl1_message_seq_num;
> 	...
> 
> and they have "vmpl" in there which makes a lot more sense for that
> they're used than msg_seqno_* does.
> 

I just choose the smaller name but I have no issues matching with the 
spec. Also those keys does not have anything to do with the VMPL level. 
The secrets page provides 4 different keys and they are referred as 
vmpck0..3 and each of them have a sequence numbers associated with it.

In GHCB v3 we probably need to rework the structure name.

>> +
>> +#define VMPCK_KEY_LEN		32
>> +
>> +/* See the SNP spec for secrets page format */
>> +struct snp_secrets_page_layout {
> 
> Simply
> 
> 	struct snp_secrets
> 
> That name says all you need to know about what that struct represents.
> 
>> +	u32 version;
>> +	u32 imien	: 1,
>> +	    rsvd1	: 31;
>> +	u32 fms;
>> +	u32 rsvd2;
>> +	u8 gosvw[16];
>> +	u8 vmpck0[VMPCK_KEY_LEN];
>> +	u8 vmpck1[VMPCK_KEY_LEN];
>> +	u8 vmpck2[VMPCK_KEY_LEN];
>> +	u8 vmpck3[VMPCK_KEY_LEN];
>> +	struct secrets_os_area os_area;
> 
> My SNP spec copy has here
> 
> 0A0h–FFFh	Reserved.
> 
> and no os area. I guess
> 
> SEV Secure Nested Paging Firmware ABI Specification 56860 Rev. 0.8 August 2020
> 
> needs updating...

The latest SNP spec here:

https://www.amd.com/system/files/TechDocs/56860.pdf

We are at spec 0.9.

> 
>> +	u8 rsvd3[3840];
>> +} __packed;
>> +
>>   /*
>>    * The error code when the data_npages is too small. The error code
>>    * is defined in the GHCB specification.
>> @@ -36,6 +71,7 @@ struct snp_guest_request_data {
>>   #ifdef CONFIG_AMD_MEM_ENCRYPT
>>   int snp_issue_guest_request(int vmgexit_type, struct snp_guest_request_data *input,
>>   			    unsigned long *fw_err);
>> +u64 snp_msg_seqno(void);
>>   #else
>>   
>>   static inline int snp_issue_guest_request(int type, struct snp_guest_request_data *input,
>> @@ -43,6 +79,7 @@ static inline int snp_issue_guest_request(int type, struct snp_guest_request_dat
>>   {
>>   	return -ENODEV;
>>   }
>> +static inline u64 snp_msg_seqno(void) { return 0; }
>>   
>>   #endif /* CONFIG_AMD_MEM_ENCRYPT */
>>   #endif /* __LINUX_SEV_GUEST_H__ */
>> -- 
>> 2.17.1
>>
>