Sadly, cf_check typechecking doesn't work through casts.  Introduce an ad-hoc
typecheck and fix *_readline_rev() checks to be cf_check.

This is a latent bug.  The affected models don't have CET-IBT, so won't
actually explode from lacking endbr64 instructions.

Reported-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/apic.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 96d73a744964..794bbc21ae2c 100644
--- 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))) }
+
 #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) }
 
-static unsigned int __init hsx_deadline_rev(void)
+static unsigned int __init cf_check hsx_deadline_rev(void)
 {
     switch ( boot_cpu_data.x86_mask )
     {
@@ -1108,7 +1113,7 @@ static unsigned int __init hsx_deadline_rev(void)
     return ~0U;
 }
 
-static unsigned int __init bdx_deadline_rev(void)
+static unsigned int __init cf_check bdx_deadline_rev(void)
 {
     switch ( boot_cpu_data.x86_mask )
     {
@@ -1121,7 +1126,7 @@ static unsigned int __init bdx_deadline_rev(void)
     return ~0U;
 }
 
-static unsigned int __init skx_deadline_rev(void)
+static unsigned int __init cf_check skx_deadline_rev(void)
 {
     switch ( boot_cpu_data.x86_mask )
     {
@@ -1135,17 +1140,17 @@ static unsigned int __init skx_deadline_rev(void)
 
 static const struct x86_cpu_id __initconstrel deadline_match[] = {
     DEADLINE_MODEL_MATCH(0x3c, 0x22),             /* Haswell */
-    DEADLINE_MODEL_MATCH(0x3f, hsx_deadline_rev), /* Haswell EP/EX */
+    DEADLINE_MODEL_FUNCT(0x3f, hsx_deadline_rev), /* Haswell EP/EX */
     DEADLINE_MODEL_MATCH(0x45, 0x20),             /* Haswell D */
     DEADLINE_MODEL_MATCH(0x46, 0x17),             /* Haswell H */
 
     DEADLINE_MODEL_MATCH(0x3d, 0x25),             /* Broadwell */
     DEADLINE_MODEL_MATCH(0x47, 0x17),             /* Broadwell H */
     DEADLINE_MODEL_MATCH(0x4f, 0x0b000020),       /* Broadwell EP/EX */
-    DEADLINE_MODEL_MATCH(0x56, bdx_deadline_rev), /* Broadwell D */
+    DEADLINE_MODEL_FUNCT(0x56, bdx_deadline_rev), /* Broadwell D */
 
     DEADLINE_MODEL_MATCH(0x4e, 0xb2),             /* Skylake M */
-    DEADLINE_MODEL_MATCH(0x55, skx_deadline_rev), /* Skylake X */
+    DEADLINE_MODEL_FUNCT(0x55, skx_deadline_rev), /* Skylake X */
     DEADLINE_MODEL_MATCH(0x5e, 0xb2),             /* Skylake D */
 
     DEADLINE_MODEL_MATCH(0x8e, 0x52),             /* Kabylake M */
-- 
2.11.0

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.

Also strictly speaking you would want to parenthesize both uses of
fn.

>  #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).

Jan

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