linux-kernel - Re: [PATCH v2 3/4] KVM: selftests: Introduce VM_MODE_PXXV48

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Aug 2019 18:03:09 +0800
From:   Peter Xu <peterx@...hat.com>
To:     Andrew Jones <drjones@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Thomas Huth <thuth@...hat.com>
Subject: Re: [PATCH v2 3/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

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@...hat.com>
> > ---
> >  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