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

[Peter Xu]

On Thu, Aug 29, 2019 at 11:45:16AM +0200, Andrew Jones wrote:
> On Thu, Aug 29, 2019 at 10:21:16AM +0800, Peter Xu wrote:
> > The naming VM_MODE_P52V48_4K is explicit but unclear when used on
> > x86_64 machines, because x86_64 machines are having various physical
> > address width rather than some static values.  Here's some examples:
> > 
> >   - Intel Xeon E3-1220:  36 bits
> >   - Intel Core i7-8650:  39 bits
> >   - AMD   EPYC 7251:     48 bits
> > 
> > All of them are using 48 bits linear address width but with totally
> > different physical address width (and most of the old machines should
> > be less than 52 bits).
> > 
> > Let's create a new guest mode called VM_MODE_PXXV48_4K for current
> > x86_64 tests and make it as the default to replace the old naming of
> > VM_MODE_P52V48_4K because it shows more clearly that the PA width is
> > not really a constant.  Meanwhile we also stop assuming all the x86
> > machines are having 52 bits PA width but instead we fetch the real
> > vm->pa_bits from CPUID 0x80000008 during runtime.
> > 
> > We currently make this exclusively used by x86_64 but no other arch.
> > 
> > As a slight touch up, moving DEBUG macro from dirty_log_test.c to
> > kvm_util.h so lib can use it too.
> > 
> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> > ---
> >  tools/testing/selftests/kvm/dirty_log_test.c  |  5 ++--
> >  .../testing/selftests/kvm/include/kvm_util.h  |  9 +++++-
> >  .../selftests/kvm/include/x86_64/processor.h  |  3 ++
> >  .../selftests/kvm/lib/aarch64/processor.c     |  3 ++
> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 29 ++++++++++++++----
> >  .../selftests/kvm/lib/x86_64/processor.c      | 30 ++++++++++++++++---
> >  6 files changed, 65 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> > index efb7746a7e99..c86f83cb33e5 100644
> > --- a/tools/testing/selftests/kvm/dirty_log_test.c
> > +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> > @@ -19,8 +19,6 @@
> >  #include "kvm_util.h"
> >  #include "processor.h"
> >  
> > -#define DEBUG printf
> > -
> >  #define VCPU_ID				1
> >  
> >  /* The memory slot index to track dirty pages */
> > @@ -256,6 +254,7 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
> >  
> >  	switch (mode) {
> >  	case VM_MODE_P52V48_4K:
> > +	case VM_MODE_PXXV48_4K:
> >  		guest_pa_bits = 52;
> >  		guest_page_shift = 12;
> >  		break;
> > @@ -446,7 +445,7 @@ int main(int argc, char *argv[])
> >  #endif
> >  
> >  #ifdef __x86_64__
> > -	vm_guest_mode_params_init(VM_MODE_P52V48_4K, true, true);
> > +	vm_guest_mode_params_init(VM_MODE_PXXV48_4K, true, true);
> >  #endif
> >  #ifdef __aarch64__
> >  	vm_guest_mode_params_init(VM_MODE_P40V48_4K, true, true);
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index c78faa2ff7f3..430edbacb9b2 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -24,6 +24,10 @@ struct kvm_vm;
> >  typedef uint64_t vm_paddr_t; /* Virtual Machine (Guest) physical address */
> >  typedef uint64_t vm_vaddr_t; /* Virtual Machine (Guest) virtual address */
> >  
> > +#ifndef DEBUG
> > +#define DEBUG printf
> > +#endif
> 
> There's no way to turn this off without modifying code. I suggested
> 
> #ifndef NDEBUG
> #define dprintf printf
> #endif
> 
> which allows the dprintf(...) statements to be removed by compiling with
> -DNDEBUG added to CFLAGS. And that would also disable all the asserts().
> That's probably not all that useful, but then again, defining printf() as
> DEBUG() isn't useful either if the intention is to always print.

Sorry I misread that...

Though, I'm afraid even if with above it won't compile with -DNDEBUG
because the compiler could start to complain about undefined
"dprintf", or even recognize the dprintf as the libc call, dprintf(3).

So instead, does below looks ok?

#ifdef NDEBUG
#define DEBUG(...)
#else
#define DEBUG(...) printf(__VA_ARGS__);
#endif

Thanks,

-- 
Peter Xu