diff mbox series

[v5,13/14] x86/tsc: Mark Secure TSC as reliable clocksource

Message ID	20231030063652.68675-14-nikunj@amd.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C From: Nikunj A Dadhania <nikunj@amd.com> To: <linux-kernel@vger.kernel.org>, <thomas.lendacky@amd.com>, <x86@kernel.org>, <kvm@vger.kernel.org> CC: <bp@alien8.de>, <mingo@redhat.com>, <tglx@linutronix.de>, <dave.hansen@linux.intel.com>, <dionnaglaze@google.com>, <pgonda@google.com>, <seanjc@google.com>, <pbonzini@redhat.com>, <nikunj@amd.com> Subject: [PATCH v5 13/14] x86/tsc: Mark Secure TSC as reliable clocksource Date: Mon, 30 Oct 2023 12:06:51 +0530 Message-ID: <20231030063652.68675-14-nikunj@amd.com> In-Reply-To: <20231030063652.68675-1-nikunj@amd.com> References: <20231030063652.68675-1-nikunj@amd.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain Precedence: bulk
Series	Add Secure TSC support for SNP guests \| [v5,00/14] Add Secure TSC support for SNP guests [v5,01/14] virt: sev-guest: Use AES GCM crypto library [v5,02/14] virt: sev-guest: Move mutex to SNP guest device structure [v5,03/14] virt: sev-guest: Replace dev_dbg with pr_debug [v5,04/14] virt: sev-guest: Add SNP guest request structure [v5,05/14] virt: sev-guest: Add vmpck_id to snp_guest_dev struct [v5,06/14] x86/sev: Cache the secrets page address [v5,07/14] x86/sev: Move and reorganize sev guest request api [v5,08/14] x86/mm: Add generic guest initialization hook [v5,09/14] x86/sev: Add Secure TSC support for SNP guests [v5,10/14] x86/sev: Change TSC MSR behavior for Secure TSC enabled guests [v5,11/14] x86/sev: Prevent RDTSC/RDTSCP interception for Secure TSC enabled guests [v5,12/14] x86/kvmclock: Skip kvmclock when Secure TSC is available [v5,13/14] x86/tsc: Mark Secure TSC as reliable clocksource [v5,14/14] x86/sev: Enable Secure TSC for SNP guests

Commit Message

Nikunj A. Dadhania Oct. 30, 2023, 6:36 a.m. UTC

  AMD SNP guests may have Secure TSC feature enabled. Secure TSC as
clocksource is wrongly marked as unstable, mark Secure TSC as
reliable.

Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/kernel/tsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dave Hansen Oct. 30, 2023, 5:18 p.m. UTC | #1

On 10/29/23 23:36, Nikunj A Dadhania wrote:
...
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 15f97c0abc9d..b0a8546d3703 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1241,7 +1241,7 @@ static void __init check_system_tsc_reliable(void)
>  			tsc_clocksource_reliable = 1;
>  	}
>  #endif
> -	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE))
> +	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE) || cc_platform_has(CC_ATTR_GUEST_SECURE_TSC))
>  		tsc_clocksource_reliable = 1;

Why can't you just set X86_FEATURE_TSC_RELIABLE?

Nikunj A. Dadhania Nov. 2, 2023, 5:53 a.m. UTC | #2

On 10/30/2023 10:48 PM, Dave Hansen wrote:
> On 10/29/23 23:36, Nikunj A Dadhania wrote:
> ...
>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>> index 15f97c0abc9d..b0a8546d3703 100644
>> --- a/arch/x86/kernel/tsc.c
>> +++ b/arch/x86/kernel/tsc.c
>> @@ -1241,7 +1241,7 @@ static void __init check_system_tsc_reliable(void)
>>  			tsc_clocksource_reliable = 1;
>>  	}
>>  #endif
>> -	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE))
>> +	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE) || cc_platform_has(CC_ATTR_GUEST_SECURE_TSC))
>>  		tsc_clocksource_reliable = 1;
> 
> Why can't you just set X86_FEATURE_TSC_RELIABLE?

Last time when I tried, I had removed my kvmclock changes and I had set the X86_FEATURE_TSC_RELIABLE similar to Kirill's patch[1], this did not select the SecureTSC.

Let me try setting X86_FEATURE_TSC_RELIABLE and retaining my patch for skipping kvmclock.

Regards
Nikunj

1: https://lore.kernel.org/lkml/20230808162320.27297-1-kirill.shutemov@linux.intel.com/

Kirill A. Shutemov Nov. 2, 2023, 10:33 a.m. UTC | #3

