Messages in this thread | | | Subject | Re: [PATCH v13 09/12] mm: x86: Invoke hypercall when page encryption status is changed | From | Paolo Bonzini <> | Date | Wed, 21 Apr 2021 14:00:42 +0200 |
| |
On 21/04/21 12:05, Borislav Petkov wrote: > On Thu, Apr 15, 2021 at 03:57:26PM +0000, Ashish Kalra wrote: >> +static inline void page_encryption_changed(unsigned long vaddr, int npages, >> + bool enc) > > When you see a function name "page_encryption_changed", what does that > tell you about what that function does? > > Dunno but it doesn't tell me a whole lot. > > Now look at the other function names in struct pv_mmu_ops. > > See the difference? > >> +static void set_memory_enc_dec_hypercall(unsigned long vaddr, int npages, > > If I had to guess what that function does just by reading its name, it > sets a memory encryption/decryption hypercall. > > Am I close?
The words are right but the order is wrong (more like "hypercall to set some memory's encrypted/decrypted state"). Perhaps? kvm_hypercall_set_page_enc_status.
page_encryption_changed does not sound bad to me though, it's a notification-like function name. Maybe notify_page_enc_status_changed?
Paolo
>> + bool enc) >> +{ >> + unsigned long sz = npages << PAGE_SHIFT; >> + unsigned long vaddr_end, vaddr_next; >> + >> + vaddr_end = vaddr + sz; >> + >> + for (; vaddr < vaddr_end; vaddr = vaddr_next) { >> + int psize, pmask, level; >> + unsigned long pfn; >> + pte_t *kpte; >> + >> + kpte = lookup_address(vaddr, &level); >> + if (!kpte || pte_none(*kpte)) >> + return; >> + >> + switch (level) { >> + case PG_LEVEL_4K: >> + pfn = pte_pfn(*kpte); >> + break; >> + case PG_LEVEL_2M: >> + pfn = pmd_pfn(*(pmd_t *)kpte); >> + break; >> + case PG_LEVEL_1G: >> + pfn = pud_pfn(*(pud_t *)kpte); >> + break; >> + default: >> + return; >> + } > > Pretty much that same thing is in __set_clr_pte_enc(). Make a helper > function pls. > >> + >> + psize = page_level_size(level); >> + pmask = page_level_mask(level); >> + >> + kvm_sev_hypercall3(KVM_HC_PAGE_ENC_STATUS, >> + pfn << PAGE_SHIFT, psize >> PAGE_SHIFT, enc); >> + >> + vaddr_next = (vaddr & pmask) + psize; >> + } > > As with other patches from Brijesh, that should be a while loop. :) > >> +} >> + >> static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc) >> { >> pgprot_t old_prot, new_prot; >> @@ -286,12 +329,13 @@ static void __init __set_clr_pte_enc(pte_t *kpte, int level, bool enc) >> static int __init early_set_memory_enc_dec(unsigned long vaddr, >> unsigned long size, bool enc) >> { >> - unsigned long vaddr_end, vaddr_next; >> + unsigned long vaddr_end, vaddr_next, start; >> unsigned long psize, pmask; >> int split_page_size_mask; >> int level, ret; >> pte_t *kpte; >> >> + start = vaddr; >> vaddr_next = vaddr; >> vaddr_end = vaddr + size; >> >> @@ -346,6 +390,8 @@ static int __init early_set_memory_enc_dec(unsigned long vaddr, >> >> ret = 0; >> >> + set_memory_enc_dec_hypercall(start, PAGE_ALIGN(size) >> PAGE_SHIFT, >> + enc); >> out: >> __flush_tlb_all(); >> return ret; >> @@ -481,6 +527,15 @@ void __init mem_encrypt_init(void) >> if (sev_active() && !sev_es_active()) >> static_branch_enable(&sev_enable_key); >> >> +#ifdef CONFIG_PARAVIRT >> + /* >> + * With SEV, we need to make a hypercall when page encryption state is >> + * changed. >> + */ >> + if (sev_active()) >> + pv_ops.mmu.page_encryption_changed = set_memory_enc_dec_hypercall; >> +#endif > > There's already a sev_active() check above it. Merge the two pls. > >> + >> print_mem_encrypt_feature_info(); >> } >> >> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c >> index 16f878c26667..3576b583ac65 100644 >> --- a/arch/x86/mm/pat/set_memory.c >> +++ b/arch/x86/mm/pat/set_memory.c >> @@ -27,6 +27,7 @@ >> #include <asm/proto.h> >> #include <asm/memtype.h> >> #include <asm/set_memory.h> >> +#include <asm/paravirt.h> >> >> #include "../mm_internal.h" >> >> @@ -2012,6 +2013,12 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc) >> */ >> cpa_flush(&cpa, 0); >> >> + /* Notify hypervisor that a given memory range is mapped encrypted >> + * or decrypted. The hypervisor will use this information during the >> + * VM migration. >> + */ > > Kernel comments style is: > > /* > * A sentence ending with a full-stop. > * Another sentence. ... > * More sentences. ... > */ >
| |