| Message ID | 20211008180453.462291-15-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, Oct 08, 2021 at 01:04:25PM -0500, Brijesh Singh wrote: > + /* SEV-SNP guest requires that GHCB must be registered. */ > + if (cc_platform_has(CC_ATTR_SEV_SNP)) > + snp_register_ghcb(data, __pa(ghcb)); This looks like more of that "let's register a GHCB at the time the first #VC fires". And there already is setup_ghcb() which is called in the #VC handler. And that thing registers a GHCB GPA. But then you have to do it here again. I think this should be changed together with the CPUID page detection stuff we talked about earlier, where, after you've established that this is an SNP guest, you call setup_ghcb() *once* and after that you have everything set up, including the GHCB GPA. And then the #VC exceptions can come. Right? Or is there a chicken-and-an-egg issue here which I'm not thinking about?
Hi Boris, On 11/2/21 11:53 AM, Borislav Petkov wrote: > On Fri, Oct 08, 2021 at 01:04:25PM -0500, Brijesh Singh wrote: >> + /* SEV-SNP guest requires that GHCB must be registered. */ >> + if (cc_platform_has(CC_ATTR_SEV_SNP)) >> + snp_register_ghcb(data, __pa(ghcb)); > > This looks like more of that "let's register a GHCB at the time the > first #VC fires". > There are two #VC handlers: 1) early exception handler [do_vc_no_ghcb()]. The handler uses the MSR protocol based VMGEXIT. https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/sev-shared.c#L147 2) exception handler setup during the idt bringup [handle_vc_boot_ghcb()]. The handler uses the full GHCB. https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/sev.c#L1472 To answer your question, GHCB is registered at the time of first #VC handling by the second exception handler. Mike can correct me, the CPUID page check is going to happen on first #VC handling inside the early exception handler (i.e case 1). The early exception handler uses the MSR protocol, so, there is no need to register the GHCB page. Before registering the page we need to map it unencrypted. > And there already is setup_ghcb() which is called in the #VC handler. > And that thing registers a GHCB GPA. > There are two cases that need to be covered 1) BSP GHCB page and 2) APs GHCB page. The setup_ghcb() is called for the BSP. Later on, per-cpu GHCB page is used by the APs. APs need to register their GHCB page before using it. > But then you have to do it here again. > > I think this should be changed together with the CPUID page detection > stuff we talked about earlier, where, after you've established that this > is an SNP guest, you call setup_ghcb() *once* and after that you have > everything set up, including the GHCB GPA. And then the #VC exceptions > can come. See if my above explanation make sense. Based on it, I don't think it makes sense to register the GHCB during the CPUID page detection. The CPUID page detection will occur in early VC handling. > Right? > > Or is there a chicken-and-an-egg issue here which I'm not thinking > about? >
On Tue, Nov 02, 2021 at 01:24:01PM -0500, Brijesh Singh wrote: > To answer your question, GHCB is registered at the time of first #VC > handling by the second exception handler. And this is what I don't like - register at use. Instead of init everything *before* use. > Mike can correct me, the CPUID page check is going to happen on first > #VC handling inside the early exception handler (i.e case 1). What is the "CPUID page check"? And no, you don't want to do any detection when an exception happens - you want to detect *everything* *first* and then do exceptions. > See if my above explanation make sense. Based on it, I don't think it > makes sense to register the GHCB during the CPUID page detection. The > CPUID page detection will occur in early VC handling. See above. If this needs more discussion, we can talk on IRC.
Hi Boris, On 11/2/21 1:44 PM, Borislav Petkov wrote: > On Tue, Nov 02, 2021 at 01:24:01PM -0500, Brijesh Singh wrote: >> To answer your question, GHCB is registered at the time of first #VC >> handling by the second exception handler. > > And this is what I don't like - register at use. Instead of init > everything *before* use. > >> Mike can correct me, the CPUID page check is going to happen on first >> #VC handling inside the early exception handler (i.e case 1). > > What is the "CPUID page check"? > > And no, you don't want to do any detection when an exception happens - > you want to detect *everything* *first* and then do exceptions. > >> See if my above explanation make sense. Based on it, I don't think it >> makes sense to register the GHCB during the CPUID page detection. The >> CPUID page detection will occur in early VC handling. > > See above. If this needs more discussion, we can talk on IRC. > Looking at the secondary CPU bring up path it seems that we will not be getting #VC until the early_setup_idt() is called. I am thinking to add function to register the GHCB from the early_setup_idt() early_setup_idt() { ... if (IS_ENABLED(CONFIG_MEM_ENCRYPT)) sev_snp_register_ghcb() ... } The above will cover the APs and for BSP case I can call the same function just after the final IDT is loaded cpu_init_exception_handling() { ... ... /* Finally load the IDT */ load_current_idt(); if (IS_ENABLED(CONFIG_MEM_ENCRYPT)) sev_snp_register_ghcb() } Please let me know if something like above is acceptable. thanks
On Wed, Nov 03, 2021 at 03:10:16PM -0500, Brijesh Singh wrote: > Looking at the secondary CPU bring up path it seems that we will not be > getting #VC until the early_setup_idt() is called. I am thinking to add > function to register the GHCB from the early_setup_idt() > > early_setup_idt() > { > ... > if (IS_ENABLED(CONFIG_MEM_ENCRYPT)) > sev_snp_register_ghcb() > ... > } > > The above will cover the APs That will cover the APs during early boot as that is being called from asm. > and for BSP case I can call the same function just after the final IDT > is loaded Why after and not before? > cpu_init_exception_handling() > { > ... > ... > /* Finally load the IDT */ > load_current_idt(); > > if (IS_ENABLED(CONFIG_MEM_ENCRYPT)) > sev_snp_register_ghcb() > > } That is also called on the APs - not only the BSP. trap_init() calls it from start_kernel() which is the BSP and cpu_init_secondary() calls it too, which is ofc the APs. I guess that should be ok since you're calling the same function from both but WTH do I know...
On 11/4/21 8:58 AM, Borislav Petkov wrote: > On Wed, Nov 03, 2021 at 03:10:16PM -0500, Brijesh Singh wrote: >> Looking at the secondary CPU bring up path it seems that we will not be >> getting #VC until the early_setup_idt() is called. I am thinking to add >> function to register the GHCB from the early_setup_idt() >> >> early_setup_idt() >> { >> ... >> if (IS_ENABLED(CONFIG_MEM_ENCRYPT)) >> sev_snp_register_ghcb() >> ... >> } >> >> The above will cover the APs > > That will cover the APs during early boot as that is being called from > asm. > >> and for BSP case I can call the same function just after the final IDT >> is loaded > > Why after and not before? > I just looked at load_current_idt() and we should not get #VC before loading the new idt, so, its safe to do is before. >> cpu_init_exception_handling() >> { >> ... >> ... >> /* Finally load the IDT */ >> load_current_idt(); >> >> if (IS_ENABLED(CONFIG_MEM_ENCRYPT)) >> sev_snp_register_ghcb() >> >> } > > That is also called on the APs - not only the BSP. trap_init() calls it > from start_kernel() which is the BSP and cpu_init_secondary() calls it > too, which is ofc the APs. > > I guess that should be ok since you're calling the same function from > both but WTH do I know... > For AP case, we will be registering the same GHCB GPA twice, that should not be an issue. The GHCB spec does not restrict us on registering the GPA twice. Of course, the current patch does not suffer with it. Let me know your preference. thanks
On November 4, 2021 3:26:56 PM UTC, Brijesh Singh <brijesh.singh@amd.com> wrote: >Of course, the current patch does not suffer with it. Let me know your >preference. Whatever keeps the code simpler. Thx.
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 2290fbcc1844..4c891d5d9651 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -77,6 +77,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 { @@ -160,55 +167,6 @@ void noinstr __sev_es_ist_exit(void) this_cpu_write(cpu_tss_rw.x86_tss.ist[IST_INDEX_VC], *(unsigned long *)ist); } -/* - * Nothing shall interrupt this code path while holding the per-CPU - * GHCB. The backup GHCB is only for NMIs interrupting this path. - * - * Callers must disable local interrupts around it. - */ -static noinstr struct ghcb *__sev_get_ghcb(struct ghcb_state *state) -{ - struct sev_es_runtime_data *data; - struct ghcb *ghcb; - - WARN_ON(!irqs_disabled()); - - data = this_cpu_read(runtime_data); - ghcb = &data->ghcb_page; - - if (unlikely(data->ghcb_active)) { - /* GHCB is already in use - save its contents */ - - if (unlikely(data->backup_ghcb_active)) { - /* - * Backup-GHCB is also already in use. There is no way - * to continue here so just kill the machine. To make - * panic() work, mark GHCBs inactive so that messages - * can be printed out. - */ - data->ghcb_active = false; - data->backup_ghcb_active = false; - - instrumentation_begin(); - panic("Unable to handle #VC exception! GHCB and Backup GHCB are already in use"); - instrumentation_end(); - } - - /* Mark backup_ghcb active before writing to it */ - data->backup_ghcb_active = true; - - state->ghcb = &data->backup_ghcb; - - /* Backup GHCB content */ - *state->ghcb = *ghcb; - } else { - state->ghcb = NULL; - data->ghcb_active = true; - } - - return ghcb; -} - /* Needed in vc_early_forward_exception */ void do_early_exception(struct pt_regs *regs, int trapnr); @@ -464,6 +422,69 @@ static enum es_result vc_slow_virt_to_phys(struct ghcb *ghcb, struct es_em_ctxt /* Include code shared with pre-decompression boot stage */ #include "sev-shared.c" +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; +} + +/* + * Nothing shall interrupt this code path while holding the per-CPU + * GHCB. The backup GHCB is only for NMIs interrupting this path. + * + * Callers must disable local interrupts around it. + */ +static noinstr struct ghcb *__sev_get_ghcb(struct ghcb_state *state) +{ + struct sev_es_runtime_data *data; + struct ghcb *ghcb; + + WARN_ON(!irqs_disabled()); + + data = this_cpu_read(runtime_data); + ghcb = &data->ghcb_page; + + if (unlikely(data->ghcb_active)) { + /* GHCB is already in use - save its contents */ + + if (unlikely(data->backup_ghcb_active)) { + /* + * Backup-GHCB is also already in use. There is no way + * to continue here so just kill the machine. To make + * panic() work, mark GHCBs inactive so that messages + * can be printed out. + */ + data->ghcb_active = false; + data->backup_ghcb_active = false; + + instrumentation_begin(); + panic("Unable to handle #VC exception! GHCB and Backup GHCB are already in use"); + instrumentation_end(); + } + + /* Mark backup_ghcb active before writing to it */ + data->backup_ghcb_active = true; + + state->ghcb = &data->backup_ghcb; + + /* Backup GHCB content */ + *state->ghcb = *ghcb; + } else { + state->ghcb = NULL; + data->ghcb_active = true; + } + + /* SEV-SNP guest requires that GHCB must be registered. */ + if (cc_platform_has(CC_ATTR_SEV_SNP)) + snp_register_ghcb(data, __pa(ghcb)); + + return ghcb; +} + static noinstr void __sev_put_ghcb(struct ghcb_state *state) { struct sev_es_runtime_data *data; @@ -650,6 +671,10 @@ static bool __init 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 (cc_platform_has(CC_ATTR_SEV_SNP)) + snp_register_ghcb_early(__pa(&boot_ghcb_page)); + return true; } @@ -739,6 +764,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 | 124 +++++++++++++++++++++++++----------------- 1 file changed, 75 insertions(+), 49 deletions(-)