diff mbox series

[PATCHv4,05/14] x86/kvm: Do not try to disable kvmclock if it was not enabled

Message ID 20231205004510.27164-6-kirill.shutemov@linux.intel.com
State New
Headers
Series x86/tdx: Add kexec support |

Commit Message

Kirill A. Shutemov Dec. 5, 2023, 12:45 a.m. UTC 
  kvm_guest_cpu_offline() tries to disable kvmclock regardless if it is
present in the VM. It leads to write to a MSR that doesn't exist on some
configurations, namely in TDX guest:

	unchecked MSR access error: WRMSR to 0x12 (tried to write 0x0000000000000000)
	at rIP: 0xffffffff8110687c (kvmclock_disable+0x1c/0x30)

kvmclock enabling is gated by CLOCKSOURCE and CLOCKSOURCE2 KVM paravirt
features.

Do not disable kvmclock if it was not enabled.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: c02027b5742b ("x86/kvm: Disable kvmclock on all CPUs on shutdown")
Reviewed-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kernel/kvmclock.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Kirill A. Shutemov Dec. 11, 2023, 11:10 p.m. UTC | #1 
On Tue, Dec 05, 2023 at 03:45:01AM +0300, Kirill A. Shutemov wrote:
> kvm_guest_cpu_offline() tries to disable kvmclock regardless if it is
> present in the VM. It leads to write to a MSR that doesn't exist on some
> configurations, namely in TDX guest:
> 
> 	unchecked MSR access error: WRMSR to 0x12 (tried to write 0x0000000000000000)
> 	at rIP: 0xffffffff8110687c (kvmclock_disable+0x1c/0x30)
> 
> kvmclock enabling is gated by CLOCKSOURCE and CLOCKSOURCE2 KVM paravirt
> features.
> 
> Do not disable kvmclock if it was not enabled.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Fixes: c02027b5742b ("x86/kvm: Disable kvmclock on all CPUs on shutdown")
> Reviewed-by: Sean Christopherson <seanjc@google.com>
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Wanpeng Li <wanpengli@tencent.com>

Paolo, Sean, any chance you can get it in through KVM tree while the rest
of kexec patchset is pending? The problem is visible on normal reboot too.
Sean Christopherson Dec. 13, 2023, 5:22 p.m. UTC | #2 
On Tue, Dec 12, 2023, Kirill A. Shutemov wrote:
> On Tue, Dec 05, 2023 at 03:45:01AM +0300, Kirill A. Shutemov wrote:
> > kvm_guest_cpu_offline() tries to disable kvmclock regardless if it is
> > present in the VM. It leads to write to a MSR that doesn't exist on some
> > configurations, namely in TDX guest:
> > 
> > 	unchecked MSR access error: WRMSR to 0x12 (tried to write 0x0000000000000000)
> > 	at rIP: 0xffffffff8110687c (kvmclock_disable+0x1c/0x30)
> > 
> > kvmclock enabling is gated by CLOCKSOURCE and CLOCKSOURCE2 KVM paravirt
> > features.
> > 
> > Do not disable kvmclock if it was not enabled.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Fixes: c02027b5742b ("x86/kvm: Disable kvmclock on all CPUs on shutdown")
> > Reviewed-by: Sean Christopherson <seanjc@google.com>
> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Wanpeng Li <wanpengli@tencent.com>
> 
> Paolo, Sean, any chance you can get it in through KVM tree while the rest
> of kexec patchset is pending? The problem is visible on normal reboot too.

Paolo is going to grab this (possibly for 6.7-rc?).  I'll keep this tagged on my
end in case that doesn't happen "soon".
Kirill A. Shutemov Jan. 4, 2024, 3:05 p.m. UTC | #3 
On Wed, Dec 13, 2023 at 09:22:34AM -0800, Sean Christopherson wrote:
> On Tue, Dec 12, 2023, Kirill A. Shutemov wrote:
> > On Tue, Dec 05, 2023 at 03:45:01AM +0300, Kirill A. Shutemov wrote:
> > > kvm_guest_cpu_offline() tries to disable kvmclock regardless if it is
> > > present in the VM. It leads to write to a MSR that doesn't exist on some
> > > configurations, namely in TDX guest:
> > > 
> > > 	unchecked MSR access error: WRMSR to 0x12 (tried to write 0x0000000000000000)
> > > 	at rIP: 0xffffffff8110687c (kvmclock_disable+0x1c/0x30)
> > > 
> > > kvmclock enabling is gated by CLOCKSOURCE and CLOCKSOURCE2 KVM paravirt
> > > features.
> > > 
> > > Do not disable kvmclock if it was not enabled.
> > > 
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > Fixes: c02027b5742b ("x86/kvm: Disable kvmclock on all CPUs on shutdown")
> > > Reviewed-by: Sean Christopherson <seanjc@google.com>
> > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Wanpeng Li <wanpengli@tencent.com>
> > 
> > Paolo, Sean, any chance you can get it in through KVM tree while the rest
> > of kexec patchset is pending? The problem is visible on normal reboot too.
> 
> Paolo is going to grab this (possibly for 6.7-rc?).  I'll keep this tagged on my
> end in case that doesn't happen "soon".

