[PATCH v2 1/7] KVM: selftests: Fix build due to ucall_uninit() removal

[PATCH v2 1/7] KVM: selftests: Fix build due to ucall_uninit() removal

[Oliver Upton]

From: Mark Brown <broonie@kernel.org>

Today's -next fails to build on arm64 due to:

In file included from include/kvm_util.h:11,
                 from aarch64/page_fault_test.c:15:
include/ucall_common.h:36:47: note: expected ‘vm_paddr_t’ {aka ‘long unsigned int’} but argument is of type ‘void *’
   36 | void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
      |                                    ~~~~~~~~~~~^~~~~~~~
aarch64/page_fault_test.c:725:2: warning: implicit declaration of function ‘ucall_uninit’; did you mean ‘ucall_init’? [-Wimplicit-function-declaration]
  725 |  ucall_uninit(vm);
      |  ^~~~~~~~~~~~
      |  ucall_init

which is caused by commit

interacting poorly with commit

   28a65567acb5 ("KVM: selftests: Drop now-unnecessary ucall_uninit()")

As is done for other ucall_uninit() users remove the call in the newly added
page_fault_test.c.

Fixes: 28a65567acb5 ("KVM: selftests: Drop now-unnecessary ucall_uninit()")
Fixes: 35c581015712 ("KVM: selftests: aarch64: Add aarch64/page_fault_test")
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Ricardo Koller <ricarkol@google.com>
Cc: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/aarch64/page_fault_test.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
index 0cda70bef5d5..92d3a91153b6 100644
--- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
+++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
@@ -722,7 +722,6 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 
 	vcpu_run_loop(vm, vcpu, test);
 
-	ucall_uninit(vm);
 	kvm_vm_free(vm);
 	free_uffd(test, pt_uffd, data_uffd);
 
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v2 2/7] KVM: selftests: Setup ucall after loading program into guest memory

[Oliver Upton]

The new ucall infrastructure needs to update a couple of guest globals
to pass through the ucall MMIO addr and pool of ucall structs. A
precondition of writing to the guest's program image is to have it
already loaded into guest memory.

Call ucall_init() after kvm_vm_elf_load(). Continue to park the ucall
MMIO addr after MEM_REGION_TEST_DATA.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/aarch64/page_fault_test.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/aarch64/page_fault_test.c b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
index 92d3a91153b6..95d22cfb7b41 100644
--- a/tools/testing/selftests/kvm/aarch64/page_fault_test.c
+++ b/tools/testing/selftests/kvm/aarch64/page_fault_test.c
@@ -609,8 +609,13 @@ static void setup_memslots(struct kvm_vm *vm, struct test_params *p)
 				    data_size / guest_page_size,
 				    p->test_desc->data_memslot_flags);
 	vm->memslots[MEM_REGION_TEST_DATA] = TEST_DATA_MEMSLOT;
+}
+
+static void setup_ucall(struct kvm_vm *vm)
+{
+	struct userspace_mem_region *region = vm_get_mem_region(vm, MEM_REGION_TEST_DATA);
 
-	ucall_init(vm, data_gpa + data_size);
+	ucall_init(vm, region->region.guest_phys_addr + region->region.memory_size);
 }
 
 static void setup_default_handlers(struct test_desc *test)
@@ -702,6 +707,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 	vm = ____vm_create(mode);
 	setup_memslots(vm, p);
 	kvm_vm_elf_load(vm, program_invocation_name);
+	setup_ucall(vm);
 	vcpu = vm_vcpu_add(vm, 0, guest_code);
 
 	setup_gva_maps(vm);
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v2 3/7] KVM: selftests: Mark correct page as mapped in virt_map()

[Oliver Upton]

The loop marks vaddr as mapped after incrementing it by page size,
thereby marking the *next* page as mapped. Set the bit in vpages_mapped
first instead.

Fixes: 56fc7732031d ("KVM: selftests: Fill in vm->vpages_mapped bitmap in virt_map() too")
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index e9607eb089be..a256ec67aff6 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1411,10 +1411,10 @@ void virt_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 
 	while (npages--) {
 		virt_pg_map(vm, vaddr, paddr);
+		sparsebit_set(vm->vpages_mapped, vaddr >> vm->page_shift);
+
 		vaddr += page_size;
 		paddr += page_size;
-
-		sparsebit_set(vm->vpages_mapped, vaddr >> vm->page_shift);
 	}
 }
 
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v2 4/7] KVM: selftests: Correctly initialize the VA space for TTBR0_EL1

