| Message ID | 20250404082921.2767593-8-ardb+git@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] x86/boot: Move accept_memory() into decompressor | expand |
On Fri, 4 Apr 2025 at 11:43, Kirill A. Shutemov <kirill.shutemov@linux.intel.com> wrote: > > On Fri, Apr 04, 2025 at 10:29:25AM +0200, Ard Biesheuvel wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > > > Switch to a different API for accepting memory in SEV-SNP guests, one > > which is actually supported at the point during boot where the EFI stub > > may need to accept memory, but the SEV-SNP init code has not executed > > yet. > > I probably miss the point, but why cannot decompressor use the same _early > version of accept too and avoid code duplication? > > Maybe spell it out in the commit message for someone like me :P > I assumed there was a reason that the shared GHCB page is used early on. Maybe it is faster than accepting memory page by page? It also depends on how important the memory acceptance is for the legacy decompressor - AIUI the use case is primarily kexec, but wouldn't the first kernel have accepted all memory already? I.e., if it is slower, we might not care if it is a rare case.
On Fri, Apr 4, 2025 at 1:46 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Fri, 4 Apr 2025 at 11:43, Kirill A. Shutemov > <kirill.shutemov@linux.intel.com> wrote: > > > > On Fri, Apr 04, 2025 at 10:29:25AM +0200, Ard Biesheuvel wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > Switch to a different API for accepting memory in SEV-SNP guests, one > > > which is actually supported at the point during boot where the EFI stub > > > may need to accept memory, but the SEV-SNP init code has not executed > > > yet. > > > > I probably miss the point, but why cannot decompressor use the same _early > > version of accept too and avoid code duplication? > > > > Maybe spell it out in the commit message for someone like me :P > > > > I assumed there was a reason that the shared GHCB page is used early > on. Maybe it is faster than accepting memory page by page? This is correct. The MSR protocol does a round trip per page, whereas the GHCB page can communicate hundreds of state changes per round trip. > > It also depends on how important the memory acceptance is for the > legacy decompressor - AIUI the use case is primarily kexec, but > wouldn't the first kernel have accepted all memory already? I.e., if The first kernel may not accept all memory due to the laziness of unaccepted memory transitions. I'm not sure if we have the planned background acceptance process in place (probably not), but we can't expect that to have finished before the first kexec. > it is slower, we might not care if it is a rare case. If the GHCB is available, we should always prefer it.
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c index bb55934c1cee..88100bf83ded 100644 --- a/arch/x86/boot/compressed/sev.c +++ b/arch/x86/boot/compressed/sev.c @@ -164,10 +164,7 @@ bool sev_snp_enabled(void) static void __page_state_change(unsigned long paddr, enum psc_op op) { - u64 val; - - if (!sev_snp_enabled()) - return; + u64 val, msr; /* * If private -> shared then invalidate the page before requesting the @@ -176,6 +173,9 @@ static void __page_state_change(unsigned long paddr, enum psc_op op) if (op == SNP_PAGE_STATE_SHARED) pvalidate_4k_page(paddr, paddr, false); + /* Save the current GHCB MSR value */ + msr = sev_es_rd_ghcb_msr(); + /* Issue VMGEXIT to change the page state in RMP table. */ sev_es_wr_ghcb_msr(GHCB_MSR_PSC_REQ_GFN(paddr >> PAGE_SHIFT, op)); VMGEXIT(); @@ -185,6 +185,9 @@ static void __page_state_change(unsigned long paddr, enum psc_op op) if ((GHCB_RESP_CODE(val) != GHCB_MSR_PSC_RESP) || GHCB_MSR_PSC_RESP_VAL(val)) sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC); + /* Restore the GHCB MSR value */ + sev_es_wr_ghcb_msr(msr); + /* * Now that page state is changed in the RMP table, validate it so that it is * consistent with the RMP entry. @@ -195,11 +198,17 @@ static void __page_state_change(unsigned long paddr, enum psc_op op) void snp_set_page_private(unsigned long paddr) { + if (!sev_snp_enabled()) + return; + __page_state_change(paddr, SNP_PAGE_STATE_PRIVATE); } void snp_set_page_shared(unsigned long paddr) { + if (!sev_snp_enabled()) + return; + __page_state_change(paddr, SNP_PAGE_STATE_SHARED); } @@ -261,6 +270,11 @@ static phys_addr_t __snp_accept_memory(struct snp_psc_desc *desc, return pa; } +/* + * The memory acceptance support uses the boot GHCB page to perform + * the required page state change operation before validating the + * pages. + */ void snp_accept_memory(phys_addr_t start, phys_addr_t end) { struct snp_psc_desc desc = {}; @@ -275,6 +289,18 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end) pa = __snp_accept_memory(&desc, pa, end); } +/* + * The early version of memory acceptance is needed when being called + * from the EFI stub driver. The pagetable manipulation to mark the + * boot GHCB page as shared can't be performed at this stage, so use + * the GHCB page state change MSR protocol instead. + */ +void snp_accept_memory_early(phys_addr_t start, phys_addr_t end) +{ + for (phys_addr_t pa = start; pa < end; pa += PAGE_SIZE) + __page_state_change(pa, SNP_PAGE_STATE_PRIVATE); +} + void sev_es_shutdown_ghcb(void) { if (!boot_ghcb) diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index 7d9cf473f4d0..dcf436dea99e 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -383,6 +383,8 @@ static bool efistub_is_sevsnp_guest(void) return sev_get_status() & MSR_AMD64_SEV_SNP_ENABLED; } +void snp_accept_memory_early(phys_addr_t start, phys_addr_t end); + void efistub_accept_memory(phys_addr_t start, phys_addr_t end) { static bool once, is_tdx, is_sevsnp; @@ -398,7 +400,7 @@ void efistub_accept_memory(phys_addr_t start, phys_addr_t end) if (is_tdx) tdx_accept_memory(start, end); else if (is_sevsnp) - snp_accept_memory(start, end); + snp_accept_memory_early(start, end); } #endif