| Message ID | 20210430121616.2295-3-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:58AM -0500, Brijesh Singh wrote: > The SEV-ES guest calls the sev_es_negotiate_protocol() to negotiate the > GHCB protocol version before establishing the GHCB. Cache the negotiated > GHCB version so that it can be used later. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/include/asm/sev.h | 2 +- > arch/x86/kernel/sev-shared.c | 15 ++++++++++++--- > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index fa5cd05d3b5b..7ec91b1359df 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -12,7 +12,7 @@ > #include <asm/insn.h> > #include <asm/sev-common.h> > > -#define GHCB_PROTO_OUR 0x0001UL > +#define GHCB_PROTOCOL_MIN 1ULL > #define GHCB_PROTOCOL_MAX 1ULL > #define GHCB_DEFAULT_USAGE 0ULL > > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > index 6ec8b3bfd76e..48a47540b85f 100644 > --- a/arch/x86/kernel/sev-shared.c > +++ b/arch/x86/kernel/sev-shared.c > @@ -14,6 +14,13 @@ > #define has_cpuflag(f) boot_cpu_has(f) > #endif > > +/* > + * Since feature negotitation related variables are set early in the boot > + * process they must reside in the .data section so as not to be zeroed > + * out when the .bss section is later cleared. * * GHCB protocol version negotiated with the hypervisor. */ > +static u16 ghcb_version __section(".data") = 0; Did you not see this when running checkpatch.pl on your patch? ERROR: do not initialise statics to 0 #141: FILE: arch/x86/kernel/sev-shared.c:22: +static u16 ghcb_version __section(".data") = 0; > static bool __init sev_es_check_cpu_features(void) > { > if (!has_cpuflag(X86_FEATURE_RDRAND)) { > @@ -54,10 +61,12 @@ static bool sev_es_negotiate_protocol(void) > if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP) > return false; > > - if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTO_OUR || > - GHCB_MSR_PROTO_MIN(val) > GHCB_PROTO_OUR) > + if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTOCOL_MIN || > + GHCB_MSR_PROTO_MIN(val) > GHCB_PROTOCOL_MAX) > return false; > > + ghcb_version = min_t(size_t, GHCB_MSR_PROTO_MAX(val), GHCB_PROTOCOL_MAX); How is that even supposed to work? GHCB_PROTOCOL_MAX is 1 so ghcb_version will be always 1 when you do min_t() on values one of which is 1. Maybe I'm missing something...
On 5/11/21 4:23 AM, Borislav Petkov wrote: > On Fri, Apr 30, 2021 at 07:15:58AM -0500, Brijesh Singh wrote: >> The SEV-ES guest calls the sev_es_negotiate_protocol() to negotiate the >> GHCB protocol version before establishing the GHCB. Cache the negotiated >> GHCB version so that it can be used later. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> arch/x86/include/asm/sev.h | 2 +- >> arch/x86/kernel/sev-shared.c | 15 ++++++++++++--- >> 2 files changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h >> index fa5cd05d3b5b..7ec91b1359df 100644 >> --- a/arch/x86/include/asm/sev.h >> +++ b/arch/x86/include/asm/sev.h >> @@ -12,7 +12,7 @@ >> #include <asm/insn.h> >> #include <asm/sev-common.h> >> >> -#define GHCB_PROTO_OUR 0x0001UL >> +#define GHCB_PROTOCOL_MIN 1ULL >> #define GHCB_PROTOCOL_MAX 1ULL >> #define GHCB_DEFAULT_USAGE 0ULL >> >> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c >> index 6ec8b3bfd76e..48a47540b85f 100644 >> --- a/arch/x86/kernel/sev-shared.c >> +++ b/arch/x86/kernel/sev-shared.c >> @@ -14,6 +14,13 @@ >> #define has_cpuflag(f) boot_cpu_has(f) >> #endif >> >> +/* >> + * Since feature negotitation related variables are set early in the boot >> + * process they must reside in the .data section so as not to be zeroed >> + * out when the .bss section is later cleared. > * > * GHCB protocol version negotiated with the hypervisor. > */ > >> +static u16 ghcb_version __section(".data") = 0; > Did you not see this when running checkpatch.pl on your patch? > > ERROR: do not initialise statics to 0 > #141: FILE: arch/x86/kernel/sev-shared.c:22: > +static u16 ghcb_version __section(".data") = 0; > I ignored it, because I thought I still need to initialize the value to zero because it does not go into .bss section which gets initialized to zero by kernel on boot. I guess I was wrong, maybe compiler or kernel build ensures that ghcb_version variable to be init'ed to zero because its marked static ? >> static bool __init sev_es_check_cpu_features(void) >> { >> if (!has_cpuflag(X86_FEATURE_RDRAND)) { >> @@ -54,10 +61,12 @@ static bool sev_es_negotiate_protocol(void) >> if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP) >> return false; >> >> - if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTO_OUR || >> - GHCB_MSR_PROTO_MIN(val) > GHCB_PROTO_OUR) >> + if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTOCOL_MIN || >> + GHCB_MSR_PROTO_MIN(val) > GHCB_PROTOCOL_MAX) >> return false; >> >> + ghcb_version = min_t(size_t, GHCB_MSR_PROTO_MAX(val), GHCB_PROTOCOL_MAX); > How is that even supposed to work? GHCB_PROTOCOL_MAX is 1 so > ghcb_version will be always 1 when you do min_t() on values one of which > is 1. > > Maybe I'm missing something... In patch #4, you will see that I increase the GHCB max protocol version to 2, and then min_t() will choose the lower value. I didn't combine version bump and caching into same patch because I felt I will be asked to break them into two logical patches. >
On Tue, May 11, 2021 at 01:29:00PM -0500, Brijesh Singh wrote: > I ignored it, because I thought I still need to initialize the value to > zero because it does not go into .bss section which gets initialized to > zero by kernel on boot. > > I guess I was wrong, maybe compiler or kernel build ensures that > ghcb_version variable to be init'ed to zero because its marked static ? Yes. If in doubt, always look at the generated asm: make arch/x86/kernel/sev.s You can see the .zero 2 gas directive there, where the variable is defined. > In patch #4, you will see that I increase the GHCB max protocol version > to 2, and then min_t() will choose the lower value. I didn't combine > version bump and caching into same patch because I felt I will be asked > to break them into two logical patches. Hmm, what would be the reasoning to keep the version bump in a separate patch?
On 5/11/21 1:41 PM, Borislav Petkov wrote: > On Tue, May 11, 2021 at 01:29:00PM -0500, Brijesh Singh wrote: >> I ignored it, because I thought I still need to initialize the value to >> zero because it does not go into .bss section which gets initialized to >> zero by kernel on boot. >> >> I guess I was wrong, maybe compiler or kernel build ensures that >> ghcb_version variable to be init'ed to zero because its marked static ? > Yes. > > If in doubt, always look at the generated asm: > > make arch/x86/kernel/sev.s > > You can see the .zero 2 gas directive there, where the variable is > defined. > >> In patch #4, you will see that I increase the GHCB max protocol version >> to 2, and then min_t() will choose the lower value. I didn't combine >> version bump and caching into same patch because I felt I will be asked >> to break them into two logical patches. > Hmm, what would be the reasoning to keep the version bump in a separate > patch? Version 2 of the spec adds bunch of NAEs, and several of them are optional except the hyervisor features NAE. IMO, a guest should bump the GHCB version only after it has implemented all the required NAEs from the version 2. It may help during the git bisect of the guest kernel -- mainly when the hypervisor supports the higher version.
On Wed, May 12, 2021 at 09:03:41AM -0500, Brijesh Singh wrote: > Version 2 of the spec adds bunch of NAEs, and several of them are > optional except the hyervisor features NAE. IMO, a guest should bump the > GHCB version only after it has implemented all the required NAEs from > the version 2. It may help during the git bisect of the guest kernel -- > mainly when the hypervisor supports the higher version. Aha, so AFAICT, the version bump should happen in patch 3 which adds that HV features NAE - not in a separate one after that. The logic being, with the patch which adds the functionality needed, you mark the guest as supporting v2.
On 5/12/21 9:31 AM, Borislav Petkov wrote: > On Wed, May 12, 2021 at 09:03:41AM -0500, Brijesh Singh wrote: >> Version 2 of the spec adds bunch of NAEs, and several of them are >> optional except the hyervisor features NAE. IMO, a guest should bump the >> GHCB version only after it has implemented all the required NAEs from >> the version 2. It may help during the git bisect of the guest kernel -- >> mainly when the hypervisor supports the higher version. > Aha, so AFAICT, the version bump should happen in patch 3 which adds > that HV features NAE - not in a separate one after that. The logic > being, with the patch which adds the functionality needed, you mark the > guest as supporting v2. Sure, I will squash the patch 3 and 4 and will do the same for the hypervisor series. thanks -Brijesh
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index fa5cd05d3b5b..7ec91b1359df 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -12,7 +12,7 @@ #include <asm/insn.h> #include <asm/sev-common.h> -#define GHCB_PROTO_OUR 0x0001UL +#define GHCB_PROTOCOL_MIN 1ULL #define GHCB_PROTOCOL_MAX 1ULL #define GHCB_DEFAULT_USAGE 0ULL diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c index 6ec8b3bfd76e..48a47540b85f 100644 --- a/arch/x86/kernel/sev-shared.c +++ b/arch/x86/kernel/sev-shared.c @@ -14,6 +14,13 @@ #define has_cpuflag(f) boot_cpu_has(f) #endif +/* + * Since feature negotitation related variables are set early in the boot + * process they must reside in the .data section so as not to be zeroed + * out when the .bss section is later cleared. + */ +static u16 ghcb_version __section(".data") = 0; + static bool __init sev_es_check_cpu_features(void) { if (!has_cpuflag(X86_FEATURE_RDRAND)) { @@ -54,10 +61,12 @@ static bool sev_es_negotiate_protocol(void) if (GHCB_MSR_INFO(val) != GHCB_MSR_SEV_INFO_RESP) return false; - if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTO_OUR || - GHCB_MSR_PROTO_MIN(val) > GHCB_PROTO_OUR) + if (GHCB_MSR_PROTO_MAX(val) < GHCB_PROTOCOL_MIN || + GHCB_MSR_PROTO_MIN(val) > GHCB_PROTOCOL_MAX) return false; + ghcb_version = min_t(size_t, GHCB_MSR_PROTO_MAX(val), GHCB_PROTOCOL_MAX); + return true; } @@ -101,7 +110,7 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, enum es_result ret; /* Fill in protocol and format specifiers */ - ghcb->protocol_version = GHCB_PROTOCOL_MAX; + ghcb->protocol_version = ghcb_version; ghcb->ghcb_usage = GHCB_DEFAULT_USAGE; ghcb_set_sw_exit_code(ghcb, exit_code);
The SEV-ES guest calls the sev_es_negotiate_protocol() to negotiate the GHCB protocol version before establishing the GHCB. Cache the negotiated GHCB version so that it can be used later. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/asm/sev.h | 2 +- arch/x86/kernel/sev-shared.c | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-)