On Tue, Apr 05, 2022, Paolo Bonzini wrote:
> On 3/4/22 20:49, isaku.yamahata@xxxxxxxxx wrote:
> > 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 45e8a02e99bf..89d04cd64cd0 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10084,7 +10084,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);
>
> I'm confused. I'm probably missing something obvious, but where does
> KVM_DEBUGREG_AUTO_SWITCH affect the behavior of KVM? vcpu_enter_guest
> would still write %db0-%db3 if KVM_DEBUGREG_BP_ENABLED is set, for
> example.
>
> Do you mean:
>
> if (unlikely(vcpu->arch.switch_db_regs) &&
> !unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH)) {
>
> ?
No, you're confused there's a crucial chunk of this patch that's missing. The
host restore path was originally
if (hw_breakpoint_active() &&
!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCHED))
hw_breakpoint_restore();
The TDX module context switches the guest _and_ host debug registers. It restores
the host DRs because it needs to write _something_ to hide guest state, so it might
as well restore the host values. The above was an optmization to avoid rewriting
all debug registers.
However, this patch was written before commit f85d40160691 ("KVM: X86: Disable
hardware breakpoints unconditionally before kvm_x86->run()"). I suspect that when
rebasing to newer KVM, the Intel folks discovered that DR7 isn't getting restored
because the GUEST_DR7 in the SEAM VMCS will hold the zero'd value.
But rather than completely drop the flag, KVM can still avoid writing everything
except DR7, i.e.
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);
}
with a comment explaining that DR7 always needs to be restored because it's
preemptively cleared to play nice with the non-instrumentable sections.
