[PATCH linux-next 0/2] x86/xen TSC related cleanups

[PATCH linux-next 0/2] x86/xen TSC related cleanups

[Krister Johansen]

Hi,
Enclosed please find a pair of patches that perform some additional cleanup
that was suggested by Boris and Jan.

Specifically: this resync's arch/x86/include/asm/xen/cpuid.h from its
upstream source in the Xen tree, and then uses one of the new #define-s to
replace a constant in x86/xen/time.c that was previously only numerically
defined.

Krister Johansen (2):
  xen: update arch/x86/include/asm/xen/cpuid.h
  x86/xen/time: cleanup xen_tsc_safe_clocksource

 arch/x86/include/asm/xen/cpuid.h | 22 ++++++++++++++++++----
 arch/x86/xen/time.c              |  4 ++--
 2 files changed, 20 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH linux-next 1/2] xen: update arch/x86/include/asm/xen/cpuid.h

[Krister Johansen]

Update arch/x86/include/asm/xen/cpuid.h from the Xen tree to get newest
definitions.  This picks up some TSC mode definitions and comment
formatting changes.

Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 arch/x86/include/asm/xen/cpuid.h | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/xen/cpuid.h b/arch/x86/include/asm/xen/cpuid.h
index 6daa9b0c8d11..a3c29b1496c8 100644
--- a/arch/x86/include/asm/xen/cpuid.h
+++ b/arch/x86/include/asm/xen/cpuid.h
@@ -89,11 +89,21 @@
  * 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_ALWAYS_EMULATE    (1u)
+#define XEN_CPUID_TSC_MODE_NEVER_EMULATE     (2u)
+#define XEN_CPUID_TSC_MODE_PVRDTSCP          (3u)
+
 /*
  * Leaf 5 (0x40000x04)
  * HVM-specific features
  * Sub-leaf 0: EAX: Features
  * Sub-leaf 0: EBX: vcpu id (iff EAX has XEN_HVM_CPUID_VCPU_ID_PRESENT flag)
+ * Sub-leaf 0: ECX: domain id (iff EAX has XEN_HVM_CPUID_DOMID_PRESENT flag)
  */
 #define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized APIC registers */
 #define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized x2APIC accesses */
@@ -102,12 +112,16 @@
 #define XEN_HVM_CPUID_VCPU_ID_PRESENT  (1u << 3) /* vcpu id is present in EBX */
 #define XEN_HVM_CPUID_DOMID_PRESENT    (1u << 4) /* domid is present in ECX */
 /*
- * Bits 55:49 from the IO-APIC RTE and bits 11:5 from the MSI address can be
- * used to store high bits for the Destination ID. This expands the Destination
- * ID field from 8 to 15 bits, allowing to target APIC IDs up 32768.
+ * With interrupt format set to 0 (non-remappable) bits 55:49 from the
+ * IO-APIC RTE and bits 11:5 from the MSI address can be used to store
+ * high bits for the Destination ID. This expands the Destination ID
+ * field from 8 to 15 bits, allowing to target APIC IDs up 32768.
  */
 #define XEN_HVM_CPUID_EXT_DEST_ID      (1u << 5)
-/* Per-vCPU event channel upcalls */
+/*
+ * Per-vCPU event channel upcalls work correctly with physical IRQs
+ * bound to event channels.
+ */
 #define XEN_HVM_CPUID_UPCALL_VECTOR    (1u << 6)
 
 /*
-- 
2.25.1

[PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Krister Johansen]

Modifies xen_tsc_safe_clocksource() to use newly defined constants from
arch/x86/include/asm/xen/cpuid.h.  This replaces a numeric value with
XEN_CPUID_TSC_MODE_NEVER_EMULATE, and deletes a comment that is now self
explanatory.

There should be no change in the function's behavior.

Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 arch/x86/xen/time.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
index 95140609c8a8..cf6dd9f9fa6a 100644
--- a/arch/x86/xen/time.c
+++ b/arch/x86/xen/time.c
@@ -20,6 +20,7 @@
 #include <asm/pvclock.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
+#include <asm/xen/cpuid.h>
 
 #include <xen/events.h>
 #include <xen/features.h>
@@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
 
-	/* tsc_mode = no_emulate (2) */
-	if (ebx != 2)
+	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
 		return 0;
 
 	return 1;
-- 
2.25.1

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Thomas Gleixner]

On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
> @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
>  	/* Leaf 4, sub-leaf 0 (0x40000x03) */
>  	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
>  
> -	/* tsc_mode = no_emulate (2) */
> -	if (ebx != 2)
> +	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
>  		return 0;
>  
>  	return 1;

What about removing more stupidity from that function?

