Messages in this thread | | | Subject | Re: [PATCH Part1 v5 34/38] x86/sev: Add snp_msg_seqno() helper | From | Brijesh Singh <> | Date | Mon, 30 Aug 2021 10:07:39 -0500 |
| |
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 >> >
| |