Message-ID: <aL8e0MMa4U2-nstQ@google.com>
Date: Mon, 8 Sep 2025 11:22:08 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Aqib Faruqui <aqibaf@...zon.com>
Cc: aqibaf@...zon.co.uk, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 5/9] KVM: selftests: Prevent PAGE_SIZE redefinition on x86

On Fri, Sep 05, 2025, Aqib Faruqui wrote:
> Thanks for the suggestion. I don't see your previous redefinition of PAGE_SIZE 
> upstream, just 3 lines above the warning redefinition:

Oh, I manually added a conflicting definition to demonstrate the warning gcc
will provide.

> > In file included from include/x86/svm_util.h:13,
> >                  from include/x86/sev.h:15,
> >                  from lib/x86/sev.c:5:
> > include/x86/processor.h:373:9: error: "PAGE_SIZE" redefined [-Werror]
> >   373 | #define PAGE_SIZE               (1ULL << PAGE_SHIFT)
> >       |         ^~~~~~~~~
> > include/x86/processor.h:370:9: note: this is the location of the previous definition
> >   370 | #define PAGE_SIZE               BIT(12)
> >       |         ^~~~~~~~~
> 
> But I investigated further and found that both glibc and musl define PAGE_SIZE in 
> sys/user.h:
> 
> glibc (sys/user.h):
>   #define PAGE_SHIFT    12
>   #define PAGE_SIZE     (1UL << PAGE_SHIFT)
> 
> musl (sys/user.h):
>   #define PAGE_SIZE     4096

But that still doesn't fully explain how the previous definition comes into play.
E.g. if I modify x86/processor.h to explicitly #include <sys/user.h>, then I see
redefinition warnings:

  In file included from x86/tsc_scaling_sync.c:8:
  include/x86/processor.h:373:9: warning: "PAGE_SIZE" redefined
    373 | #define PAGE_SIZE               (1ULL << PAGE_SHIFT)
        |         ^~~~~~~~~
  In file included from include/x86/processor.h:13:
  /usr/include/x86_64-linux-gnu/sys/user.h:173:9: note: this is the location of the previous definition
     173 | #define PAGE_SIZE               (1UL << PAGE_SHIFT)
         |         ^~~~~~~~~

> KVM selftests (include/x86/processor.h):
>   #define PAGE_SHIFT		12
>   #define PAGE_SIZE     (1ULL << PAGE_SHIFT)
> 
> This creates redefinition warnings with both C libraries on my system. I've already 
> sent a v2 patch series with the PAGE_SIZE patch dropped but I'm not sure what the 
> next course of action would be for this?

I don't see a clean way to resolve this other than to eliminate the #include of
whatever is leading to musl defining PAGE_SIZE.  I don't want to #undef and
re-define PAGE_SIZE because then different compilation units (or even code chunks)
could see different definitions.  And as below, relying on libc for the #define
isn't viable.  So I think before we change any code, we need to first figure out
exactly how musl's conflicting definition comes into existence, and then go from
there.

Ideally, we would skip selftests' definition and assert that the existing definition
is compatible, but that won't work because musl's definition isn't actually
compatible with KVM selftests' definition, and more importantly isn't compatible
with glibc's definition.  KVM selftests and glibc both effectively #define PAGE_SIZE
to be a u64 (KVM selftests only support 64-bit builds, so glibc's 1UL is an unsigned
64-bit value).  But musl's definition is a signed int.  I.e. we can't solve the
64-bit unsigned vs. 32-bit signed by relying on libc.


64-bit unsigned vs. 32-bit signed is unlikely to cause problems in practice, and
in fact there are no problems in the current code base.  But I don't love creating
a potential pitfall for musl, especially since it's quite obviously not a common
libc for KVM selftests, i.e. could lead to very latent bugs.

E.g. building with this

diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h
index 488d516c4f6f..1b5e92debd20 100644
--- a/tools/testing/selftests/kvm/include/x86/processor.h
+++ b/tools/testing/selftests/kvm/include/x86/processor.h
@@ -368,7 +368,11 @@ static inline unsigned int x86_model(unsigned int eax)
 #define PHYSICAL_PAGE_MASK      GENMASK_ULL(51, 12)
 
 #define PAGE_SHIFT             12
+#define PAGE_SIZE              4096
+#ifndef PAGE_SIZE
 #define PAGE_SIZE              (1ULL << PAGE_SHIFT)
+#endif
+kvm_static_assert(PAGE_SIZE == (1ULL << PAGE_SHIFT));
 #define PAGE_MASK              (~(PAGE_SIZE-1) & PHYSICAL_PAGE_MASK)
 
 #define HUGEPAGE_SHIFT(x)      (PAGE_SHIFT + (((x) - 1) * 9))
diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
index ce3ac0fd6dfb..822119e8853a 100644
--- a/tools/testing/selftests/kvm/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/set_memory_region_test.c
@@ -608,9 +608,12 @@ static void test_mmio_during_vectoring(void)
 int main(int argc, char *argv[])
 {
 #ifdef __x86_64__
+       u64 total_size = PAGE_SIZE * 1048576;
        int i, loops;
        int j, disable_slot_zap_quirk = 0;
 
+       printf("Total size = %lx\n", total_size);
+
        if (kvm_check_cap(KVM_CAP_DISABLE_QUIRKS2) & KVM_X86_QUIRK_SLOT_ZAP_ALL)
                disable_slot_zap_quirk = 1;
        /*

Generates a warning:

  set_memory_region_test.c: In function ‘main’:
  set_memory_region_test.c:611:36: warning: integer overflow in expression of type ‘int’ results in ‘0’ [-Woverflow]
    611 |         u64 total_size = PAGE_SIZE * 1048576;
        |     

And yields:

  $ ./set_memory_region_test 
  Total size = 0

> > Please keep discussions on-list unless there's something that can't/shouldn't be
> > posted publicly, e.g. for confidentiality or security reasons.
> 
> Apologies, doing this for the first time! 

No worries, we've all been there :-)

Powered by blists - more mailing lists