Patchwork [v3] kvm: selftests: ucall: fix exit mmio address guessing

login
register
mail settings
Submitter Andrew Jones
Date Dec. 20, 2018, 3:36 p.m.
Message ID <20181220153633.18626-1-drjones@redhat.com>
Download mbox | patch
Permalink /patch/687435/
State New
Headers show

Comments

Andrew Jones - Dec. 20, 2018, 3:36 p.m.
Dan pointed out the unsigned less than zero comparison and Paolo
suggested a different approach to the iteration. I pulled together
something that also ensures we maintain page aligned addresses
by using power-of-two divisors (8 and 16) and fixes two other bugs.
The first other bug was that the start and step calculations were
wrong since they were dividing the number of address bits instead
of the address space. The second other bug was that the guessing
algorithm wasn't considering the valid physical and virtual address
ranges correctly for an identity map. Sigh... Hopefully we've finally
got this thing right!

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 tools/testing/selftests/kvm/lib/ucall.c | 36 ++++++++++++++-----------
 1 file changed, 21 insertions(+), 15 deletions(-)
Radim Krčmář - Dec. 20, 2018, 7:39 p.m.
2018-12-20 16:36+0100, Andrew Jones:
> Dan pointed out the unsigned less than zero comparison and Paolo
> suggested a different approach to the iteration. I pulled together
> something that also ensures we maintain page aligned addresses
> by using power-of-two divisors (8 and 16) and fixes two other bugs.
> The first other bug was that the start and step calculations were
> wrong since they were dividing the number of address bits instead
> of the address space. The second other bug was that the guessing
> algorithm wasn't considering the valid physical and virtual address
> ranges correctly for an identity map. Sigh... Hopefully we've finally
> got this thing right!

Paolo actually sneaked in a fix for this already, 5132411985e1 ("kvm:
selftests: ucall: improve ucall placement in memory, fix unsigned
comparison"), so the physical range fix should come on top.

> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> diff --git a/tools/testing/selftests/kvm/lib/ucall.c b/tools/testing/selftests/kvm/lib/ucall.c
> @@ -45,25 +46,30 @@ void ucall_init(struct kvm_vm *vm, ucall_type_t type, void *arg)
>  		}
>  
>  		/*
> -		 * Find an address within the allowed virtual address space,
> -		 * that does _not_ have a KVM memory region associated with it.
> -		 * Identity mapping an address like this allows the guest to
> +		 * Find an address within the allowed physical and virtual address
> +		 * spaces, that does _not_ have a KVM memory region associated with
> +		 * it. Identity mapping an address like this allows the guest to
>  		 * access it, but as KVM doesn't know what to do with it, it
>  		 * will assume it's something userspace handles and exit with
>  		 * KVM_EXIT_MMIO. Well, at least that's how it works for AArch64.
> -		 * Here we start with a guess that the addresses around two
> -		 * thirds of the VA space are unmapped and then work both down
> -		 * and up from there in 1/6 VA space sized steps.
> +		 * Here we start with a guess that the addresses around 5/8th
> +		 * of the allowed space are unmapped and then work both down and
> +		 * up from there in 1/16th allowed space sized steps.
> +		 *
> +		 * Note, we need to use VA-bits - 1 when calculating the allowed
> +		 * virtual address space for an identity mapping because the upper
> +		 * half of the virtual address space is the two's complement of the
> +		 * lower and won't match physical addresses.

Do you mean the split address space due to cannonical address encoding
in e.g. x86?  (It's more like sign extension.)

We could leave the canonization to architectural code, instead of
halving the range in that case.

>  		 */
> -		start = 1ul << (vm->va_bits * 2 / 3);
> -		end = 1ul << vm->va_bits;
> -		step = 1ul << (vm->va_bits / 6);
> -		for (gpa = start; gpa >= 0; gpa -= step) {
> -			if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1)))
> +		bits = vm->va_bits - 1;
> +		bits = vm->pa_bits < bits ? vm->pa_bits : bits;
> +		end = 1ul << bits;

I'd use MIN(va, pa) directly in end computation, even though it's
unlikely we'll get more physical bits than virtual.

