Re: [PATCH v4 4/4] x86/time: use fake read_tsc()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 31, 2022 at 11:31:38AM +0200, Jan Beulich wrote:
> Go a step further than bed9ae54df44 ("x86/time: switch platform timer
> hooks to altcall") did and eliminate the "real" read_tsc() altogether:
> It's not used except in pointer comparisons, and hence it looks overall
> more safe to simply poison plt_tsc's read_counter hook.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> I wasn't really sure whether it would be better to use simply void * for
> the type of the expression, resulting in an undesirable data -> function
> pointer conversion, but making it impossible to mistakenly try and call
> the (fake) function directly.

I think it's slightly better to avoid being able to call the function,
hence using void * would be my preference. What's wrong with the data
-> function pointer conversion for the comparisons?

> ---
> v2: Comment wording.
> 
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -607,10 +607,12 @@ static s64 __init cf_check init_tsc(stru
>      return ret;
>  }
>  
> -static uint64_t __init cf_check read_tsc(void)
> -{
> -    return rdtsc_ordered();
> -}
> +/*
> + * plt_tsc's read_counter hook is not (and should not be) invoked via the
> + * struct field. To avoid carrying an unused, indirectly reachable function,
> + * poison the field with an easily identifiable non-canonical pointer.
> + */
> +#define read_tsc ((uint64_t(*)(void))0x75C75C75C75C75C0ul)

Instead of naming this like a suitable function, I would rather use
READ_TSC_PTR_POISON or some such.

Thanks, Roger.