On Thu, Aug 29, 2019 at 06:03:09PM +0800, Peter Xu wrote:
> 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
yeah, that's what I was looking for, but I wasn't thinking clearly when
I suggested just the name redefinition.
drew
