All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: SVM: Fix reading of DR6
@ 2013-12-19 13:24 Jan Kiszka
  2013-12-27 17:21 ` Marcelo Tosatti
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Kiszka @ 2013-12-19 13:24 UTC (permalink / raw)
  To: Paolo Bonzini, Gleb Natapov; +Cc: kvm

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.

Fixes a regression of 020df0794f.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Should go to stable as well.

 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/svm.c              | 9 +++++++++
 arch/x86/kvm/vmx.c              | 6 ++++++
 arch/x86/kvm/x86.c              | 2 +-
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ae5d783..f115f46 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -699,6 +699,7 @@ 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_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..48fa63e 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1671,6 +1671,14 @@ 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_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -4286,6 +4294,7 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
 	.set_gdt = svm_set_gdt,
+	.get_dr6 = svm_get_dr6,
 	.set_dr7 = svm_set_dr7,
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ee3bf54..c5c7e62 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5153,6 +5153,11 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.dr6;
+}
+
 static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);
@@ -8573,6 +8578,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.set_idt = vmx_set_idt,
 	.get_gdt = vmx_get_gdt,
 	.set_gdt = vmx_set_gdt,
+	.get_dr6 = vmx_get_dr6,
 	.set_dr7 = vmx_set_dr7,
 	.cache_reg = vmx_cache_reg,
 	.get_rflags = vmx_get_rflags,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1dc0359..8fe227c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -788,7 +788,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 			return 1;
 		/* fall through */
 	case 6:
-		*val = vcpu->arch.dr6;
+		*val = kvm_x86_ops->get_dr6(vcpu);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
-- 
1.8.1.1.298.ge7eed54

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: SVM: Fix reading of DR6
  2013-12-19 13:24 [PATCH] KVM: SVM: Fix reading of DR6 Jan Kiszka
@ 2013-12-27 17:21 ` Marcelo Tosatti
  2013-12-27 18:17   ` [PATCH v2] " Jan Kiszka
  0 siblings, 1 reply; 6+ messages in thread
From: Marcelo Tosatti @ 2013-12-27 17:21 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Paolo Bonzini, Gleb Natapov, kvm

On Thu, Dec 19, 2013 at 02:24:59PM +0100, Jan Kiszka wrote:
> 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.
> 
> Fixes a regression of 020df0794f.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Should go to stable as well.
> 
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/svm.c              | 9 +++++++++
>  arch/x86/kvm/vmx.c              | 6 ++++++
>  arch/x86/kvm/x86.c              | 2 +-
>  4 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index ae5d783..f115f46 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -699,6 +699,7 @@ 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_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..48fa63e 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1671,6 +1671,14 @@ 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_dr7(struct kvm_vcpu *vcpu, unsigned long value)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -4286,6 +4294,7 @@ static struct kvm_x86_ops svm_x86_ops = {
>  	.set_idt = svm_set_idt,
>  	.get_gdt = svm_get_gdt,
>  	.set_gdt = svm_set_gdt,
> +	.get_dr6 = svm_get_dr6,
>  	.set_dr7 = svm_set_dr7,
>  	.cache_reg = svm_cache_reg,
>  	.get_rflags = svm_get_rflags,
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ee3bf54..c5c7e62 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5153,6 +5153,11 @@ static int handle_dr(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> +static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->arch.dr6;
> +}
> +
>  static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
>  {
>  	vmcs_writel(GUEST_DR7, val);
> @@ -8573,6 +8578,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
>  	.set_idt = vmx_set_idt,
>  	.get_gdt = vmx_get_gdt,
>  	.set_gdt = vmx_set_gdt,
> +	.get_dr6 = vmx_get_dr6,
>  	.set_dr7 = vmx_set_dr7,
>  	.cache_reg = vmx_cache_reg,
>  	.get_rflags = vmx_get_rflags,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1dc0359..8fe227c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -788,7 +788,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
>  			return 1;
>  		/* fall through */
>  	case 6:
> -		*val = vcpu->arch.dr6;
> +		*val = kvm_x86_ops->get_dr6(vcpu);
>  		break;
>  	case 5:
>  		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
> -- 
> 1.8.1.1.298.ge7eed54

It allows 

kvm_set_dr(a)
val = kvm_get_dr()

to have 'val' different than a. 

Is this OK ? (its certainly counter intuitive).


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2] KVM: SVM: Fix reading of DR6
  2013-12-27 17:21 ` Marcelo Tosatti
@ 2013-12-27 18:17   ` Jan Kiszka
  2013-12-31 15:28     ` Paolo Bonzini
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Kiszka @ 2013-12-27 18:17 UTC (permalink / raw)
  To: Marcelo Tosatti; +Cc: Paolo Bonzini, Gleb Natapov, kvm

