[v10,077/108] KVM: x86: Add a switch_db_regs flag to handle TDX's auto-switched behavior
Commit Message
Isaku Yamahata
Oct. 30, 2022, 6:23 a.m. UTC
From: Isaku Yamahata <isaku.yamahata@intel.com> Add a flag, KVM_DEBUGREG_AUTO_SWITCHED_GUEST, to skip saving/restoring DRs irrespective of any other flags. TDX-SEAM unconditionally saves and restores guest DRs and reset to architectural INIT state on TD exit. So, KVM needs to save host DRs before TD enter without restoring guest DRs and restore host DRs after TD exit. Opportunistically convert the KVM_DEBUGREG_* definitions to use BIT(). Reported-by: Xiaoyao Li <xiaoyao.li@intel.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> Co-developed-by: Chao Gao <chao.gao@intel.com> Signed-off-by: Chao Gao <chao.gao@intel.com> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> --- arch/x86/include/asm/kvm_host.h | 9 +++++++-- arch/x86/kvm/vmx/tdx.c | 1 + arch/x86/kvm/x86.c | 11 ++++++++--- 3 files changed, 16 insertions(+), 5 deletions(-)
Comments
On 10/30/2022 2:23 PM, isaku.yamahata@intel.com wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > Add a flag, KVM_DEBUGREG_AUTO_SWITCHED_GUEST, to skip saving/restoring DRs > irrespective of any other flags. TDX-SEAM unconditionally saves and > restores guest DRs and reset to architectural INIT state on TD exit. > So, KVM needs to save host DRs before TD enter without restoring guest DRs > and restore host DRs after TD exit. > > Opportunistically convert the KVM_DEBUGREG_* definitions to use BIT(). > > Reported-by: Xiaoyao Li <xiaoyao.li@intel.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > Co-developed-by: Chao Gao <chao.gao@intel.com> > Signed-off-by: Chao Gao <chao.gao@intel.com> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 9 +++++++-- > arch/x86/kvm/vmx/tdx.c | 1 + > arch/x86/kvm/x86.c | 11 ++++++++--- > 3 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index fdb00d96e954..082e94f78c66 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -583,8 +583,13 @@ struct kvm_pmu { > struct kvm_pmu_ops; > > enum { > - KVM_DEBUGREG_BP_ENABLED = 1, > - KVM_DEBUGREG_WONT_EXIT = 2, > + KVM_DEBUGREG_BP_ENABLED = BIT(0), > + KVM_DEBUGREG_WONT_EXIT = BIT(1), > + /* > + * Guest debug registers are saved/restored by hardware on exit from > + * or enter guest. KVM needn't switch them. > + */ > + KVM_DEBUGREG_AUTO_SWITCH = BIT(2), > }; > > struct kvm_mtrr_range { > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c > index fc4de83a2df8..57767ef3353b 100644 > --- a/arch/x86/kvm/vmx/tdx.c > +++ b/arch/x86/kvm/vmx/tdx.c > @@ -429,6 +429,7 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu) > > vcpu->arch.efer = EFER_SCE | EFER_LME | EFER_LMA | EFER_NX; > > + vcpu->arch.switch_db_regs = KVM_DEBUGREG_AUTO_SWITCH; > vcpu->arch.cr0_guest_owned_bits = -1ul; > vcpu->arch.cr4_guest_owned_bits = -1ul; > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 4d4b71c4cdb1..ad7b227b68dd 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -10779,7 +10779,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > if (vcpu->arch.guest_fpu.xfd_err) > wrmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err); > > - if (unlikely(vcpu->arch.switch_db_regs)) { > + if (unlikely(vcpu->arch.switch_db_regs & ~KVM_DEBUGREG_AUTO_SWITCH)) { > set_debugreg(0, 7); > set_debugreg(vcpu->arch.eff_db[0], 0); > set_debugreg(vcpu->arch.eff_db[1], 1); > @@ -10822,6 +10822,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > */ > if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) { > WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP); > + WARN_ON(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH); > static_call(kvm_x86_sync_dirty_debug_regs)(vcpu); > kvm_update_dr0123(vcpu); > kvm_update_dr7(vcpu); > @@ -10834,8 +10835,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > * care about the messed up debug address registers. But if > * we have some of them active, restore the old state. > */ > - if (hw_breakpoint_active()) > - hw_breakpoint_restore(); > + if (hw_breakpoint_active()) { > + if (!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH)) > + hw_breakpoint_restore(); > + else > + set_debugreg(__this_cpu_read(cpu_dr7), 7); Why only restore dr7 when TD exit? According to the commit message, dr0~dr3 are also reset to architectural INIT value on TD exit. > + } > > vcpu->arch.last_vmentry_cpu = vcpu->cpu; > vcpu->arch.last_guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());
On Wed, Nov 16, 2022 at 10:41:34AM +0800, Binbin Wu <binbin.wu@linux.intel.com> wrote: > > On 10/30/2022 2:23 PM, isaku.yamahata@intel.com wrote: > > From: Isaku Yamahata <isaku.yamahata@intel.com> > > > > Add a flag, KVM_DEBUGREG_AUTO_SWITCHED_GUEST, to skip saving/restoring DRs > > irrespective of any other flags. TDX-SEAM unconditionally saves and > > restores guest DRs and reset to architectural INIT state on TD exit. > > So, KVM needs to save host DRs before TD enter without restoring guest DRs > > and restore host DRs after TD exit. > > > > Opportunistically convert the KVM_DEBUGREG_* definitions to use BIT(). > > > > Reported-by: Xiaoyao Li <xiaoyao.li@intel.com> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Co-developed-by: Chao Gao <chao.gao@intel.com> > > Signed-off-by: Chao Gao <chao.gao@intel.com> > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com> > > --- > > arch/x86/include/asm/kvm_host.h | 9 +++++++-- > > arch/x86/kvm/vmx/tdx.c | 1 + > > arch/x86/kvm/x86.c | 11 ++++++++--- > > 3 files changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index fdb00d96e954..082e94f78c66 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -583,8 +583,13 @@ struct kvm_pmu { > > struct kvm_pmu_ops; > > enum { > > - KVM_DEBUGREG_BP_ENABLED = 1, > > - KVM_DEBUGREG_WONT_EXIT = 2, > > + KVM_DEBUGREG_BP_ENABLED = BIT(0), > > + KVM_DEBUGREG_WONT_EXIT = BIT(1), > > + /* > > + * Guest debug registers are saved/restored by hardware on exit from > > + * or enter guest. KVM needn't switch them. > > + */ > > + KVM_DEBUGREG_AUTO_SWITCH = BIT(2), > > }; > > struct kvm_mtrr_range { > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c > > index fc4de83a2df8..57767ef3353b 100644 > > --- a/arch/x86/kvm/vmx/tdx.c > > +++ b/arch/x86/kvm/vmx/tdx.c > > @@ -429,6 +429,7 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu) > > vcpu->arch.efer = EFER_SCE | EFER_LME | EFER_LMA | EFER_NX; > > + vcpu->arch.switch_db_regs = KVM_DEBUGREG_AUTO_SWITCH; > > vcpu->arch.cr0_guest_owned_bits = -1ul; > > vcpu->arch.cr4_guest_owned_bits = -1ul; > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 4d4b71c4cdb1..ad7b227b68dd 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -10779,7 +10779,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > if (vcpu->arch.guest_fpu.xfd_err) > > wrmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err); > > - if (unlikely(vcpu->arch.switch_db_regs)) { > > + if (unlikely(vcpu->arch.switch_db_regs & ~KVM_DEBUGREG_AUTO_SWITCH)) { > > set_debugreg(0, 7); > > set_debugreg(vcpu->arch.eff_db[0], 0); > > set_debugreg(vcpu->arch.eff_db[1], 1); > > @@ -10822,6 +10822,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > */ > > if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) { > > WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP); > > + WARN_ON(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH); > > static_call(kvm_x86_sync_dirty_debug_regs)(vcpu); > > kvm_update_dr0123(vcpu); > > kvm_update_dr7(vcpu); > > @@ -10834,8 +10835,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > * care about the messed up debug address registers. But if > > * we have some of them active, restore the old state. > > */ > > - if (hw_breakpoint_active()) > > - hw_breakpoint_restore(); > > + if (hw_breakpoint_active()) { > > + if (!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH)) > > + hw_breakpoint_restore(); > > + else > > + set_debugreg(__this_cpu_read(cpu_dr7), 7); > > Why only restore dr7 when TD exit? > > According to the commit message, dr0~dr3 are also reset to architectural > INIT value on TD exit. Probably you're referring to the old version of the spec. context switched: DR0-3, DR6, IA32_DS_AREA reset on TD exit: RFLAGS, IA32_DEBUGCTL, DR7 I'll add a comment.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index fdb00d96e954..082e94f78c66 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -583,8 +583,13 @@ struct kvm_pmu { struct kvm_pmu_ops; enum { - KVM_DEBUGREG_BP_ENABLED = 1, - KVM_DEBUGREG_WONT_EXIT = 2, + KVM_DEBUGREG_BP_ENABLED = BIT(0), + KVM_DEBUGREG_WONT_EXIT = BIT(1), + /* + * Guest debug registers are saved/restored by hardware on exit from + * or enter guest. KVM needn't switch them. + */ + KVM_DEBUGREG_AUTO_SWITCH = BIT(2), }; struct kvm_mtrr_range { diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index fc4de83a2df8..57767ef3353b 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -429,6 +429,7 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu) vcpu->arch.efer = EFER_SCE | EFER_LME | EFER_LMA | EFER_NX; + vcpu->arch.switch_db_regs = KVM_DEBUGREG_AUTO_SWITCH; vcpu->arch.cr0_guest_owned_bits = -1ul; vcpu->arch.cr4_guest_owned_bits = -1ul; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 4d4b71c4cdb1..ad7b227b68dd 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10779,7 +10779,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) if (vcpu->arch.guest_fpu.xfd_err) wrmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err); - if (unlikely(vcpu->arch.switch_db_regs)) { + if (unlikely(vcpu->arch.switch_db_regs & ~KVM_DEBUGREG_AUTO_SWITCH)) { set_debugreg(0, 7); set_debugreg(vcpu->arch.eff_db[0], 0); set_debugreg(vcpu->arch.eff_db[1], 1); @@ -10822,6 +10822,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) */ if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) { WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP); + WARN_ON(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH); static_call(kvm_x86_sync_dirty_debug_regs)(vcpu); kvm_update_dr0123(vcpu); kvm_update_dr7(vcpu); @@ -10834,8 +10835,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) * care about the messed up debug address registers. But if * we have some of them active, restore the old state. */ - if (hw_breakpoint_active()) - hw_breakpoint_restore(); + if (hw_breakpoint_active()) { + if (!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH)) + hw_breakpoint_restore(); + else + set_debugreg(__this_cpu_read(cpu_dr7), 7); + } vcpu->arch.last_vmentry_cpu = vcpu->cpu; vcpu->arch.last_guest_tsc = kvm_read_l1_tsc(vcpu, rdtsc());