static bool __init xen_tsc_safe_clocksource(void)
{
	u32 eax, ebx. ecx, edx;
 
	/* Leaf 4, sub-leaf 0 (0x40000x03) */
	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);

	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
}

Thanks,

        tglx

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Krister Johansen]

On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
> On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
> > @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
> >  	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> >  	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> >  
> > -	/* tsc_mode = no_emulate (2) */
> > -	if (ebx != 2)
> > +	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
> >  		return 0;
> >  
> >  	return 1;
> 
> What about removing more stupidity from that function?
> 
> static bool __init xen_tsc_safe_clocksource(void)
> {
> 	u32 eax, ebx. ecx, edx;
>  
> 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> 
> 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> }

I'm all for simplifying.  I'm happy to clean up that return to be more
idiomatic.  I was under the impression, perhaps mistaken, though, that
the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
check_tsc_unstable() checks were actually serving a purpose: to ensure
that we don't rely on the tsc in environments where it's being emulated
and the OS would be better served by using a PV clock.  Specifically,
kvmclock_init() makes a very similar set of checks that I also thought
were load-bearing.

-K

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Krister Johansen]

On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
> On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
> > On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
> > > @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
> > >  	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> > >  	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> > >  
> > > -	/* tsc_mode = no_emulate (2) */
> > > -	if (ebx != 2)
> > > +	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
> > >  		return 0;
> > >  
> > >  	return 1;
> > 
> > What about removing more stupidity from that function?
> > 
> > static bool __init xen_tsc_safe_clocksource(void)
> > {
> > 	u32 eax, ebx. ecx, edx;
> >  
> > 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> > 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> > 
> > 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> > }
> 
> I'm all for simplifying.  I'm happy to clean up that return to be more
> idiomatic.  I was under the impression, perhaps mistaken, though, that
> the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
> check_tsc_unstable() checks were actually serving a purpose: to ensure
> that we don't rely on the tsc in environments where it's being emulated
> and the OS would be better served by using a PV clock.  Specifically,
> kvmclock_init() makes a very similar set of checks that I also thought
> were load-bearing.

Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
for use as a clocksource.  IOW, even if TSC_MODE_NEVER_EMULATE is
set, it's possible that a user is attempting a migration from a cpu
that's not invariant, and we'd still want to check for that case and
fall back to a PV clocksource, correct?

-K

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2049 bytes --]

On 21.02.23 06:51, Krister Johansen wrote:
> On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
>> On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
>>> On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
>>>> @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
>>>>   	/* Leaf 4, sub-leaf 0 (0x40000x03) */
>>>>   	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
>>>>   
>>>> -	/* tsc_mode = no_emulate (2) */
>>>> -	if (ebx != 2)
>>>> +	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
>>>>   		return 0;
>>>>   
>>>>   	return 1;
>>>
>>> What about removing more stupidity from that function?
>>>
>>> static bool __init xen_tsc_safe_clocksource(void)
>>> {
>>> 	u32 eax, ebx. ecx, edx;
>>>   
>>> 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
>>> 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
>>>
>>> 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
>>> }
>>
>> I'm all for simplifying.  I'm happy to clean up that return to be more
>> idiomatic.  I was under the impression, perhaps mistaken, though, that
>> the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
>> check_tsc_unstable() checks were actually serving a purpose: to ensure
>> that we don't rely on the tsc in environments where it's being emulated
>> and the OS would be better served by using a PV clock.  Specifically,
>> kvmclock_init() makes a very similar set of checks that I also thought
>> were load-bearing.
> 
> Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
> for use as a clocksource.  IOW, even if TSC_MODE_NEVER_EMULATE is
> set, it's possible that a user is attempting a migration from a cpu
> that's not invariant, and we'd still want to check for that case and
> fall back to a PV clocksource, correct?

But Thomas' suggestion wasn't changing any behavior compared to your
patch. It just makes it easier to read.

If you are unsure your patch is correct, please verify the correctness
before sending it.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Krister Johansen]

