On Mon, Sep 9, 2024 at 11:38 AM Jan Beulich <jbeul...@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
