lkml.org 
[lkml]   [2022]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [V4 4/8] KVM: selftests: handle encryption bits in page tables
On Mon, Aug 29, 2022, Peter Gonda wrote:
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 53b9a509c1d5..de13be62d52d 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1388,6 +1388,58 @@ void virt_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
> }
> }
>
> +/*
> + * Mask off any special bits from raw GPA
> + *
> + * Input Args:
> + * vm - Virtual Machine
> + * gpa_raw - Raw VM physical address
> + *
> + * Output Args: None
> + *
> + * Return:
> + * GPA with special bits (e.g. shared/encrypted) masked off.
> + */
> +vm_paddr_t addr_raw2gpa(struct kvm_vm *vm, vm_paddr_t gpa_raw)
> +{
> + if (!vm->memcrypt.has_enc_bit)
> + return gpa_raw;
> +
> + return gpa_raw & ~(1ULL << vm->memcrypt.enc_bit);

1. The notion of stealing GPA bits to tag the page should not be tied to memory
encryption.

2. Assuming that the shared vs. private bit is active-high is wrong, e.g. TDX
has active-low behavior.

3. "raw" is not super untuitive.

4. addr_gpa2raw() should not exist. It assumes the "raw" address always wants
the encryption bit set.

5. enc_by_default is an SEV-centric flag that should not exist outside of x86.

I think the easiest and most familiar solution for #1 will be to borrow the
kernel's tag/untag terminology for handling virtual addresses that can be tagged
for ASAN, e.g. ARM's top-byte-ignore and x86's linear address masking (LAM).

So instead of addr_raw2gpa(), just do:

static inline vm_paddr_t vm_untag_gpa(struct kvm_vm *vm, vm_vaddr_t gpa)
{
return gpa & ~vm->gpa_tag_mask;
}

That way zero-allocating the VM will Just Work.

> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 2e6e61bbe81b..b2df259ce706 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -118,7 +118,7 @@ void virt_arch_pgd_alloc(struct kvm_vm *vm)
>
> /* If needed, create page map l4 table. */
> if (!vm->pgd_created) {
> - vm->pgd = vm_alloc_page_table(vm);
> + vm->pgd = addr_gpa2raw(vm, vm_alloc_page_table(vm));

Rather than add "struct vm_memcrypt", I think it makes sense to introduce
"struct kvm_vm_arch" and then the x86 implementation can add pte_me_mask, similar
to what KVM does for SPTEs. THen this code because

vm->pgd = vm_alloc_page_table(vm) | vm->arch.pte_me_mask;

That will Just Work for TDX, because its pte_me_mask will be '0', even though it
effectively has an encryption bit (active-low).

> vm->pgd_created = true;
> }
> }
> @@ -140,13 +140,15 @@ static uint64_t *virt_create_upper_pte(struct kvm_vm *vm,
> int target_level)
> {
> uint64_t *pte = virt_get_pte(vm, pt_pfn, vaddr, current_level);
> + uint64_t paddr_raw = addr_gpa2raw(vm, paddr);
>
> if (!(*pte & PTE_PRESENT_MASK)) {
> *pte = PTE_PRESENT_MASK | PTE_WRITABLE_MASK;
> if (current_level == target_level)
> - *pte |= PTE_LARGE_MASK | (paddr & PHYSICAL_PAGE_MASK);
> + *pte |= PTE_LARGE_MASK | (paddr_raw & PHYSICAL_PAGE_MASK);
> else
> - *pte |= vm_alloc_page_table(vm) & PHYSICAL_PAGE_MASK;
> + *pte |= addr_gpa2raw(vm, vm_alloc_page_table(vm)) & PHYSICAL_PAGE_MASK;

Prefer to write this as:

*pte |= vm_alloc_page_table(vm) & PHYSICAL_PAGE_MASK;
*pte |= vm->arch.pte_me_mask;

so that selftests don't assume the encryption bit is stolen from the GPA.

> +
> } else {
> /*
> * Entry already present. Assert that the caller doesn't want
> @@ -184,6 +186,8 @@ void __virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr, int level)
> "Physical address beyond maximum supported,\n"
> " paddr: 0x%lx vm->max_gfn: 0x%lx vm->page_size: 0x%x",
> paddr, vm->max_gfn, vm->page_size);
> + TEST_ASSERT(addr_raw2gpa(vm, paddr) == paddr,
> + "Unexpected bits in paddr: %lx", paddr);
>
> /*
> * Allocate upper level page tables, if not already present. Return
> @@ -206,7 +210,8 @@ void __virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr, int level)
> pte = virt_get_pte(vm, PTE_GET_PFN(*pde), vaddr, PG_LEVEL_4K);
> TEST_ASSERT(!(*pte & PTE_PRESENT_MASK),
> "PTE already present for 4k page at vaddr: 0x%lx\n", vaddr);
> - *pte = PTE_PRESENT_MASK | PTE_WRITABLE_MASK | (paddr & PHYSICAL_PAGE_MASK);
> + *pte = PTE_PRESENT_MASK | PTE_WRITABLE_MASK |
> + (addr_gpa2raw(vm, paddr) & PHYSICAL_PAGE_MASK);

And with the above suggestions, this can become something like:

if (vm_is_gpa_encrypted(vm, paddr))
*pte |= vm->arch.c_bit;
else
*pte |= vm->arch.s_bit;

where SEV sets c_bit and TDX sets s_bit.

> void virt_arch_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr)
> @@ -515,7 +520,7 @@ vm_paddr_t addr_arch_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
> if (!(pte[index[0]] & PTE_PRESENT_MASK))
> goto unmapped_gva;
>
> - return (PTE_GET_PFN(pte[index[0]]) * vm->page_size) + (gva & ~PAGE_MASK);
> + return addr_raw2gpa(vm, PTE_GET_PFN(pte[index[0]]) * vm->page_size) + (gva & ~PAGE_MASK);

Aha! A use for my rework[*] of this mess! I don't think you need to take a
dependency on that work, at a glance the conflicts should be minor, i.e. doesn't
matter that much which lands first.

[*] https://lore.kernel.org/all/20221006004512.666529-1-seanjc@google.com

\
 
 \ /
  Last update: 2022-10-06 19:35    [W:0.093 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site