On Tue, Feb 21, 2023 at 09:47:24AM +0100, Juergen Gross wrote:
> On 21.02.23 06:51, Krister Johansen wrote:
> > On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
> > > On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
> > > > On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
> > > > > @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
> > > > >   	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> > > > >   	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> > > > > -	/* tsc_mode = no_emulate (2) */
> > > > > -	if (ebx != 2)
> > > > > +	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
> > > > >   		return 0;
> > > > >   	return 1;
> > > > 
> > > > What about removing more stupidity from that function?
> > > > 
> > > > static bool __init xen_tsc_safe_clocksource(void)
> > > > {
> > > > 	u32 eax, ebx. ecx, edx;
> > > > 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> > > > 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> > > > 
> > > > 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> > > > }
> > > 
> > > I'm all for simplifying.  I'm happy to clean up that return to be more
> > > idiomatic.  I was under the impression, perhaps mistaken, though, that
> > > the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
> > > check_tsc_unstable() checks were actually serving a purpose: to ensure
> > > that we don't rely on the tsc in environments where it's being emulated
> > > and the OS would be better served by using a PV clock.  Specifically,
> > > kvmclock_init() makes a very similar set of checks that I also thought
> > > were load-bearing.
> > 
> > Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
> > for use as a clocksource.  IOW, even if TSC_MODE_NEVER_EMULATE is
> > set, it's possible that a user is attempting a migration from a cpu
> > that's not invariant, and we'd still want to check for that case and
> > fall back to a PV clocksource, correct?
> 
> But Thomas' suggestion wasn't changing any behavior compared to your
> patch. It just makes it easier to read.
> 
> If you are unsure your patch is correct, please verify the correctness
> before sending it.

Thanks, and apologies. I misunderstood and thought a more complex change
was requested.

-K

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Thomas Gleixner]

On Mon, Feb 20 2023 at 21:51, Krister Johansen wrote:
> On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
>> > static bool __init xen_tsc_safe_clocksource(void)
>> > {
>> > 	u32 eax, ebx. ecx, edx;
>> >  
>> > 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
>> > 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
>> > 
>> > 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
>> > }
>> 
>> I'm all for simplifying.  I'm happy to clean up that return to be more
>> idiomatic.  I was under the impression, perhaps mistaken, though, that
>> the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
>> check_tsc_unstable() checks were actually serving a purpose: to ensure
>> that we don't rely on the tsc in environments where it's being emulated
>> and the OS would be better served by using a PV clock.  Specifically,
>> kvmclock_init() makes a very similar set of checks that I also thought
>> were load-bearing.
>
> Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
> for use as a clocksource.  IOW, even if TSC_MODE_NEVER_EMULATE is
> set, it's possible that a user is attempting a migration from a cpu
> that's not invariant, and we'd still want to check for that case and
> fall back to a PV clocksource, correct?

Sure. But a life migration from a NEVER_EMULATE to a non-invariant host
is a patently bad idea and has nothing to do with the __init function,
which is gone at that point already.

What I wanted to say:

static bool __init xen_tsc_safe_clocksource(void)
{
        ......        

	/* Leaf 4, sub-leaf 0 (0x40000x03) */
	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);

	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
}

I didn't have the full context and was just looking at the condition.
Now I checked the full context and I think that except for the

	if (check_tsc_unstable())

check everything else can go away unless you do not trust the hypervisor
that it only sets the NEVER_EMULATE bit when CONSTANT and NONSTOP are
set as well. But yeah, you might prefer to be paranoid. It's virt after
all.

Thanks,

        tglx

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Krister Johansen]

On Tue, Feb 21, 2023 at 10:14:54AM +0100, Thomas Gleixner wrote:
> On Mon, Feb 20 2023 at 21:51, Krister Johansen wrote:
> > On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
> >> > static bool __init xen_tsc_safe_clocksource(void)
> >> > {
> >> > 	u32 eax, ebx. ecx, edx;
> >> >  
> >> > 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> >> > 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> >> > 
> >> > 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> >> > }
> >> 
> >> I'm all for simplifying.  I'm happy to clean up that return to be more
> >> idiomatic.  I was under the impression, perhaps mistaken, though, that
> >> the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
> >> check_tsc_unstable() checks were actually serving a purpose: to ensure
> >> that we don't rely on the tsc in environments where it's being emulated
> >> and the OS would be better served by using a PV clock.  Specifically,
> >> kvmclock_init() makes a very similar set of checks that I also thought
> >> were load-bearing.
> >
> > Bah, what I meant to say was emulated, unstable, or otherwise unsuitable
> > for use as a clocksource.  IOW, even if TSC_MODE_NEVER_EMULATE is
> > set, it's possible that a user is attempting a migration from a cpu
> > that's not invariant, and we'd still want to check for that case and
> > fall back to a PV clocksource, correct?
> 
> Sure. But a life migration from a NEVER_EMULATE to a non-invariant host
> is a patently bad idea and has nothing to do with the __init function,
> which is gone at that point already.
> 
> What I wanted to say:
> 
> static bool __init xen_tsc_safe_clocksource(void)
> {
>         ......        
> 
> 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> 
> 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> }

Thanks, I'm happy to make it look like ^ that.  I should have thought to
do this myself. :/

I'll send out a v2 making this correction.

