| Message ID | 20210430121616.2295-4-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, Apr 30, 2021 at 07:15:59AM -0500, Brijesh Singh wrote: > Version 2 of GHCB specification introduced advertisement of a features > that are supported by the hypervisor. Define the GHCB MSR protocol and NAE > for the hypervisor feature request and query the feature during the GHCB > protocol negotitation. See the GHCB specification for more details. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/include/asm/sev-common.h | 17 +++++++++++++++++ > arch/x86/include/uapi/asm/svm.h | 2 ++ > arch/x86/kernel/sev-shared.c | 24 ++++++++++++++++++++++++ > 3 files changed, 43 insertions(+) > > diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h > index 9f1b66090a4c..8142e247d8da 100644 > --- a/arch/x86/include/asm/sev-common.h > +++ b/arch/x86/include/asm/sev-common.h > @@ -51,6 +51,22 @@ > #define GHCB_MSR_AP_RESET_HOLD_RESULT_POS 12 > #define GHCB_MSR_AP_RESET_HOLD_RESULT_MASK 0xfffffffffffff > > +/* GHCB Hypervisor Feature Request */ > +#define GHCB_MSR_HV_FEATURES_REQ 0x080 > +#define GHCB_MSR_HV_FEATURES_RESP 0x081 > +#define GHCB_MSR_HV_FEATURES_POS 12 > +#define GHCB_MSR_HV_FEATURES_MASK 0xfffffffffffffUL > +#define GHCB_MSR_HV_FEATURES_RESP_VAL(v) \ > + (((v) >> GHCB_MSR_HV_FEATURES_POS) & GHCB_MSR_HV_FEATURES_MASK) > + > +#define GHCB_HV_FEATURES_SNP BIT_ULL(0) > +#define GHCB_HV_FEATURES_SNP_AP_CREATION \ > + (BIT_ULL(1) | GHCB_HV_FEATURES_SNP) > +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION \ > + (BIT_ULL(2) | GHCB_HV_FEATURES_SNP_AP_CREATION) > +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER \ > + (BIT_ULL(3) | GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION) Please add those in the patches which use them - not in bulk here. And GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER is a mouthfull and looks like BIOS code to me. But this is still the kernel, remember? :-) So let's do GHCB_MSR_HV_FT_* GHCB_SNP_AP_CREATION GHCB_SNP_RESTRICTED_INJ GHCB_SNP_RESTRICTED_INJ_TMR and so on so that we can all keep our sanity when reading that code. > + > #define GHCB_MSR_TERM_REQ 0x100 > #define GHCB_MSR_TERM_REASON_SET_POS 12 > #define GHCB_MSR_TERM_REASON_SET_MASK 0xf > @@ -62,6 +78,7 @@ > > #define GHCB_SEV_ES_REASON_GENERAL_REQUEST 0 > #define GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED 1 > +#define GHCB_SEV_ES_REASON_SNP_UNSUPPORTED 2 I remember asking for those to get shortened too GHCB_SEV_ES_GEN_REQ GHCB_SEV_ES_PROT_UNSUPPORTED GHCB_SEV_ES_SNP_UNSUPPORTED Perhaps in a prepatch? > #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK) > > diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h > index 554f75fe013c..7fbc311e2de1 100644 > --- a/arch/x86/include/uapi/asm/svm.h > +++ b/arch/x86/include/uapi/asm/svm.h > @@ -108,6 +108,7 @@ > #define SVM_VMGEXIT_AP_JUMP_TABLE 0x80000005 > #define SVM_VMGEXIT_SET_AP_JUMP_TABLE 0 > #define SVM_VMGEXIT_GET_AP_JUMP_TABLE 1 > +#define SVM_VMGEXIT_HYPERVISOR_FEATURES 0x8000fffd SVM_VMGEXIT_HV_FT you get the idea. > #define SVM_VMGEXIT_UNSUPPORTED_EVENT 0x8000ffff > > #define SVM_EXIT_ERR -1 > @@ -215,6 +216,7 @@ > { SVM_VMGEXIT_NMI_COMPLETE, "vmgexit_nmi_complete" }, \ > { SVM_VMGEXIT_AP_HLT_LOOP, "vmgexit_ap_hlt_loop" }, \ > { SVM_VMGEXIT_AP_JUMP_TABLE, "vmgexit_ap_jump_table" }, \ > + { SVM_VMGEXIT_HYPERVISOR_FEATURES, "vmgexit_hypervisor_feature" }, \ > { SVM_EXIT_ERR, "invalid_guest_state" } > > > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > index 48a47540b85f..874f911837db 100644 > --- a/arch/x86/kernel/sev-shared.c > +++ b/arch/x86/kernel/sev-shared.c > @@ -20,6 +20,7 @@ > * out when the .bss section is later cleared. > */ > static u16 ghcb_version __section(".data") = 0; > +static u64 hv_features __section(".data") = 0; ERROR: do not initialise statics to 0 #181: FILE: arch/x86/kernel/sev-shared.c:23: +static u64 hv_features __section(".data") = 0; > static bool __init sev_es_check_cpu_features(void) > { > @@ -49,6 +50,26 @@ static void __noreturn sev_es_terminate(unsigned int reason) > asm volatile("hlt\n" : : : "memory"); > } > > +static bool ghcb_get_hv_features(void) Used only once here - no need for the ghcb_ prefix. > +{ > + u64 val; > + > + /* The hypervisor features are available from version 2 onward. */ > + if (ghcb_version < 2) > + return true; return false; no? Also, this should be done differently: if (ghcb_version >= 2) get_hv_features(); at the call site. Thx.
On 5/11/21 5:01 AM, Borislav Petkov wrote: > On Fri, Apr 30, 2021 at 07:15:59AM -0500, Brijesh Singh wrote: >> Version 2 of GHCB specification introduced advertisement of a features >> that are supported by the hypervisor. Define the GHCB MSR protocol and NAE >> for the hypervisor feature request and query the feature during the GHCB >> protocol negotitation. See the GHCB specification for more details. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> arch/x86/include/asm/sev-common.h | 17 +++++++++++++++++ >> arch/x86/include/uapi/asm/svm.h | 2 ++ >> arch/x86/kernel/sev-shared.c | 24 ++++++++++++++++++++++++ >> 3 files changed, 43 insertions(+) >> >> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h >> index 9f1b66090a4c..8142e247d8da 100644 >> --- a/arch/x86/include/asm/sev-common.h >> +++ b/arch/x86/include/asm/sev-common.h >> @@ -51,6 +51,22 @@ >> #define GHCB_MSR_AP_RESET_HOLD_RESULT_POS 12 >> #define GHCB_MSR_AP_RESET_HOLD_RESULT_MASK 0xfffffffffffff >> >> +/* GHCB Hypervisor Feature Request */ >> +#define GHCB_MSR_HV_FEATURES_REQ 0x080 >> +#define GHCB_MSR_HV_FEATURES_RESP 0x081 >> +#define GHCB_MSR_HV_FEATURES_POS 12 >> +#define GHCB_MSR_HV_FEATURES_MASK 0xfffffffffffffUL >> +#define GHCB_MSR_HV_FEATURES_RESP_VAL(v) \ >> + (((v) >> GHCB_MSR_HV_FEATURES_POS) & GHCB_MSR_HV_FEATURES_MASK) >> + >> +#define GHCB_HV_FEATURES_SNP BIT_ULL(0) >> +#define GHCB_HV_FEATURES_SNP_AP_CREATION \ >> + (BIT_ULL(1) | GHCB_HV_FEATURES_SNP) >> +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION \ >> + (BIT_ULL(2) | GHCB_HV_FEATURES_SNP_AP_CREATION) >> +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER \ >> + (BIT_ULL(3) | GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION) > Please add those in the patches which use them - not in bulk here. > > And GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER is a mouthfull and > looks like BIOS code to me. But this is still the kernel, remember? :-) > > So let's do > > GHCB_MSR_HV_FT_* > > GHCB_SNP_AP_CREATION > GHCB_SNP_RESTRICTED_INJ > GHCB_SNP_RESTRICTED_INJ_TMR > > and so on so that we can all keep our sanity when reading that code. I am fine with the reduced name, I just hope that "TMR" does not create confusion with "Trusted Memory Region" documented in SEV-ES firmware specification. Since I am working on both guest and OVMF patches simultaneously so its possible that I just worked on this code after OVMF and used the same mouthful name ;) I apologies for those nits. >> + >> #define GHCB_MSR_TERM_REQ 0x100 >> #define GHCB_MSR_TERM_REASON_SET_POS 12 >> #define GHCB_MSR_TERM_REASON_SET_MASK 0xf >> @@ -62,6 +78,7 @@ >> >> #define GHCB_SEV_ES_REASON_GENERAL_REQUEST 0 >> #define GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED 1 >> +#define GHCB_SEV_ES_REASON_SNP_UNSUPPORTED 2 > I remember asking for those to get shortened too > > GHCB_SEV_ES_GEN_REQ > GHCB_SEV_ES_PROT_UNSUPPORTED > GHCB_SEV_ES_SNP_UNSUPPORTED > > Perhaps in a prepatch? Sure, I will send prepatch. >> #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK) >> >> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h >> index 554f75fe013c..7fbc311e2de1 100644 >> --- a/arch/x86/include/uapi/asm/svm.h >> +++ b/arch/x86/include/uapi/asm/svm.h >> @@ -108,6 +108,7 @@ >> #define SVM_VMGEXIT_AP_JUMP_TABLE 0x80000005 >> #define SVM_VMGEXIT_SET_AP_JUMP_TABLE 0 >> #define SVM_VMGEXIT_GET_AP_JUMP_TABLE 1 >> +#define SVM_VMGEXIT_HYPERVISOR_FEATURES 0x8000fffd > SVM_VMGEXIT_HV_FT > > you get the idea. > >> #define SVM_VMGEXIT_UNSUPPORTED_EVENT 0x8000ffff >> >> #define SVM_EXIT_ERR -1 >> @@ -215,6 +216,7 @@ >> { SVM_VMGEXIT_NMI_COMPLETE, "vmgexit_nmi_complete" }, \ >> { SVM_VMGEXIT_AP_HLT_LOOP, "vmgexit_ap_hlt_loop" }, \ >> { SVM_VMGEXIT_AP_JUMP_TABLE, "vmgexit_ap_jump_table" }, \ >> + { SVM_VMGEXIT_HYPERVISOR_FEATURES, "vmgexit_hypervisor_feature" }, \ >> { SVM_EXIT_ERR, "invalid_guest_state" } >> >> >> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c >> index 48a47540b85f..874f911837db 100644 >> --- a/arch/x86/kernel/sev-shared.c >> +++ b/arch/x86/kernel/sev-shared.c >> @@ -20,6 +20,7 @@ >> * out when the .bss section is later cleared. >> */ >> static u16 ghcb_version __section(".data") = 0; >> +static u64 hv_features __section(".data") = 0; > ERROR: do not initialise statics to 0 > #181: FILE: arch/x86/kernel/sev-shared.c:23: > +static u64 hv_features __section(".data") = 0; > >> static bool __init sev_es_check_cpu_features(void) >> { >> @@ -49,6 +50,26 @@ static void __noreturn sev_es_terminate(unsigned int reason) >> asm volatile("hlt\n" : : : "memory"); >> } >> >> +static bool ghcb_get_hv_features(void) > Used only once here - no need for the ghcb_ prefix. Noted. > >> +{ >> + u64 val; >> + >> + /* The hypervisor features are available from version 2 onward. */ >> + if (ghcb_version < 2) >> + return true; > return false; > no? > > Also, this should be done differently: > > if (ghcb_version >= 2) > get_hv_features(); > > at the call site. I can go with the change in the call site itself. > > Thx. >
On Tue, May 11, 2021 at 01:53:53PM -0500, Brijesh Singh wrote: > I am fine with the reduced name, I just hope that "TMR" does not create > confusion with "Trusted Memory Region" documented in SEV-ES firmware > specification. Since I am working on both guest and OVMF patches > simultaneously so its possible that I just worked on this code after > OVMF and used the same mouthful name ;) I'm not surprised :-) But sure, call this GHCB_SNP_RESTRICTED_INJ_TIMER Still short enough. > I apologies for those nits. Oh, it's not a nit - it pays off later when chasing bugs and one is trying to swap in the whole situation back into her/his L1. :-) > Sure, I will send prepatch. Thx.
diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h index 9f1b66090a4c..8142e247d8da 100644 --- a/arch/x86/include/asm/sev-common.h +++ b/arch/x86/include/asm/sev-common.h @@ -51,6 +51,22 @@ #define GHCB_MSR_AP_RESET_HOLD_RESULT_POS 12 #define GHCB_MSR_AP_RESET_HOLD_RESULT_MASK 0xfffffffffffff +/* GHCB Hypervisor Feature Request */ +#define GHCB_MSR_HV_FEATURES_REQ 0x080 +#define GHCB_MSR_HV_FEATURES_RESP 0x081 +#define GHCB_MSR_HV_FEATURES_POS 12 +#define GHCB_MSR_HV_FEATURES_MASK 0xfffffffffffffUL +#define GHCB_MSR_HV_FEATURES_RESP_VAL(v) \ + (((v) >> GHCB_MSR_HV_FEATURES_POS) & GHCB_MSR_HV_FEATURES_MASK) + +#define GHCB_HV_FEATURES_SNP BIT_ULL(0) +#define GHCB_HV_FEATURES_SNP_AP_CREATION \ + (BIT_ULL(1) | GHCB_HV_FEATURES_SNP) +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION \ + (BIT_ULL(2) | GHCB_HV_FEATURES_SNP_AP_CREATION) +#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER \ + (BIT_ULL(3) | GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION) + #define GHCB_MSR_TERM_REQ 0x100 #define GHCB_MSR_TERM_REASON_SET_POS 12 #define GHCB_MSR_TERM_REASON_SET_MASK 0xf @@ -62,6 +78,7 @@ #define GHCB_SEV_ES_REASON_GENERAL_REQUEST 0 #define GHCB_SEV_ES_REASON_PROTOCOL_UNSUPPORTED 1 +#define GHCB_SEV_ES_REASON_SNP_UNSUPPORTED 2 #define GHCB_RESP_CODE(v) ((v) & GHCB_MSR_INFO_MASK) diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h index 554f75fe013c..7fbc311e2de1 100644 --- a/arch/x86/include/uapi/asm/svm.h +++ b/arch/x86/include/uapi/asm/svm.h @@ -108,6 +108,7 @@ #define SVM_VMGEXIT_AP_JUMP_TABLE 0x80000005 #define SVM_VMGEXIT_SET_AP_JUMP_TABLE 0 #define SVM_VMGEXIT_GET_AP_JUMP_TABLE 1 +#define SVM_VMGEXIT_HYPERVISOR_FEATURES 0x8000fffd #define SVM_VMGEXIT_UNSUPPORTED_EVENT 0x8000ffff #define SVM_EXIT_ERR -1 @@ -215,6 +216,7 @@ { SVM_VMGEXIT_NMI_COMPLETE, "vmgexit_nmi_complete" }, \ { SVM_VMGEXIT_AP_HLT_LOOP, "vmgexit_ap_hlt_loop" }, \ { SVM_VMGEXIT_AP_JUMP_TABLE, "vmgexit_ap_jump_table" }, \ + { SVM_VMGEXIT_HYPERVISOR_FEATURES, "vmgexit_hypervisor_feature" }, \ { SVM_EXIT_ERR, "invalid_guest_state" } diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c index 48a47540b85f..874f911837db 100644 --- a/arch/x86/kernel/sev-shared.c +++ b/arch/x86/kernel/sev-shared.c @@ -20,6 +20,7 @@ * out when the .bss section is later cleared. */ static u16 ghcb_version __section(".data") = 0; +static u64 hv_features __section(".data") = 0; static bool __init sev_es_check_cpu_features(void) { @@ -49,6 +50,26 @@ static void __noreturn sev_es_terminate(unsigned int reason) asm volatile("hlt\n" : : : "memory"); } +static bool ghcb_get_hv_features(void) +{ + u64 val; + + /* The hypervisor features are available from version 2 onward. */ + if (ghcb_version < 2) + return true; + + sev_es_wr_ghcb_msr(GHCB_MSR_HV_FEATURES_REQ); + VMGEXIT(); + + val = sev_es_rd_ghcb_msr(); + if (GHCB_RESP_CODE(val) != GHCB_MSR_HV_FEATURES_RESP) + return false; + + hv_features = GHCB_MSR_HV_FEATURES_RESP_VAL(val); + + return true; +} + static bool sev_es_negotiate_protocol(void) { u64 val; @@ -67,6 +88,9 @@ static bool sev_es_negotiate_protocol(void) ghcb_version = min_t(size_t, GHCB_MSR_PROTO_MAX(val), GHCB_PROTOCOL_MAX); + if (!ghcb_get_hv_features()) + return false; + return true; }
Version 2 of GHCB specification introduced advertisement of a features that are supported by the hypervisor. Define the GHCB MSR protocol and NAE for the hypervisor feature request and query the feature during the GHCB protocol negotitation. See the GHCB specification for more details. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/asm/sev-common.h | 17 +++++++++++++++++ arch/x86/include/uapi/asm/svm.h | 2 ++ arch/x86/kernel/sev-shared.c | 24 ++++++++++++++++++++++++ 3 files changed, 43 insertions(+)