Re: [PATCH] x86/apic: Fix function typechecking in TSC Deadline errata check

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jbeulich@xxxxxxxx>

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