Thanks.
Paolo Bonzini - Dec. 21, 2018, 11:27 a.m.
On 20/12/18 20:39, Radim Krčmář wrote:
> 2018-12-20 16:36+0100, Andrew Jones:
>> Dan pointed out the unsigned less than zero comparison and Paolo
>> suggested a different approach to the iteration. I pulled together
>> something that also ensures we maintain page aligned addresses
>> by using power-of-two divisors (8 and 16) and fixes two other bugs.
>> The first other bug was that the start and step calculations were
>> wrong since they were dividing the number of address bits instead
>> of the address space. The second other bug was that the guessing
>> algorithm wasn't considering the valid physical and virtual address
>> ranges correctly for an identity map. Sigh... Hopefully we've finally
>> got this thing right!
> 
> Paolo actually sneaked in a fix for this already, 5132411985e1 ("kvm:
> selftests: ucall: improve ucall placement in memory, fix unsigned
> comparison"), so the physical range fix should come on top.
> 
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> ---
>> diff --git a/tools/testing/selftests/kvm/lib/ucall.c b/tools/testing/selftests/kvm/lib/ucall.c
>> @@ -45,25 +46,30 @@ void ucall_init(struct kvm_vm *vm, ucall_type_t type, void *arg)
>>  		}
>>  
>>  		/*
>> -		 * Find an address within the allowed virtual address space,
>> -		 * that does _not_ have a KVM memory region associated with it.
>> -		 * Identity mapping an address like this allows the guest to
>> +		 * Find an address within the allowed physical and virtual address
>> +		 * spaces, that does _not_ have a KVM memory region associated with
>> +		 * it. Identity mapping an address like this allows the guest to
>>  		 * access it, but as KVM doesn't know what to do with it, it
>>  		 * will assume it's something userspace handles and exit with
>>  		 * KVM_EXIT_MMIO. Well, at least that's how it works for AArch64.
>> -		 * Here we start with a guess that the addresses around two
>> -		 * thirds of the VA space are unmapped and then work both down
>> -		 * and up from there in 1/6 VA space sized steps.
>> +		 * Here we start with a guess that the addresses around 5/8th
>> +		 * of the allowed space are unmapped and then work both down and
>> +		 * up from there in 1/16th allowed space sized steps.
>> +		 *
>> +		 * Note, we need to use VA-bits - 1 when calculating the allowed
>> +		 * virtual address space for an identity mapping because the upper
>> +		 * half of the virtual address space is the two's complement of the
>> +		 * lower and won't match physical addresses.
> 
> Do you mean the split address space due to cannonical address encoding
> in e.g. x86?  (It's more like sign extension.)

Yes.  In general it's common for userspace to assume that addresses are
either all positive or all negative.  C sneaks in that pointer
difference is only legal within the same object, and limits the *object
size* to 2^31 on 32-bit systems even if they can address 2^32 bytes like
x32 could; otherwise you'd have sizeof(ptrdiff_t) > sizeof(size_t)).
However, in our case we want to do arbitrary address arithmetic so using
va_bits - 1 is a good idea in general.

I have applied a combo of reverting 5132411985e1 and applying Drew's
patch, so that you get a pure bugfix on top of the changes that I had
made earlier.

Paolo

Patch

diff --git a/tools/testing/selftests/kvm/lib/ucall.c b/tools/testing/selftests/kvm/lib/ucall.c
index 4777f9bb5194..a2ab38be2f47 100644
--- a/tools/testing/selftests/kvm/lib/ucall.c
+++ b/tools/testing/selftests/kvm/lib/ucall.c
@@ -34,7 +34,8 @@  void ucall_init(struct kvm_vm *vm, ucall_type_t type, void *arg)
 		return;
 
 	if (type == UCALL_MMIO) {
-		vm_paddr_t gpa, start, end, step;
+		vm_paddr_t gpa, start, end, step, offset;
+		unsigned bits;
 		bool ret;
 
 		if (arg) {
@@ -45,25 +46,30 @@  void ucall_init(struct kvm_vm *vm, ucall_type_t type, void *arg)
 		}
 
 		/*
-		 * Find an address within the allowed virtual address space,
-		 * that does _not_ have a KVM memory region associated with it.
-		 * Identity mapping an address like this allows the guest to
+		 * Find an address within the allowed physical and virtual address
+		 * spaces, that does _not_ have a KVM memory region associated with
+		 * it. Identity mapping an address like this allows the guest to
 		 * access it, but as KVM doesn't know what to do with it, it
 		 * will assume it's something userspace handles and exit with
 		 * KVM_EXIT_MMIO. Well, at least that's how it works for AArch64.
-		 * Here we start with a guess that the addresses around two
-		 * thirds of the VA space are unmapped and then work both down
-		 * and up from there in 1/6 VA space sized steps.
+		 * Here we start with a guess that the addresses around 5/8th
+		 * of the allowed space are unmapped and then work both down and
+		 * up from there in 1/16th allowed space sized steps.
+		 *
+		 * Note, we need to use VA-bits - 1 when calculating the allowed
+		 * virtual address space for an identity mapping because the upper
+		 * half of the virtual address space is the two's complement of the
+		 * lower and won't match physical addresses.
 		 */
-		start = 1ul << (vm->va_bits * 2 / 3);
-		end = 1ul << vm->va_bits;
-		step = 1ul << (vm->va_bits / 6);
-		for (gpa = start; gpa >= 0; gpa -= step) {
-			if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1)))
+		bits = vm->va_bits - 1;
+		bits = vm->pa_bits < bits ? vm->pa_bits : bits;
+		end = 1ul << bits;
+		start = end * 5 / 8;
+		step = end / 16;
+		for (offset = 0; offset < end - start; offset += step) {
+			if (ucall_mmio_init(vm, start - offset))
 				return;
-		}
-		for (gpa = start + step; gpa < end; gpa += step) {
-			if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1)))
+			if (ucall_mmio_init(vm, start + offset))
 				return;
 		}
 		TEST_ASSERT(false, "Can't find a ucall mmio address");