[Oliver Upton]

An interesting feature of the Arm architecture is that the stage-1 MMU
supports two distinct VA regions, controlled by TTBR{0,1}_EL1. As KVM
selftests on arm64 only uses TTBR0_EL1, the VA space is constrained to
[0, 2^(va_bits)). This is different from other architectures that
allow for addressing low and high regions of the VA space from a single
page table.

KVM selftests' VA space allocator presumes the valid address range is
split between low and high memory based the MSB, which of course is a
poor match for arm64's TTBR0 region.

Add a helper that correctly handles both addressing schemes with a
comment describing each.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 .../selftests/kvm/include/kvm_util_base.h     |  1 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 49 ++++++++++++++++---
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 6cd86da698b3..b193863d754f 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -103,6 +103,7 @@ struct kvm_vm {
 	struct sparsebit *vpages_mapped;
 	bool has_irqchip;
 	bool pgd_created;
+	bool has_split_va_space;
 	vm_paddr_t ucall_mmio_addr;
 	vm_paddr_t pgd;
 	vm_vaddr_t gdt;
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index a256ec67aff6..53d15f32f220 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -186,6 +186,43 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = {
 _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES,
 	       "Missing new mode params?");
 
+/*
+ * Initializes vm->vpages_valid to match the canonical VA space of the
+ * architecture.
+ *
+ * Most architectures split the range addressed by a single page table into a
+ * low and high region based on the MSB of the VA. On architectures with this
+ * behavior the VA region spans [0, 2^(va_bits - 1)), [-(2^(va_bits - 1), -1].
+ *
+ * arm64 is a bit different from the rest of the crowd, as the low and high
+ * regions of the VA space are addressed by distinct paging structures
+ * (TTBR{0,1}_EL1). KVM selftests on arm64 only uses TTBR0_EL1, meaning that we
+ * only have a low VA region. As there is no VA split based on the MSB, the VA
+ * region spans [0, 2^va_bits).
+ */
+static void vm_vaddr_populate_bitmap(struct kvm_vm *vm)
+{
+	sparsebit_num_t contig_va_bits = vm->va_bits;
+	sparsebit_num_t nr_contig_pages;
+
+	/*
+	 * Depending on the architecture, the MSB of the VA could split between
+	 * low and high regions. When that is the case each region has
+	 * va_bits - 1 of address.
+	 */
+	if (vm->has_split_va_space)
+		contig_va_bits--;
+
+	nr_contig_pages = (1ULL << contig_va_bits) >> vm->page_shift;
+
+	sparsebit_set_num(vm->vpages_valid, 0, nr_contig_pages);
+
+	if (vm->has_split_va_space)
+		sparsebit_set_num(vm->vpages_valid,
+				  -(1ULL << contig_va_bits),
+				  nr_contig_pages);
+}
+
 struct kvm_vm *____vm_create(enum vm_guest_mode mode)
 {
 	struct kvm_vm *vm;
@@ -268,17 +305,17 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode)
 #ifdef __aarch64__
 	if (vm->pa_bits != 40)
 		vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits);
+
+	/* selftests use TTBR0 only, meaning there is a single VA region. */
+	vm->has_split_va_space = false;
+#else
+	vm->has_split_va_space = true;
 #endif
 
 	vm_open(vm);
 
-	/* Limit to VA-bit canonical virtual addresses. */
 	vm->vpages_valid = sparsebit_alloc();
-	sparsebit_set_num(vm->vpages_valid,
-		0, (1ULL << (vm->va_bits - 1)) >> vm->page_shift);
-	sparsebit_set_num(vm->vpages_valid,
-		(~((1ULL << (vm->va_bits - 1)) - 1)) >> vm->page_shift,
-		(1ULL << (vm->va_bits - 1)) >> vm->page_shift);
+	vm_vaddr_populate_bitmap(vm);
 
 	/* Limit physical addresses to PA-bits. */
 	vm->max_gfn = vm_compute_max_gfn(vm);
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v2 4/7] KVM: selftests: Correctly initialize the VA space for TTBR0_EL1

