| Message ID | 20210324164424.28124-10-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Wed, Mar 24, 2021 at 11:44:20AM -0500, Brijesh Singh wrote: > @@ -63,6 +63,10 @@ struct __packed snp_page_state_change { > #define GHCB_REGISTER_GPA_RESP 0x013UL > #define GHCB_REGISTER_GPA_RESP_VAL(val) ((val) >> 12) > > +/* Macro to convert the x86 page level to the RMP level and vice versa */ > +#define X86_RMP_PG_LEVEL(level) (((level) == PG_LEVEL_4K) ? RMP_PG_SIZE_4K : RMP_PG_SIZE_2M) > +#define RMP_X86_PG_LEVEL(level) (((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M) Please add those with the patch which uses them for the first time. Also, it seems to me the names should be X86_TO_RMP_PG_LEVEL RMP_TO_X86_PG_LEVEL ... > @@ -56,3 +56,108 @@ void sev_snp_register_ghcb(unsigned long paddr) > /* Restore the GHCB MSR value */ > sev_es_wr_ghcb_msr(old); > } > + > +static void sev_snp_issue_pvalidate(unsigned long vaddr, unsigned int npages, bool validate) pvalidate_pages() I guess. > +{ > + unsigned long eflags, vaddr_end, vaddr_next; > + int rc; > + > + vaddr = vaddr & PAGE_MASK; > + vaddr_end = vaddr + (npages << PAGE_SHIFT); > + > + for (; vaddr < vaddr_end; vaddr = vaddr_next) { Yuck, that vaddr_next gets initialized at the end of the loop. How about using a while loop here instead? while (vaddr < vaddr_end) { ... vaddr += PAGE_SIZE; } then you don't need vaddr_next at all. Ditto for all the other loops in this patch which iterate over pages. > + rc = __pvalidate(vaddr, RMP_PG_SIZE_4K, validate, &eflags); So this function gets only 4K pages to pvalidate? > + ^ Superfluous newline. > + if (rc) { > + pr_err("Failed to validate address 0x%lx ret %d\n", vaddr, rc); You can combine the pr_err and dump_stack() below into a WARN() here: WARN(rc, ...); > + goto e_fail; > + } > + > + /* Check for the double validation condition */ > + if (eflags & X86_EFLAGS_CF) { > + pr_err("Double %salidation detected (address 0x%lx)\n", > + validate ? "v" : "inv", vaddr); > + goto e_fail; > + } As before - this should be communicated by a special retval from __pvalidate(). > + > + vaddr_next = vaddr + PAGE_SIZE; > + } > + > + return; > + > +e_fail: > + /* Dump stack for the debugging purpose */ > + dump_stack(); > + > + /* Ask to terminate the guest */ > + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); Another termination reason to #define. > +} > + > +static void __init early_snp_set_page_state(unsigned long paddr, unsigned int npages, int op) > +{ > + unsigned long paddr_end, paddr_next; > + u64 old, val; > + > + paddr = paddr & PAGE_MASK; > + paddr_end = paddr + (npages << PAGE_SHIFT); > + > + /* save the old GHCB MSR */ > + old = sev_es_rd_ghcb_msr(); > + > + for (; paddr < paddr_end; paddr = paddr_next) { > + > + /* > + * Use the MSR protocol VMGEXIT to request the page state change. We use the MSR > + * protocol VMGEXIT because in early boot we may not have the full GHCB setup > + * yet. > + */ > + sev_es_wr_ghcb_msr(GHCB_SNP_PAGE_STATE_REQ_GFN(paddr >> PAGE_SHIFT, op)); > + VMGEXIT(); Yeah, I know we don't always strictly adhere to 80 columns but there's no real need not to fit that in 80 cols here so please shorten names and comments. Ditto for the rest. > + > + val = sev_es_rd_ghcb_msr(); > + > + /* Read the response, if the page state change failed then terminate the guest. */ > + if (GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_SNP_PAGE_STATE_CHANGE_RESP) > + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); if (...) goto fail; and add that fail label at the end so that all the error handling path is out of the way. > + > + if (GHCB_SNP_PAGE_STATE_RESP_VAL(val) != 0) { s/!= 0// > + pr_err("Failed to change page state to '%s' paddr 0x%lx error 0x%llx\n", > + op == SNP_PAGE_STATE_PRIVATE ? "private" : "shared", > + paddr, GHCB_SNP_PAGE_STATE_RESP_VAL(val)); > + > + /* Dump stack for the debugging purpose */ > + dump_stack(); WARN as above. > @@ -49,6 +50,27 @@ bool sev_enabled __section(".data"); > /* Buffer used for early in-place encryption by BSP, no locking needed */ > static char sme_early_buffer[PAGE_SIZE] __initdata __aligned(PAGE_SIZE); > > +/* > + * When SNP is active, this routine changes the page state from private to shared before > + * copying the data from the source to destination and restore after the copy. This is required > + * because the source address is mapped as decrypted by the caller of the routine. > + */ > +static inline void __init snp_aware_memcpy(void *dst, void *src, size_t sz, > + unsigned long paddr, bool dec) snp_memcpy() simply. > +{ > + unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT; > + > + /* If the paddr need to accessed decrypted, make the page shared before memcpy. */ *needs* > + if (sev_snp_active() && dec) Flip that test so that you don't have it twice in the code: if (!sev_snp_active()) { memcpy(dst, src, sz); } else { ... }
On 4/8/21 6:40 AM, Borislav Petkov wrote: > On Wed, Mar 24, 2021 at 11:44:20AM -0500, Brijesh Singh wrote: >> @@ -63,6 +63,10 @@ struct __packed snp_page_state_change { >> #define GHCB_REGISTER_GPA_RESP 0x013UL >> #define GHCB_REGISTER_GPA_RESP_VAL(val) ((val) >> 12) >> >> +/* Macro to convert the x86 page level to the RMP level and vice versa */ >> +#define X86_RMP_PG_LEVEL(level) (((level) == PG_LEVEL_4K) ? RMP_PG_SIZE_4K : RMP_PG_SIZE_2M) >> +#define RMP_X86_PG_LEVEL(level) (((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M) > Please add those with the patch which uses them for the first time. > > Also, it seems to me the names should be > > X86_TO_RMP_PG_LEVEL > RMP_TO_X86_PG_LEVEL > > ... Noted. >> @@ -56,3 +56,108 @@ void sev_snp_register_ghcb(unsigned long paddr) >> /* Restore the GHCB MSR value */ >> sev_es_wr_ghcb_msr(old); >> } >> + >> +static void sev_snp_issue_pvalidate(unsigned long vaddr, unsigned int npages, bool validate) > pvalidate_pages() I guess. Noted. > >> +{ >> + unsigned long eflags, vaddr_end, vaddr_next; >> + int rc; >> + >> + vaddr = vaddr & PAGE_MASK; >> + vaddr_end = vaddr + (npages << PAGE_SHIFT); >> + >> + for (; vaddr < vaddr_end; vaddr = vaddr_next) { > Yuck, that vaddr_next gets initialized at the end of the loop. How about > using a while loop here instead? > > while (vaddr < vaddr_end) { > > ... > > vaddr += PAGE_SIZE; > } > > then you don't need vaddr_next at all. Ditto for all the other loops in > this patch which iterate over pages. Yes, I will switch to use a while loop() in next rev. > >> + rc = __pvalidate(vaddr, RMP_PG_SIZE_4K, validate, &eflags); > So this function gets only 4K pages to pvalidate? The early routines uses the GHCB MSR protocol for the validation. The GHCB MSR protocol supports 4K only. The early routine can be called before the GHCB is established. > >> + > ^ Superfluous newline. Noted. >> + if (rc) { >> + pr_err("Failed to validate address 0x%lx ret %d\n", vaddr, rc); > You can combine the pr_err and dump_stack() below into a WARN() here: > > WARN(rc, ...); Noted. >> + goto e_fail; >> + } >> + >> + /* Check for the double validation condition */ >> + if (eflags & X86_EFLAGS_CF) { >> + pr_err("Double %salidation detected (address 0x%lx)\n", >> + validate ? "v" : "inv", vaddr); >> + goto e_fail; >> + } > As before - this should be communicated by a special retval from > __pvalidate(). Yes. > >> + >> + vaddr_next = vaddr + PAGE_SIZE; >> + } >> + >> + return; >> + >> +e_fail: >> + /* Dump stack for the debugging purpose */ >> + dump_stack(); >> + >> + /* Ask to terminate the guest */ >> + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); > Another termination reason to #define. > >> +} >> + >> +static void __init early_snp_set_page_state(unsigned long paddr, unsigned int npages, int op) >> +{ >> + unsigned long paddr_end, paddr_next; >> + u64 old, val; >> + >> + paddr = paddr & PAGE_MASK; >> + paddr_end = paddr + (npages << PAGE_SHIFT); >> + >> + /* save the old GHCB MSR */ >> + old = sev_es_rd_ghcb_msr(); >> + >> + for (; paddr < paddr_end; paddr = paddr_next) { >> + >> + /* >> + * Use the MSR protocol VMGEXIT to request the page state change. We use the MSR >> + * protocol VMGEXIT because in early boot we may not have the full GHCB setup >> + * yet. >> + */ >> + sev_es_wr_ghcb_msr(GHCB_SNP_PAGE_STATE_REQ_GFN(paddr >> PAGE_SHIFT, op)); >> + VMGEXIT(); > Yeah, I know we don't always strictly adhere to 80 columns but there's > no real need not to fit that in 80 cols here so please shorten names and > comments. Ditto for the rest. Noted. > >> + >> + val = sev_es_rd_ghcb_msr(); >> + >> + /* Read the response, if the page state change failed then terminate the guest. */ >> + if (GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_SNP_PAGE_STATE_CHANGE_RESP) >> + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); > if (...) > goto fail; > > and add that fail label at the end so that all the error handling path > is out of the way. Noted. > >> + >> + if (GHCB_SNP_PAGE_STATE_RESP_VAL(val) != 0) { > s/!= 0// Noted. > >> + pr_err("Failed to change page state to '%s' paddr 0x%lx error 0x%llx\n", >> + op == SNP_PAGE_STATE_PRIVATE ? "private" : "shared", >> + paddr, GHCB_SNP_PAGE_STATE_RESP_VAL(val)); >> + >> + /* Dump stack for the debugging purpose */ >> + dump_stack(); > WARN as above. Noted. > >> @@ -49,6 +50,27 @@ bool sev_enabled __section(".data"); >> /* Buffer used for early in-place encryption by BSP, no locking needed */ >> static char sme_early_buffer[PAGE_SIZE] __initdata __aligned(PAGE_SIZE); >> >> +/* >> + * When SNP is active, this routine changes the page state from private to shared before >> + * copying the data from the source to destination and restore after the copy. This is required >> + * because the source address is mapped as decrypted by the caller of the routine. >> + */ >> +static inline void __init snp_aware_memcpy(void *dst, void *src, size_t sz, >> + unsigned long paddr, bool dec) > snp_memcpy() simply. Noted. > >> +{ >> + unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT; >> + >> + /* If the paddr need to accessed decrypted, make the page shared before memcpy. */ > *needs* > >> + if (sev_snp_active() && dec) > Flip that test so that you don't have it twice in the code: > > if (!sev_snp_active()) { > memcpy(dst, src, sz); > } else { > ... > } > > I will look into it. thanks
diff --git a/arch/x86/include/asm/sev-snp.h b/arch/x86/include/asm/sev-snp.h index 0523eb21abd7..c4b096206062 100644 --- a/arch/x86/include/asm/sev-snp.h +++ b/arch/x86/include/asm/sev-snp.h @@ -63,6 +63,10 @@ struct __packed snp_page_state_change { #define GHCB_REGISTER_GPA_RESP 0x013UL #define GHCB_REGISTER_GPA_RESP_VAL(val) ((val) >> 12) +/* Macro to convert the x86 page level to the RMP level and vice versa */ +#define X86_RMP_PG_LEVEL(level) (((level) == PG_LEVEL_4K) ? RMP_PG_SIZE_4K : RMP_PG_SIZE_2M) +#define RMP_X86_PG_LEVEL(level) (((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M) + #ifdef CONFIG_AMD_MEM_ENCRYPT static inline int __pvalidate(unsigned long vaddr, int rmp_psize, int validate, unsigned long *rflags) @@ -82,6 +86,11 @@ static inline int __pvalidate(unsigned long vaddr, int rmp_psize, int validate, void sev_snp_register_ghcb(unsigned long paddr); +void __init early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr, + unsigned int npages); +void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr, + unsigned int npages); + #else /* !CONFIG_AMD_MEM_ENCRYPT */ static inline int __pvalidate(unsigned long vaddr, int psize, int validate, unsigned long *eflags) @@ -91,6 +100,17 @@ static inline int __pvalidate(unsigned long vaddr, int psize, int validate, unsi static inline void sev_snp_register_ghcb(unsigned long paddr) { } +static inline void __init +early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr, unsigned int npages) +{ + return 0; +} +static inline void __init +early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr, unsigned int npages) +{ + return 0; +} + #endif /* CONFIG_AMD_MEM_ENCRYPT */ #endif /* __ASSEMBLY__ */ diff --git a/arch/x86/kernel/sev-snp.c b/arch/x86/kernel/sev-snp.c index d32225c2b653..ff9b35bfb05c 100644 --- a/arch/x86/kernel/sev-snp.c +++ b/arch/x86/kernel/sev-snp.c @@ -56,3 +56,108 @@ void sev_snp_register_ghcb(unsigned long paddr) /* Restore the GHCB MSR value */ sev_es_wr_ghcb_msr(old); } + +static void sev_snp_issue_pvalidate(unsigned long vaddr, unsigned int npages, bool validate) +{ + unsigned long eflags, vaddr_end, vaddr_next; + int rc; + + vaddr = vaddr & PAGE_MASK; + vaddr_end = vaddr + (npages << PAGE_SHIFT); + + for (; vaddr < vaddr_end; vaddr = vaddr_next) { + rc = __pvalidate(vaddr, RMP_PG_SIZE_4K, validate, &eflags); + + if (rc) { + pr_err("Failed to validate address 0x%lx ret %d\n", vaddr, rc); + goto e_fail; + } + + /* Check for the double validation condition */ + if (eflags & X86_EFLAGS_CF) { + pr_err("Double %salidation detected (address 0x%lx)\n", + validate ? "v" : "inv", vaddr); + goto e_fail; + } + + vaddr_next = vaddr + PAGE_SIZE; + } + + return; + +e_fail: + /* Dump stack for the debugging purpose */ + dump_stack(); + + /* Ask to terminate the guest */ + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); +} + +static void __init early_snp_set_page_state(unsigned long paddr, unsigned int npages, int op) +{ + unsigned long paddr_end, paddr_next; + u64 old, val; + + paddr = paddr & PAGE_MASK; + paddr_end = paddr + (npages << PAGE_SHIFT); + + /* save the old GHCB MSR */ + old = sev_es_rd_ghcb_msr(); + + for (; paddr < paddr_end; paddr = paddr_next) { + + /* + * Use the MSR protocol VMGEXIT to request the page state change. We use the MSR + * protocol VMGEXIT because in early boot we may not have the full GHCB setup + * yet. + */ + sev_es_wr_ghcb_msr(GHCB_SNP_PAGE_STATE_REQ_GFN(paddr >> PAGE_SHIFT, op)); + VMGEXIT(); + + val = sev_es_rd_ghcb_msr(); + + /* Read the response, if the page state change failed then terminate the guest. */ + if (GHCB_SEV_GHCB_RESP_CODE(val) != GHCB_SNP_PAGE_STATE_CHANGE_RESP) + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); + + if (GHCB_SNP_PAGE_STATE_RESP_VAL(val) != 0) { + pr_err("Failed to change page state to '%s' paddr 0x%lx error 0x%llx\n", + op == SNP_PAGE_STATE_PRIVATE ? "private" : "shared", + paddr, GHCB_SNP_PAGE_STATE_RESP_VAL(val)); + + /* Dump stack for the debugging purpose */ + dump_stack(); + + /* Ask to terminate the guest */ + sev_es_terminate(GHCB_SEV_ES_REASON_GENERAL_REQUEST); + } + + paddr_next = paddr + PAGE_SIZE; + } + + /* Restore the GHCB MSR value */ + sev_es_wr_ghcb_msr(old); +} + +void __init early_snp_set_memory_private(unsigned long vaddr, unsigned long paddr, + unsigned int npages) +{ + /* Ask hypervisor to add the memory in RMP table as a 'private'. */ + early_snp_set_page_state(paddr, npages, SNP_PAGE_STATE_PRIVATE); + + /* Validate the memory region after its added in the RMP table. */ + sev_snp_issue_pvalidate(vaddr, npages, true); +} + +void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr, + unsigned int npages) +{ + /* + * We are chaning the memory from private to shared, invalidate the memory region + * before making it shared in the RMP table. + */ + sev_snp_issue_pvalidate(vaddr, npages, false); + + /* Ask hypervisor to make the memory shared in the RMP table. */ + early_snp_set_page_state(paddr, npages, SNP_PAGE_STATE_SHARED); +} diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c index 5bd50008fc9a..35af2f21b8f1 100644 --- a/arch/x86/mm/mem_encrypt.c +++ b/arch/x86/mm/mem_encrypt.c @@ -29,6 +29,7 @@ #include <asm/processor-flags.h> #include <asm/msr.h> #include <asm/cmdline.h> +#include <asm/sev-snp.h> #include "mm_internal.h" @@ -49,6 +50,27 @@ bool sev_enabled __section(".data"); /* Buffer used for early in-place encryption by BSP, no locking needed */ static char sme_early_buffer[PAGE_SIZE] __initdata __aligned(PAGE_SIZE); +/* + * When SNP is active, this routine changes the page state from private to shared before + * copying the data from the source to destination and restore after the copy. This is required + * because the source address is mapped as decrypted by the caller of the routine. + */ +static inline void __init snp_aware_memcpy(void *dst, void *src, size_t sz, + unsigned long paddr, bool dec) +{ + unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT; + + /* If the paddr need to accessed decrypted, make the page shared before memcpy. */ + if (sev_snp_active() && dec) + early_snp_set_memory_shared((unsigned long)__va(paddr), paddr, npages); + + memcpy(dst, src, sz); + + /* Restore the page state after the memcpy. */ + if (sev_snp_active() && dec) + early_snp_set_memory_private((unsigned long)__va(paddr), paddr, npages); +} + /* * This routine does not change the underlying encryption setting of the * page(s) that map this memory. It assumes that eventually the memory is @@ -97,8 +119,8 @@ static void __init __sme_early_enc_dec(resource_size_t paddr, * Use a temporary buffer, of cache-line multiple size, to * avoid data corruption as documented in the APM. */ - memcpy(sme_early_buffer, src, len); - memcpy(dst, sme_early_buffer, len); + snp_aware_memcpy(sme_early_buffer, src, len, paddr, enc); + snp_aware_memcpy(dst, sme_early_buffer, len, paddr, !enc); early_memunmap(dst, len); early_memunmap(src, len); @@ -278,9 +300,23 @@ static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc) else sme_early_decrypt(pa, size); + /* + * If SEV-SNP is active, rescind validation of the page in the RMP entry before encryption + * attribute is changed from C=1 to C=0. + */ + if (sev_snp_active() && !enc) + early_snp_set_memory_shared((unsigned long)__va(pa), pa, 1); + /* Change the page encryption mask. */ new_pte = pfn_pte(pfn, new_prot); set_pte_atomic(kpte, new_pte); + + /* + * If SEV-SNP is active, validate the page in the RMP entry after encryption attribute is + * changed from C=0 to C=1. + */ + if (sev_snp_active() && enc) + early_snp_set_memory_private((unsigned long)__va(pa), pa, 1); } static int __init early_set_memory_enc_dec(unsigned long vaddr,
The early_set_memory_{encrypt,decrypt}() are used for changing the page from decrypted (shared) to encrypted (private) and vice versa. When SEV-SNP is active, the page state transition needs to go through additional steps. If the page is transitioned from shared to private, then perform the following after the encryption attribute is set in the page table: 1. Issue the page state change VMGEXIT to add the page as a private in the RMP table. 2. Validate the page after its successfully added in the RMP table. To maintain the security guarantees, if the page is transitioned from private to shared, then perform the following before clearing the encryption attribute from the page table. 1. Invalidate the page. 2. Issue the page state change VMGEXIT to make the page shared in the RMP table. The early_set_memory_{encryot,decrypt} can be called before the full GHCB is setup, use the SNP page state MSR protocol VMGEXIT defined in the GHCB section 2.3.1 to request the page state change in the RMP table. Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Joerg Roedel <jroedel@suse.de> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Tony Luck <tony.luck@intel.com> Cc: Dave Hansen <dave.hansen@intel.com> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: David Rientjes <rientjes@google.com> Cc: Sean Christopherson <seanjc@google.com> Cc: x86@kernel.org Cc: kvm@vger.kernel.org Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/include/asm/sev-snp.h | 20 +++++++ arch/x86/kernel/sev-snp.c | 105 +++++++++++++++++++++++++++++++++ arch/x86/mm/mem_encrypt.c | 40 ++++++++++++- 3 files changed, 163 insertions(+), 2 deletions(-)