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

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

On Mon, Apr 04, 2022 at 05:33:04PM +0200, Jan Beulich wrote:
> 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 *.

I see, there are architectures with different sizes for function and
data pointers. It's also not clear all compilers will be happy with
the conversion.

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

I think it's safer to use a function pointer type like you currently
have from a correctness PoV, but in order to prevent stray calls to
read_tsc() I would rename to READ_TSC_PTR_POISON. This was already
static, so I guess it's hard anyway for any of such direct calls to
appear without us realizing.

With that:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.