[Sean Christopherson]

On Fri, Dec 09, 2022, Oliver Upton wrote:
> An interesting feature of the Arm architecture is that the stage-1 MMU
> supports two distinct VA regions, controlled by TTBR{0,1}_EL1. As KVM
> selftests on arm64 only uses TTBR0_EL1, the VA space is constrained to
> [0, 2^(va_bits)). This is different from other architectures that
> allow for addressing low and high regions of the VA space from a single
> page table.
> 
> KVM selftests' VA space allocator presumes the valid address range is
> split between low and high memory based the MSB, which of course is a
> poor match for arm64's TTBR0 region.
> 
> Add a helper that correctly handles both addressing schemes with a
> comment describing each.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---

Thanks much!  Looks awesome, especially the comment!

Reviewed-by: Sean Christopherson <seanjc@google.com>

>  .../selftests/kvm/include/kvm_util_base.h     |  1 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 49 ++++++++++++++++---
>  2 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 6cd86da698b3..b193863d754f 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -103,6 +103,7 @@ struct kvm_vm {
>  	struct sparsebit *vpages_mapped;
>  	bool has_irqchip;
>  	bool pgd_created;
> +	bool has_split_va_space;
>  	vm_paddr_t ucall_mmio_addr;
>  	vm_paddr_t pgd;
>  	vm_vaddr_t gdt;
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index a256ec67aff6..53d15f32f220 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -186,6 +186,43 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = {
>  _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES,
>  	       "Missing new mode params?");
>  
> +/*
> + * Initializes vm->vpages_valid to match the canonical VA space of the
> + * architecture.
> + *
> + * Most architectures split the range addressed by a single page table into a
> + * low and high region based on the MSB of the VA. On architectures with this
> + * behavior the VA region spans [0, 2^(va_bits - 1)), [-(2^(va_bits - 1), -1].
> + *
> + * arm64 is a bit different from the rest of the crowd, as the low and high
> + * regions of the VA space are addressed by distinct paging structures
> + * (TTBR{0,1}_EL1).

Oooh, they're different CR3s in x86 terminology?

Re: [PATCH v2 4/7] KVM: selftests: Correctly initialize the VA space for TTBR0_EL1

[Oliver Upton]

On Fri, Dec 09, 2022 at 08:45:01PM +0000, Sean Christopherson wrote:
> On Fri, Dec 09, 2022, Oliver Upton wrote:
> > An interesting feature of the Arm architecture is that the stage-1 MMU
> > supports two distinct VA regions, controlled by TTBR{0,1}_EL1. As KVM
> > selftests on arm64 only uses TTBR0_EL1, the VA space is constrained to
> > [0, 2^(va_bits)). This is different from other architectures that
> > allow for addressing low and high regions of the VA space from a single
> > page table.
> > 
> > KVM selftests' VA space allocator presumes the valid address range is
> > split between low and high memory based the MSB, which of course is a
> > poor match for arm64's TTBR0 region.
> > 
> > Add a helper that correctly handles both addressing schemes with a
> > comment describing each.
> > 
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > ---
> 
> Thanks much!  Looks awesome, especially the comment!
> 
> Reviewed-by: Sean Christopherson <seanjc@google.com>

ty!

> >  .../selftests/kvm/include/kvm_util_base.h     |  1 +
> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 49 ++++++++++++++++---
> >  2 files changed, 44 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index 6cd86da698b3..b193863d754f 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -103,6 +103,7 @@ struct kvm_vm {
> >  	struct sparsebit *vpages_mapped;
> >  	bool has_irqchip;
> >  	bool pgd_created;
> > +	bool has_split_va_space;
> >  	vm_paddr_t ucall_mmio_addr;
> >  	vm_paddr_t pgd;
> >  	vm_vaddr_t gdt;
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index a256ec67aff6..53d15f32f220 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -186,6 +186,43 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = {
> >  _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES,
> >  	       "Missing new mode params?");
> >  
> > +/*
> > + * Initializes vm->vpages_valid to match the canonical VA space of the
> > + * architecture.
> > + *
> > + * Most architectures split the range addressed by a single page table into a
> > + * low and high region based on the MSB of the VA. On architectures with this
> > + * behavior the VA region spans [0, 2^(va_bits - 1)), [-(2^(va_bits - 1), -1].
> > + *
> > + * arm64 is a bit different from the rest of the crowd, as the low and high
> > + * regions of the VA space are addressed by distinct paging structures
> > + * (TTBR{0,1}_EL1).
> 
> Oooh, they're different CR3s in x86 terminology?

Right, we can have two active table roots at any given time, each
mapping their own portion of the address space.

--
Thanks,
Oliver

Re: [PATCH v2 4/7] KVM: selftests: Correctly initialize the VA space for TTBR0_EL1

[Paolo Bonzini]

On 12/9/22 02:53, Oliver Upton wrote:
> @@ -268,17 +305,17 @@ struct kvm_vm *____vm_create(enum vm_guest_mode mode)
>   #ifdef __aarch64__
>   	if (vm->pa_bits != 40)
>   		vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(vm->pa_bits);
> +
> +	/* selftests use TTBR0 only, meaning there is a single VA region. */
> +	vm->has_split_va_space = false;
> +#else
> +	vm->has_split_va_space = true;
>   #endif
>   

Even though there happens to be already a suitable #ifdef, I don't
really like them and don't think there should be a new bool unless
something else uses it.

However, the new comment is very useful, so I added it to kvm_util.c as
follows:

/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 759a45540108..56d5ea949cbb 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -186,6 +186,15 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = {
  _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES,
  	       "Missing new mode params?");
  
+/*
+ * Initializes vm->vpages_valid to match the canonical VA space of the
+ * architecture.
+ *
+ * The default implementation is valid for architectures which split the
+ * range addressed by a single page table into a low and high region
+ * based on the MSB of the VA. On architectures with this behavior
+ * the VA region spans [0, 2^(va_bits - 1)), [-(2^(va_bits - 1), -1].
+ */
  __weak void vm_vaddr_populate_bitmap(struct kvm_vm *vm)
  {
  	sparsebit_set_num(vm->vpages_valid,

[PATCH v2 5/7] KVM: arm64: selftests: Don't identity map the ucall MMIO hole

[Oliver Upton]

Currently the ucall MMIO hole is placed immediately after slot0, which
is a relatively safe address in the PA space. However, it is possible
that the same address has already been used for something else (like the
guest program image) in the VA space. At least in my own testing,
building the vgic_irq test with clang leads to the MMIO hole appearing
underneath gicv3_ops.

Stop identity mapping the MMIO hole and instead find an unused VA to map
to it. Yet another subtle detail of the KVM selftests library is that
virt_pg_map() does not update vm->vpages_mapped. Switch over to
virt_map() instead to guarantee that the chosen VA isn't to something
else.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/lib/aarch64/ucall.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
index 562c16dfbb00..f212bd8ab93d 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
@@ -14,11 +14,13 @@ static vm_vaddr_t *ucall_exit_mmio_addr;
 
 void ucall_arch_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa)
 {
-	virt_pg_map(vm, mmio_gpa, mmio_gpa);
+	vm_vaddr_t mmio_gva = vm_vaddr_unused_gap(vm, vm->page_size, KVM_UTIL_MIN_VADDR);
+
+	virt_map(vm, mmio_gva, mmio_gpa, 1);
 
 	vm->ucall_mmio_addr = mmio_gpa;
 
-	write_guest_global(vm, ucall_exit_mmio_addr, (vm_vaddr_t *)mmio_gpa);
+	write_guest_global(vm, ucall_exit_mmio_addr, (vm_vaddr_t *)mmio_gva);
 }
 
 void ucall_arch_do_ucall(vm_vaddr_t uc)
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v2 6/7] KVM: selftests: Allocate ucall pool from MEM_REGION_DATA

[Oliver Upton]

MEM_REGION_TEST_DATA is meant to hold data explicitly used by a
selftest, not implicit allocations due to the selftests infrastructure.
Allocate the ucall pool from MEM_REGION_DATA much like the rest of the
selftests library allocations.

Reviewed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/lib/ucall_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index 820ce6c82829..0cc0971ce60e 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -22,7 +22,7 @@ void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa)
 	vm_vaddr_t vaddr;
 	int i;
 
