Re: [PATCH v2] KVM: SVM: Fix reading of DR6

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3549 bytes --]

On 2013-12-31 16:28, Paolo Bonzini wrote:
> Il 27/12/2013 19:17, Jan Kiszka ha scritto:
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> In contrast to VMX, SVM dose not automatically transfer DR6 into the
>> VCPU's arch.dr6. So if we face a DR6 read, we must consult a new vendor
>> hook to obtain the current value. And as SVM now picks the DR6 state
>> from its VMCB, we also need a set callback in order to write updates of
>> DR6 back.
>>
>> Fixes a regression of 020df0794f.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h |  2 ++
>>  arch/x86/kvm/svm.c              | 18 ++++++++++++++++++
>>  arch/x86/kvm/vmx.c              | 11 +++++++++++
>>  arch/x86/kvm/x86.c              |  3 ++-
>>  4 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index ae5d783..e73651b 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -699,6 +699,8 @@ struct kvm_x86_ops {
>>  	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
>>  	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
>>  	void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
>> +	u64 (*get_dr6)(struct kvm_vcpu *vcpu);
>> +	void (*set_dr6)(struct kvm_vcpu *vcpu, unsigned long value);
>>  	void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value);
>>  	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
>>  	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index c7168a5..5987414 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1671,6 +1671,22 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd)
>>  	mark_dirty(svm->vmcb, VMCB_ASID);
>>  }
>>  
>> +static u64 svm_get_dr6(struct kvm_vcpu *vcpu)
>> +{
>> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
>> +		return vcpu->arch.dr6;
>> +	else
>> +		return to_svm(vcpu)->vmcb->save.dr6;
>> +}
>> +
>> +static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> +	svm->vmcb->save.dr6 = value;
>> +	mark_dirty(svm->vmcb, VMCB_DR);
> 
> The code here is a bit different from the existing DR7 hooks.  Why isn't
> get_dr7 needed?  I cannot find anything in the AMD manuals that suggests
> a difference between DR6 and DR7.

DR7 doesn't change while the guest is running, only when the guest
writes to it, and we intercept that.

> 
> Also, set_dr7 is only called when !(vcpu->guest_debug &
> KVM_GUESTDBG_USE_HW_BP).  I think it makes sense to do the same for all
> the new hooks you need to introduce (get_dr6, set_dr6 and possibly get_dr7).

We could make set_dr6 conditional, but it doesn't make a difference in
practice. If guest debugging is enabled, we effectively only use DR6
when leaving the guest, i.e. when the hardware defined DR6.

Hmm, SVM is not updating kvm_run::debug.arch.dr6/7... There might be
more broken, need to check. But that would not be guest visible then.

> 
> And finally, is it necessary to write to both vcpu->arch.dr6/7 and
> svm->vmcb->save.dr6/7, or is it simply okay to do that?

arch.dr6/7 holds the guest visible state, save.dr6/7 the effective one.

Jan

PS: I'll look into a kvm unit test for hardware debugging features these
days. Guest-side at least, no idea yet how to implement host-side
debugging with unit tests.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]