On 12.12.2022 23:05, Krister Johansen wrote: > 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?
No, I'm unconvinced of if/where this transformation is really appropriate. My comment was merely to indicate that the justification for ... >>> + if (!(xen_hvm_domain() || xen_pvh_domain())) >>> + return 0; ... this isn't really correct. > 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. Oh, I didn't expect cpuid() to be more than a trivial wrapper around the the pvops hook, and I merely looked at native_cpuid() and xen_cpuid(). I'm sorry for the noise then. Yet still, with there being sub-leaves, I'd recommend switching to cpuid_count() just for clarity. Jan
