| Message ID | 20210820151933.22401-34-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Fri, Aug 20, 2021 at 10:19:28AM -0500, Brijesh Singh wrote: > +int snp_issue_guest_request(int type, struct snp_guest_request_data *input, unsigned long *fw_err) > +{ > + struct ghcb_state state; > + unsigned long id, flags; > + struct ghcb *ghcb; > + int ret; > + > + if (!sev_feature_enabled(SEV_SNP)) > + return -ENODEV; > + > + local_irq_save(flags); > + > + ghcb = __sev_get_ghcb(&state); > + if (!ghcb) { > + ret = -EIO; > + goto e_restore_irq; > + } > + > + vc_ghcb_invalidate(ghcb); > + > + if (type == GUEST_REQUEST) { > + id = SVM_VMGEXIT_GUEST_REQUEST; > + } else if (type == EXT_GUEST_REQUEST) { > + id = SVM_VMGEXIT_EXT_GUEST_REQUEST; > + ghcb_set_rax(ghcb, input->data_gpa); > + ghcb_set_rbx(ghcb, input->data_npages); Hmmm, now I'm not sure. We did enum psc_op where you simply pass in the op directly to the hardware because the enum uses the same numbers as the actual command. But here that @type thing is simply used to translate to the SVM_VMGEXIT thing. So maybe you don't need it here and you can hand in the exit_code directly: int snp_issue_guest_request(u64 exit_code, struct snp_guest_request_data *input, unsigned long *fw_err) which you then pass in directly to... > + } else { > + ret = -EINVAL; > + goto e_put; > + } > + > + ret = sev_es_ghcb_hv_call(ghcb, NULL, id, input->req_gpa, input->resp_gpa); ... this guy here: ret = sev_es_ghcb_hv_call(ghcb, NULL, exit_code, input->req_gpa, input->resp_gpa); > + if (ret) > + goto e_put; > + > + if (ghcb->save.sw_exit_info_2) { > + /* Number of expected pages are returned in RBX */ > + if (id == EXT_GUEST_REQUEST && > + ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) > + input->data_npages = ghcb_get_rbx(ghcb); > + > + if (fw_err) > + *fw_err = ghcb->save.sw_exit_info_2; > + > + ret = -EIO; > + } > + > +e_put: > + __sev_put_ghcb(&state); > +e_restore_irq: > + local_irq_restore(flags); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(snp_issue_guest_request); > diff --git a/include/linux/sev-guest.h b/include/linux/sev-guest.h Why is this a separate header in the include/linux/ namespace? Is SNP available on something which is !x86, all of a sudden? > new file mode 100644 > index 000000000000..24dd17507789 > --- /dev/null > +++ b/include/linux/sev-guest.h > @@ -0,0 +1,48 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * AMD Secure Encrypted Virtualization (SEV) guest driver interface > + * > + * Copyright (C) 2021 Advanced Micro Devices, Inc. > + * > + * Author: Brijesh Singh <brijesh.singh@amd.com> > + * > + */ > + > +#ifndef __LINUX_SEV_GUEST_H_ > +#define __LINUX_SEV_GUEST_H_ > + > +#include <linux/types.h> > + > +enum vmgexit_type { > + GUEST_REQUEST, > + EXT_GUEST_REQUEST, > + > + GUEST_REQUEST_MAX > +}; > + > +/* > + * The error code when the data_npages is too small. The error code > + * is defined in the GHCB specification. > + */ > +#define SNP_GUEST_REQ_INVALID_LEN 0x100000000ULL so basically BIT_ULL(32) > + > +struct snp_guest_request_data { "snp_req_data" I guess is shorter. And having "guest" in there is probably not needed because snp is always guest-related anyway. > + unsigned long req_gpa; > + unsigned long resp_gpa; > + unsigned long data_gpa; > + unsigned int data_npages; > +}; > + > +#ifdef CONFIG_AMD_MEM_ENCRYPT > +int snp_issue_guest_request(int vmgexit_type, struct snp_guest_request_data *input, > + unsigned long *fw_err); > +#else > + > +static inline int snp_issue_guest_request(int type, struct snp_guest_request_data *input, > + unsigned long *fw_err) > +{ > + return -ENODEV; > +} > + > +#endif /* CONFIG_AMD_MEM_ENCRYPT */ > +#endif /* __LINUX_SEV_GUEST_H__ */ > -- > 2.17.1 >
On 8/27/21 12:44 PM, Borislav Petkov wrote: > On Fri, Aug 20, 2021 at 10:19:28AM -0500, Brijesh Singh wrote: >> +int snp_issue_guest_request(int type, struct snp_guest_request_data *input, unsigned long *fw_err) >> +{ >> + struct ghcb_state state; >> + unsigned long id, flags; >> + struct ghcb *ghcb; >> + int ret; >> + >> + if (!sev_feature_enabled(SEV_SNP)) >> + return -ENODEV; >> + >> + local_irq_save(flags); >> + >> + ghcb = __sev_get_ghcb(&state); >> + if (!ghcb) { >> + ret = -EIO; >> + goto e_restore_irq; >> + } >> + >> + vc_ghcb_invalidate(ghcb); >> + >> + if (type == GUEST_REQUEST) { >> + id = SVM_VMGEXIT_GUEST_REQUEST; >> + } else if (type == EXT_GUEST_REQUEST) { >> + id = SVM_VMGEXIT_EXT_GUEST_REQUEST; >> + ghcb_set_rax(ghcb, input->data_gpa); >> + ghcb_set_rbx(ghcb, input->data_npages); > Hmmm, now I'm not sure. We did enum psc_op where you simply pass in the > op directly to the hardware because the enum uses the same numbers as > the actual command. > > But here that @type thing is simply used to translate to the SVM_VMGEXIT > thing. So maybe you don't need it here and you can hand in the exit_code > directly: > > int snp_issue_guest_request(u64 exit_code, struct snp_guest_request_data *input, > unsigned long *fw_err) > > which you then pass in directly to... Okay, works for me. The main reason why I choose the enum was to not expose the GHCB exit code to the driver but I guess that attestation driver need to know which VMGEXIT need to be called, so, its okay to have it pass the VMGEXIT number instead of enum. >> + } else { >> + ret = -EINVAL; >> + goto e_put; >> + } >> + >> + ret = sev_es_ghcb_hv_call(ghcb, NULL, id, input->req_gpa, input->resp_gpa); > ... this guy here: > > ret = sev_es_ghcb_hv_call(ghcb, NULL, exit_code, input->req_gpa, input->resp_gpa); > >> + if (ret) >> + goto e_put; >> + >> + if (ghcb->save.sw_exit_info_2) { >> + /* Number of expected pages are returned in RBX */ >> + if (id == EXT_GUEST_REQUEST && >> + ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) >> + input->data_npages = ghcb_get_rbx(ghcb); >> + >> + if (fw_err) >> + *fw_err = ghcb->save.sw_exit_info_2; >> + >> + ret = -EIO; >> + } >> + >> +e_put: >> + __sev_put_ghcb(&state); >> +e_restore_irq: >> + local_irq_restore(flags); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(snp_issue_guest_request); >> diff --git a/include/linux/sev-guest.h b/include/linux/sev-guest.h > Why is this a separate header in the include/linux/ namespace? > > Is SNP available on something which is !x86, all of a sudden? So far most of the changes were in x86 specific files. However, the attestation service is available through a driver to the userspace. The driver needs to use the VMGEXIT routines provides by the x86 core. I created the said header file so that driver does not need to include <asm/sev.h/sev-common.h> etc and will compile for !x86. >> new file mode 100644 >> index 000000000000..24dd17507789 >> --- /dev/null >> +++ b/include/linux/sev-guest.h >> @@ -0,0 +1,48 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * AMD Secure Encrypted Virtualization (SEV) guest driver interface >> + * >> + * Copyright (C) 2021 Advanced Micro Devices, Inc. >> + * >> + * Author: Brijesh Singh <brijesh.singh@amd.com> >> + * >> + */ >> + >> +#ifndef __LINUX_SEV_GUEST_H_ >> +#define __LINUX_SEV_GUEST_H_ >> + >> +#include <linux/types.h> >> + >> +enum vmgexit_type { >> + GUEST_REQUEST, >> + EXT_GUEST_REQUEST, >> + >> + GUEST_REQUEST_MAX >> +}; >> + >> +/* >> + * The error code when the data_npages is too small. The error code >> + * is defined in the GHCB specification. >> + */ >> +#define SNP_GUEST_REQ_INVALID_LEN 0x100000000ULL > so basically > > BIT_ULL(32) Noted. > >> + >> +struct snp_guest_request_data { > "snp_req_data" I guess is shorter. And having "guest" in there is > probably not needed because snp is always guest-related anyway. Noted. >> + unsigned long req_gpa; >> + unsigned long resp_gpa; >> + unsigned long data_gpa; >> + unsigned int data_npages; >> +}; >> + >> +#ifdef CONFIG_AMD_MEM_ENCRYPT >> +int snp_issue_guest_request(int vmgexit_type, struct snp_guest_request_data *input, >> + unsigned long *fw_err); >> +#else >> + >> +static inline int snp_issue_guest_request(int type, struct snp_guest_request_data *input, >> + unsigned long *fw_err) >> +{ >> + return -ENODEV; >> +} >> + >> +#endif /* CONFIG_AMD_MEM_ENCRYPT */ >> +#endif /* __LINUX_SEV_GUEST_H__ */ >> -- >> 2.17.1 >>
On Fri, Aug 27, 2021 at 01:07:59PM -0500, Brijesh Singh wrote: > Okay, works for me. The main reason why I choose the enum was to not > expose the GHCB exit code to the driver Why does that matter? > but I guess that attestation driver need to know which VMGEXIT need to > be called, so, its okay to have it pass the VMGEXIT number instead of > enum. Well, they're in an uapi header - can't be more public than that. :-) > So far most of the changes were in x86 specific files. However, the > attestation service is available through a driver to the userspace. The > driver needs to use the VMGEXIT routines provides by the x86 core. I > created the said header file so that driver does not need to include > <asm/sev.h/sev-common.h> etc and will compile for !x86. Lemme ask my question again: Is SNP available on something which is !x86, all of a sudden? Why would you want to compile that driver on anything but x86?
On 8/27/21 1:13 PM, Borislav Petkov wrote: > On Fri, Aug 27, 2021 at 01:07:59PM -0500, Brijesh Singh wrote: >> Okay, works for me. The main reason why I choose the enum was to not >> expose the GHCB exit code to the driver > Why does that matter? Those definitions are present in <asm/xxx>. Somewhere I read said that if possible a new drivers should avoid including the <asm/xxx>. This is one of the motivation to create a new file to provide the selective information. > Lemme ask my question again: > > Is SNP available on something which is !x86, all of a sudden? Nope > > Why would you want to compile that driver on anything but x86? > Nobody should compile it for !x86. If my read about including <asm/xx> in driver is wrong then I do not see any need for creating new header file. I can drop the header file inclusion in next update. thanks
On Fri, Aug 27, 2021 at 01:27:14PM -0500, Brijesh Singh wrote: > Those definitions are present in <asm/xxx>. Somewhere I read said that > if possible a new drivers should avoid including the <asm/xxx>. Where? That is news to me. It is likely possible that I might've missed that rule but it doesn't look like there's a rule like that at the moment: $ git grep -E "include.*asm" drivers/ | wc -l 4475
On 8/27/21 1:07 PM, Brijesh Singh wrote: > On 8/27/21 12:44 PM, Borislav Petkov wrote: >> On Fri, Aug 20, 2021 at 10:19:28AM -0500, Brijesh Singh wrote: ... >>> + >>> +/* >>> + * The error code when the data_npages is too small. The error code >>> + * is defined in the GHCB specification. >>> + */ >>> +#define SNP_GUEST_REQ_INVALID_LEN 0x100000000ULL >> so basically >> >> BIT_ULL(32) > > Noted. The main thing about this is that it is an error code from the HV on extended guest requests. The HV error code sits in the high-order 32-bits of the SW_EXIT_INFO_2 field. So defining it either way seems a bit confusing. To me, the value should just be 1ULL and then it should be shifted when assigning it to the SW_EXIT_INFO_2. Thanks, Tom >
On Fri, Aug 27, 2021 at 02:57:11PM -0500, Tom Lendacky wrote: > The main thing about this is that it is an error code from the HV on > extended guest requests. The HV error code sits in the high-order 32-bits of > the SW_EXIT_INFO_2 field. So defining it either way seems a bit confusing. > To me, the value should just be 1ULL and then it should be shifted when > assigning it to the SW_EXIT_INFO_2. Err, that's from the GHCB spec: "The hypervisor must validate that the guest has supplied enough pages ... certificate data in the RBX register and set the SW_EXITINFO2 field to 0x0000000100000000." So if you wanna do the above, you need to fix the spec first. I'd say. :-)
On 8/27/21 3:17 PM, Borislav Petkov wrote: > On Fri, Aug 27, 2021 at 02:57:11PM -0500, Tom Lendacky wrote: >> The main thing about this is that it is an error code from the HV on >> extended guest requests. The HV error code sits in the high-order 32-bits of >> the SW_EXIT_INFO_2 field. So defining it either way seems a bit confusing. >> To me, the value should just be 1ULL and then it should be shifted when >> assigning it to the SW_EXIT_INFO_2. > > Err, that's from the GHCB spec: > > "The hypervisor must validate that the guest has supplied enough pages > > ... > > certificate data in the RBX register and set the SW_EXITINFO2 field to > 0x0000000100000000." > > So if you wanna do the above, you need to fix the spec first. I'd say. > > :-) See the NAE Event table that documents "State from Hypervisor" where it says the upper 32-bits (63:32) will contain the return code from the hypervisor. In the case you quoted, that specific situation is documented to return a hypervisor return code of 1 (since the hypervisor return code occupies bits 63:32). The hypervisor is free to return other values, that need not be documented in spec, if it encounters other types of unforeseeable errors. Thanks, Tom >
diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h index 8b4c57baec52..5b8bc2b65a5e 100644 --- a/arch/x86/include/uapi/asm/svm.h +++ b/arch/x86/include/uapi/asm/svm.h @@ -109,6 +109,8 @@ #define SVM_VMGEXIT_SET_AP_JUMP_TABLE 0 #define SVM_VMGEXIT_GET_AP_JUMP_TABLE 1 #define SVM_VMGEXIT_PSC 0x80000010 +#define SVM_VMGEXIT_GUEST_REQUEST 0x80000011 +#define SVM_VMGEXIT_EXT_GUEST_REQUEST 0x80000012 #define SVM_VMGEXIT_AP_CREATION 0x80000013 #define SVM_VMGEXIT_AP_CREATE_ON_INIT 0 #define SVM_VMGEXIT_AP_CREATE 1 @@ -225,6 +227,8 @@ { SVM_VMGEXIT_AP_HLT_LOOP, "vmgexit_ap_hlt_loop" }, \ { SVM_VMGEXIT_AP_JUMP_TABLE, "vmgexit_ap_jump_table" }, \ { SVM_VMGEXIT_PSC, "vmgexit_page_state_change" }, \ + { SVM_VMGEXIT_GUEST_REQUEST, "vmgexit_guest_request" }, \ + { SVM_VMGEXIT_EXT_GUEST_REQUEST, "vmgexit_ext_guest_request" }, \ { SVM_VMGEXIT_AP_CREATION, "vmgexit_ap_creation" }, \ { SVM_VMGEXIT_HV_FEATURES, "vmgexit_hypervisor_feature" }, \ { SVM_EXIT_ERR, "invalid_guest_state" } diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index d7b6f7420551..319a40fc57ce 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -21,6 +21,7 @@ #include <linux/cpumask.h> #include <linux/log2.h> #include <linux/efi.h> +#include <linux/sev-guest.h> #include <asm/cpu_entry_area.h> #include <asm/stacktrace.h> @@ -2028,3 +2029,59 @@ bool __init handle_vc_boot_ghcb(struct pt_regs *regs) while (true) halt(); } + +int snp_issue_guest_request(int type, struct snp_guest_request_data *input, unsigned long *fw_err) +{ + struct ghcb_state state; + unsigned long id, flags; + struct ghcb *ghcb; + int ret; + + if (!sev_feature_enabled(SEV_SNP)) + return -ENODEV; + + local_irq_save(flags); + + ghcb = __sev_get_ghcb(&state); + if (!ghcb) { + ret = -EIO; + goto e_restore_irq; + } + + vc_ghcb_invalidate(ghcb); + + if (type == GUEST_REQUEST) { + id = SVM_VMGEXIT_GUEST_REQUEST; + } else if (type == EXT_GUEST_REQUEST) { + id = SVM_VMGEXIT_EXT_GUEST_REQUEST; + ghcb_set_rax(ghcb, input->data_gpa); + ghcb_set_rbx(ghcb, input->data_npages); + } else { + ret = -EINVAL; + goto e_put; + } + + ret = sev_es_ghcb_hv_call(ghcb, NULL, id, input->req_gpa, input->resp_gpa); + if (ret) + goto e_put; + + if (ghcb->save.sw_exit_info_2) { + /* Number of expected pages are returned in RBX */ + if (id == EXT_GUEST_REQUEST && + ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) + input->data_npages = ghcb_get_rbx(ghcb); + + if (fw_err) + *fw_err = ghcb->save.sw_exit_info_2; + + ret = -EIO; + } + +e_put: + __sev_put_ghcb(&state); +e_restore_irq: + local_irq_restore(flags); + + return ret; +} +EXPORT_SYMBOL_GPL(snp_issue_guest_request); diff --git a/include/linux/sev-guest.h b/include/linux/sev-guest.h new file mode 100644 index 000000000000..24dd17507789 --- /dev/null +++ b/include/linux/sev-guest.h @@ -0,0 +1,48 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * AMD Secure Encrypted Virtualization (SEV) guest driver interface + * + * Copyright (C) 2021 Advanced Micro Devices, Inc. + * + * Author: Brijesh Singh <brijesh.singh@amd.com> + * + */ + +#ifndef __LINUX_SEV_GUEST_H_ +#define __LINUX_SEV_GUEST_H_ + +#include <linux/types.h> + +enum vmgexit_type { + GUEST_REQUEST, + EXT_GUEST_REQUEST, + + GUEST_REQUEST_MAX +}; + +/* + * The error code when the data_npages is too small. The error code + * is defined in the GHCB specification. + */ +#define SNP_GUEST_REQ_INVALID_LEN 0x100000000ULL + +struct snp_guest_request_data { + unsigned long req_gpa; + unsigned long resp_gpa; + unsigned long data_gpa; + unsigned int data_npages; +}; + +#ifdef CONFIG_AMD_MEM_ENCRYPT +int snp_issue_guest_request(int vmgexit_type, struct snp_guest_request_data *input, + unsigned long *fw_err); +#else + +static inline int snp_issue_guest_request(int type, struct snp_guest_request_data *input, + unsigned long *fw_err) +{ + return -ENODEV; +} + +#endif /* CONFIG_AMD_MEM_ENCRYPT */ +#endif /* __LINUX_SEV_GUEST_H__ */
Version 2 of GHCB specification provides SNP_GUEST_REQUEST and SNP_EXT_GUEST_REQUEST NAE that can be used by the SNP guest to communicate with the PSP. While at it, add a snp_issue_guest_request() helper that can be used by driver or other subsystem to issue the request to PSP. See SEV-SNP and GHCB spec for more details. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/uapi/asm/svm.h | 4 +++ arch/x86/kernel/sev.c | 57 +++++++++++++++++++++++++++++++++ include/linux/sev-guest.h | 48 +++++++++++++++++++++++++++ 3 files changed, 109 insertions(+) create mode 100644 include/linux/sev-guest.h