Re: [PATCH] Define and use UINT64_C and INT64_C

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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