On 10/23/25 13:23, Jan Beulich wrote:
> On 23.10.2025 12:00, Dmytro Prokopchuk1 wrote:
>> On 10/17/25 10:09, Nicola Vetrini wrote:
>>> On 2025-10-15 08:20, Jan Beulich wrote:
>>>> On 14.10.2025 18:16, Dmytro Prokopchuk1 wrote:
>>>>> --- a/xen/common/version.c
>>>>> +++ b/xen/common/version.c
>>>>> @@ -217,6 +217,20 @@ void __init xen_build_init(void)
>>>>> #endif /* CONFIG_X86 */
>>>>> }
>>>>> #endif /* BUILD_ID */
>>>>> +
>>>>> +#if defined(__i386__) || defined(__x86_64__) || defined(__arm__) ||
>>>>> defined(__aarch64__)
>>>>
>>>> Why __i386__? Also (nit): Line too long.
>>
>> Well, I copied this line from Xen codebase,
>> but yeah, __i386__ is outdated now.
>> I'll remove it.
>>
>>>>
>>>> And why this restriction without any comment here or ...
>>>>
>>>>> +static void __init __maybe_unused build_assertions(void)
>>>>> +{
>>>>> + /*
>>>>> + * To confirm conversion compatibility between unsigned long,
>>>>> (void *)
>>>>> + * and function pointers for X86 and ARM architectures only.
>>>>
>>>> ... explanation here? More generally - how would people know to update
>>>> the condition if another port was to be certified?
>>>>
>>>> Finally, with the v3 addition here, is Nicola's R-b really still
>>>> applicable?
>>>>
>>>
>>> I agree with the point you make about i386 (e.g., C-language-
>>> toolchain.rst may be mentioned to provide some context about the
>>> preprocessor guard); that said, my R-by can be retained
>>>
>>>> Jan
>>>>
>>>>> + */
>>>>> +
>>>>> + BUILD_BUG_ON(sizeof(unsigned long) != sizeof(void (*)(void)));
>>>>> + BUILD_BUG_ON(sizeof(void *) != sizeof(void (*)(void)));
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> /*
>>>>> * Local variables:
>>>>> * mode: C
>>>
>>
>> And probably v4 can have the following wording:
>>
>> /*
>> * This assertion checks compatibility between 'unsigned long', 'void *',
>> * and function pointers. This is true for X86 (x86_64) and ARM (arm,
>> aarch64)
>> * architectures, which is why the check is restricted to these.
>> *
>> * For more context on architecture-specific preprocessor guards, see
>> * docs/misc/C-language-toolchain.rst.
>> *
>> * If Xen is ported to a new architecture, verify that this
>> compatibility holds
>> * before adding its macro to the condition below. If the compatibility
>> does not
>> * hold, this assertion may need to be revised or removed for that
>> architecture.
>> */
>
> Except that this doesn't address my concern. Imo the checks want to be there
> unconditionally, and ports where they're _not_ applicable would then need
> excluding (with suitable commentary and/or alternative checks).
>
> Jan
Ok, below is the updated logic:
/*
* This assertion checks compatibility between 'unsigned long', 'void *',
* and function pointers. This is true for most supported architectures,
* including X86 (x86_64) and ARM (arm, aarch64).
*
* For more context on architecture-specific preprocessor guards, see
* docs/misc/C-language-toolchain.rst.
*
* If porting Xen to a new architecture where this compatibility does
not hold,
* exclude that architecture from these checks and provide suitable
commentary
* and/or alternative checks as appropriate.
*/
static void __init __maybe_unused build_assertions(void)
{
/*
* Exclude architectures where function pointers are larger than
data pointers:
* - IA-64: uses 'fat' function pointers (code address + global
pointer)
*/
#if !defined(__ia64__)
BUILD_BUG_ON(sizeof(unsigned long) != sizeof(void (*)(void)));
BUILD_BUG_ON(sizeof(void *) != sizeof(void (*)(void)));
#endif
}
Dmytro.
