Re: [PATCH v3 02/13] KVM: selftests: aarch64: Add vm_get_pte_gpa library function

[Oliver Upton <oupton@google.com>]

Hey Ricardo,

Sorry about my last email hitting your v2. I fudged my inbox filtering
so v3 missed my explicit-cc inbox. Oops!

On Thu, Apr 07, 2022 at 05:41:09PM -0700, Ricardo Koller wrote:
> Add a library function (in-guest)

This function is called from host userspace, no?

> to get the GPA of the PTE of a
> particular GVA.  This will be used in a future commit by a test to clear
> and check the AF (access flag) of a particular page.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  .../selftests/kvm/include/aarch64/processor.h |  2 ++
>  .../selftests/kvm/lib/aarch64/processor.c     | 24 +++++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
> index 8f9f46979a00..caa572d83062 100644
> --- a/tools/testing/selftests/kvm/include/aarch64/processor.h
> +++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
> @@ -125,6 +125,8 @@ void vm_install_exception_handler(struct kvm_vm *vm,
>  void vm_install_sync_handler(struct kvm_vm *vm,
>  		int vector, int ec, handler_fn handler);
>  
> +vm_paddr_t vm_get_pte_gpa(struct kvm_vm *vm, vm_vaddr_t gva);
> +
>  static inline void cpu_relax(void)
>  {
>  	asm volatile("yield" ::: "memory");
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 9343d82519b4..ee006d354b79 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -139,7 +139,7 @@ void virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr)
>  	_virt_pg_map(vm, vaddr, paddr, attr_idx);
>  }
>  
> -vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
> +vm_paddr_t vm_get_pte_gpa(struct kvm_vm *vm, vm_vaddr_t gva)
>  {
>  	uint64_t *ptep;
>  
> @@ -162,7 +162,7 @@ vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
>  			goto unmapped_gva;
>  		/* fall through */
>  	case 2:
> -		ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pte_index(vm, gva) * 8;
> +		ptep = (uint64_t *)(pte_addr(vm, *ptep) + pte_index(vm, gva) * 8);

this seems a bit odd. ptep is an HVA in the above cases, but really a
GPA here.

Also -- not your code but the baked-in assumption that the stage-1 MMU
always maps at leaf page granularity might be a bit of a mess if we ever
do anything more interesting inside of the guest.

>  		if (!ptep)
>  			goto unmapped_gva;
>  		break;
> @@ -170,6 +170,26 @@ vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
>  		TEST_FAIL("Page table levels must be 2, 3, or 4");
>  	}
>  
> +	return (vm_paddr_t)ptep;
> +
> +unmapped_gva:
> +	TEST_FAIL("No mapping for vm virtual address, gva: 0x%lx", gva);
> +	exit(1);

Isn't this just a workaround for the fact that TEST_FAIL() doesn't have
the noreturn attribute specified somewhere?

> +}
> +
> +vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
> +{
> +	uint64_t *ptep;
> +	vm_paddr_t ptep_gpa;
> +
> +	ptep_gpa = vm_get_pte_gpa(vm, gva);
> +	if (!ptep_gpa)
> +		goto unmapped_gva;

This branch will never be taken since vm_get_pte_gpa() will explode on
its own, right?

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm