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
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
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)
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;
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.
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
linux-kselftest-mirror@lists.linaro.org
-
Oliver Upton -
Ryan Roberts