Re: [PATCH 3/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[Peter Xu]

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