| Message ID | 20181129160813.26532-1-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kvm: selftests: ucall: fix unsigned comparison | expand |
On 29/11/18 17:08, Andrew Jones wrote: > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > tools/testing/selftests/kvm/lib/ucall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/lib/ucall.c b/tools/testing/selftests/kvm/lib/ucall.c > index 4777f9bb5194..decb595c2965 100644 > --- a/tools/testing/selftests/kvm/lib/ucall.c > +++ b/tools/testing/selftests/kvm/lib/ucall.c > @@ -58,7 +58,7 @@ void ucall_init(struct kvm_vm *vm, ucall_type_t type, void *arg) > 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) { > + for (gpa = start; gpa < end; gpa -= step) { > if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1))) > return; > } > Uhm, I'm confused. The 1/6 address space means that in practice you'll only do a few tries. Also, going too much down towards the beginning of the address space is probably not what you want. What about this: diff --git a/tools/testing/selftests/kvm/lib/ucall.c b/tools/testing/selftests/kvm/lib/ucall.c index decb595c2965..694bc2eaa63e 100644 --- a/tools/testing/selftests/kvm/lib/ucall.c +++ b/tools/testing/selftests/kvm/lib/ucall.c @@ -34,7 +34,7 @@ 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; bool ret; if (arg) { @@ -53,17 +53,15 @@ void ucall_init(struct kvm_vm *vm, ucall_type_t type, void *arg) * 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. + * and up from there in 1/12 VA space sized steps. */ start = 1ul << (vm->va_bits * 2 / 3); end = 1ul << vm->va_bits; - step = 1ul << (vm->va_bits / 6); - for (gpa = start; gpa < end; gpa -= step) { - if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1))) + step = 1ul << (vm->va_bits / 12); + for (offset = 0; offset < end - start; offset += step) { + if (ucall_mmio_init(vm, (gpa - offset) & ~(vm->page_size - 1))) return; - } - for (gpa = start + step; gpa < end; gpa += step) { - if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1))) + if (ucall_mmio_init(vm, (gpa + offset) & ~(vm->page_size - 1))) return; } TEST_ASSERT(false, "Can't find a ucall mmio address"); Paolo
On Fri, Dec 14, 2018 at 12:29:04PM +0100, Paolo Bonzini wrote: > On 29/11/18 17:08, Andrew Jones wrote: > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > tools/testing/selftests/kvm/lib/ucall.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/kvm/lib/ucall.c b/tools/testing/selftests/kvm/lib/ucall.c > > index 4777f9bb5194..decb595c2965 100644 > > --- a/tools/testing/selftests/kvm/lib/ucall.c > > +++ b/tools/testing/selftests/kvm/lib/ucall.c > > @@ -58,7 +58,7 @@ void ucall_init(struct kvm_vm *vm, ucall_type_t type, void *arg) > > 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) { > > + for (gpa = start; gpa < end; gpa -= step) { > > if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1))) > > return; > > } > > > > Uhm, I'm confused. The 1/6 address space means that in practice you'll > only do a few tries. In practice it was fine, but it wasn't safe. The change above should makes it safe. > Also, going too much down towards the beginning of > the address space is probably not what you want. > > What about this: > > diff --git a/tools/testing/selftests/kvm/lib/ucall.c > b/tools/testing/selftests/kvm/lib/ucall.c > index decb595c2965..694bc2eaa63e 100644 > --- a/tools/testing/selftests/kvm/lib/ucall.c > +++ b/tools/testing/selftests/kvm/lib/ucall.c > @@ -34,7 +34,7 @@ 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; > bool ret; > > if (arg) { > @@ -53,17 +53,15 @@ void ucall_init(struct kvm_vm *vm, ucall_type_t > type, void *arg) > * 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. > + * and up from there in 1/12 VA space sized steps. > */ > start = 1ul << (vm->va_bits * 2 / 3); > end = 1ul << vm->va_bits; > - step = 1ul << (vm->va_bits / 6); > - for (gpa = start; gpa < end; gpa -= step) { > - if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1))) > + step = 1ul << (vm->va_bits / 12); > + for (offset = 0; offset < end - start; offset += step) { > + if (ucall_mmio_init(vm, (gpa - offset) & ~(vm->page_size - 1))) > return; > - } > - for (gpa = start + step; gpa < end; gpa += step) { > - if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1))) > + if (ucall_mmio_init(vm, (gpa + offset) & ~(vm->page_size - 1))) > return; > } > TEST_ASSERT(false, "Can't find a ucall mmio address"); This is a nicer approach, but there are couple of bugs. We need to do s/gpa/start/ in the ucall_mmio_init's. And another bug, even from before, is that step should include the page_shift step = 1ul << (vm->va_bits / 12 + vm->page_shift) and then we can drop the page size masking from the ucall_mmio_init's. Thanks, drew
diff --git a/tools/testing/selftests/kvm/lib/ucall.c b/tools/testing/selftests/kvm/lib/ucall.c index 4777f9bb5194..decb595c2965 100644 --- a/tools/testing/selftests/kvm/lib/ucall.c +++ b/tools/testing/selftests/kvm/lib/ucall.c @@ -58,7 +58,7 @@ void ucall_init(struct kvm_vm *vm, ucall_type_t type, void *arg) 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) { + for (gpa = start; gpa < end; gpa -= step) { if (ucall_mmio_init(vm, gpa & ~(vm->page_size - 1))) return; }
Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Andrew Jones <drjones@redhat.com> --- tools/testing/selftests/kvm/lib/ucall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)