Hi Oliver,
Here is a respin of the KVM selftests fixes, with your nits addressed; I've fixed the footer whitespace issue and I'm now using FIELD_GET() in the place where you suggested and a couple of other places too. I've also included the 3rd patch in this series (the ttbr0_el1 fix), which I originally sent separately - this is now using FIELD_GET() too.
So this series superceeds [1] and [2].
Thanks, Ryan
[1] https://lore.kernel.org/kvmarm/e8ed178a-0c67-3e00-a085-1d88fb3cb41f@arm.com/ [2] https://lore.kernel.org/kvmarm/20230302152033.242073-1-ryan.roberts@arm.com/
Ryan Roberts (3): KVM: selftests: Fixup config fragment for access_tracking_perf_test KVM: selftests: arm64: Fix pte encode/decode for PA bits > 48 KVM: selftests: arm64: Fix ttbr0_el1 encoding for PA bits > 48
tools/testing/selftests/kvm/config | 1 + .../selftests/kvm/lib/aarch64/processor.c | 39 ++++++++++++++----- 2 files changed, 30 insertions(+), 10 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
On Wed, Mar 08, 2023, Ryan Roberts wrote:
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
Reviewed-by: Sean Christopherson seanjc@google.com
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..c413085d58b7 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 |= FIELD_GET(GENMASK(51, 48), pa) << 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 |= FIELD_GET(GENMASK(15, 12), pte) << 48; + + 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)
Bits [51:48] of the pgd address are stored at bits [5:2] of ttbr0_el1. page_table_test stores its page tables at the far end of IPA space so was tripping over this when run on a system that supports FEAT_LPA (or FEAT_LPA2).
Signed-off-by: Ryan Roberts ryan.roberts@arm.com --- tools/testing/selftests/kvm/lib/aarch64/processor.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c index c413085d58b7..233357d2f1cc 100644 --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c @@ -242,7 +242,7 @@ void aarch64_vcpu_setup(struct kvm_vcpu *vcpu, struct kvm_vcpu_init *init) { struct kvm_vcpu_init default_init = { .target = -1, }; struct kvm_vm *vm = vcpu->vm; - uint64_t sctlr_el1, tcr_el1; + uint64_t sctlr_el1, tcr_el1, ttbr0_el1;
if (!init) init = &default_init; @@ -293,10 +293,13 @@ void aarch64_vcpu_setup(struct kvm_vcpu *vcpu, struct kvm_vcpu_init *init) TEST_FAIL("Unknown guest mode, mode: 0x%x", vm->mode); }
+ ttbr0_el1 = vm->pgd & GENMASK(47, vm->page_shift); + /* Configure output size */ switch (vm->mode) { case VM_MODE_P52V48_64K: tcr_el1 |= 6ul << 32; /* IPS = 52 bits */ + ttbr0_el1 |= FIELD_GET(GENMASK(51, 48), vm->pgd) << 2; break; case VM_MODE_P48V48_4K: case VM_MODE_P48V48_16K: @@ -326,7 +329,7 @@ void aarch64_vcpu_setup(struct kvm_vcpu *vcpu, struct kvm_vcpu_init *init) vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_SCTLR_EL1), sctlr_el1); vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_TCR_EL1), tcr_el1); vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_MAIR_EL1), DEFAULT_MAIR_EL1); - vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_TTBR0_EL1), vm->pgd); + vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_TTBR0_EL1), ttbr0_el1); vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_TPIDR_EL1), vcpu->id); }
Hi Oliver,
Just a polite nudge on this: I was originally hoping to get these into 6.3 since I thought they were fairly uncontroversial and clearly fixing bugs. What are my chances?
Thanks, Ryan
On 08/03/2023 11:09, Ryan Roberts wrote:
Hi Oliver,
Here is a respin of the KVM selftests fixes, with your nits addressed; I've fixed the footer whitespace issue and I'm now using FIELD_GET() in the place where you suggested and a couple of other places too. I've also included the 3rd patch in this series (the ttbr0_el1 fix), which I originally sent separately - this is now using FIELD_GET() too.
So this series superceeds [1] and [2].
Thanks, Ryan
[1] https://lore.kernel.org/kvmarm/e8ed178a-0c67-3e00-a085-1d88fb3cb41f@arm.com/ [2] https://lore.kernel.org/kvmarm/20230302152033.242073-1-ryan.roberts@arm.com/
Ryan Roberts (3): KVM: selftests: Fixup config fragment for access_tracking_perf_test KVM: selftests: arm64: Fix pte encode/decode for PA bits > 48 KVM: selftests: arm64: Fix ttbr0_el1 encoding for PA bits > 48
tools/testing/selftests/kvm/config | 1 + .../selftests/kvm/lib/aarch64/processor.c | 39 ++++++++++++++----- 2 files changed, 30 insertions(+), 10 deletions(-)
-- 2.25.1
Hey Ryan,
On Thu, Mar 23, 2023 at 12:56:18PM +0000, Ryan Roberts wrote:
Hi Oliver,
Just a polite nudge on this: I was originally hoping to get these into 6.3 since I thought they were fairly uncontroversial and clearly fixing bugs. What are my chances?
Yes, your changes are indeed uncontroversial :) At least for me, fixes to selftests take a strictly lower priority than fixes to the kernel outside of a merge window. AFAICT, only LPA systems are affected by the changes here and I'm not aware of any of those out in the wild.
So, unless there is a burning issue, I'd like to defer these patches to the 6.4 merge window. Nonetheless, it all looks good to me:
Reviewed-by: Oliver Upton oliver.upton@linux.dev
On 2023-03-23 17:38, Oliver Upton wrote:
Hey Ryan,
On Thu, Mar 23, 2023 at 12:56:18PM +0000, Ryan Roberts wrote:
Hi Oliver,
Just a polite nudge on this: I was originally hoping to get these into 6.3 since I thought they were fairly uncontroversial and clearly fixing bugs. What are my chances?
Yes, your changes are indeed uncontroversial :) At least for me, fixes to selftests take a strictly lower priority than fixes to the kernel outside of a merge window. AFAICT, only LPA systems are affected by the changes here and I'm not aware of any of those out in the wild.
Agreed. My usual take on fixing tests is that unless the test has been broken in the current cycle, we can safely delay merging the fix until the following cycle.
And yes, LPA-capable HW is essentially vapourware at this stage.
So, unless there is a burning issue, I'd like to defer these patches to the 6.4 merge window. Nonetheless, it all looks good to me:
Reviewed-by: Oliver Upton oliver.upton@linux.dev
Thanks for that. I'll start queuing 6.4 material once I'm back to my usual time zone, beginning of next week.
Cheers,
M.
On 23/03/2023 18:56, Marc Zyngier wrote:
On 2023-03-23 17:38, Oliver Upton wrote:
Hey Ryan,
On Thu, Mar 23, 2023 at 12:56:18PM +0000, Ryan Roberts wrote:
Hi Oliver,
Just a polite nudge on this: I was originally hoping to get these into 6.3 since I thought they were fairly uncontroversial and clearly fixing bugs. What are my chances?
Yes, your changes are indeed uncontroversial :) At least for me, fixes to selftests take a strictly lower priority than fixes to the kernel outside of a merge window. AFAICT, only LPA systems are affected by the changes here and I'm not aware of any of those out in the wild.
The first patch is not related to LPA, it adds a missing kernel config to the config fragment so that one of the tests (access_tracking_perf_test) is not skipped. This change will mean our CI system starts actually running the test.
Agreed. My usual take on fixing tests is that unless the test has been broken in the current cycle, we can safely delay merging the fix until the following cycle.
Thanks for the explanation. I have a slightly different opinion though (please bare with me through the rant):
Being fairly new to Linux development, I'd like to be able to run (all) the selftests as a matter of course to be able to quickly answer the "did I obviously break anything?" question. But there is a lot of friction to even being able to compile, let alone run, the things - undocumented dependencies on libraries (even more difficult when needing to cross compile), undocumented dependencies on kernel configs, test code that is broken and fails to compile, tests that silently skip for difficult to determine reasons, tests that fail even when run against the unmodified kernel, and results buried in copious amounts of logs. These are all paper cuts that make them difficult to use and trust. Or perhaps I'm just doing it wrong...
I would love to live in a world where I could confidently take a mainline release, compile and run tests at close-to-zero effort and see all tests running and passing... one day, perhaps. But only if we give more priority to the test code ;-)
And yes, LPA-capable HW is essentially vapourware at this stage.
So, unless there is a burning issue, I'd like to defer these patches to the 6.4 merge window. Nonetheless, it all looks good to me:
Reviewed-by: Oliver Upton oliver.upton@linux.dev
Thanks for that.
Thanks for that. I'll start queuing 6.4 material once I'm back to my usual time zone, beginning of next week.
Appreciated. Thanks for taking the patches. If you have a rule-of-thumb about the best time to post different types of patches (and what it's best to base them on), I'll try to follow it in future.
Thanks, Ryan
Cheers,
M.
On Wed, 8 Mar 2023 11:09:45 +0000, Ryan Roberts wrote:
Here is a respin of the KVM selftests fixes, with your nits addressed; I've fixed the footer whitespace issue and I'm now using FIELD_GET() in the place where you suggested and a couple of other places too. I've also included the 3rd patch in this series (the ttbr0_el1 fix), which I originally sent separately - this is now using FIELD_GET() too.
So this series superceeds [1] and [2].
[...]
Applied to next, thanks!
[1/3] KVM: selftests: Fixup config fragment for access_tracking_perf_test commit: a2bed39057b434c4fd816005d1b950fefc61569d [2/3] KVM: selftests: arm64: Fix pte encode/decode for PA bits > 48 commit: e659babfc5a693553cf9473470840464f0ed5d77 [3/3] KVM: selftests: arm64: Fix ttbr0_el1 encoding for PA bits > 48 commit: e17071754cf50e5f6ff8ebee077e0e4114d3bea5
Cheers,
M.
linux-kselftest-mirror@lists.linaro.org
-
Marc Zyngier -
Oliver Upton -
Ryan Roberts -
Sean Christopherson