-	vaddr = vm_vaddr_alloc(vm, sizeof(*hdr), KVM_UTIL_MIN_VADDR);
+	vaddr = __vm_vaddr_alloc(vm, sizeof(*hdr), KVM_UTIL_MIN_VADDR, MEM_REGION_DATA);
 	hdr = (struct ucall_header *)addr_gva2hva(vm, vaddr);
 	memset(hdr, 0, sizeof(*hdr));
 
-- 
2.39.0.rc1.256.g54fd8350bd-goog

[PATCH v2 7/7] KVM: selftests: Avoid infinite loop if ucall_alloc() fails

[Oliver Upton]

Guest assertions depend on successfully allocating a ucall structure. As
such, the use of guest assertions when ucall_alloc() fails simply leads
to an infinite loop in guest code.

Use GUEST_UCALL_NONE() to indicate failure instead. Though not
technically necessary, use a goto to have a single callsite and an
associated comment about why assertions don't work here. It isn't
perfect, at least the poor developer gets some signal out of the
guest...

Fixes: 426729b2cf2e ("KVM: selftests: Add ucall pool based implementation")
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/lib/ucall_common.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index 0cc0971ce60e..e8370da3de24 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -41,7 +41,8 @@ static struct ucall *ucall_alloc(void)
 	struct ucall *uc;
 	int i;
 
