On Wed, Aug 28, 2019 at 01:41:04PM +0200, Andrew Jones wrote:
[...]
> > +#define DEBUG printf
>
> If this is going to be some general thing, then maybe we should do it
> like this
>
> #ifndef NDEBUG
> #define dprintf printf
> #endif
Ok.
>
>
> > +
> > /* Minimum allocated guest virtual and physical addresses */
> > #define KVM_UTIL_MIN_VADDR 0x2000
> >
> > @@ -38,12 +40,19 @@ enum vm_guest_mode {
> > VM_MODE_P48V48_64K,
> > VM_MODE_P40V48_4K,
> > VM_MODE_P40V48_64K,
> > + VM_MODE_PXXV48_4K, /* For 48bits VA but ANY bits PA */
> > NUM_VM_MODES,
> > };
> >
> > #ifdef __aarch64__
> > #define VM_MODE_DEFAULT VM_MODE_P40V48_4K
> > -#else
> > +#endif
> > +
> > +#ifdef __x86_64__
> > +#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K
> > +#endif
> > +
> > +#ifndef VM_MODE_DEFAULT
> > #define VM_MODE_DEFAULT VM_MODE_P52V48_4K
> > #endif
>
> nit: how about
>
> #if defined(__aarch64__)
> ...
> #elif defined(__x86_64__)
> ...
> #else
> ...
> #endif
Yes, better!
>
> >
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 486400a97374..86036a59a668 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -264,6 +264,9 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini
> > case VM_MODE_P52V48_4K:
> > TEST_ASSERT(false, "AArch64 does not support 4K sized pages "
> > "with 52-bit physical address ranges");
> > + case VM_MODE_PXXV48_4K:
> > + TEST_ASSERT(false, "AArch64 does not support 4K sized pages "
> > + "with ANY-bit physical address ranges");
> > case VM_MODE_P52V48_64K:
> > tcr_el1 |= 1ul << 14; /* TG0 = 64KB */
> > tcr_el1 |= 6ul << 32; /* IPS = 52 bits */
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 0c7c4368bc14..8c6f872a8793 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -8,6 +8,7 @@
> > #include "test_util.h"
> > #include "kvm_util.h"
> > #include "kvm_util_internal.h"
> > +#include "processor.h"
> >
> > #include <assert.h>
> > #include <sys/mman.h>
> > @@ -101,12 +102,13 @@ static void vm_open(struct kvm_vm *vm, int perm)
> > }
> >
> > const char * const vm_guest_mode_string[] = {
> > - "PA-bits:52, VA-bits:48, 4K pages",
> > - "PA-bits:52, VA-bits:48, 64K pages",
> > - "PA-bits:48, VA-bits:48, 4K pages",
> > - "PA-bits:48, VA-bits:48, 64K pages",
> > - "PA-bits:40, VA-bits:48, 4K pages",
> > - "PA-bits:40, VA-bits:48, 64K pages",
> > + "PA-bits:52, VA-bits:48, 4K pages",
> > + "PA-bits:52, VA-bits:48, 64K pages",
> > + "PA-bits:48, VA-bits:48, 4K pages",
> > + "PA-bits:48, VA-bits:48, 64K pages",
> > + "PA-bits:40, VA-bits:48, 4K pages",
> > + "PA-bits:40, VA-bits:48, 64K pages",
> > + "PA-bits:ANY, VA-bits:48, 4K pages",
>
> nit: while formatting we could align the 'K's in the 64/4K column
Ok.
>
> > };
> > _Static_assert(sizeof(vm_guest_mode_string)/sizeof(char *) == NUM_VM_MODES,
> > "Missing new mode strings?");
> > @@ -185,6 +187,32 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
> > vm->page_size = 0x10000;
> > vm->page_shift = 16;
> > break;
> > + case VM_MODE_PXXV48_4K:
> > +#ifdef __x86_64__
> > + {
> > + struct kvm_cpuid_entry2 *entry;
> > +
> > + /* SDM 4.1.4 */
> > + entry = kvm_get_supported_cpuid_entry(0x80000008);
> > + if (entry) {
> > + vm->pa_bits = entry->eax & 0xff;
> > + vm->va_bits = (entry->eax >> 8) & 0xff;
> > + } else {
> > + vm->pa_bits = vm->va_bits = 32;
> > + }
> > + TEST_ASSERT(vm->va_bits == 48, "Linear address width "
> > + "(%d bits) not supported", vm->va_bits);
> > + vm->pgtable_levels = 4;
> > + vm->page_size = 0x1000;
> > + vm->page_shift = 12;
> > + DEBUG("Guest physical address width detected: %d\n",
> > + vm->pa_bits);
> > + }
>
> How about making this a function that lives in x86_64/processor.c?
Done. Also I fixed up the overlooked PAE calculation which should be
36 rather than 32 bits PA.
>
> > +#else
> > + TEST_ASSERT(false, "VM_MODE_PXXV48_4K not supported on "
> > + "non-x86 platforms");
>
> This should make the above aarch64_vcpu_setup() change unnecessary.
I tend to prefer keeping both because if some non-arm arch removed it
then aarch64 has another chance to assert.
Thanks,
--
Peter Xu