On Thu, Nov 02, 2023 at 11:23:34AM +0530, Nikunj A. Dadhania wrote:
> On 10/30/2023 10:48 PM, Dave Hansen wrote:
> > On 10/29/23 23:36, Nikunj A Dadhania wrote:
> > ...
> >> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> >> index 15f97c0abc9d..b0a8546d3703 100644
> >> --- a/arch/x86/kernel/tsc.c
> >> +++ b/arch/x86/kernel/tsc.c
> >> @@ -1241,7 +1241,7 @@ static void __init check_system_tsc_reliable(void)
> >>  			tsc_clocksource_reliable = 1;
> >>  	}
> >>  #endif
> >> -	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE))
> >> +	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE) || cc_platform_has(CC_ATTR_GUEST_SECURE_TSC))
> >>  		tsc_clocksource_reliable = 1;
> > 
> > Why can't you just set X86_FEATURE_TSC_RELIABLE?
> 
> Last time when I tried, I had removed my kvmclock changes and I had set
> the X86_FEATURE_TSC_RELIABLE similar to Kirill's patch[1], this did not
> select the SecureTSC.
> 
> Let me try setting X86_FEATURE_TSC_RELIABLE and retaining my patch for
> skipping kvmclock.

kvmclock lowers its rating if TSC is good enough:

	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
	    boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
	    !check_tsc_unstable())
		kvm_clock.rating = 299;

Does your TSC meet the requirements?

Nikunj A. Dadhania Nov. 2, 2023, 12:07 p.m. UTC | #4

On 11/2/2023 4:03 PM, Kirill A. Shutemov wrote:
> On Thu, Nov 02, 2023 at 11:23:34AM +0530, Nikunj A. Dadhania wrote:
>> On 10/30/2023 10:48 PM, Dave Hansen wrote:
>>> On 10/29/23 23:36, Nikunj A Dadhania wrote:
>>> ...
>>>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>>>> index 15f97c0abc9d..b0a8546d3703 100644
>>>> --- a/arch/x86/kernel/tsc.c
>>>> +++ b/arch/x86/kernel/tsc.c
>>>> @@ -1241,7 +1241,7 @@ static void __init check_system_tsc_reliable(void)
>>>>  			tsc_clocksource_reliable = 1;
>>>>  	}
>>>>  #endif
>>>> -	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE))
>>>> +	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE) || cc_platform_has(CC_ATTR_GUEST_SECURE_TSC))
>>>>  		tsc_clocksource_reliable = 1;
>>>
>>> Why can't you just set X86_FEATURE_TSC_RELIABLE?
>>
>> Last time when I tried, I had removed my kvmclock changes and I had set
>> the X86_FEATURE_TSC_RELIABLE similar to Kirill's patch[1], this did not
>> select the SecureTSC.
>>
>> Let me try setting X86_FEATURE_TSC_RELIABLE and retaining my patch for
>> skipping kvmclock.
> 
> kvmclock lowers its rating if TSC is good enough:
> 
> 	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> 	    boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
> 	    !check_tsc_unstable())
> 		kvm_clock.rating = 299;
> 
> Does your TSC meet the requirements?

I have set TscInvariant (bit 8) in CPUID_8000_0007_edx and TSC is set as reliable.

With this I see kvm_clock rating being lowered, but kvm-clock is still being picked as clock-source.

[    0.000834] kvmclock_init: lowering kvm_clock rating
[    0.002623] clocksource: kvm-clock: mask: 0xffffffffffffffff max_cycles: 0x1cd42e4dffb, max_idle_ns: 881590591483 ns
[    2.295082] clocksource: Switched to clocksource kvm-clock

Regards
Nikunj

Nikunj A. Dadhania Nov. 2, 2023, 12:16 p.m. UTC | #5

On 11/2/2023 5:37 PM, Nikunj A. Dadhania wrote:
> On 11/2/2023 4:03 PM, Kirill A. Shutemov wrote:
>> On Thu, Nov 02, 2023 at 11:23:34AM +0530, Nikunj A. Dadhania wrote:
>>> On 10/30/2023 10:48 PM, Dave Hansen wrote:
>>>> On 10/29/23 23:36, Nikunj A Dadhania wrote:
>>>> ...
>>>>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>>>>> index 15f97c0abc9d..b0a8546d3703 100644
>>>>> --- a/arch/x86/kernel/tsc.c
>>>>> +++ b/arch/x86/kernel/tsc.c
>>>>> @@ -1241,7 +1241,7 @@ static void __init check_system_tsc_reliable(void)
>>>>>  			tsc_clocksource_reliable = 1;
>>>>>  	}
>>>>>  #endif
>>>>> -	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE))
>>>>> +	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE) || cc_platform_has(CC_ATTR_GUEST_SECURE_TSC))
>>>>>  		tsc_clocksource_reliable = 1;
>>>>
>>>> Why can't you just set X86_FEATURE_TSC_RELIABLE?
>>>
>>> Last time when I tried, I had removed my kvmclock changes and I had set
>>> the X86_FEATURE_TSC_RELIABLE similar to Kirill's patch[1], this did not
>>> select the SecureTSC.
>>>
>>> Let me try setting X86_FEATURE_TSC_RELIABLE and retaining my patch for
>>> skipping kvmclock.
>>
>> kvmclock lowers its rating if TSC is good enough:
>>
>> 	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
>> 	    boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
>> 	    !check_tsc_unstable())
>> 		kvm_clock.rating = 299;
>>
>> Does your TSC meet the requirements?
> 
> I have set TscInvariant (bit 8) in CPUID_8000_0007_edx and TSC is set as reliable.
> 
> With this I see kvm_clock rating being lowered, but kvm-clock is still being picked as clock-source.

