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@xxxxxxxxxxx>
>> ---
>>
>> 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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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
--
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