On 2013-12-27 18:21, Marcelo Tosatti wrote:
> On Thu, Dec 19, 2013 at 02:24:59PM +0100, Jan Kiszka wrote:
>> 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.
>>
>> Fixes a regression of 020df0794f.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Should go to stable as well.
>>
>>  arch/x86/include/asm/kvm_host.h | 1 +
>>  arch/x86/kvm/svm.c              | 9 +++++++++
>>  arch/x86/kvm/vmx.c              | 6 ++++++
>>  arch/x86/kvm/x86.c              | 2 +-
>>  4 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index ae5d783..f115f46 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -699,6 +699,7 @@ 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_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..48fa63e 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1671,6 +1671,14 @@ 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_dr7(struct kvm_vcpu *vcpu, unsigned long value)
>>  {
>>  	struct vcpu_svm *svm = to_svm(vcpu);
>> @@ -4286,6 +4294,7 @@ static struct kvm_x86_ops svm_x86_ops = {
>>  	.set_idt = svm_set_idt,
>>  	.get_gdt = svm_get_gdt,
>>  	.set_gdt = svm_set_gdt,
>> +	.get_dr6 = svm_get_dr6,
>>  	.set_dr7 = svm_set_dr7,
>>  	.cache_reg = svm_cache_reg,
>>  	.get_rflags = svm_get_rflags,
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index ee3bf54..c5c7e62 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -5153,6 +5153,11 @@ static int handle_dr(struct kvm_vcpu *vcpu)
>>  	return 1;
>>  }
>>  
>> +static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
>> +{
>> +	return vcpu->arch.dr6;
>> +}
>> +
>>  static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
>>  {
>>  	vmcs_writel(GUEST_DR7, val);
>> @@ -8573,6 +8578,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
>>  	.set_idt = vmx_set_idt,
>>  	.get_gdt = vmx_get_gdt,
>>  	.set_gdt = vmx_set_gdt,
>> +	.get_dr6 = vmx_get_dr6,
>>  	.set_dr7 = vmx_set_dr7,
>>  	.cache_reg = vmx_cache_reg,
>>  	.get_rflags = vmx_get_rflags,
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 1dc0359..8fe227c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -788,7 +788,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
>>  			return 1;
>>  		/* fall through */
>>  	case 6:
>> -		*val = vcpu->arch.dr6;
>> +		*val = kvm_x86_ops->get_dr6(vcpu);
>>  		break;
>>  	case 5:
>>  		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
>> -- 
>> 1.8.1.1.298.ge7eed54
> 
> It allows 
> 
> kvm_set_dr(a)
> val = kvm_get_dr()
> 
> to have 'val' different than a. 
> 
> Is this OK ? (its certainly counter intuitive).

No, it's not ok. We also need to sync the guest-visible state to the
VMCB on updates.

----8<----

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);
+}
+
 static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -4286,6 +4302,8 @@ static struct kvm_x86_ops svm_x86_ops = {
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
 	.set_gdt = svm_set_gdt,
+	.get_dr6 = svm_get_dr6,
+	.set_dr6 = svm_set_dr6,
 	.set_dr7 = svm_set_dr7,
 	.cache_reg = svm_cache_reg,
 	.get_rflags = svm_get_rflags,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ee3bf54..1d9b0ec 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5153,6 +5153,15 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
+{
+	return vcpu->arch.dr6;
+}
+
+static void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
+{
+}
+
 static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);
@@ -8573,6 +8582,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
 	.set_idt = vmx_set_idt,
 	.get_gdt = vmx_get_gdt,
 	.set_gdt = vmx_set_gdt,
+	.get_dr6 = vmx_get_dr6,
+	.set_dr6 = vmx_set_dr6,
 	.set_dr7 = vmx_set_dr7,
 	.cache_reg = vmx_cache_reg,
 	.get_rflags = vmx_get_rflags,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1dc0359..be000ec 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -747,6 +747,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
 		if (val & 0xffffffff00000000ULL)
 			return -1; /* #GP */
 		vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
+		kvm_x86_ops->set_dr6(vcpu, vcpu->arch.dr6);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
@@ -788,7 +789,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 			return 1;
 		/* fall through */
 	case 6:
-		*val = vcpu->arch.dr6;
+		*val = kvm_x86_ops->get_dr6(vcpu);
 		break;
 	case 5:
 		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
