| Message ID | 20200416101509.73526-3-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: x86: Cleanup and optimazation of switch_db_regs | expand |
On Thu, Apr 16, 2020 at 06:15:08PM +0800, Xiaoyao Li wrote: > Once any #BP enabled in DR7, it will set KVM_DEBUGREG_BP_ENABLED, which > leads to reload DRn before every VM entry even if none of DRn changed. > > Drop KVM_DEBUGREG_BP_ENABLED flag and set KVM_DEBUGREG_NEED_RELOAD flag > for the cases that DRn need to be reloaded instead, to avoid unnecessary > DRn reload. Loading DRs on every VM-Enter _is_ necessary if there are breakpoints enabled for the guest. The hardware DR values are not "stable", e.g. they are loaded with the host's values immediately after saving the guest's value (if DR_EXITING is disabled) in vcpu_enter_guest(), notably iff the host has an active/enabled breakpoint. My bet is that DRs can be changed from interrupt context as well. Loading DRs for the guest (not necessarily the same as the guest's DRs) is necessary if a breakpoint is enabled so that the #DB is actually hit in guest. It's a similar concept to instructions that consume MSR values, e.g. SYSCALL, RDTSCP, etc..., even if KVM intercepts the MSR/DR, hardware still needs the correct value so that the guest behavior is correct. > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 3 +-- > arch/x86/kvm/x86.c | 4 ++-- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index f465c76e6e5a..87e2d020351e 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -509,9 +509,8 @@ struct kvm_pmu { > struct kvm_pmu_ops; > > enum { > - KVM_DEBUGREG_BP_ENABLED = 1, > + KVM_DEBUGREG_NEED_RELOAD = 1, > KVM_DEBUGREG_WONT_EXIT = 2, > - KVM_DEBUGREG_NEED_RELOAD = 4, > }; > > struct kvm_mtrr_range { > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index cce926658d10..71264df64001 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1086,9 +1086,8 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu) > else > dr7 = vcpu->arch.dr7; > kvm_x86_ops.set_dr7(vcpu, dr7); > - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_BP_ENABLED; > if (dr7 & DR7_BP_EN_MASK) > - vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED; > + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; > } > > static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu) > @@ -1128,6 +1127,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) > break; > } > > + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; > return 0; > } > > -- > 2.20.1 >
On 4/24/2020 3:29 AM, Sean Christopherson wrote: > On Thu, Apr 16, 2020 at 06:15:08PM +0800, Xiaoyao Li wrote: >> Once any #BP enabled in DR7, it will set KVM_DEBUGREG_BP_ENABLED, which >> leads to reload DRn before every VM entry even if none of DRn changed. >> >> Drop KVM_DEBUGREG_BP_ENABLED flag and set KVM_DEBUGREG_NEED_RELOAD flag >> for the cases that DRn need to be reloaded instead, to avoid unnecessary >> DRn reload. > > Loading DRs on every VM-Enter _is_ necessary if there are breakpoints > enabled for the guest. The hardware DR values are not "stable", e.g. they > are loaded with the host's values immediately after saving the guest's > value (if DR_EXITING is disabled) in vcpu_enter_guest(), notably iff the > host has an active/enabled breakpoint. May bad, bbviously I didn't think about it. > My bet is that DRs can be changed > from interrupt context as well. So set KVM_DEBUGREG_NEED_RELOAD in vcpu_load won't help. > Loading DRs for the guest (not necessarily the same as the guest's DRs) is > necessary if a breakpoint is enabled so that the #DB is actually hit in > guest. It's a similar concept to instructions that consume MSR values, > e.g. SYSCALL, RDTSCP, etc..., even if KVM intercepts the MSR/DR, hardware > still needs the correct value so that the guest behavior is correct. > >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> arch/x86/include/asm/kvm_host.h | 3 +-- >> arch/x86/kvm/x86.c | 4 ++-- >> 2 files changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> index f465c76e6e5a..87e2d020351e 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -509,9 +509,8 @@ struct kvm_pmu { >> struct kvm_pmu_ops; >> >> enum { >> - KVM_DEBUGREG_BP_ENABLED = 1, >> + KVM_DEBUGREG_NEED_RELOAD = 1, >> KVM_DEBUGREG_WONT_EXIT = 2, >> - KVM_DEBUGREG_NEED_RELOAD = 4, >> }; >> >> struct kvm_mtrr_range { >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index cce926658d10..71264df64001 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -1086,9 +1086,8 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu) >> else >> dr7 = vcpu->arch.dr7; >> kvm_x86_ops.set_dr7(vcpu, dr7); >> - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_BP_ENABLED; >> if (dr7 & DR7_BP_EN_MASK) >> - vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED; >> + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; >> } >> >> static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu) >> @@ -1128,6 +1127,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) >> break; >> } >> >> + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; >> return 0; >> } >> >> -- >> 2.20.1 >>
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index f465c76e6e5a..87e2d020351e 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -509,9 +509,8 @@ struct kvm_pmu { struct kvm_pmu_ops; enum { - KVM_DEBUGREG_BP_ENABLED = 1, + KVM_DEBUGREG_NEED_RELOAD = 1, KVM_DEBUGREG_WONT_EXIT = 2, - KVM_DEBUGREG_NEED_RELOAD = 4, }; struct kvm_mtrr_range { diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index cce926658d10..71264df64001 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1086,9 +1086,8 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu) else dr7 = vcpu->arch.dr7; kvm_x86_ops.set_dr7(vcpu, dr7); - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_BP_ENABLED; if (dr7 & DR7_BP_EN_MASK) - vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED; + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; } static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu) @@ -1128,6 +1127,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) break; } + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; return 0; }
Once any #BP enabled in DR7, it will set KVM_DEBUGREG_BP_ENABLED, which leads to reload DRn before every VM entry even if none of DRn changed. Drop KVM_DEBUGREG_BP_ENABLED flag and set KVM_DEBUGREG_NEED_RELOAD flag for the cases that DRn need to be reloaded instead, to avoid unnecessary DRn reload. Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- arch/x86/include/asm/kvm_host.h | 3 +-- arch/x86/kvm/x86.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)