On 09.09.2024 15:41, Frediano Ziglio wrote:
> 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/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
