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 <[email protected]>
> ---
> 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.
