[PATCH v2 0/3] KVM: selftests: Fixes for broken tests

[PATCH v2 0/3] KVM: selftests: Fixes for broken tests

[Ryan Roberts]

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

[PATCH v2 1/3] 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 v2 2/3] 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..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)
-- 
2.25.1

[PATCH v2 3/3] KVM: selftests: arm64: Fix ttbr0_el1 encoding for PA bits > 48

[Ryan Roberts]

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);
 }
 
-- 
2.25.1

Re: [PATCH v2 0/3] KVM: selftests: Fixes for broken tests

[Ryan Roberts]

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
> 
> 

Re: [PATCH v2 0/3] KVM: selftests: Fixes for broken tests

[Oliver Upton]

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>

-- 
Thanks,
Oliver

Re: [PATCH v2 0/3] KVM: selftests: Fixes for broken tests

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 0/3] KVM: selftests: Fixes for broken tests

[Ryan Roberts]

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.

Re: [PATCH v2 1/3] KVM: selftests: Fixup config fragment for access_tracking_perf_test

[Sean Christopherson]

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>

Re: [PATCH v2 0/3] KVM: selftests: Fixes for broken tests

[Marc Zyngier]

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.
-- 
Without deviation from the norm, progress is not possible.