xen/arch/x86/include/asm/config.h | 6 +++--- xen/arch/x86/include/asm/mm.h | 12 ++++++------ xen/arch/x86/include/asm/x86_64/page.h | 4 ++-- xen/arch/x86/setup.c | 4 ++-- xen/common/efi/boot.c | 2 +- xen/crypto/vmac.c | 2 ++ xen/include/crypto/vmac.h | 4 ++++ xen/include/xen/const.h | 9 +++++++++ xen/include/xen/stdint.h | 2 ++ 9 files changed, 31 insertions(+), 14 deletions(-)
Makes code and constants work on both 32 and 64 bit.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
xen/arch/x86/include/asm/config.h | 6 +++---
xen/arch/x86/include/asm/mm.h | 12 ++++++------
xen/arch/x86/include/asm/x86_64/page.h | 4 ++--
xen/arch/x86/setup.c | 4 ++--
xen/common/efi/boot.c | 2 +-
xen/crypto/vmac.c | 2 ++
xen/include/crypto/vmac.h | 4 ++++
xen/include/xen/const.h | 9 +++++++++
xen/include/xen/stdint.h | 2 ++
9 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
index 1f828bfd52..7f91a478f5 100644
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -86,10 +86,10 @@
#include <xen/const.h>
#define PML4_ENTRY_BITS 39
-#define PML4_ENTRY_BYTES (_AC(1,UL) << PML4_ENTRY_BITS)
+#define PML4_ENTRY_BYTES (UINT64_C(1) << PML4_ENTRY_BITS)
#define PML4_ADDR(_slot) \
- (((_AC(_slot, UL) >> 8) * _AC(0xffff000000000000,UL)) | \
- (_AC(_slot, UL) << PML4_ENTRY_BITS))
+ (((UINT64_C(_slot) >> 8) * UINT64_C(0xffff000000000000)) | \
+ (UINT64_C(_slot) << PML4_ENTRY_BITS))
/*
* Memory layout:
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index b3853ae734..1077544c76 100644
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -20,7 +20,7 @@
#define PFN_ORDER(_pfn) ((_pfn)->v.free.order)
#define PG_shift(idx) (BITS_PER_LONG - (idx))
-#define PG_mask(x, idx) (x ## UL << PG_shift(idx))
+#define PG_mask(x, idx) (UINT64_C(x) << PG_shift(idx))
/* The following page types are MUTUALLY EXCLUSIVE. */
#define PGT_none PG_mask(0, 3) /* no special uses of this page */
@@ -59,7 +59,7 @@
/* Count of uses of this frame as its current type. */
#define PGT_count_width PG_shift(9)
-#define PGT_count_mask ((1UL<<PGT_count_width)-1)
+#define PGT_count_mask ((UINT64_C(1)<<PGT_count_width)-1)
/* Are the 'type mask' bits identical? */
#define PGT_type_equal(x, y) (!(((x) ^ (y)) & PGT_type_mask))
@@ -97,7 +97,7 @@
#else
#define PGC_count_width PG_shift(6)
#endif
-#define PGC_count_mask ((1UL<<PGC_count_width)-1)
+#define PGC_count_mask ((UINT64_C(1)<<PGC_count_width)-1)
/*
* Page needs to be scrubbed. Since this bit can only be set on a page that is
@@ -499,9 +499,9 @@ static inline int get_page_and_type(struct page_info *page,
*/
#undef machine_to_phys_mapping
#define machine_to_phys_mapping ((unsigned long *)RDWR_MPT_VIRT_START)
-#define INVALID_M2P_ENTRY (~0UL)
-#define VALID_M2P(_e) (!((_e) & (1UL<<(BITS_PER_LONG-1))))
-#define SHARED_M2P_ENTRY (~0UL - 1UL)
+#define INVALID_M2P_ENTRY (~UINT64_C(0))
+#define VALID_M2P(_e) (!((_e) & (UINT64_C(1)<<(BITS_PER_LONG-1))))
+#define SHARED_M2P_ENTRY (~UINT64_C(0) - UINT64_C(1))
#define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY)
/*
diff --git a/xen/arch/x86/include/asm/x86_64/page.h b/xen/arch/x86/include/asm/x86_64/page.h
index 201b79f99e..dbf2fd061c 100644
--- a/xen/arch/x86/include/asm/x86_64/page.h
+++ b/xen/arch/x86/include/asm/x86_64/page.h
@@ -4,8 +4,8 @@
#define __XEN_VIRT_START XEN_VIRT_START
-#define VADDR_TOP_BIT (1UL << (VADDR_BITS - 1))
-#define CANONICAL_MASK (~0UL & ~VADDR_MASK)
+#define VADDR_TOP_BIT (UINT64_C(1) << (VADDR_BITS - 1))
+#define CANONICAL_MASK (~UINT64_C(0) & ~VADDR_MASK)
#define is_canonical_address(x) (((long)(x) >> 47) == ((long)(x) >> 63))
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a6e77c9ed9..6330549e8f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1384,9 +1384,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
}
if ( e > min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
- 1UL << (PAGE_SHIFT + 32)) )
+ UINT64_C(1) << (PAGE_SHIFT + 32)) )
e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
- 1UL << (PAGE_SHIFT + 32));
+ UINT64_C(1) << (PAGE_SHIFT + 32));
#define reloc_size ((__pa(__2M_rwdata_end) + mask) & ~mask)
/* Is the region suitable for relocating Xen? */
if ( !xen_phys_start && e <= limit )
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index efbec00af9..185a471a04 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -770,7 +770,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
what = what ?: L"Seek";
else
{
- file->addr = min(1UL << (32 + PAGE_SHIFT),
+ file->addr = min(UINT64_C(1) << (32 + PAGE_SHIFT),
HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
PFN_UP(size), &file->addr);
diff --git a/xen/crypto/vmac.c b/xen/crypto/vmac.c
index 294dd16a52..d831114f9c 100644
--- a/xen/crypto/vmac.c
+++ b/xen/crypto/vmac.c
@@ -11,7 +11,9 @@
#include <xen/types.h>
#include <xen/lib.h>
#include <crypto/vmac.h>
+#ifndef UINT64_C
#define UINT64_C(x) x##ULL
+#endif
/* end for Xen */
/* Enable code tuned for 64-bit registers; otherwise tuned for 32-bit */
diff --git a/xen/include/crypto/vmac.h b/xen/include/crypto/vmac.h
index 457f3f5dd6..ab8c644e70 100644
--- a/xen/include/crypto/vmac.h
+++ b/xen/include/crypto/vmac.h
@@ -51,12 +51,16 @@
#elif (_MSC_VER) /* Microsoft C does not have stdint.h */
typedef unsigned __int32 uint32_t;
typedef unsigned __int64 uint64_t;
+#ifndef UINT64_C
#define UINT64_C(v) v ## UI64
+#endif
#else /* Guess sensibly - may need adaptation */
typedef unsigned int uint32_t;
typedef unsigned long long uint64_t;
+#ifndef UINT64_C
#define UINT64_C(v) v ## ULL
#endif
+#endif
/* --------------------------------------------------------------------------
* This implementation supports two free AES implementations: OpenSSL's and
diff --git a/xen/include/xen/const.h b/xen/include/xen/const.h
index baf28ef144..e2fb0fd3a4 100644
--- a/xen/include/xen/const.h
+++ b/xen/include/xen/const.h
@@ -15,10 +15,19 @@
#ifdef __ASSEMBLY__
#define _AC(X,Y) X
#define _AT(T,X) X
+#define UINT64_C(X) X
+#define INT64_C(X) X
#else
#define __AC(X,Y) (X##Y)
#define _AC(X,Y) __AC(X,Y)
#define _AT(T,X) ((T)(X))
+#if __SIZEOF_LONG__ >= 8
+#define UINT64_C(X) X ## UL
+#define INT64_C(X) X ## L
+#else
+#define UINT64_C(X) X ## ULL
+#define INT64_C(X) X ## LL
+#endif
#endif
#define BIT(pos, sfx) (_AC(1, sfx) << (pos))
diff --git a/xen/include/xen/stdint.h b/xen/include/xen/stdint.h
index a40165c6ae..e80756c4a4 100644
--- a/xen/include/xen/stdint.h
+++ b/xen/include/xen/stdint.h
@@ -30,4 +30,6 @@ typedef __UINT64_TYPE__ uint64_t;
#endif
+#include <xen/const.h>
+
#endif /* __XEN_STDINT_H__ */
--
2.34.1On 09.09.2024 12:08, Frediano Ziglio wrote: > --- a/xen/arch/x86/include/asm/config.h > +++ b/xen/arch/x86/include/asm/config.h > @@ -86,10 +86,10 @@ > #include <xen/const.h> > > #define PML4_ENTRY_BITS 39 > -#define PML4_ENTRY_BYTES (_AC(1,UL) << PML4_ENTRY_BITS) > +#define PML4_ENTRY_BYTES (UINT64_C(1) << PML4_ENTRY_BITS) > #define PML4_ADDR(_slot) \ > - (((_AC(_slot, UL) >> 8) * _AC(0xffff000000000000,UL)) | \ > - (_AC(_slot, UL) << PML4_ENTRY_BITS)) > + (((UINT64_C(_slot) >> 8) * UINT64_C(0xffff000000000000)) | \ > + (UINT64_C(_slot) << PML4_ENTRY_BITS)) > > /* > * Memory layout: > --- a/xen/arch/x86/include/asm/mm.h > +++ b/xen/arch/x86/include/asm/mm.h > @@ -20,7 +20,7 @@ > #define PFN_ORDER(_pfn) ((_pfn)->v.free.order) > > #define PG_shift(idx) (BITS_PER_LONG - (idx)) > -#define PG_mask(x, idx) (x ## UL << PG_shift(idx)) > +#define PG_mask(x, idx) (UINT64_C(x) << PG_shift(idx)) > > /* The following page types are MUTUALLY EXCLUSIVE. */ > #define PGT_none PG_mask(0, 3) /* no special uses of this page */ > @@ -59,7 +59,7 @@ > > /* Count of uses of this frame as its current type. */ > #define PGT_count_width PG_shift(9) > -#define PGT_count_mask ((1UL<<PGT_count_width)-1) > +#define PGT_count_mask ((UINT64_C(1)<<PGT_count_width)-1) > > /* Are the 'type mask' bits identical? */ > #define PGT_type_equal(x, y) (!(((x) ^ (y)) & PGT_type_mask)) > @@ -97,7 +97,7 @@ > #else > #define PGC_count_width PG_shift(6) > #endif > -#define PGC_count_mask ((1UL<<PGC_count_width)-1) > +#define PGC_count_mask ((UINT64_C(1)<<PGC_count_width)-1) > > /* > * Page needs to be scrubbed. Since this bit can only be set on a page that is > @@ -499,9 +499,9 @@ static inline int get_page_and_type(struct page_info *page, > */ > #undef machine_to_phys_mapping > #define machine_to_phys_mapping ((unsigned long *)RDWR_MPT_VIRT_START) > -#define INVALID_M2P_ENTRY (~0UL) > -#define VALID_M2P(_e) (!((_e) & (1UL<<(BITS_PER_LONG-1)))) > -#define SHARED_M2P_ENTRY (~0UL - 1UL) > +#define INVALID_M2P_ENTRY (~UINT64_C(0)) > +#define VALID_M2P(_e) (!((_e) & (UINT64_C(1)<<(BITS_PER_LONG-1)))) > +#define SHARED_M2P_ENTRY (~UINT64_C(0) - UINT64_C(1)) > #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) > > /* > --- a/xen/arch/x86/include/asm/x86_64/page.h > +++ b/xen/arch/x86/include/asm/x86_64/page.h > @@ -4,8 +4,8 @@ > > #define __XEN_VIRT_START XEN_VIRT_START > > -#define VADDR_TOP_BIT (1UL << (VADDR_BITS - 1)) > -#define CANONICAL_MASK (~0UL & ~VADDR_MASK) > +#define VADDR_TOP_BIT (UINT64_C(1) << (VADDR_BITS - 1)) > +#define CANONICAL_MASK (~UINT64_C(0) & ~VADDR_MASK) > > #define is_canonical_address(x) (((long)(x) >> 47) == ((long)(x) >> 63)) > > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1384,9 +1384,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > } > > if ( e > min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START, > - 1UL << (PAGE_SHIFT + 32)) ) > + UINT64_C(1) << (PAGE_SHIFT + 32)) ) > e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START, > - 1UL << (PAGE_SHIFT + 32)); > + UINT64_C(1) << (PAGE_SHIFT + 32)); I disagree - we're dealing with virtual addresses here, which better wouldn't use fixed-width quantities. While not always virtual addresses, I similarly disagree for most or all I've left in context further up: If the underlying type to deal with is unsigned long, constants should match. > --- a/xen/crypto/vmac.c > +++ b/xen/crypto/vmac.c > @@ -11,7 +11,9 @@ > #include <xen/types.h> > #include <xen/lib.h> > #include <crypto/vmac.h> > +#ifndef UINT64_C > #define UINT64_C(x) x##ULL > +#endif > /* end for Xen */ Here the #define should probably just be dropped? > --- a/xen/include/crypto/vmac.h > +++ b/xen/include/crypto/vmac.h > @@ -51,12 +51,16 @@ > #elif (_MSC_VER) /* Microsoft C does not have stdint.h */ > typedef unsigned __int32 uint32_t; > typedef unsigned __int64 uint64_t; > +#ifndef UINT64_C > #define UINT64_C(v) v ## UI64 > +#endif This part surely isn't needed? > --- a/xen/include/xen/const.h > +++ b/xen/include/xen/const.h > @@ -15,10 +15,19 @@ > #ifdef __ASSEMBLY__ > #define _AC(X,Y) X > #define _AT(T,X) X > +#define UINT64_C(X) X > +#define INT64_C(X) X > #else > #define __AC(X,Y) (X##Y) > #define _AC(X,Y) __AC(X,Y) > #define _AT(T,X) ((T)(X)) > +#if __SIZEOF_LONG__ >= 8 This is available with gcc 4.3 and newer, yet for now our docs still specify 4.1.2 as the baseline. I'm also unconvinced of the >= - we're talking of fixed-width types here, so imo it needs to be == and then also ... > +#define UINT64_C(X) X ## UL > +#define INT64_C(X) X ## L > +#else #elif __SIZEOF_LONG_LONG__ == 8 here. > +#define UINT64_C(X) X ## ULL > +#define INT64_C(X) X ## LL > +#endif > #endif Finally if we introduce these, imo we should introduce the other UINT<n>_C() as well, and in a header named after the one mandated by the C library spec. > --- a/xen/include/xen/stdint.h > +++ b/xen/include/xen/stdint.h > @@ -30,4 +30,6 @@ typedef __UINT64_TYPE__ uint64_t; > > #endif > > +#include <xen/const.h> Why's this needed? Jan
On Mon, Sep 9, 2024 at 11:38 AM Jan Beulich <jbeulich@suse.com> wrote: > On 09.09.2024 12:08, Frediano Ziglio wrote: > > --- a/xen/arch/x86/include/asm/config.h > > +++ b/xen/arch/x86/include/asm/config.h > > @@ -86,10 +86,10 @@ > > #include <xen/const.h> > > > > #define PML4_ENTRY_BITS 39 > > -#define PML4_ENTRY_BYTES (_AC(1,UL) << PML4_ENTRY_BITS) > > +#define PML4_ENTRY_BYTES (UINT64_C(1) << PML4_ENTRY_BITS) > > #define PML4_ADDR(_slot) \ > > - (((_AC(_slot, UL) >> 8) * _AC(0xffff000000000000,UL)) | \ > > - (_AC(_slot, UL) << PML4_ENTRY_BITS)) > > + (((UINT64_C(_slot) >> 8) * UINT64_C(0xffff000000000000)) | \ > > + (UINT64_C(_slot) << PML4_ENTRY_BITS)) > > > > /* > > * Memory layout: > > --- a/xen/arch/x86/include/asm/mm.h > > +++ b/xen/arch/x86/include/asm/mm.h > > @@ -20,7 +20,7 @@ > > #define PFN_ORDER(_pfn) ((_pfn)->v.free.order) > > > > #define PG_shift(idx) (BITS_PER_LONG - (idx)) > > -#define PG_mask(x, idx) (x ## UL << PG_shift(idx)) > > +#define PG_mask(x, idx) (UINT64_C(x) << PG_shift(idx)) > > > > /* The following page types are MUTUALLY EXCLUSIVE. */ > > #define PGT_none PG_mask(0, 3) /* no special uses of this > page */ > > @@ -59,7 +59,7 @@ > > > > /* Count of uses of this frame as its current type. */ > > #define PGT_count_width PG_shift(9) > > -#define PGT_count_mask ((1UL<<PGT_count_width)-1) > > +#define PGT_count_mask ((UINT64_C(1)<<PGT_count_width)-1) > > > > /* Are the 'type mask' bits identical? */ > > #define PGT_type_equal(x, y) (!(((x) ^ (y)) & PGT_type_mask)) > > @@ -97,7 +97,7 @@ > > #else > > #define PGC_count_width PG_shift(6) > > #endif > > -#define PGC_count_mask ((1UL<<PGC_count_width)-1) > > +#define PGC_count_mask ((UINT64_C(1)<<PGC_count_width)-1) > > > > /* > > * Page needs to be scrubbed. Since this bit can only be set on a page > that is > > @@ -499,9 +499,9 @@ static inline int get_page_and_type(struct page_info > *page, > > */ > > #undef machine_to_phys_mapping > > #define machine_to_phys_mapping ((unsigned long *)RDWR_MPT_VIRT_START) > > -#define INVALID_M2P_ENTRY (~0UL) > > -#define VALID_M2P(_e) (!((_e) & (1UL<<(BITS_PER_LONG-1)))) > > -#define SHARED_M2P_ENTRY (~0UL - 1UL) > > +#define INVALID_M2P_ENTRY (~UINT64_C(0)) > > +#define VALID_M2P(_e) (!((_e) & > (UINT64_C(1)<<(BITS_PER_LONG-1)))) > > +#define SHARED_M2P_ENTRY (~UINT64_C(0) - UINT64_C(1)) > > #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) > > > > /* > > --- a/xen/arch/x86/include/asm/x86_64/page.h > > +++ b/xen/arch/x86/include/asm/x86_64/page.h > > @@ -4,8 +4,8 @@ > > > > #define __XEN_VIRT_START XEN_VIRT_START > > > > -#define VADDR_TOP_BIT (1UL << (VADDR_BITS - 1)) > > -#define CANONICAL_MASK (~0UL & ~VADDR_MASK) > > +#define VADDR_TOP_BIT (UINT64_C(1) << (VADDR_BITS - 1)) > > +#define CANONICAL_MASK (~UINT64_C(0) & ~VADDR_MASK) > > > > #define is_canonical_address(x) (((long)(x) >> 47) == ((long)(x) >> 63)) > > > > --- a/xen/arch/x86/setup.c > > +++ b/xen/arch/x86/setup.c > > @@ -1384,9 +1384,9 @@ void asmlinkage __init noreturn > __start_xen(unsigned long mbi_p) > > } > > > > if ( e > min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START, > > - 1UL << (PAGE_SHIFT + 32)) ) > > + UINT64_C(1) << (PAGE_SHIFT + 32)) ) > > e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START, > > - 1UL << (PAGE_SHIFT + 32)); > > + UINT64_C(1) << (PAGE_SHIFT + 32)); > > I disagree - we're dealing with virtual addresses here, which better > wouldn't use fixed-width quantities. > > I suppose you are suggesting type-based macros instead of fixed-type macros, so something like PADDR_C and VADDR_C. That makes sense. > While not always virtual addresses, I similarly disagree for most or all > I've left in context further up: If the underlying type to deal with is > unsigned long, constants should match. > > Sure, in this case the underlying type if used as 32 bit cannot be unsigned long but they should be unsigned long long (or any 64 bit type). > --- a/xen/crypto/vmac.c > > +++ b/xen/crypto/vmac.c > > @@ -11,7 +11,9 @@ > > #include <xen/types.h> > > #include <xen/lib.h> > > #include <crypto/vmac.h> > > +#ifndef UINT64_C > > #define UINT64_C(x) x##ULL > > +#endif > > /* end for Xen */ > > Here the #define should probably just be dropped? > > If we go for newer type-base macros, we won't need to change here. > > --- a/xen/include/crypto/vmac.h > > +++ b/xen/include/crypto/vmac.h > > @@ -51,12 +51,16 @@ > > #elif (_MSC_VER) /* Microsoft C does not have > stdint.h */ > > typedef unsigned __int32 uint32_t; > > typedef unsigned __int64 uint64_t; > > +#ifndef UINT64_C > > #define UINT64_C(v) v ## UI64 > > +#endif > > This part surely isn't needed? > > Indeed :-) > > --- a/xen/include/xen/const.h > > +++ b/xen/include/xen/const.h > > @@ -15,10 +15,19 @@ > > #ifdef __ASSEMBLY__ > > #define _AC(X,Y) X > > #define _AT(T,X) X > > +#define UINT64_C(X) X > > +#define INT64_C(X) X > > #else > > #define __AC(X,Y) (X##Y) > > #define _AC(X,Y) __AC(X,Y) > > #define _AT(T,X) ((T)(X)) > > +#if __SIZEOF_LONG__ >= 8 > > This is available with gcc 4.3 and newer, yet for now our docs still > specify 4.1.2 as the baseline. > > Do we have some sort of configure generated macro for this? > I'm also unconvinced of the >= - we're talking of fixed-width types here, > so imo it needs to be == and then also ... > > > +#define UINT64_C(X) X ## UL > > +#define INT64_C(X) X ## L > > +#else > > #elif __SIZEOF_LONG_LONG__ == 8 > > here. > > > +#define UINT64_C(X) X ## ULL > > +#define INT64_C(X) X ## LL > > +#endif > > #endif > > Finally if we introduce these, imo we should introduce the other > UINT<n>_C() > as well, and in a header named after the one mandated by the C library > spec. > > > --- a/xen/include/xen/stdint.h > > +++ b/xen/include/xen/stdint.h > > @@ -30,4 +30,6 @@ typedef __UINT64_TYPE__ uint64_t; > > > > #endif > > > > +#include <xen/const.h> > > Why's this needed? > > Not strictly needed, but in the standard headers they are usually defined including stdint.h. > Jan > Frediano
On 09.09.2024 15:41, Frediano Ziglio wrote:
> On Mon, Sep 9, 2024 at 11:38 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 09.09.2024 12:08, Frediano Ziglio wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1384,9 +1384,9 @@ void asmlinkage __init noreturn
>> __start_xen(unsigned long mbi_p)
>>> }
>>>
>>> if ( e > min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
>>> - 1UL << (PAGE_SHIFT + 32)) )
>>> + UINT64_C(1) << (PAGE_SHIFT + 32)) )
>>> e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
>>> - 1UL << (PAGE_SHIFT + 32));
>>> + UINT64_C(1) << (PAGE_SHIFT + 32));
>>
>> I disagree - we're dealing with virtual addresses here, which better
>> wouldn't use fixed-width quantities.
>>
> I suppose you are suggesting type-based macros instead of fixed-type
> macros, so something like PADDR_C and VADDR_C.
> That makes sense.
No, I'm suggesting to somply leave this code alone. On x86 we have no vaddr_t,
and hence I see no reason to have VADDR_C().
>> While not always virtual addresses, I similarly disagree for most or all
>> I've left in context further up: If the underlying type to deal with is
>> unsigned long, constants should match.
>>
> Sure, in this case the underlying type if used as 32 bit cannot be unsigned
> long but they should be unsigned long long (or any 64 bit type).
My primary request here is: Code that won't be built as 32-bit doesn't need
changing if it's not explicitly using {,u}int64_t-type variables /
expressions.
>> --- a/xen/crypto/vmac.c
>>> +++ b/xen/crypto/vmac.c
>>> @@ -11,7 +11,9 @@
>>> #include <xen/types.h>
>>> #include <xen/lib.h>
>>> #include <crypto/vmac.h>
>>> +#ifndef UINT64_C
>>> #define UINT64_C(x) x##ULL
>>> +#endif
>>> /* end for Xen */
>>
>> Here the #define should probably just be dropped?
>>
>>
> If we go for newer type-base macros, we won't need to change here.
I'm afraid I don't understand this reply.
>>> --- a/xen/include/xen/const.h
>>> +++ b/xen/include/xen/const.h
>>> @@ -15,10 +15,19 @@
>>> #ifdef __ASSEMBLY__
>>> #define _AC(X,Y) X
>>> #define _AT(T,X) X
>>> +#define UINT64_C(X) X
>>> +#define INT64_C(X) X
>>> #else
>>> #define __AC(X,Y) (X##Y)
>>> #define _AC(X,Y) __AC(X,Y)
>>> #define _AT(T,X) ((T)(X))
>>> +#if __SIZEOF_LONG__ >= 8
>>
>> This is available with gcc 4.3 and newer, yet for now our docs still
>> specify 4.1.2 as the baseline.
>>
> Do we have some sort of configure generated macro for this?
I don#t think we do. And I also don't think we need one - we have
BITS_PER_LONG, which ought to be sufficient here.
>> I'm also unconvinced of the >= - we're talking of fixed-width types here,
>> so imo it needs to be == and then also ...
>>
>>> +#define UINT64_C(X) X ## UL
>>> +#define INT64_C(X) X ## L
>>> +#else
>>
>> #elif __SIZEOF_LONG_LONG__ == 8
>>
>> here.
>>
>>> +#define UINT64_C(X) X ## ULL
>>> +#define INT64_C(X) X ## LL
>>> +#endif
>>> #endif
>>
>> Finally if we introduce these, imo we should introduce the other
>> UINT<n>_C()
>> as well, and in a header named after the one mandated by the C library
>> spec.
I'm sorry, I was actually wrong here (alluding to inttypes.h), so ...
>>> --- a/xen/include/xen/stdint.h
>>> +++ b/xen/include/xen/stdint.h
>>> @@ -30,4 +30,6 @@ typedef __UINT64_TYPE__ uint64_t;
>>>
>>> #endif
>>>
>>> +#include <xen/const.h>
>>
>> Why's this needed?
>>
> Not strictly needed, but in the standard headers they are usually defined
> including stdint.h.
... yes, but imo the definitions then would better live here in the first
place.
Jan
On 09/09/2024 11:38 am, Jan Beulich wrote: > On 09.09.2024 12:08, Frediano Ziglio wrote: >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1384,9 +1384,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> } >> >> if ( e > min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START, >> - 1UL << (PAGE_SHIFT + 32)) ) >> + UINT64_C(1) << (PAGE_SHIFT + 32)) ) >> e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START, >> - 1UL << (PAGE_SHIFT + 32)); >> + UINT64_C(1) << (PAGE_SHIFT + 32)); > I disagree - we're dealing with virtual addresses here, which better > wouldn't use fixed-width quantities. > > While not always virtual addresses, I similarly disagree for most or all > I've left in context further up: If the underlying type to deal with is > unsigned long, constants should match. This is the same problem I ran into with fixmap mappings. GB() and friends used in config.h are ULL constants, and promote the underlying variables from UL. 64bit mostly copes (give or take some printk formatting), but 32bit fails to compile. ~Andrew
On 09.09.2024 13:09, Andrew Cooper wrote: > On 09/09/2024 11:38 am, Jan Beulich wrote: >> On 09.09.2024 12:08, Frediano Ziglio wrote: >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -1384,9 +1384,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >>> } >>> >>> if ( e > min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START, >>> - 1UL << (PAGE_SHIFT + 32)) ) >>> + UINT64_C(1) << (PAGE_SHIFT + 32)) ) >>> e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START, >>> - 1UL << (PAGE_SHIFT + 32)); >>> + UINT64_C(1) << (PAGE_SHIFT + 32)); >> I disagree - we're dealing with virtual addresses here, which better >> wouldn't use fixed-width quantities. >> >> While not always virtual addresses, I similarly disagree for most or all >> I've left in context further up: If the underlying type to deal with is >> unsigned long, constants should match. > > This is the same problem I ran into with fixmap mappings. > > GB() and friends used in config.h are ULL constants, and promote the > underlying variables from UL. > > 64bit mostly copes (give or take some printk formatting), but 32bit > fails to compile. Yet the code above is never built as 32-bit. Jan
Macros are defined to avoid type mismatch in format strings
but also to unify format amongst code.
In the meantime expands to 9 hexadecimal digits.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
xen/common/grant_table.c | 6 +++---
xen/include/xen/mm-frame.h | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index ab36f45ded..775cd7e065 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
if ( rc )
{
gprintk(XENLOG_ERR,
- "Could not remove status frame %u (GFN %#lx) from P2M\n",
+ "Could not remove status frame %u (GFN %"PRI_gfn") from P2M\n",
i, gfn_x(gfn));
domain_crash(d);
return rc;
@@ -1863,7 +1863,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
if ( paging_mode_translate(d) )
{
gprintk(XENLOG_ERR,
- "Wrong page state %#lx of status frame %u (GFN %#lx)\n",
+ "Wrong page state %#lx of status frame %u (GFN %"PRI_gfn")\n",
pg->count_info, i, gfn_x(gfn));
domain_crash(d);
}
@@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain *d)
if ( nr_active <= WARN_GRANT_MAX )
printk(XENLOG_G_DEBUG "d%d has active grant %x ("
#ifndef NDEBUG
- "GFN %lx, "
+ "GFN %"PRI_gfn", "
#endif
"MFN: %#"PRI_mfn")\n",
d->domain_id, ref,
diff --git a/xen/include/xen/mm-frame.h b/xen/include/xen/mm-frame.h
index d973aec901..6f9fcc2a6a 100644
--- a/xen/include/xen/mm-frame.h
+++ b/xen/include/xen/mm-frame.h
@@ -5,7 +5,7 @@
#include <xen/typesafe.h>
TYPE_SAFE(unsigned long, mfn);
-#define PRI_mfn "05lx"
+#define PRI_mfn "09lx"
#define INVALID_MFN_RAW (~0UL)
#define INVALID_MFN _mfn(INVALID_MFN_RAW)
/*
@@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y)
}
TYPE_SAFE(unsigned long, gfn);
-#define PRI_gfn "05lx"
+#define PRI_gfn "09lx"
#define INVALID_GFN_RAW (~0UL)
#define INVALID_GFN _gfn(INVALID_GFN_RAW)
/*
--
2.34.1On 09.09.2024 12:08, Frediano Ziglio wrote:
> Macros are defined to avoid type mismatch in format strings
> but also to unify format amongst code.
I'm certainly fine with this part.
> In the meantime expands to 9 hexadecimal digits.
What makes 9 special? What will the extra padding zeroes buy us?
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
> xen/common/grant_table.c | 6 +++---
> xen/include/xen/mm-frame.h | 4 ++--
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index ab36f45ded..775cd7e065 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
> if ( rc )
> {
> gprintk(XENLOG_ERR,
> - "Could not remove status frame %u (GFN %#lx) from P2M\n",
> + "Could not remove status frame %u (GFN %"PRI_gfn") from P2M\n",
The lost # means the number won't identify itself as hex anymore. Rather
than ...
> @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain *d)
> if ( nr_active <= WARN_GRANT_MAX )
> printk(XENLOG_G_DEBUG "d%d has active grant %x ("
> #ifndef NDEBUG
> - "GFN %lx, "
> + "GFN %"PRI_gfn", "
> #endif
> "MFN: %#"PRI_mfn")\n",
(note this for below)
> --- a/xen/include/xen/mm-frame.h
> +++ b/xen/include/xen/mm-frame.h
> @@ -5,7 +5,7 @@
> #include <xen/typesafe.h>
>
> TYPE_SAFE(unsigned long, mfn);
> -#define PRI_mfn "05lx"
> +#define PRI_mfn "09lx"
> #define INVALID_MFN_RAW (~0UL)
> #define INVALID_MFN _mfn(INVALID_MFN_RAW)
> /*
> @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y)
> }
>
> TYPE_SAFE(unsigned long, gfn);
> -#define PRI_gfn "05lx"
> +#define PRI_gfn "09lx"
... moving to 09 (twice) here, how about we move to #? Requiring, of course,
to drop already-questionable hashes like the one pointed out in the middle.
Jan
On Mon, Sep 9, 2024 at 11:45 AM Jan Beulich <jbeulich@suse.com> wrote:
> On 09.09.2024 12:08, Frediano Ziglio wrote:
> > Macros are defined to avoid type mismatch in format strings
> > but also to unify format amongst code.
>
> I'm certainly fine with this part.
>
> > In the meantime expands to 9 hexadecimal digits.
>
> What makes 9 special? What will the extra padding zeroes buy us?
>
>
I think either we want kind of fixed size or dynamic. 9 == (48 - 12) / 4,
although possibly you would prefer 13 == (64 - 12) / 4.
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> > ---
> > xen/common/grant_table.c | 6 +++---
> > xen/include/xen/mm-frame.h | 4 ++--
> > 2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index ab36f45ded..775cd7e065 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d,
> struct grant_table *gt)
> > if ( rc )
> > {
> > gprintk(XENLOG_ERR,
> > - "Could not remove status frame %u (GFN %#lx)
> from P2M\n",
> > + "Could not remove status frame %u (GFN
> %"PRI_gfn") from P2M\n",
>
> The lost # means the number won't identify itself as hex anymore. Rather
> than ...
>
> > @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain
> *d)
> > if ( nr_active <= WARN_GRANT_MAX )
> > printk(XENLOG_G_DEBUG "d%d has active grant %x ("
> > #ifndef NDEBUG
> > - "GFN %lx, "
> > + "GFN %"PRI_gfn", "
> > #endif
> > "MFN: %#"PRI_mfn")\n",
>
> (note this for below)
>
> > --- a/xen/include/xen/mm-frame.h
> > +++ b/xen/include/xen/mm-frame.h
> > @@ -5,7 +5,7 @@
> > #include <xen/typesafe.h>
> >
> > TYPE_SAFE(unsigned long, mfn);
> > -#define PRI_mfn "05lx"
> > +#define PRI_mfn "09lx"
> > #define INVALID_MFN_RAW (~0UL)
> > #define INVALID_MFN _mfn(INVALID_MFN_RAW)
> > /*
> > @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y)
> > }
> >
> > TYPE_SAFE(unsigned long, gfn);
> > -#define PRI_gfn "05lx"
> > +#define PRI_gfn "09lx"
>
> ... moving to 09 (twice) here, how about we move to #? Requiring, of
> course,
> to drop already-questionable hashes like the one pointed out in the middle.
>
>
I suppose you are suggesting getting rid of the padding entirely and move
to prefix, like "%#lx".
> Jan
>
I can do it
Frediano
On 09.09.2024 14:53, Frediano Ziglio wrote:
> On Mon, Sep 9, 2024 at 11:45 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 09.09.2024 12:08, Frediano Ziglio wrote:
>>> Macros are defined to avoid type mismatch in format strings
>>> but also to unify format amongst code.
>>
>> I'm certainly fine with this part.
>>
>>> In the meantime expands to 9 hexadecimal digits.
>>
>> What makes 9 special? What will the extra padding zeroes buy us?
>>
>>
> I think either we want kind of fixed size or dynamic. 9 == (48 - 12) / 4,
> although possibly you would prefer 13 == (64 - 12) / 4.
64 is too much for x86; it would want to be 52 there. The way it is right
now this is (imo deliberately) not arch-specific, though.
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d,
>> struct grant_table *gt)
>>> if ( rc )
>>> {
>>> gprintk(XENLOG_ERR,
>>> - "Could not remove status frame %u (GFN %#lx)
>> from P2M\n",
>>> + "Could not remove status frame %u (GFN
>> %"PRI_gfn") from P2M\n",
>>
>> The lost # means the number won't identify itself as hex anymore. Rather
>> than ...
>>
>>> @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain
>> *d)
>>> if ( nr_active <= WARN_GRANT_MAX )
>>> printk(XENLOG_G_DEBUG "d%d has active grant %x ("
>>> #ifndef NDEBUG
>>> - "GFN %lx, "
>>> + "GFN %"PRI_gfn", "
>>> #endif
>>> "MFN: %#"PRI_mfn")\n",
>>
>> (note this for below)
>>
>>> --- a/xen/include/xen/mm-frame.h
>>> +++ b/xen/include/xen/mm-frame.h
>>> @@ -5,7 +5,7 @@
>>> #include <xen/typesafe.h>
>>>
>>> TYPE_SAFE(unsigned long, mfn);
>>> -#define PRI_mfn "05lx"
>>> +#define PRI_mfn "09lx"
>>> #define INVALID_MFN_RAW (~0UL)
>>> #define INVALID_MFN _mfn(INVALID_MFN_RAW)
>>> /*
>>> @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y)
>>> }
>>>
>>> TYPE_SAFE(unsigned long, gfn);
>>> -#define PRI_gfn "05lx"
>>> +#define PRI_gfn "09lx"
>>
>> ... moving to 09 (twice) here, how about we move to #? Requiring, of
>> course,
>> to drop already-questionable hashes like the one pointed out in the middle.
>>
> I suppose you are suggesting getting rid of the padding entirely and move
> to prefix, like "%#lx".
Yes, i.e.
#define PRI_mfn "#lx"
Then again I don't really know why "05lx" was chosen originally.
Jan
On Mon, Sep 9, 2024 at 1:58 PM Jan Beulich <jbeulich@suse.com> wrote:
> On 09.09.2024 14:53, Frediano Ziglio wrote:
> > On Mon, Sep 9, 2024 at 11:45 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 09.09.2024 12:08, Frediano Ziglio wrote:
> >>> Macros are defined to avoid type mismatch in format strings
> >>> but also to unify format amongst code.
> >>
> >> I'm certainly fine with this part.
> >>
> >>> In the meantime expands to 9 hexadecimal digits.
> >>
> >> What makes 9 special? What will the extra padding zeroes buy us?
> >>
> >>
> > I think either we want kind of fixed size or dynamic. 9 == (48 - 12) / 4,
> > although possibly you would prefer 13 == (64 - 12) / 4.
>
> 64 is too much for x86; it would want to be 52 there. The way it is right
> now this is (imo deliberately) not arch-specific, though.
>
>
Yes, but still given the canonic form of x64 you would need to use 13
digits to have all the same size.
> >>> --- a/xen/common/grant_table.c
> >>> +++ b/xen/common/grant_table.c
> >>> @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d,
> >> struct grant_table *gt)
> >>> if ( rc )
> >>> {
> >>> gprintk(XENLOG_ERR,
> >>> - "Could not remove status frame %u (GFN %#lx)
> >> from P2M\n",
> >>> + "Could not remove status frame %u (GFN
> >> %"PRI_gfn") from P2M\n",
> >>
> >> The lost # means the number won't identify itself as hex anymore. Rather
> >> than ...
> >>
> >>> @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain
> >> *d)
> >>> if ( nr_active <= WARN_GRANT_MAX )
> >>> printk(XENLOG_G_DEBUG "d%d has active grant %x ("
> >>> #ifndef NDEBUG
> >>> - "GFN %lx, "
> >>> + "GFN %"PRI_gfn", "
> >>> #endif
> >>> "MFN: %#"PRI_mfn")\n",
> >>
> >> (note this for below)
> >>
> >>> --- a/xen/include/xen/mm-frame.h
> >>> +++ b/xen/include/xen/mm-frame.h
> >>> @@ -5,7 +5,7 @@
> >>> #include <xen/typesafe.h>
> >>>
> >>> TYPE_SAFE(unsigned long, mfn);
> >>> -#define PRI_mfn "05lx"
> >>> +#define PRI_mfn "09lx"
> >>> #define INVALID_MFN_RAW (~0UL)
> >>> #define INVALID_MFN _mfn(INVALID_MFN_RAW)
> >>> /*
> >>> @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y)
> >>> }
> >>>
> >>> TYPE_SAFE(unsigned long, gfn);
> >>> -#define PRI_gfn "05lx"
> >>> +#define PRI_gfn "09lx"
> >>
> >> ... moving to 09 (twice) here, how about we move to #? Requiring, of
> >> course,
> >> to drop already-questionable hashes like the one pointed out in the
> middle.
> >>
> > I suppose you are suggesting getting rid of the padding entirely and move
> > to prefix, like "%#lx".
>
> Yes, i.e.
>
> #define PRI_mfn "#lx"
>
> Surely more portable amongst different platforms.
> Then again I don't really know why "05lx" was chosen originally.
>
>
I assume x86 without PAE, 32 bit, so 5 == (32 - 12) / 4.
> Jan
>
Sent updated one
Frediano
© 2016 - 2024 Red Hat, Inc.