[PATCH v1 0/2] KVM: selftests: Fixes for broken tests

[PATCH v1 0/2] KVM: selftests: Fixes for broken tests

[Ryan Roberts]

During the course of implementing FEAT_LPA2 within the arm64 KVM port, I found a
couple of issues within the KVM selftest code, which I thought were worth
posting independently. The LPA2 patches, for which I will post v2 in the next
few days, depend on these fixes for its testing.

Ryan Roberts (2):
  KVM: selftests: Fixup config fragment for access_tracking_perf_test
  KVM: selftests: arm64: Fix pte encode/decode for PA bits > 48

 tools/testing/selftests/kvm/config            |  1 +
 .../selftests/kvm/lib/aarch64/processor.c     | 32 ++++++++++++++-----
 2 files changed, 25 insertions(+), 8 deletions(-)

--
2.25.1

[PATCH v1 1/2] KVM: selftests: Fixup config fragment for access_tracking_perf_test

[Ryan Roberts]

access_tracking_perf_test requires CONFIG_IDLE_PAGE_TRACKING. However
this is missing from the config fragment, so add it in so that this test
is no longer skipped.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/kvm/config | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/config b/tools/testing/selftests/kvm/config
index d011b38e259e..8835fed09e9f 100644
--- a/tools/testing/selftests/kvm/config
+++ b/tools/testing/selftests/kvm/config
@@ -2,3 +2,4 @@ CONFIG_KVM=y
 CONFIG_KVM_INTEL=y
 CONFIG_KVM_AMD=y
 CONFIG_USERFAULTFD=y
+CONFIG_IDLE_PAGE_TRACKING=y
-- 
2.25.1

[PATCH v1 2/2] KVM: selftests: arm64: Fix pte encode/decode for PA bits > 48

[Ryan Roberts]

The high bits [51:48] of a physical address should appear at [15:12] in
a 64K pte, not at [51:48] as was previously being programmed. Fix this
with new helper functions that do the conversion correctly. This also
sets us up nicely for adding LPA2 encodings in future.

Fixes: 7a6629ef746d ("kvm: selftests: add virt mem support for aarch64")

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 .../selftests/kvm/lib/aarch64/processor.c     | 32 ++++++++++++++-----
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 5972a23b2765..13f28d96521c 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -58,10 +58,27 @@ static uint64_t pte_index(struct kvm_vm *vm, vm_vaddr_t gva)
 	return (gva >> vm->page_shift) & mask;
 }
 
-static uint64_t pte_addr(struct kvm_vm *vm, uint64_t entry)
+static uint64_t addr_pte(struct kvm_vm *vm, uint64_t pa, uint64_t attrs)
 {
-	uint64_t mask = ((1UL << (vm->va_bits - vm->page_shift)) - 1) << vm->page_shift;
-	return entry & mask;
+	uint64_t pte;
+
+	pte = pa & GENMASK(47, vm->page_shift);
+	if (vm->page_shift == 16)
+		pte |= (pa & GENMASK(51, 48)) >> (48 - 12);
+	pte |= attrs;
+
+	return pte;
+}
+
+static uint64_t pte_addr(struct kvm_vm *vm, uint64_t pte)
+{
+	uint64_t pa;
+
+	pa = pte & GENMASK(47, vm->page_shift);
+	if (vm->page_shift == 16)
+		pa |= (pte & GENMASK(15, 12)) << (48 - 12);
+
+	return pa;
 }
 
 static uint64_t ptrs_per_pgd(struct kvm_vm *vm)
@@ -110,18 +127,18 @@ static void _virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 
 	ptep = addr_gpa2hva(vm, vm->pgd) + pgd_index(vm, vaddr) * 8;
 	if (!*ptep)
-		*ptep = vm_alloc_page_table(vm) | 3;
+		*ptep = addr_pte(vm, vm_alloc_page_table(vm), 3);
 
 	switch (vm->pgtable_levels) {
 	case 4:
 		ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pud_index(vm, vaddr) * 8;
 		if (!*ptep)
-			*ptep = vm_alloc_page_table(vm) | 3;
+			*ptep = addr_pte(vm, vm_alloc_page_table(vm), 3);
 		/* fall through */
 	case 3:
 		ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pmd_index(vm, vaddr) * 8;
 		if (!*ptep)
-			*ptep = vm_alloc_page_table(vm) | 3;
+			*ptep = addr_pte(vm, vm_alloc_page_table(vm), 3);
 		/* fall through */
 	case 2:
 		ptep = addr_gpa2hva(vm, pte_addr(vm, *ptep)) + pte_index(vm, vaddr) * 8;
@@ -130,8 +147,7 @@ static void _virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 		TEST_FAIL("Page table levels must be 2, 3, or 4");
 	}
 
-	*ptep = paddr | 3;
-	*ptep |= (attr_idx << 2) | (1 << 10) /* Access Flag */;
+	*ptep = addr_pte(vm, paddr, (attr_idx << 2) | (1 << 10) | 3);  /* AF */
 }
 
 void virt_arch_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr)
-- 
2.25.1

Re: [PATCH v1 0/2] KVM: selftests: Fixes for broken tests

[Ryan Roberts]

On 28/02/2023 17:07, Ryan Roberts wrote:
> During the course of implementing FEAT_LPA2 within the arm64 KVM port, I found a
> couple of issues within the KVM selftest code, which I thought were worth
> posting independently. The LPA2 patches, for which I will post v2 in the next
> few days, depend on these fixes for its testing.

