[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Define and use UINT64_C and INT64_C
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
|