Ah.. at later point TSC is picked up, is this expected ?

[    2.564052] clocksource: Switched to clocksource kvm-clock
[    2.678136] clocksource: Switched to clocksource tsc

Regards
Nikunj

Kirill A. Shutemov Nov. 2, 2023, 12:38 p.m. UTC | #6

On Thu, Nov 02, 2023 at 05:46:26PM +0530, Nikunj A. Dadhania wrote:
> On 11/2/2023 5:37 PM, Nikunj A. Dadhania wrote:
> > On 11/2/2023 4:03 PM, Kirill A. Shutemov wrote:
> >> On Thu, Nov 02, 2023 at 11:23:34AM +0530, Nikunj A. Dadhania wrote:
> >>> On 10/30/2023 10:48 PM, Dave Hansen wrote:
> >>>> On 10/29/23 23:36, Nikunj A Dadhania wrote:
> >>>> ...
> >>>>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> >>>>> index 15f97c0abc9d..b0a8546d3703 100644
> >>>>> --- a/arch/x86/kernel/tsc.c
> >>>>> +++ b/arch/x86/kernel/tsc.c
> >>>>> @@ -1241,7 +1241,7 @@ static void __init check_system_tsc_reliable(void)
> >>>>>  			tsc_clocksource_reliable = 1;
> >>>>>  	}
> >>>>>  #endif
> >>>>> -	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE))
> >>>>> +	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE) || cc_platform_has(CC_ATTR_GUEST_SECURE_TSC))
> >>>>>  		tsc_clocksource_reliable = 1;
> >>>>
> >>>> Why can't you just set X86_FEATURE_TSC_RELIABLE?
> >>>
> >>> Last time when I tried, I had removed my kvmclock changes and I had set
> >>> the X86_FEATURE_TSC_RELIABLE similar to Kirill's patch[1], this did not
> >>> select the SecureTSC.
> >>>
> >>> Let me try setting X86_FEATURE_TSC_RELIABLE and retaining my patch for
> >>> skipping kvmclock.
> >>
> >> kvmclock lowers its rating if TSC is good enough:
> >>
> >> 	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> >> 	    boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
> >> 	    !check_tsc_unstable())
> >> 		kvm_clock.rating = 299;
> >>
> >> Does your TSC meet the requirements?
> > 
> > I have set TscInvariant (bit 8) in CPUID_8000_0007_edx and TSC is set as reliable.
> > 
> > With this I see kvm_clock rating being lowered, but kvm-clock is still being picked as clock-source.
> 
> Ah.. at later point TSC is picked up, is this expected ?
> 
> [    2.564052] clocksource: Switched to clocksource kvm-clock
> [    2.678136] clocksource: Switched to clocksource tsc

On bare metal I see switch from tsc-early to tsc. tsc-early rating is
equal to kvmclock rating after it gets lowered.

Maybe kvmclock rating has to be even lower after detecting sane TSC?

Sean, Paolo, any comments?

Nikunj A. Dadhania Nov. 6, 2023, 11:53 a.m. UTC | #7