Sean, any update on this?
Sean Christopherson Jan. 9, 2024, 2:59 p.m. UTC | #4 
On Thu, Jan 04, 2024, Kirill A. Shutemov wrote:
> On Wed, Dec 13, 2023 at 09:22:34AM -0800, Sean Christopherson wrote:
> > On Tue, Dec 12, 2023, Kirill A. Shutemov wrote:
> > > On Tue, Dec 05, 2023 at 03:45:01AM +0300, Kirill A. Shutemov wrote:
> > > > kvm_guest_cpu_offline() tries to disable kvmclock regardless if it is
> > > > present in the VM. It leads to write to a MSR that doesn't exist on some
> > > > configurations, namely in TDX guest:
> > > > 
> > > > 	unchecked MSR access error: WRMSR to 0x12 (tried to write 0x0000000000000000)
> > > > 	at rIP: 0xffffffff8110687c (kvmclock_disable+0x1c/0x30)
> > > > 
> > > > kvmclock enabling is gated by CLOCKSOURCE and CLOCKSOURCE2 KVM paravirt
> > > > features.
> > > > 
> > > > Do not disable kvmclock if it was not enabled.
> > > > 
> > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > Fixes: c02027b5742b ("x86/kvm: Disable kvmclock on all CPUs on shutdown")
> > > > Reviewed-by: Sean Christopherson <seanjc@google.com>
> > > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Cc: Wanpeng Li <wanpengli@tencent.com>
> > > 
> > > Paolo, Sean, any chance you can get it in through KVM tree while the rest
> > > of kexec patchset is pending? The problem is visible on normal reboot too.
> > 
> > Paolo is going to grab this (possibly for 6.7-rc?).  I'll keep this tagged on my
> > end in case that doesn't happen "soon".
> 
> Sean, any update on this?

'Tis now in kvm/next, commit 1c6d984f523f ("x86/kvm: Do not try to disable kvmclock
if it was not enabled").  The one time procrastinating on responding actually worked. ;-)
diff mbox series

Patch

diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
index fb8f52149be9..f2fff625576d 100644
--- a/arch/x86/kernel/kvmclock.c
+++ b/arch/x86/kernel/kvmclock.c
@@ -24,8 +24,8 @@ 
 
 static int kvmclock __initdata = 1;
 static int kvmclock_vsyscall __initdata = 1;
-static int msr_kvm_system_time __ro_after_init = MSR_KVM_SYSTEM_TIME;
-static int msr_kvm_wall_clock __ro_after_init = MSR_KVM_WALL_CLOCK;
+static int msr_kvm_system_time __ro_after_init;
+static int msr_kvm_wall_clock __ro_after_init;
 static u64 kvm_sched_clock_offset __ro_after_init;
 
 static int __init parse_no_kvmclock(char *arg)
@@ -195,7 +195,8 @@  static void kvm_setup_secondary_clock(void)
 
 void kvmclock_disable(void)
 {
-	native_write_msr(msr_kvm_system_time, 0, 0);
+	if (msr_kvm_system_time)
+		native_write_msr(msr_kvm_system_time, 0, 0);
 }
 
 static void __init kvmclock_init_mem(void)
@@ -294,7 +295,10 @@  void __init kvmclock_init(void)
 	if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE2)) {
 		msr_kvm_system_time = MSR_KVM_SYSTEM_TIME_NEW;
 		msr_kvm_wall_clock = MSR_KVM_WALL_CLOCK_NEW;
-	} else if (!kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE)) {
+	} else if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE)) {
+		msr_kvm_system_time = MSR_KVM_SYSTEM_TIME;
+		msr_kvm_wall_clock = MSR_KVM_WALL_CLOCK;
+	} else {
 		return;
 	}