> I didn't have the full context and was just looking at the condition.
> Now I checked the full context and I think that except for the
> 
> 	if (check_tsc_unstable())
> 
> check everything else can go away unless you do not trust the hypervisor
> that it only sets the NEVER_EMULATE bit when CONSTANT and NONSTOP are
> set as well. But yeah, you might prefer to be paranoid. It's virt after
> all.

Unless there are objections, I think I'd prefer to err on the side of
paranoia here.  Sorry for the confusion.

-K

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Marcelo Tosatti]

On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
> On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
> > On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
> > > @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
> > >  	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> > >  	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> > >  
> > > -	/* tsc_mode = no_emulate (2) */
> > > -	if (ebx != 2)
> > > +	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
> > >  		return 0;
> > >  
> > >  	return 1;
> > 
> > What about removing more stupidity from that function?
> > 
> > static bool __init xen_tsc_safe_clocksource(void)
> > {
> > 	u32 eax, ebx. ecx, edx;
> >  
> > 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> > 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> > 
> > 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> > }
> 
> I'm all for simplifying.  I'm happy to clean up that return to be more
> idiomatic.  I was under the impression, perhaps mistaken, though, that
> the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
> check_tsc_unstable() checks were actually serving a purpose: to ensure
> that we don't rely on the tsc in environments where it's being emulated
> and the OS would be better served by using a PV clock.  Specifically,
> kvmclock_init() makes a very similar set of checks that I also thought
> were load-bearing.

kvmclock_init will lower the rating of kvmclock so that TSC clocksource
can be used instead:

        /*
         * X86_FEATURE_NONSTOP_TSC is TSC runs at constant rate
         * with P/T states and does not stop in deep C-states.
         *
         * Invariant TSC exposed by host means kvmclock is not necessary:
         * can use TSC as clocksource.
         *
         */
        if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
            boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
            !check_tsc_unstable())
                kvm_clock.rating = 299;

Re: [PATCH linux-next 2/2] x86/xen/time: cleanup xen_tsc_safe_clocksource

[Krister Johansen]

Hi Marcelo,

On Thu, Feb 23, 2023 at 11:34:06AM -0300, Marcelo Tosatti wrote:
> On Mon, Feb 20, 2023 at 08:14:40PM -0800, Krister Johansen wrote:
> > On Mon, Feb 20, 2023 at 11:01:18PM +0100, Thomas Gleixner wrote:
> > > On Mon, Feb 20 2023 at 09:17, Krister Johansen wrote:
> > > > @@ -495,8 +496,7 @@ static int __init xen_tsc_safe_clocksource(void)
> > > >  	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> > > >  	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> > > >  
> > > > -	/* tsc_mode = no_emulate (2) */
> > > > -	if (ebx != 2)
> > > > +	if (ebx != XEN_CPUID_TSC_MODE_NEVER_EMULATE)
> > > >  		return 0;
> > > >  
> > > >  	return 1;
> > > 
> > > What about removing more stupidity from that function?
> > > 
> > > static bool __init xen_tsc_safe_clocksource(void)
> > > {
> > > 	u32 eax, ebx. ecx, edx;
> > >  
> > > 	/* Leaf 4, sub-leaf 0 (0x40000x03) */
> > > 	cpuid_count(xen_cpuid_base() + 3, 0, &eax, &ebx, &ecx, &edx);
> > > 
> > > 	return ebx == XEN_CPUID_TSC_MODE_NEVER_EMULATE;
> > > }
> > 
> > I'm all for simplifying.  I'm happy to clean up that return to be more
> > idiomatic.  I was under the impression, perhaps mistaken, though, that
> > the X86_FEATURE_CONSTANT_TSC, X86_FEATURE_NONSTOP_TSC, and
> > check_tsc_unstable() checks were actually serving a purpose: to ensure
> > that we don't rely on the tsc in environments where it's being emulated
> > and the OS would be better served by using a PV clock.  Specifically,
> > kvmclock_init() makes a very similar set of checks that I also thought
> > were load-bearing.
> 
> kvmclock_init will lower the rating of kvmclock so that TSC clocksource
> can be used instead:
> 
>         /*
>          * X86_FEATURE_NONSTOP_TSC is TSC runs at constant rate
>          * with P/T states and does not stop in deep C-states.
>          *
>          * Invariant TSC exposed by host means kvmclock is not necessary:
>          * can use TSC as clocksource.
>          *
>          */
>         if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
>             boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
>             !check_tsc_unstable())
>                 kvm_clock.rating = 299;

Yes, I saw the change you made to the kvmclock to do this and was
inspired to try to do something similar for Xen:

https://lore.kernel.org/xen-devel/20221216162118.GB2633@templeofstupid.com/

Thanks,

-K