On 11/2/2023 6:08 PM, Kirill A. Shutemov wrote:
> On Thu, Nov 02, 2023 at 05:46:26PM +0530, Nikunj A. Dadhania wrote:
>> On 11/2/2023 5:37 PM, Nikunj A. Dadhania wrote:
>>> On 11/2/2023 4:03 PM, Kirill A. Shutemov wrote:
>>>> On Thu, Nov 02, 2023 at 11:23:34AM +0530, Nikunj A. Dadhania wrote:
>>>>> On 10/30/2023 10:48 PM, Dave Hansen wrote:
>>>>>> On 10/29/23 23:36, Nikunj A Dadhania wrote:
>>>>>> ...
>>>>>>> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
>>>>>>> index 15f97c0abc9d..b0a8546d3703 100644
>>>>>>> --- a/arch/x86/kernel/tsc.c
>>>>>>> +++ b/arch/x86/kernel/tsc.c
>>>>>>> @@ -1241,7 +1241,7 @@ static void __init check_system_tsc_reliable(void)
>>>>>>>  			tsc_clocksource_reliable = 1;
>>>>>>>  	}
>>>>>>>  #endif
>>>>>>> -	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE))
>>>>>>> +	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE) || cc_platform_has(CC_ATTR_GUEST_SECURE_TSC))
>>>>>>>  		tsc_clocksource_reliable = 1;
>>>>>>
>>>>>> Why can't you just set X86_FEATURE_TSC_RELIABLE?
>>>>>
>>>>> Last time when I tried, I had removed my kvmclock changes and I had set
>>>>> the X86_FEATURE_TSC_RELIABLE similar to Kirill's patch[1], this did not
>>>>> select the SecureTSC.
>>>>>
>>>>> Let me try setting X86_FEATURE_TSC_RELIABLE and retaining my patch for
>>>>> skipping kvmclock.
>>>>
>>>> kvmclock lowers its rating if TSC is good enough:
>>>>
>>>> 	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
>>>> 	    boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
>>>> 	    !check_tsc_unstable())
>>>> 		kvm_clock.rating = 299;
>>>>
>>>> Does your TSC meet the requirements?
>>>
>>> I have set TscInvariant (bit 8) in CPUID_8000_0007_edx and TSC is set as reliable.
>>>
>>> With this I see kvm_clock rating being lowered, but kvm-clock is still being picked as clock-source.
>>
>> Ah.. at later point TSC is picked up, is this expected ?
>>
>> [    2.564052] clocksource: Switched to clocksource kvm-clock
>> [    2.678136] clocksource: Switched to clocksource tsc
> 
> On bare metal I see switch from tsc-early to tsc. tsc-early rating is
> equal to kvmclock rating after it gets lowered.

For SNP guest with secure tsc enabled, kvm-clock and tsc-early both are at 299.
Initially, kvm-clock is selected as clocksource and when tsc with 300 rating is enqueued, 
clocksource then switches to tsc.

[    0.004231] clocksource: clocksource_enqueue: name kvm-clock rating 299
[...]
[    2.046319] clocksource: clocksource_enqueue: name tsc-early rating 299
[...]
[    3.399179] clocksource: Switched to clocksource kvm-clock
[...]
[    3.513652] clocksource: clocksource_enqueue: name tsc rating 300
[    3.517314] clocksource: Switched to clocksource tsc
 
> Maybe kvmclock rating has to be even lower after detecting sane TSC?

If I set kvmclock rating to 298, I do see exact behavior as you have seen on the bare-metal.

[    0.004520] clocksource: clocksource_enqueue: name kvm-clock rating 298
[...]
[    1.827422] clocksource: clocksource_enqueue: name tsc-early rating 299
[...]
[    3.485059] clocksource: Switched to clocksource tsc-early
[...]
[    3.623625] clocksource: clocksource_enqueue: name tsc rating 300
[    3.628954] clocksource: Switched to clocksource tsc

Regards
Nikunj

Kirill A. Shutemov Nov. 6, 2023, 1:03 p.m. UTC | #8

On Mon, Nov 06, 2023 at 05:23:44PM +0530, Nikunj A. Dadhania wrote:
> > Maybe kvmclock rating has to be even lower after detecting sane TSC?
> 
> If I set kvmclock rating to 298, I do see exact behavior as you have seen on the bare-metal.
> 
> [    0.004520] clocksource: clocksource_enqueue: name kvm-clock rating 298
> [...]
> [    1.827422] clocksource: clocksource_enqueue: name tsc-early rating 299
> [...]
> [    3.485059] clocksource: Switched to clocksource tsc-early
> [...]
> [    3.623625] clocksource: clocksource_enqueue: name tsc rating 300
> [    3.628954] clocksource: Switched to clocksource tsc

This looks more reasonable to me. But I don't really understand
timekeeping. It would be nice to hear from someone who knows what he
saying.

diff mbox series

Patch

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 15f97c0abc9d..b0a8546d3703 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1241,7 +1241,7 @@  static void __init check_system_tsc_reliable(void)
 			tsc_clocksource_reliable = 1;
 	}
 #endif
-	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE))
+	if (boot_cpu_has(X86_FEATURE_TSC_RELIABLE) || cc_platform_has(CC_ATTR_GUEST_SECURE_TSC))
 		tsc_clocksource_reliable = 1;
 
 	/*