-	GUEST_ASSERT(ucall_pool);
+	if (!ucall_pool)
+		goto out;
 
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		if (!test_and_set_bit(i, ucall_pool->in_use)) {
@@ -51,7 +52,14 @@ static struct ucall *ucall_alloc(void)
 		}
 	}
 
-	GUEST_ASSERT(0);
+out:
+	/*
+	 * If the guest cannot grab a ucall structure from the pool then the
+	 * only option to get out to userspace is a bare ucall. This is probably
+	 * a good time to mention that guest assertions depend on ucalls with
+	 * arguments too.
+	 */
+	GUEST_UCALL_NONE();
 	return NULL;
 }
 
-- 
2.39.0.rc1.256.g54fd8350bd-goog

Re: [PATCH v2 7/7] KVM: selftests: Avoid infinite loop if ucall_alloc() fails

[Sean Christopherson]

On Fri, Dec 09, 2022, Oliver Upton wrote:
> Guest assertions depend on successfully allocating a ucall structure. As
> such, the use of guest assertions when ucall_alloc() fails simply leads
> to an infinite loop in guest code.
> 
> Use GUEST_UCALL_NONE() to indicate failure instead. Though not
> technically necessary, use a goto to have a single callsite and an
> associated comment about why assertions don't work here. It isn't
> perfect, at least the poor developer gets some signal out of the
> guest...
> 
> Fixes: 426729b2cf2e ("KVM: selftests: Add ucall pool based implementation")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  tools/testing/selftests/kvm/lib/ucall_common.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index 0cc0971ce60e..e8370da3de24 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -41,7 +41,8 @@ static struct ucall *ucall_alloc(void)
>  	struct ucall *uc;
>  	int i;
>  
> -	GUEST_ASSERT(ucall_pool);
> +	if (!ucall_pool)
> +		goto out;
>  
>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>  		if (!test_and_set_bit(i, ucall_pool->in_use)) {
> @@ -51,7 +52,14 @@ static struct ucall *ucall_alloc(void)
>  		}
>  	}
>  
> -	GUEST_ASSERT(0);
> +out:
> +	/*
> +	 * If the guest cannot grab a ucall structure from the pool then the
> +	 * only option to get out to userspace is a bare ucall. This is probably
> +	 * a good time to mention that guest assertions depend on ucalls with
> +	 * arguments too.
> +	 */
> +	GUEST_UCALL_NONE();

UCALL_NONE isn't much better than infinite stack recursion, e.g. a test might end
up passing by dumb luck, or go in the wrong direction because it sometimes handles
UCALL_NONE.

How about this?

From: Sean Christopherson <seanjc@google.com>
Date: Fri, 9 Dec 2022 12:55:44 -0800
Subject: [PATCH] KVM: selftests: Use magic value to signal ucall_alloc()
 failure

