Avi Kivity wrote:
> On 09/04/2009 03:51 PM, Jan Kiszka wrote:
>> Based on Avi's suggestion: Do not save the host debug registers on guest
>> entry as they are already present in the thread state. Moreover, only
>> restore them if the current host thread is being debugged. But as KGDB
>> accesses the debug register directly, we have to fall back to existing
>> pattern in that case.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@xxxxxxxxxxx>
>> ---
>>
>> arch/x86/kvm/x86.c | 48 +++++++++++++++++++++++++++++++++---------------
>> 1 files changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 891234b..036a2c5 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -37,6 +37,7 @@
>> #include<linux/iommu.h>
>> #include<linux/intel-iommu.h>
>> #include<linux/cpufreq.h>
>> +#include<linux/kgdb.h>
>> #include<trace/events/kvm.h>
>> #undef TRACE_INCLUDE_FILE
>> #define CREATE_TRACE_POINTS
>> @@ -3627,14 +3628,21 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>
>> kvm_guest_enter();
>>
>> - get_debugreg(vcpu->arch.host_dr6, 6);
>> - get_debugreg(vcpu->arch.host_dr7, 7);
>> + /*
>> + * kgdb accesses the debug registers directly, so we have to save them
>> + * and restore those values on return from the guest.
>> + */
>> + if (unlikely(kgdb_in_use())) {
>> + if (unlikely(vcpu->arch.switch_db_regs)) {
>> + get_debugreg(vcpu->arch.host_db[0], 0);
>> + get_debugreg(vcpu->arch.host_db[1], 1);
>> + get_debugreg(vcpu->arch.host_db[2], 2);
>> + get_debugreg(vcpu->arch.host_db[3], 3);
>> + }
>> + get_debugreg(vcpu->arch.host_dr6, 6);
>> + get_debugreg(vcpu->arch.host_dr7, 7);
>> + }
>> if (unlikely(vcpu->arch.switch_db_regs)) {
>> - get_debugreg(vcpu->arch.host_db[0], 0);
>> - get_debugreg(vcpu->arch.host_db[1], 1);
>> - get_debugreg(vcpu->arch.host_db[2], 2);
>> - get_debugreg(vcpu->arch.host_db[3], 3);
>> -
>> set_debugreg(0, 7);
>> set_debugreg(vcpu->arch.eff_db[0], 0);
>> set_debugreg(vcpu->arch.eff_db[1], 1);
>> @@ -3645,15 +3653,25 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>> trace_kvm_entry(vcpu->vcpu_id);
>> kvm_x86_ops->run(vcpu);
>>
>> - if (unlikely(vcpu->arch.switch_db_regs)) {
>> - set_debugreg(0, 7);
>> - set_debugreg(vcpu->arch.host_db[0], 0);
>> - set_debugreg(vcpu->arch.host_db[1], 1);
>> - set_debugreg(vcpu->arch.host_db[2], 2);
>> - set_debugreg(vcpu->arch.host_db[3], 3);
>> + if (unlikely(kgdb_in_use())) {
>> + if (unlikely(vcpu->arch.switch_db_regs)) {
>> + set_debugreg(vcpu->arch.host_db[0], 0);
>> + set_debugreg(vcpu->arch.host_db[1], 1);
>> + set_debugreg(vcpu->arch.host_db[2], 2);
>> + set_debugreg(vcpu->arch.host_db[3], 3);
>> + }
>> + set_debugreg(vcpu->arch.host_dr6, 6);
>> + set_debugreg(vcpu->arch.host_dr7, 7);
>> + } else if (unlikely(test_thread_flag(TIF_DEBUG))) {
>> + if (unlikely(vcpu->arch.switch_db_regs)) {
>> + set_debugreg(current->thread.debugreg0, 0);
>> + set_debugreg(current->thread.debugreg1, 1);
>> + set_debugreg(current->thread.debugreg2, 2);
>> + set_debugreg(current->thread.debugreg3, 3);
>> + }
>> + set_debugreg(current->thread.debugreg6, 6);
>> + set_debugreg(current->thread.debugreg7, 7);
>> }
>> - set_debugreg(vcpu->arch.host_dr6, 6);
>> - set_debugreg(vcpu->arch.host_dr7, 7);
>>
>>
>
> Please, let's have just save/nosave, not different kinds of save/restore.
>
> But really, this looks very hacky. It's better to have kgdb integrate
> more closely with the kernel debug register support instead of kvm
> juggling between the two.
>
> Something like
>
> struct debug_registers thread_info::debugregs
> extern struct debug_registers global_debug_registers;
>
> and a function that loads a mix of the debug registers from the
> thread-local and global settings. The context switch path can call that
> function as well as kvm.
For sure this approach is not nice. /me just wanted to avoid the area of
the "generic hw-breakpoint API". K. Prassad and Frederic Weisbecker
already try to push this abstraction for a fairly long time now.
OK, will go around and poll for status and perspectives of this effort.
Maybe throwing our requirements into the ring also helps accelerating it
a bit.
Jan
--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
