Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Oct 2022 17:34:19 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Peter Gonda <pgonda@google.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        marcorr@google.com, michael.roth@amd.com, thomas.lendacky@amd.com,
        joro@8bytes.org, mizhang@google.com, pbonzini@redhat.com,
        andrew.jones@linux.dev
Subject: Re: [V4 4/8] KVM: selftests: handle encryption bits in page tables
Message-ID: <Yz8Rm7zjXDHhdFw1@google.com>
References: <20220829171021.701198-1-pgonda@google.com>
 <20220829171021.701198-5-pgonda@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220829171021.701198-5-pgonda@google.com>
Precedence: bulk

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