Use a magic value to signal a ucall_alloc() failure instead of simply
doing GUEST_ASSERT().  GUEST_ASSERT() relies on ucall_alloc() and so a
failure puts the guest into an infinite loop.

Use -1 as the magic value, as a real ucall struct should never wrap.

Reported-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/lib/ucall_common.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index 0cc0971ce60e..2f0e2ea941cc 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -4,6 +4,8 @@
 #include "linux/bitmap.h"
 #include "linux/atomic.h"
 
+#define GUEST_UCALL_FAILED -1
+
 struct ucall_header {
 	DECLARE_BITMAP(in_use, KVM_MAX_VCPUS);
 	struct ucall ucalls[KVM_MAX_VCPUS];
@@ -41,7 +43,8 @@ static struct ucall *ucall_alloc(void)
 	struct ucall *uc;
 	int i;
 
-	GUEST_ASSERT(ucall_pool);
+	if (!ucall_pool)
+		goto ucall_failed;
 
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		if (!test_and_set_bit(i, ucall_pool->in_use)) {
@@ -51,7 +54,13 @@ static struct ucall *ucall_alloc(void)
 		}
 	}
 
-	GUEST_ASSERT(0);
+ucall_failed:
+	/*
+	 * If the vCPU cannot grab a ucall structure, make a bare ucall with a
+	 * magic value to signal to get_ucall() that things went sideways.
+	 * GUEST_ASSERT() depends on ucall_alloc() and so cannot be used here.
+	 */
+	ucall_arch_do_ucall(GUEST_UCALL_FAILED);
 	return NULL;
 }
 