-- 
1.8.1.1.298.ge7eed54

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: SVM: Fix reading of DR6
  2013-12-27 18:17   ` [PATCH v2] " Jan Kiszka
@ 2013-12-31 15:28     ` Paolo Bonzini
  2013-12-31 17:29       ` Jan Kiszka
  0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2013-12-31 15:28 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Marcelo Tosatti, Gleb Natapov, kvm

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.

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).

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?

Paolo

> +}
> +
>  static void svm_set_dr7(struct kvm_vcpu *vcpu, unsigned long value)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -4286,6 +4302,8 @@ static struct kvm_x86_ops svm_x86_ops = {
>  	.set_idt = svm_set_idt,
>  	.get_gdt = svm_get_gdt,
>  	.set_gdt = svm_set_gdt,
> +	.get_dr6 = svm_get_dr6,
> +	.set_dr6 = svm_set_dr6,
>  	.set_dr7 = svm_set_dr7,
>  	.cache_reg = svm_cache_reg,
>  	.get_rflags = svm_get_rflags,
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ee3bf54..1d9b0ec 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5153,6 +5153,15 @@ static int handle_dr(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> +static u64 vmx_get_dr6(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->arch.dr6;
> +}
> +
> +static void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
> +{
> +}
> +
>  static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
>  {
>  	vmcs_writel(GUEST_DR7, val);
> @@ -8573,6 +8582,8 @@ static struct kvm_x86_ops vmx_x86_ops = {
>  	.set_idt = vmx_set_idt,
>  	.get_gdt = vmx_get_gdt,
>  	.set_gdt = vmx_set_gdt,
> +	.get_dr6 = vmx_get_dr6,
> +	.set_dr6 = vmx_set_dr6,
>  	.set_dr7 = vmx_set_dr7,
>  	.cache_reg = vmx_cache_reg,
>  	.get_rflags = vmx_get_rflags,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1dc0359..be000ec 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -747,6 +747,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
>  		if (val & 0xffffffff00000000ULL)
>  			return -1; /* #GP */
>  		vcpu->arch.dr6 = (val & DR6_VOLATILE) | DR6_FIXED_1;
> +		kvm_x86_ops->set_dr6(vcpu, vcpu->arch.dr6);
>  		break;
>  	case 5:
>  		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))
> @@ -788,7 +789,7 @@ static int _kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
>  			return 1;
>  		/* fall through */
>  	case 6:
> -		*val = vcpu->arch.dr6;
> +		*val = kvm_x86_ops->get_dr6(vcpu);
>  		break;
>  	case 5:
>  		if (kvm_read_cr4_bits(vcpu, X86_CR4_DE))


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: SVM: Fix reading of DR6
  2013-12-31 15:28     ` Paolo Bonzini
@ 2013-12-31 17:29       ` Jan Kiszka
  2014-01-02  8:24         ` Paolo Bonzini
  0 siblings, 1 reply; 6+ messages in thread
From: Jan Kiszka @ 2013-12-31 17:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Marcelo Tosatti, Gleb Natapov, kvm

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: SVM: Fix reading of DR6
  2013-12-31 17:29       ` Jan Kiszka
@ 2014-01-02  8:24         ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2014-01-02  8:24 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Marcelo Tosatti, Gleb Natapov, kvm

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 31/12/2013 18:29, Jan Kiszka ha scritto:
>> 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.

No, it doesn't indeed.  It's just a matter of consistency.

> 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.

It would be possible to do it with infrastructure like qtest or
autotest.  You could speak the gdbserver protocol yourself, or open a
pty to gdb.  But guest-side is already an improvement over nothing!

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSxSI0AAoJEBvWZb6bTYbyoiYP/REKgPL/dUwJoSA8AEm7h6Ip
wkXlaxBcyBiKlIzfiN3abBXBOZvDy7dSzNfXUZ4bY6DitQIJZYSVc93XfvWYRToY
ORC6qZej+P/bD/rNwATpvVhGG6RbRpmihmVgkoX5w5J3GWZ52QREcyoW7mZEhIoR
fCMJmNCq2l9vxa5hhXLAuiLYtq7mto6hQpFE8TXG81PyzE+c3WWK1D5vy/2cpv48
qabaVsJs7aj0KV2rVDvyQkO+GSdm+8SDpf4GyJcLo1Y1yYjPW97xC0Ey4ZyEXXWf
37zb3oOoH9NPPXtt1oJhUFCFZ34v1MxW2j0IIGvtUfRPSjphW+T/nQ5y17GG+efQ
TbKnSlibNP0L7fX/xa/sytaYcslIJVIKvvq0Wz9a3Ka1OHNufgtBIreUjtAPvSmy
TLxTm2w2s0S/mD3yMDK62uAtQMV/FW7qV6WHTHHZzj8VJpgfAAO0j4gkN0PlLB91
XfuR8ANmYoAKm9yafOSlr6Xw4uzHh41ZODMIY9w/pwB6nGB8SMRVBUWAILkyiyoN
btJdVCFdmxTlUmszAeHGTkd4VMtOMqva8MmgnV5isVEUy5A6gJZwNSZ8KJCdny+J
N+jRLj/j3SUlxGFnsK4FPBlMQzK1DDYS3C2QKd+Z5No42kZg4w2g4n0zQamDi2XR
2lcuexvRePHTZglNkP82
=dDQc
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-02  8:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-19 13:24 [PATCH] KVM: SVM: Fix reading of DR6 Jan Kiszka
2013-12-27 17:21 ` Marcelo Tosatti
2013-12-27 18:17   ` [PATCH v2] " Jan Kiszka
2013-12-31 15:28     ` Paolo Bonzini
2013-12-31 17:29       ` Jan Kiszka
2014-01-02  8:24         ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.