On 21.03.2022 15:26, Jan Beulich wrote:
> On 21.03.2022 15:12, Andrew Cooper wrote:
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -1092,12 +1092,17 @@ static void setup_APIC_timer(void)
>> local_irq_restore(flags);
>> }
>>
>> +#define DEADLINE_MODEL_FUNCT(m, fn) \
>> + { .vendor = X86_VENDOR_INTEL, .family = 6, .model = (m), \
>> + .feature = X86_FEATURE_TSC_DEADLINE, \
>> + .driver_data = fn + (0 * sizeof(fn == ((unsigned int
>> (*)(void))NULL))) }
>
> Are you sure all compiler versions we support are happy about +
> of a function pointer and a constant? Even if that constant is zero,
> this is not legal as per the plain C spec.
Thanks for the pointer to
https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html - this is indeed
fine then, with the assumption that this is also only meaningful with
the non-upstream -fcf-check-attribute= patch in place.
Hence with ...
> Also strictly speaking you would want to parenthesize both uses of
> fn.
... this taken care of (also to please Misra)
Reviewed-by: Jan Beulich <jbeul...@suse.com>
IOW ...
>> #define DEADLINE_MODEL_MATCH(m, fr) \
>> { .vendor = X86_VENDOR_INTEL, .family = 6, .model = (m), \
>> .feature = X86_FEATURE_TSC_DEADLINE, \
>> .driver_data = (void *)(unsigned long)(fr) }
>
> As long as we leave this in place, there's a (small) risk of the
> wrong macro being used again if another hook would need adding here.
> We might be safer if driver_data became "unsigned long" and the
> void * cast was dropped from here (with an "unsigned long" cast
> added in the new macro, which at the same time would address my
> other concern above).
... while I continue to be concerned here, we can as well deal with
that if and when a new such hook appeared.
Jan