@@ -93,6 +102,9 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
 
 	addr = ucall_arch_get_ucall(vcpu);
 	if (addr) {
+		TEST_ASSERT(addr != (void *)GUEST_UCALL_FAILED,
+			    "Guest failed to allocate ucall struct");
+
 		memcpy(uc, addr, sizeof(*uc));
 		vcpu_run_complete_io(vcpu);
 	} else {

base-commit: dc2efbe4813e0dc4368779bc36c5f0e636cb8eb2
-- 

Re: [PATCH v2 7/7] KVM: selftests: Avoid infinite loop if ucall_alloc() fails

[Oliver Upton]

On Fri, Dec 09, 2022 at 09:03:45PM +0000, Sean Christopherson wrote:

[...]

> > -	GUEST_ASSERT(0);
> > +out:
> > +	/*
> > +	 * If the guest cannot grab a ucall structure from the pool then the
> > +	 * only option to get out to userspace is a bare ucall. This is probably
> > +	 * a good time to mention that guest assertions depend on ucalls with
> > +	 * arguments too.
> > +	 */
> > +	GUEST_UCALL_NONE();
> 
> UCALL_NONE isn't much better than infinite stack recursion, e.g. a test might end
> up passing by dumb luck, or go in the wrong direction because it sometimes handles
> UCALL_NONE.

Oh, I was just seeking an end to my misery. Yeah, we can use a magic
value to signal this instead.

> How about this?

LGTM.

--
Thanks,
Oliver

> From: Sean Christopherson <seanjc@google.com>
> Date: Fri, 9 Dec 2022 12:55:44 -0800
> Subject: [PATCH] KVM: selftests: Use magic value to signal ucall_alloc()
>  failure
> 
> Use a magic value to signal a ucall_alloc() failure instead of simply
> doing GUEST_ASSERT().  GUEST_ASSERT() relies on ucall_alloc() and so a
> failure puts the guest into an infinite loop.
> 
> Use -1 as the magic value, as a real ucall struct should never wrap.
> 
> Reported-by: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  tools/testing/selftests/kvm/lib/ucall_common.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index 0cc0971ce60e..2f0e2ea941cc 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -4,6 +4,8 @@
>  #include "linux/bitmap.h"
>  #include "linux/atomic.h"
>  
> +#define GUEST_UCALL_FAILED -1
> +
>  struct ucall_header {
>  	DECLARE_BITMAP(in_use, KVM_MAX_VCPUS);
>  	struct ucall ucalls[KVM_MAX_VCPUS];
> @@ -41,7 +43,8 @@ static struct ucall *ucall_alloc(void)
>  	struct ucall *uc;
>  	int i;
>  
> -	GUEST_ASSERT(ucall_pool);
> +	if (!ucall_pool)
> +		goto ucall_failed;
>  
>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>  		if (!test_and_set_bit(i, ucall_pool->in_use)) {
> @@ -51,7 +54,13 @@ static struct ucall *ucall_alloc(void)
>  		}
>  	}
>  
> -	GUEST_ASSERT(0);
> +ucall_failed:
> +	/*
> +	 * If the vCPU cannot grab a ucall structure, make a bare ucall with a
> +	 * magic value to signal to get_ucall() that things went sideways.
> +	 * GUEST_ASSERT() depends on ucall_alloc() and so cannot be used here.
> +	 */
> +	ucall_arch_do_ucall(GUEST_UCALL_FAILED);
>  	return NULL;
>  }
>  
> @@ -93,6 +102,9 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
>  
>  	addr = ucall_arch_get_ucall(vcpu);
>  	if (addr) {
> +		TEST_ASSERT(addr != (void *)GUEST_UCALL_FAILED,
> +			    "Guest failed to allocate ucall struct");
> +
>  		memcpy(uc, addr, sizeof(*uc));
>  		vcpu_run_complete_io(vcpu);
>  	} else {
> 
> base-commit: dc2efbe4813e0dc4368779bc36c5f0e636cb8eb2
> -- 
> 

Re: [PATCH v2 7/7] KVM: selftests: Avoid infinite loop if ucall_alloc() fails

[Paolo Bonzini]

On 12/9/22 22:03, Sean Christopherson wrote:
> From: Sean Christopherson<seanjc@google.com>
> Date: Fri, 9 Dec 2022 12:55:44 -0800
> Subject: [PATCH] KVM: selftests: Use magic value to signal ucall_alloc()
>   failure
> 
> Use a magic value to signal a ucall_alloc() failure instead of simply
> doing GUEST_ASSERT().  GUEST_ASSERT() relies on ucall_alloc() and so a
> failure puts the guest into an infinite loop.
> 
> Use -1 as the magic value, as a real ucall struct should never wrap.
> 
> Reported-by: Oliver Upton<oliver.upton@linux.dev>
> Signed-off-by: Sean Christopherson<seanjc@google.com>
> ---
>   tools/testing/selftests/kvm/lib/ucall_common.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index 0cc0971ce60e..2f0e2ea941cc 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -4,6 +4,8 @@
>   #include "linux/bitmap.h"
>   #include "linux/atomic.h"
>   
> +#define GUEST_UCALL_FAILED -1
> +
>   struct ucall_header {
>   	DECLARE_BITMAP(in_use, KVM_MAX_VCPUS);
>   	struct ucall ucalls[KVM_MAX_VCPUS];
> @@ -41,7 +43,8 @@ static struct ucall *ucall_alloc(void)
>   	struct ucall *uc;
>   	int i;
>   
> -	GUEST_ASSERT(ucall_pool);
> +	if (!ucall_pool)
> +		goto ucall_failed;
>   
>   	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>   		if (!test_and_set_bit(i, ucall_pool->in_use)) {
> @@ -51,7 +54,13 @@ static struct ucall *ucall_alloc(void)
>   		}
>   	}
>   
> -	GUEST_ASSERT(0);
> +ucall_failed:
> +	/*
> +	 * If the vCPU cannot grab a ucall structure, make a bare ucall with a
> +	 * magic value to signal to get_ucall() that things went sideways.
> +	 * GUEST_ASSERT() depends on ucall_alloc() and so cannot be used here.
> +	 */
> +	ucall_arch_do_ucall(GUEST_UCALL_FAILED);
>   	return NULL;
>   }
>   
> @@ -93,6 +102,9 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc)
>   
>   	addr = ucall_arch_get_ucall(vcpu);
>   	if (addr) {
> +		TEST_ASSERT(addr != (void *)GUEST_UCALL_FAILED,
> +			    "Guest failed to allocate ucall struct");
> +
>   		memcpy(uc, addr, sizeof(*uc));
>   		vcpu_run_complete_io(vcpu);
>   	} else {
> 
> base-commit: dc2efbe4813e0dc4368779bc36c5f0e636cb8eb2
> -- 

Queued, thanks.

Paolo