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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.04.2022 15:22, Roger Pau Monné wrote:
> 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?

There's no data -> function pointer conversion for the comparisons; the
situation there is even less pleasant. What I referred to was actually
the initializer, where there would be a data -> function pointer
conversion if I used void *.

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

I'll be happy to name it something like this; the primary thing to
settle on is the type to use.

Jan