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?
> + 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. ...
*/
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