It turned out that there was another issue, for which I have posted a fix at
https://lore.kernel.org/kvmarm/20230302152033.242073-1-ryan.roberts@arm.com/

With these 3 fixes, I see a clean run of the kvm selftests with the 64K kernel +
FEAT_LPA on FVP.

Thanks,
Ryan


> 
> Ryan Roberts (2):
>   KVM: selftests: Fixup config fragment for access_tracking_perf_test
>   KVM: selftests: arm64: Fix pte encode/decode for PA bits > 48
> 
>  tools/testing/selftests/kvm/config            |  1 +
>  .../selftests/kvm/lib/aarch64/processor.c     | 32 ++++++++++++++-----
>  2 files changed, 25 insertions(+), 8 deletions(-)
> 
> --
> 2.25.1
> 

Re: [PATCH v1 2/2] KVM: selftests: arm64: Fix pte encode/decode for PA bits > 48

[Oliver Upton]

Hi Ryan,

Thanks for fixing this. Couple of nits:

On Tue, Feb 28, 2023 at 05:07:56PM +0000, Ryan Roberts wrote:
> The high bits [51:48] of a physical address should appear at [15:12] in
> a 64K pte, not at [51:48] as was previously being programmed. Fix this
> with new helper functions that do the conversion correctly. This also
> sets us up nicely for adding LPA2 encodings in future.
> 
> Fixes: 7a6629ef746d ("kvm: selftests: add virt mem support for aarch64")
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>

nit: no whitespace between footers.

> ---
>  .../selftests/kvm/lib/aarch64/processor.c     | 32 ++++++++++++++-----
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 5972a23b2765..13f28d96521c 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -58,10 +58,27 @@ static uint64_t pte_index(struct kvm_vm *vm, vm_vaddr_t gva)
>  	return (gva >> vm->page_shift) & mask;
>  }
>  
> -static uint64_t pte_addr(struct kvm_vm *vm, uint64_t entry)
> +static uint64_t addr_pte(struct kvm_vm *vm, uint64_t pa, uint64_t attrs)
>  {
> -	uint64_t mask = ((1UL << (vm->va_bits - vm->page_shift)) - 1) << vm->page_shift;
> -	return entry & mask;
> +	uint64_t pte;
> +
> +	pte = pa & GENMASK(47, vm->page_shift);
> +	if (vm->page_shift == 16)
> +		pte |= (pa & GENMASK(51, 48)) >> (48 - 12);

nit: this is a bit of an odd transformation, of course courtesy of the
architecture. FIELD_GET() might make it a bit more readable:

		pte |= FIELD_GET(GENMASK(51, 48), pa) << 12;

-- 
Thanks,
Oliver

Re: [PATCH v1 2/2] KVM: selftests: arm64: Fix pte encode/decode for PA bits > 48

[Ryan Roberts]

On 07/03/2023 17:44, Oliver Upton wrote:
> Hi Ryan,
> 
> Thanks for fixing this. Couple of nits:
> 
> On Tue, Feb 28, 2023 at 05:07:56PM +0000, Ryan Roberts wrote:
>> The high bits [51:48] of a physical address should appear at [15:12] in
>> a 64K pte, not at [51:48] as was previously being programmed. Fix this
>> with new helper functions that do the conversion correctly. This also
>> sets us up nicely for adding LPA2 encodings in future.
>>
>> Fixes: 7a6629ef746d ("kvm: selftests: add virt mem support for aarch64")
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> 
> nit: no whitespace between footers.

Sorry my bad; I'm slowly learning the conventions - I'll get there eventually!

> 
>> ---
>>  .../selftests/kvm/lib/aarch64/processor.c     | 32 ++++++++++++++-----
>>  1 file changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
>> index 5972a23b2765..13f28d96521c 100644
>> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
>> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
>> @@ -58,10 +58,27 @@ static uint64_t pte_index(struct kvm_vm *vm, vm_vaddr_t gva)
>>  	return (gva >> vm->page_shift) & mask;
>>  }
>>  
>> -static uint64_t pte_addr(struct kvm_vm *vm, uint64_t entry)
>> +static uint64_t addr_pte(struct kvm_vm *vm, uint64_t pa, uint64_t attrs)
>>  {
>> -	uint64_t mask = ((1UL << (vm->va_bits - vm->page_shift)) - 1) << vm->page_shift;
>> -	return entry & mask;
>> +	uint64_t pte;
>> +
>> +	pte = pa & GENMASK(47, vm->page_shift);
>> +	if (vm->page_shift == 16)
>> +		pte |= (pa & GENMASK(51, 48)) >> (48 - 12);
> 
> nit: this is a bit of an odd transformation, of course courtesy of the
> architecture. FIELD_GET() might make it a bit more readable:
> 
> 		pte |= FIELD_GET(GENMASK(51, 48), pa) << 12;
> 

Ahh yes, that does look better. I did consider this originally, but thought I
would also need FIELD_PREP() which selftests is not currently using anywhere. So
thought I would steer clear entirely. Anyway, on review, I don't need
FIELD_PREP(), so I've just sent you a respun series using FIELD_GET() in all the
sensible places. I hope I'm not jumping the gun by respinning so quickly - I
didn't think the series was particularly controversial so unlikely to get any
more comments.