Message ID | 20231030063652.68675-14-nikunj@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Add Secure TSC support for SNP guests | expand |
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?
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/
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?
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
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
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?
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
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 --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; /*
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(-)