Re: [PATCH linux-next v2] x86/xen/time: prefer tsc as clocksource when it is invariant

[Krister Johansen <kjlx@xxxxxxxxxxxxxxxxxx>]

On Wed, Dec 14, 2022 at 04:46:10PM -0500, Boris Ostrovsky wrote:
> 
> On 12/14/22 1:01 PM, Krister Johansen wrote:
> > On Tue, Dec 13, 2022 at 04:25:32PM -0500, Boris Ostrovsky wrote:
> > > On 12/12/22 5:09 PM, Krister Johansen wrote:
> > > > On Mon, Dec 12, 2022 at 01:48:24PM -0500, Boris Ostrovsky wrote:
> > > > > On 12/12/22 11:05 AM, Krister Johansen wrote:
> > > > > > diff --git a/arch/x86/include/asm/xen/cpuid.h 
> > > > > > b/arch/x86/include/asm/xen/cpuid.h
> > > > > > index 6daa9b0c8d11..d9d7432481e9 100644
> > > > > > --- a/arch/x86/include/asm/xen/cpuid.h
> > > > > > +++ b/arch/x86/include/asm/xen/cpuid.h
> > > > > > @@ -88,6 +88,12 @@
> > > > > >      *             EDX: shift amount for tsc->ns conversion
> > > > > >      * Sub-leaf 2: EAX: host tsc frequency in kHz
> > > > > >      */
> > > > > > +#define XEN_CPUID_TSC_EMULATED       (1u << 0)
> > > > > > +#define XEN_CPUID_HOST_TSC_RELIABLE  (1u << 1)
> > > > > > +#define XEN_CPUID_RDTSCP_INSTR_AVAIL (1u << 2)
> > > > > > +#define XEN_CPUID_TSC_MODE_DEFAULT   (0)
> > > > > > +#define XEN_CPUID_TSC_MODE_EMULATE   (1u)
> > > > > > +#define XEN_CPUID_TSC_MODE_NOEMULATE (2u)
> > > > > This file is a copy of Xen public interface so this change should go 
> > > > > to Xen first.
> > > > Ok, should I split this into a separate patch on the linux side too?
> > > Yes. Once the Xen patch has been accepted you will either submit the same 
> > > patch for Linux or sync Linux file with Xen (if there are more 
> > > differences).
> > Thanks.  Based upon the feedback I received from you and Jan, I may try
> > to shrink the check in xen_tsc_safe_clocksource() down a bit.  In that
> > case, I may only need to refer to a single field in the leaf that
> > provides this information.  In that case, are you alright with dropping
> > the change to the header and referring to the value directly, or would
> > you prefer that I proceed with adding these to the public API?
> 
> It would certainly be appreciated if you updated the header files but it's up 
> to maintainers to decide whether it's required.

Sure, I'm just trying to avoid generating extra work for the maintainers
if this patch isn't likely to make it in.  I'm cutting a v3 that doesn't
reference the header.  If it's acceptable, and this looks otherwise
unobjectionable, then I'll go ahead and put together the pieces for the
public API, if that's still a desireable change.

> > > > > > +static int __init xen_tsc_safe_clocksource(void)
> > > > > > +{
> > > > > > +   u32 eax, ebx, ecx, edx;
> > > > > > +
> > > > > > +   if (!(xen_hvm_domain() || xen_pvh_domain()))
> > > > > > +           return 0;
> > > > > > +
> > > > > > +   if (!(boot_cpu_has(X86_FEATURE_CONSTANT_TSC)))
> > > > > > +           return 0;
> > > > > > +
> > > > > > +   if (!(boot_cpu_has(X86_FEATURE_NONSTOP_TSC)))
> > > > > > +           return 0;
> > > > > > +
> > > > > > +   if (check_tsc_unstable())
> > > > > > +           return 0;
> > > > > > +
> > > > > > +   cpuid(xen_cpuid_base() + 3, &eax, &ebx, &ecx, &edx);
> > > > > > +
> > > > > > +   if (eax & XEN_CPUID_TSC_EMULATED)
> > > > > > +           return 0;
> > > > > > +
> > > > > > +   if (ebx != XEN_CPUID_TSC_MODE_NOEMULATE)
> > > > > > +           return 0;
> > > > > Why is the last test needed?
> > > > I was under the impression that if the mode was 0 (default) it would be
> > > > possible for the tsc to become emulated in the future, perhaps after a
> > > > migration.  The presence of the tsc_mode noemulate meant that we could
> > > > count on the falseneess of the XEN_CPUID_TSC_EMULATED check remaining
> > > > constant.
> > > This will filter out most modern processors with TSC scaling support 
> > > where in default mode we don't intercept RDTCS after migration. But I 
> > > don't think we have proper interface to determine this so we don't have 
> > > much choice but to indeed make this check.
> > Yes, if this remains a single boot-time check, I'm not sure that knowing
> > whether the processor supports tsc scaling helps us.  If tsc_mode is
> > default, there's always a possibility of the tsc becoming emulated later
> > on as part of migration, correct?
> 
> If the processor supports TSC scaling I don't think it's possible (it can 
> happen in theory) but if it doesn't and you migrate to a CPU running at 
> different frequency then yes, hypervisor will start emulating RDTSC.

Yes, I wondered whether it's reasonable to expect that users migrate
between hardware that is pretty similar, or if this case of moving from
a CPU that supports tsc scaling to one that doesn't is likely to happen
in practice.

-K