| Message ID | 20210430121616.2295-14-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:16:09AM -0500, Brijesh Singh wrote: > The SEV-SNP guest is required to perform GHCB GPA registration. This is > because the hypervisor may prefer that a guest use a consistent and/or > specific GPA for the GHCB associated with a vCPU. For more information, > see the GHCB specification section GHCB GPA Registration. > > During the boot, init_ghcb() allocates a per-cpu GHCB page. On very first > VC exception, the exception handler switch to using the per-cpu GHCB page > allocated during the init_ghcb(). The GHCB page must be registered in > the current vcpu context. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/kernel/sev.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index 8c8c939a1754..e6819f170ec4 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -88,6 +88,13 @@ struct sev_es_runtime_data { > * is currently unsupported in SEV-ES guests. > */ > unsigned long dr7; > + > + /* > + * SEV-SNP requires that the GHCB must be registered before using it. > + * The flag below will indicate whether the GHCB is registered, if its > + * not registered then sev_es_get_ghcb() will perform the registration. > + */ > + bool snp_ghcb_registered; > }; > > struct ghcb_state { > @@ -100,6 +107,9 @@ DEFINE_STATIC_KEY_FALSE(sev_es_enable_key); > /* Needed in vc_early_forward_exception */ > void do_early_exception(struct pt_regs *regs, int trapnr); > > +/* Defined in sev-shared.c */ > +static void snp_register_ghcb(unsigned long paddr); Can we get rid of those forward declarations pls? Due to sev-shared.c this file is starting to spawn those and that's ugly. Either through a code reorg or even defining a sev-internal.h header which contains all those so that they don't pollute the code? Thx. > + > static void __init setup_vc_stacks(int cpu) > { > struct sev_es_runtime_data *data; > @@ -218,6 +228,12 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state) > data->ghcb_active = true; > } > > + /* SEV-SNP guest requires that GHCB must be registered before using it. */ > + if (sev_snp_active() && !data->snp_ghcb_registered) { > + snp_register_ghcb(__pa(ghcb)); > + data->snp_ghcb_registered = true; > + } More missed review from last time: "This needs to be set to true in the function itself, in the success case." Can you please be more careful and go through all review comments so that I don't have to do the same work twice? Thx.
On 5/25/21 6:11 AM, Borislav Petkov wrote: > On Fri, Apr 30, 2021 at 07:16:09AM -0500, Brijesh Singh wrote: >> The SEV-SNP guest is required to perform GHCB GPA registration. This is >> because the hypervisor may prefer that a guest use a consistent and/or >> specific GPA for the GHCB associated with a vCPU. For more information, >> see the GHCB specification section GHCB GPA Registration. >> >> During the boot, init_ghcb() allocates a per-cpu GHCB page. On very first >> VC exception, the exception handler switch to using the per-cpu GHCB page >> allocated during the init_ghcb(). The GHCB page must be registered in >> the current vcpu context. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> arch/x86/kernel/sev.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c >> index 8c8c939a1754..e6819f170ec4 100644 >> --- a/arch/x86/kernel/sev.c >> +++ b/arch/x86/kernel/sev.c >> @@ -88,6 +88,13 @@ struct sev_es_runtime_data { >> * is currently unsupported in SEV-ES guests. >> */ >> unsigned long dr7; >> + >> + /* >> + * SEV-SNP requires that the GHCB must be registered before using it. >> + * The flag below will indicate whether the GHCB is registered, if its >> + * not registered then sev_es_get_ghcb() will perform the registration. >> + */ >> + bool snp_ghcb_registered; >> }; >> >> struct ghcb_state { >> @@ -100,6 +107,9 @@ DEFINE_STATIC_KEY_FALSE(sev_es_enable_key); >> /* Needed in vc_early_forward_exception */ >> void do_early_exception(struct pt_regs *regs, int trapnr); >> >> +/* Defined in sev-shared.c */ >> +static void snp_register_ghcb(unsigned long paddr); > Can we get rid of those forward declarations pls? Due to sev-shared.c > this file is starting to spawn those and that's ugly. > > Either through a code reorg or even defining a sev-internal.h header > which contains all those so that they don't pollute the code? Okay, I will see what I can do to avoid the forward declarations. > Thx. > >> + >> static void __init setup_vc_stacks(int cpu) >> { >> struct sev_es_runtime_data *data; >> @@ -218,6 +228,12 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state) >> data->ghcb_active = true; >> } >> >> + /* SEV-SNP guest requires that GHCB must be registered before using it. */ >> + if (sev_snp_active() && !data->snp_ghcb_registered) { >> + snp_register_ghcb(__pa(ghcb)); >> + data->snp_ghcb_registered = true; >> + } > More missed review from last time: > > "This needs to be set to true in the function itself, in the success > case." > > Can you please be more careful and go through all review comments so > that I don't have to do the same work twice? I am not ignoring your valuable feedback; I am sorry if I came across like that. In this particular case, the snp_register_ghcb() is shared between the decompress and main kernel. The variable data->snp_ghcb_registered is not visible in the decompressed path, so I choose not to set it to true inside the function itself. > Thx. >
On Tue, May 25, 2021 at 09:28:14AM -0500, Brijesh Singh wrote: > I am not ignoring your valuable feedback; I am sorry if I came across > like that. Sure, no worries, but you have to say something when you don't agree with the feedback. How am I supposed to know? > In this particular case, the snp_register_ghcb() is shared between the > decompress and main kernel. The variable data->snp_ghcb_registered is > not visible in the decompressed path, Why is it not visible?
On 5/25/21 9:35 AM, Borislav Petkov wrote: >> In this particular case, the snp_register_ghcb() is shared between the >> decompress and main kernel. The variable data->snp_ghcb_registered is >> not visible in the decompressed path, > Why is it not visible? Maybe I should have said, its not applicable in the decompressed path. The snp_ghcb_register is defined in the per-CPU structure, and used in the per-CPU #VC handler. We don't have the per-CPU #VC handler in the decompression code path. Please see the arch/x86/kernel/sev.c /* #VC handler runtime per-CPU data */ struct sev_es_runtime_data { ... ... bool snp_ghcb_registered; }
On Tue, May 25, 2021 at 09:47:24AM -0500, Brijesh Singh wrote:
> Maybe I should have said, its not applicable in the decompressed path.
Aha, ok. How's that, ontop of yours:
---
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 07b9529d7d95..c9dd98b9dcdf 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -208,7 +208,7 @@ static bool early_setup_sev_es(void)
/* SEV-SNP guest requires the GHCB GPA must be registered */
if (sev_snp_enabled())
- snp_register_ghcb(__pa(&boot_ghcb_page));
+ snp_register_ghcb_early(__pa(&boot_ghcb_page));
return true;
}
diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c
index 37a23c524f8c..7200f44d6b6b 100644
--- a/arch/x86/kernel/sev-shared.c
+++ b/arch/x86/kernel/sev-shared.c
@@ -81,7 +81,7 @@ static bool ghcb_get_hv_features(void)
return true;
}
-static void snp_register_ghcb(unsigned long paddr)
+static void snp_register_ghcb_early(unsigned long paddr)
{
unsigned long pfn = paddr >> PAGE_SHIFT;
u64 val;
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 5544557d9fb6..144c20479cae 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -108,7 +108,18 @@ DEFINE_STATIC_KEY_FALSE(sev_es_enable_key);
void do_early_exception(struct pt_regs *regs, int trapnr);
/* Defined in sev-shared.c */
-static void snp_register_ghcb(unsigned long paddr);
+static void snp_register_ghcb_early(unsigned long paddr);
+
+static void snp_register_ghcb(struct sev_es_runtime_data *data,
+ unsigned long paddr)
+{
+ if (data->snp_ghcb_registered)
+ return;
+
+ snp_register_ghcb_early(paddr);
+
+ data->snp_ghcb_registered = true;
+}
static void __init setup_vc_stacks(int cpu)
{
@@ -239,10 +250,8 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)
}
/* SEV-SNP guest requires that GHCB must be registered before using it. */
- if (sev_snp_active() && !data->snp_ghcb_registered) {
- snp_register_ghcb(__pa(ghcb));
- data->snp_ghcb_registered = true;
- }
+ if (sev_snp_active())
+ snp_register_ghcb(data, __pa(ghcb));
return ghcb;
}
@@ -681,7 +690,7 @@ static bool __init sev_es_setup_ghcb(void)
/* SEV-SNP guest requires that GHCB GPA must be registered */
if (sev_snp_active())
- snp_register_ghcb(__pa(&boot_ghcb_page));
+ snp_register_ghcb_early(__pa(&boot_ghcb_page));
return true;
}
On 5/26/21 4:57 AM, Borislav Petkov wrote: > On Tue, May 25, 2021 at 09:47:24AM -0500, Brijesh Singh wrote: >> Maybe I should have said, its not applicable in the decompressed path. > Aha, ok. How's that, ontop of yours: Sure, its fine with me. I will apply this change. -Birjesh > --- > diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c > index 07b9529d7d95..c9dd98b9dcdf 100644 > --- a/arch/x86/boot/compressed/sev.c > +++ b/arch/x86/boot/compressed/sev.c > @@ -208,7 +208,7 @@ static bool early_setup_sev_es(void) > > /* SEV-SNP guest requires the GHCB GPA must be registered */ > if (sev_snp_enabled()) > - snp_register_ghcb(__pa(&boot_ghcb_page)); > + snp_register_ghcb_early(__pa(&boot_ghcb_page)); > > return true; > } > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > index 37a23c524f8c..7200f44d6b6b 100644 > --- a/arch/x86/kernel/sev-shared.c > +++ b/arch/x86/kernel/sev-shared.c > @@ -81,7 +81,7 @@ static bool ghcb_get_hv_features(void) > return true; > } > > -static void snp_register_ghcb(unsigned long paddr) > +static void snp_register_ghcb_early(unsigned long paddr) > { > unsigned long pfn = paddr >> PAGE_SHIFT; > u64 val; > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index 5544557d9fb6..144c20479cae 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -108,7 +108,18 @@ DEFINE_STATIC_KEY_FALSE(sev_es_enable_key); > void do_early_exception(struct pt_regs *regs, int trapnr); > > /* Defined in sev-shared.c */ > -static void snp_register_ghcb(unsigned long paddr); > +static void snp_register_ghcb_early(unsigned long paddr); > + > +static void snp_register_ghcb(struct sev_es_runtime_data *data, > + unsigned long paddr) > +{ > + if (data->snp_ghcb_registered) > + return; > + > + snp_register_ghcb_early(paddr); > + > + data->snp_ghcb_registered = true; > +} > > static void __init setup_vc_stacks(int cpu) > { > @@ -239,10 +250,8 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state) > } > > /* SEV-SNP guest requires that GHCB must be registered before using it. */ > - if (sev_snp_active() && !data->snp_ghcb_registered) { > - snp_register_ghcb(__pa(ghcb)); > - data->snp_ghcb_registered = true; > - } > + if (sev_snp_active()) > + snp_register_ghcb(data, __pa(ghcb)); > > return ghcb; > } > @@ -681,7 +690,7 @@ static bool __init sev_es_setup_ghcb(void) > > /* SEV-SNP guest requires that GHCB GPA must be registered */ > if (sev_snp_active()) > - snp_register_ghcb(__pa(&boot_ghcb_page)); > + snp_register_ghcb_early(__pa(&boot_ghcb_page)); > > return true; > } >
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 8c8c939a1754..e6819f170ec4 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -88,6 +88,13 @@ struct sev_es_runtime_data { * is currently unsupported in SEV-ES guests. */ unsigned long dr7; + + /* + * SEV-SNP requires that the GHCB must be registered before using it. + * The flag below will indicate whether the GHCB is registered, if its + * not registered then sev_es_get_ghcb() will perform the registration. + */ + bool snp_ghcb_registered; }; struct ghcb_state { @@ -100,6 +107,9 @@ DEFINE_STATIC_KEY_FALSE(sev_es_enable_key); /* Needed in vc_early_forward_exception */ void do_early_exception(struct pt_regs *regs, int trapnr); +/* Defined in sev-shared.c */ +static void snp_register_ghcb(unsigned long paddr); + static void __init setup_vc_stacks(int cpu) { struct sev_es_runtime_data *data; @@ -218,6 +228,12 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state) data->ghcb_active = true; } + /* SEV-SNP guest requires that GHCB must be registered before using it. */ + if (sev_snp_active() && !data->snp_ghcb_registered) { + snp_register_ghcb(__pa(ghcb)); + data->snp_ghcb_registered = true; + } + return ghcb; } @@ -622,6 +638,10 @@ static bool __init sev_es_setup_ghcb(void) /* Alright - Make the boot-ghcb public */ boot_ghcb = &boot_ghcb_page; + /* SEV-SNP guest requires that GHCB GPA must be registered */ + if (sev_snp_active()) + snp_register_ghcb(__pa(&boot_ghcb_page)); + return true; } @@ -711,6 +731,7 @@ static void __init init_ghcb(int cpu) data->ghcb_active = false; data->backup_ghcb_active = false; + data->snp_ghcb_registered = false; } void __init sev_es_init_vc_handling(void)
The SEV-SNP guest is required to perform GHCB GPA registration. This is because the hypervisor may prefer that a guest use a consistent and/or specific GPA for the GHCB associated with a vCPU. For more information, see the GHCB specification section GHCB GPA Registration. During the boot, init_ghcb() allocates a per-cpu GHCB page. On very first VC exception, the exception handler switch to using the per-cpu GHCB page allocated during the init_ghcb(). The GHCB page must be registered in the current vcpu context. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/kernel/sev.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)