| Message ID | 20200416101509.73526-2-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:07PM +0800, Xiaoyao Li wrote: > To make it more clear that the flag means DRn (except DR7) need to be > reloaded before vm entry. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 2 +- > arch/x86/kvm/x86.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index c7da23aed79a..f465c76e6e5a 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -511,7 +511,7 @@ struct kvm_pmu_ops; > enum { > KVM_DEBUGREG_BP_ENABLED = 1, > KVM_DEBUGREG_WONT_EXIT = 2, > - KVM_DEBUGREG_RELOAD = 4, > + KVM_DEBUGREG_NEED_RELOAD = 4, My vote would be for KVM_DEBUGREG_DIRTY Any bit that is set switch_db_regs triggers a reload, whereas I would expect a RELOAD flag to be set _every_ time a load is needed and thus be the only bit that's checked > }; > > struct kvm_mtrr_range { > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index de77bc9bd0d7..cce926658d10 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1067,7 +1067,7 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu) > if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) { > for (i = 0; i < KVM_NR_DB_REGS; i++) > vcpu->arch.eff_db[i] = vcpu->arch.db[i]; > - vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD; > + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; > } > } > > @@ -8407,7 +8407,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > set_debugreg(vcpu->arch.eff_db[2], 2); > set_debugreg(vcpu->arch.eff_db[3], 3); > set_debugreg(vcpu->arch.dr6, 6); > - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; > + vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD; > } > > kvm_x86_ops.run(vcpu); > @@ -8424,7 +8424,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > kvm_update_dr0123(vcpu); > kvm_update_dr6(vcpu); > kvm_update_dr7(vcpu); > - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; > + vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD; This is the path that I think would really benefit from DIRTY, it took me several reads to catch that kvm_update_dr0123() will set RELOAD. > } > > /* > -- > 2.20.1 >
On 4/24/2020 3:09 AM, Sean Christopherson wrote: > On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote: >> To make it more clear that the flag means DRn (except DR7) need to be >> reloaded before vm entry. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> >> --- >> arch/x86/include/asm/kvm_host.h | 2 +- >> arch/x86/kvm/x86.c | 6 +++--- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> index c7da23aed79a..f465c76e6e5a 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -511,7 +511,7 @@ struct kvm_pmu_ops; >> enum { >> KVM_DEBUGREG_BP_ENABLED = 1, >> KVM_DEBUGREG_WONT_EXIT = 2, >> - KVM_DEBUGREG_RELOAD = 4, >> + KVM_DEBUGREG_NEED_RELOAD = 4, > > My vote would be for KVM_DEBUGREG_DIRTY Any bit that is set switch_db_regs > triggers a reload, whereas I would expect a RELOAD flag to be set _every_ > time a load is needed and thus be the only bit that's checked > That's what I intended to do with this series, apparently I failed it. :( >> }; >> >> struct kvm_mtrr_range { >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index de77bc9bd0d7..cce926658d10 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -1067,7 +1067,7 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu) >> if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) { >> for (i = 0; i < KVM_NR_DB_REGS; i++) >> vcpu->arch.eff_db[i] = vcpu->arch.db[i]; >> - vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD; >> + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; >> } >> } >> >> @@ -8407,7 +8407,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >> set_debugreg(vcpu->arch.eff_db[2], 2); >> set_debugreg(vcpu->arch.eff_db[3], 3); >> set_debugreg(vcpu->arch.dr6, 6); >> - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; >> + vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD; >> } >> >> kvm_x86_ops.run(vcpu); >> @@ -8424,7 +8424,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >> kvm_update_dr0123(vcpu); >> kvm_update_dr6(vcpu); >> kvm_update_dr7(vcpu); >> - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; >> + vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD; > > This is the path that I think would really benefit from DIRTY, it took me > several reads to catch that kvm_update_dr0123() will set RELOAD. > >> } >> >> /* >> -- >> 2.20.1 >>
On Thu, Apr 23, 2020 at 12:09:42PM -0700, Sean Christopherson wrote: > On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote: > > To make it more clear that the flag means DRn (except DR7) need to be > > reloaded before vm entry. > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > --- > > arch/x86/include/asm/kvm_host.h | 2 +- > > arch/x86/kvm/x86.c | 6 +++--- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index c7da23aed79a..f465c76e6e5a 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -511,7 +511,7 @@ struct kvm_pmu_ops; > > enum { > > KVM_DEBUGREG_BP_ENABLED = 1, > > KVM_DEBUGREG_WONT_EXIT = 2, > > - KVM_DEBUGREG_RELOAD = 4, > > + KVM_DEBUGREG_NEED_RELOAD = 4, > > My vote would be for KVM_DEBUGREG_DIRTY Any bit that is set switch_db_regs > triggers a reload, whereas I would expect a RELOAD flag to be set _every_ > time a load is needed and thus be the only bit that's checked But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every time before vmenter? Then it'll somehow go back to switch_db_regs, iiuc... IIUC RELOAD actually wants to say "reload only for this iteration", that's why it's cleared after each reload. So maybe... RELOAD_ONCE? (Btw, do we have debug regs tests somewhere no matter inside guest or with KVM_SET_GUEST_DEBUG?) Thanks,
On Fri, Apr 24, 2020 at 04:21:03PM -0400, Peter Xu wrote: > On Thu, Apr 23, 2020 at 12:09:42PM -0700, Sean Christopherson wrote: > > On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote: > > > To make it more clear that the flag means DRn (except DR7) need to be > > > reloaded before vm entry. > > > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > > --- > > > arch/x86/include/asm/kvm_host.h | 2 +- > > > arch/x86/kvm/x86.c | 6 +++--- > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > index c7da23aed79a..f465c76e6e5a 100644 > > > --- a/arch/x86/include/asm/kvm_host.h > > > +++ b/arch/x86/include/asm/kvm_host.h > > > @@ -511,7 +511,7 @@ struct kvm_pmu_ops; > > > enum { > > > KVM_DEBUGREG_BP_ENABLED = 1, > > > KVM_DEBUGREG_WONT_EXIT = 2, > > > - KVM_DEBUGREG_RELOAD = 4, > > > + KVM_DEBUGREG_NEED_RELOAD = 4, > > > > My vote would be for KVM_DEBUGREG_DIRTY Any bit that is set switch_db_regs > > triggers a reload, whereas I would expect a RELOAD flag to be set _every_ > > time a load is needed and thus be the only bit that's checked > > But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every > time before vmenter? Then it'll somehow go back to switch_db_regs, iiuc... > > IIUC RELOAD actually wants to say "reload only for this iteration", that's why > it's cleared after each reload. So maybe... RELOAD_ONCE? Or FORCE_LOAD, or FORCE_RELOAD? Those crossed my mind as well. > (Btw, do we have debug regs tests somewhere no matter inside guest or with > KVM_SET_GUEST_DEBUG?) I don't think so?
On Fri, Apr 24, 2020 at 01:29:22PM -0700, Sean Christopherson wrote: > On Fri, Apr 24, 2020 at 04:21:03PM -0400, Peter Xu wrote: > > On Thu, Apr 23, 2020 at 12:09:42PM -0700, Sean Christopherson wrote: > > > On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote: > > > > To make it more clear that the flag means DRn (except DR7) need to be > > > > reloaded before vm entry. > > > > > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > > > --- > > > > arch/x86/include/asm/kvm_host.h | 2 +- > > > > arch/x86/kvm/x86.c | 6 +++--- > > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > > index c7da23aed79a..f465c76e6e5a 100644 > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > @@ -511,7 +511,7 @@ struct kvm_pmu_ops; > > > > enum { > > > > KVM_DEBUGREG_BP_ENABLED = 1, > > > > KVM_DEBUGREG_WONT_EXIT = 2, > > > > - KVM_DEBUGREG_RELOAD = 4, > > > > + KVM_DEBUGREG_NEED_RELOAD = 4, > > > > > > My vote would be for KVM_DEBUGREG_DIRTY Any bit that is set switch_db_regs > > > triggers a reload, whereas I would expect a RELOAD flag to be set _every_ > > > time a load is needed and thus be the only bit that's checked > > > > But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every > > time before vmenter? Then it'll somehow go back to switch_db_regs, iiuc... > > > > IIUC RELOAD actually wants to say "reload only for this iteration", that's why > > it's cleared after each reload. So maybe... RELOAD_ONCE? > > Or FORCE_LOAD, or FORCE_RELOAD? Those crossed my mind as well. Yep, FORCE_RELOAD sounds better than DIRTY. > > > (Btw, do we have debug regs tests somewhere no matter inside guest or with > > KVM_SET_GUEST_DEBUG?) > > I don't think so? OK, I'll see whether I can write some up. Thanks,
On 24/04/20 22:21, Peter Xu wrote: > But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every > time before vmenter? Then it'll somehow go back to switch_db_regs, iiuc... > > IIUC RELOAD actually wants to say "reload only for this iteration", that's why > it's cleared after each reload. So maybe... RELOAD_ONCE? > > (Btw, do we have debug regs tests somewhere no matter inside guest or with > KVM_SET_GUEST_DEBUG?) What about KVM_DEBUGREG_EFF_DB_DIRTY? We have them in kvm-unit-tests for debug regs inside the guest, but no selftests covering KVM_SET_GUEST_DEBUG. Paolo
On 16/04/20 12:15, Xiaoyao Li wrote: > To make it more clear that the flag means DRn (except DR7) need to be > reloaded before vm entry. > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> I wonder if KVM_DEBUGREG_RELOAD is needed at all. It should be easy to write selftests for it, using the testcase in commit message 172b2386ed16 and the information in commit ae561edeb421. Paolo > --- > arch/x86/include/asm/kvm_host.h | 2 +- > arch/x86/kvm/x86.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index c7da23aed79a..f465c76e6e5a 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -511,7 +511,7 @@ struct kvm_pmu_ops; > enum { > KVM_DEBUGREG_BP_ENABLED = 1, > KVM_DEBUGREG_WONT_EXIT = 2, > - KVM_DEBUGREG_RELOAD = 4, > + KVM_DEBUGREG_NEED_RELOAD = 4, > }; > > struct kvm_mtrr_range { > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index de77bc9bd0d7..cce926658d10 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1067,7 +1067,7 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu) > if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) { > for (i = 0; i < KVM_NR_DB_REGS; i++) > vcpu->arch.eff_db[i] = vcpu->arch.db[i]; > - vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD; > + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; > } > } > > @@ -8407,7 +8407,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > set_debugreg(vcpu->arch.eff_db[2], 2); > set_debugreg(vcpu->arch.eff_db[3], 3); > set_debugreg(vcpu->arch.dr6, 6); > - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; > + vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD; > } > > kvm_x86_ops.run(vcpu); > @@ -8424,7 +8424,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > kvm_update_dr0123(vcpu); > kvm_update_dr6(vcpu); > kvm_update_dr7(vcpu); > - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; > + vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD; > } > > /* >
> On Apr 25, 2020, at 1:07 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 16/04/20 12:15, Xiaoyao Li wrote: >> To make it more clear that the flag means DRn (except DR7) need to be >> reloaded before vm entry. >> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> > > I wonder if KVM_DEBUGREG_RELOAD is needed at all. It should be easy to > write selftests for it, using the testcase in commit message > 172b2386ed16 and the information in commit ae561edeb421. I must be missing something, since I did not follow this thread and other KVM changes very closely. Yet, for the record, I added KVM_DEBUGREG_RELOAD due to real experienced issues that I had while running Intel’s fuzzing tests on KVM: IIRC, the DRs were not reloaded after an INIT event that clears them. Personally, I would prefer that a test for that, if added, would be added to KVM-unit-tests, based on Liran’s INIT test. This would allow to confirm bare-metal behaves as the VM.
On 25/04/20 18:54, Nadav Amit wrote: >> I wonder if KVM_DEBUGREG_RELOAD is needed at all. It should be easy to >> write selftests for it, using the testcase in commit message >> 172b2386ed16 and the information in commit ae561edeb421. > I must be missing something, since I did not follow this thread and other > KVM changes very closely. > > Yet, for the record, I added KVM_DEBUGREG_RELOAD due to real experienced > issues that I had while running Intel’s fuzzing tests on KVM: IIRC, the DRs > were not reloaded after an INIT event that clears them. Indeed, but the code has changed since then and I'm not sure it is still needed. > Personally, I would prefer that a test for that, if added, would be added > to KVM-unit-tests, based on Liran’s INIT test. This would allow to confirm > bare-metal behaves as the VM. Yes, that would be good as well of course. Paolo
On Sat, Apr 25, 2020 at 09:48:17AM +0200, Paolo Bonzini wrote: > On 24/04/20 22:21, Peter Xu wrote: > > But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every > > time before vmenter? Then it'll somehow go back to switch_db_regs, iiuc... > > > > IIUC RELOAD actually wants to say "reload only for this iteration", that's why > > it's cleared after each reload. So maybe... RELOAD_ONCE? > > > > (Btw, do we have debug regs tests somewhere no matter inside guest or with > > KVM_SET_GUEST_DEBUG?) > > What about KVM_DEBUGREG_EFF_DB_DIRTY? The problem is iiuc we always reload eff_db[] no matter which bit in switch_db_regs is set, so this may still not clearly identify this bit from the rest of the two bits... Actually I think eff_db[] is a bit confusing here in that it can be either the host specified dbreg values or the guest specified depends on the dynamic value of KVM_GUESTDBG_USE_HW_BP. I am thinking maybe it's clearer to have host_db[] and guest_db[], then only until vmenter do we load either of them by: if (KVM_GUESTDBG_USE_HW_BP) load(host_db[]); else load(gueet_db[]); Then each db[] will be very clear on what's the data is about. And we don't need to check KVM_GUESTDBG_USE_HW_BP every time when accessing eff_db[]. > > We have them in kvm-unit-tests for debug regs inside the guest, but no > selftests covering KVM_SET_GUEST_DEBUG. I see! Thanks,
On 4/27/2020 10:37 PM, Peter Xu wrote: > On Sat, Apr 25, 2020 at 09:48:17AM +0200, Paolo Bonzini wrote: >> On 24/04/20 22:21, Peter Xu wrote: >>> But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every >>> time before vmenter? Then it'll somehow go back to switch_db_regs, iiuc... >>> >>> IIUC RELOAD actually wants to say "reload only for this iteration", that's why >>> it's cleared after each reload. So maybe... RELOAD_ONCE? >>> >>> (Btw, do we have debug regs tests somewhere no matter inside guest or with >>> KVM_SET_GUEST_DEBUG?) >> >> What about KVM_DEBUGREG_EFF_DB_DIRTY? > > The problem is iiuc we always reload eff_db[] no matter which bit in > switch_db_regs is set, so this may still not clearly identify this bit from the > rest of the two bits... > > Actually I think eff_db[] is a bit confusing here in that it can be either the > host specified dbreg values or the guest specified depends on the dynamic value > of KVM_GUESTDBG_USE_HW_BP. > > I am thinking maybe it's clearer to have host_db[] and guest_db[], then only > until vmenter do we load either of them by: host_db[] is somewhat misleading, how about user_db[] (just like user_fpu) > if (KVM_GUESTDBG_USE_HW_BP) > load(host_db[]); > else > load(gueet_db[]); > > Then each db[] will be very clear on what's the data is about. And we don't > need to check KVM_GUESTDBG_USE_HW_BP every time when accessing eff_db[]. >
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index c7da23aed79a..f465c76e6e5a 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -511,7 +511,7 @@ struct kvm_pmu_ops; enum { KVM_DEBUGREG_BP_ENABLED = 1, KVM_DEBUGREG_WONT_EXIT = 2, - KVM_DEBUGREG_RELOAD = 4, + KVM_DEBUGREG_NEED_RELOAD = 4, }; struct kvm_mtrr_range { diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index de77bc9bd0d7..cce926658d10 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1067,7 +1067,7 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu) if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) { for (i = 0; i < KVM_NR_DB_REGS; i++) vcpu->arch.eff_db[i] = vcpu->arch.db[i]; - vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD; + vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD; } } @@ -8407,7 +8407,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) set_debugreg(vcpu->arch.eff_db[2], 2); set_debugreg(vcpu->arch.eff_db[3], 3); set_debugreg(vcpu->arch.dr6, 6); - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; + vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD; } kvm_x86_ops.run(vcpu); @@ -8424,7 +8424,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) kvm_update_dr0123(vcpu); kvm_update_dr6(vcpu); kvm_update_dr7(vcpu); - vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; + vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD; } /*
To make it more clear that the flag means DRn (except DR7) need to be reloaded before vm entry. Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- arch/x86/include/asm/kvm_host.h | 2 +- arch/x86/kvm/x86.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)