Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Dec 2022 14:05:19 -0800
From:   Krister Johansen <kjlx@templeofstupid.com>
To:     Jan Beulich <jbeulich@suse.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, xen-devel@lists.xenproject.org,
        linux-kernel@vger.kernel.org,
        Marcelo Tosatti <mtosatti@redhat.com>,
        Anthony Liguori <aliguori@amazon.com>,
        David Reaver <me@davidreaver.com>,
        Brendan Gregg <brendan@intel.com>,
        Juergen Gross <jgross@suse.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH linux-next v2] x86/xen/time: prefer tsc as clocksource
 when it is invariant
Message-ID: <20221212220519.GA1935@templeofstupid.com>
References: <20221208163650.GA3225@templeofstupid.com>
 <1e6c1b08-d573-fba9-61fd-d40a74427d46@oracle.com>
 <20221212155730.GA1973@templeofstupid.com>
 <20221212160524.GB1973@templeofstupid.com>
 <1eb6048b-bf23-78a0-9c3c-54bbd12c3864@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1eb6048b-bf23-78a0-9c3c-54bbd12c3864@suse.com>
Precedence: bulk

On Mon, Dec 12, 2022 at 05:46:29PM +0100, Jan Beulich wrote:
> On 12.12.2022 17:05, Krister Johansen wrote:
> > Both the Intel SDM[4] and the Xen tsc documentation explain that marking
> > a tsc as invariant means that it should be considered stable by the OS
> > and is elibile to be used as a wall clock source.  The Xen documentation
> > further clarifies that this is only reliable on HVM and PVH because PV
> > cannot intercept a cpuid instruction.
> 
> Without meaning to express a view on the argumentation as a whole, this
> PV aspect is suspicious. Unless you open-code a use of the CPUID insn
> in the kernel, all uses of CPUID are going to be processed by Xen by
> virtue of the respective pvops hook. Documentation says what it says
> for environments where this might not be the case.

Thanks, appreciate the clarification here. Just restating this for my
own understanding: your advice would be to drop this check below?

> > +	if (!(xen_hvm_domain() || xen_pvh_domain()))
> > +		return 0;

And then update the commit message to dispense with the distinction
between HVM, PV, and PVH?

> > +	cpuid(xen_cpuid_base() + 3, &eax, &ebx, &ecx, &edx);
> 
> Xen leaf 3 has sub-leaves, so I think you need to set ecx to zero before
> this call.

The cpuid() inline in arch/x86/include/asm/processor.h assigns zero to
ecx prior to calling __cpuid.  In arch/x86/boot/cpuflags.c the macros
are a little different, but it looks like there too, the macro passes 0
as an input argument to cpuid_count which ends up being %ecx.  Happy to
fix this up if I'm looking at the wrong cpuid functions, though.

-K