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@xxxxxxxxxx>
>> Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
>> ---
>> 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
