Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
On 1/28/2021 3:25 PM, Paolo Bonzini wrote: On 28/01/21 08:17, Xiaoyao Li wrote: "Active low" means that the bit is usually 1 and goes to 0 when the condition (such as RTM or bus lock) happens. For almost all those DR6 bits the value is in fact always 1, but if they are defined in the future it will require no code change. Why not keep use DR6_INIT, or DR6_RESET_VALUE? or any other better name. It's just the default clear value of DR6 that no debug condition is hit. I preferred "DR6_ACTIVE_LOW" because the value is used only once or twice to initialize dr6, and many times to invert those bits. For example: vcpu->arch.dr6 &= ~DR_TRAP_BITS; vcpu->arch.dr6 |= DR6_ACTIVE_LOW; vcpu->arch.dr6 |= payload; vcpu->arch.dr6 ^= payload & DR6_ACTIVE_LOW; payload = vcpu->arch.dr6; payload &= ~DR6_BT; payload ^= DR6_ACTIVE_LOW; The name conveys that it's not just the initialization value; it's also the XOR mask between the #DB exit qualification (which we also use as the "payload") and DR6. Fair enough.
Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
On 28/01/21 08:17, Xiaoyao Li wrote: "Active low" means that the bit is usually 1 and goes to 0 when the condition (such as RTM or bus lock) happens. For almost all those DR6 bits the value is in fact always 1, but if they are defined in the future it will require no code change. Why not keep use DR6_INIT, or DR6_RESET_VALUE? or any other better name. It's just the default clear value of DR6 that no debug condition is hit. I preferred "DR6_ACTIVE_LOW" because the value is used only once or twice to initialize dr6, and many times to invert those bits. For example: vcpu->arch.dr6 &= ~DR_TRAP_BITS; vcpu->arch.dr6 |= DR6_ACTIVE_LOW; vcpu->arch.dr6 |= payload; vcpu->arch.dr6 ^= payload & DR6_ACTIVE_LOW; payload = vcpu->arch.dr6; payload &= ~DR6_BT; payload ^= DR6_ACTIVE_LOW; The name conveys that it's not just the initialization value; it's also the XOR mask between the #DB exit qualification (which we also use as the "payload") and DR6. Paolo
Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
On 1/27/2021 6:04 PM, Paolo Bonzini wrote: On 27/01/21 04:41, Xiaoyao Li wrote: On 1/27/2021 12:31 AM, Paolo Bonzini wrote: On 08/01/21 07:49, Chenyi Qiang wrote: To avoid breaking the CPUs without bus lock detection, activate the DR6_BUS_LOCK bit (bit 11) conditionally in DR6_FIXED_1 bits. The set/clear of DR6_BUS_LOCK is similar to the DR6_RTM in DR6 register. The processor clears DR6_BUS_LOCK when bus lock debug exception is generated. (For all other #DB the processor sets this bit to 1.) Software #DB handler should set this bit before returning to the interrupted task. For VM exit caused by debug exception, bit 11 of the exit qualification is set to indicate that a bus lock debug exception condition was detected. The VMM should emulate the exception by clearing bit 11 of the guest DR6. Please rename DR6_INIT to DR6_ACTIVE_LOW, and then a lot of changes become simpler: Paolo, What do you want to convey with the new name DR6_ACTIVE_LOW? To be honest, the new name is confusing to me. "Active low" means that the bit is usually 1 and goes to 0 when the condition (such as RTM or bus lock) happens. For almost all those DR6 bits the value is in fact always 1, but if they are defined in the future it will require no code change. Why not keep use DR6_INIT, or DR6_RESET_VALUE? or any other better name. It's just the default clear value of DR6 that no debug condition is hit. Paolo - dr6 |= DR6_BD | DR6_RTM; + dr6 |= DR6_BD | DR6_RTM | DR6_BUS_LOCK; dr6 |= DR6_BD | DR6_ACTIVE_LOW;
Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
On 27/01/21 04:41, Xiaoyao Li wrote: On 1/27/2021 12:31 AM, Paolo Bonzini wrote: On 08/01/21 07:49, Chenyi Qiang wrote: To avoid breaking the CPUs without bus lock detection, activate the DR6_BUS_LOCK bit (bit 11) conditionally in DR6_FIXED_1 bits. The set/clear of DR6_BUS_LOCK is similar to the DR6_RTM in DR6 register. The processor clears DR6_BUS_LOCK when bus lock debug exception is generated. (For all other #DB the processor sets this bit to 1.) Software #DB handler should set this bit before returning to the interrupted task. For VM exit caused by debug exception, bit 11 of the exit qualification is set to indicate that a bus lock debug exception condition was detected. The VMM should emulate the exception by clearing bit 11 of the guest DR6. Please rename DR6_INIT to DR6_ACTIVE_LOW, and then a lot of changes become simpler: Paolo, What do you want to convey with the new name DR6_ACTIVE_LOW? To be honest, the new name is confusing to me. "Active low" means that the bit is usually 1 and goes to 0 when the condition (such as RTM or bus lock) happens. For almost all those DR6 bits the value is in fact always 1, but if they are defined in the future it will require no code change. Paolo - dr6 |= DR6_BD | DR6_RTM; + dr6 |= DR6_BD | DR6_RTM | DR6_BUS_LOCK; dr6 |= DR6_BD | DR6_ACTIVE_LOW;
Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
On 1/27/2021 12:31 AM, Paolo Bonzini wrote: On 08/01/21 07:49, Chenyi Qiang wrote: To avoid breaking the CPUs without bus lock detection, activate the DR6_BUS_LOCK bit (bit 11) conditionally in DR6_FIXED_1 bits. The set/clear of DR6_BUS_LOCK is similar to the DR6_RTM in DR6 register. The processor clears DR6_BUS_LOCK when bus lock debug exception is generated. (For all other #DB the processor sets this bit to 1.) Software #DB handler should set this bit before returning to the interrupted task. For VM exit caused by debug exception, bit 11 of the exit qualification is set to indicate that a bus lock debug exception condition was detected. The VMM should emulate the exception by clearing bit 11 of the guest DR6. Please rename DR6_INIT to DR6_ACTIVE_LOW, and then a lot of changes become simpler: Paolo, What do you want to convey with the new name DR6_ACTIVE_LOW? To be honest, the new name is confusing to me. - dr6 |= DR6_BD | DR6_RTM; + dr6 |= DR6_BD | DR6_RTM | DR6_BUS_LOCK; dr6 |= DR6_BD | DR6_ACTIVE_LOW;
Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
On 1/27/2021 12:31 AM, Paolo Bonzini wrote:
On 08/01/21 07:49, Chenyi Qiang wrote:
To avoid breaking the CPUs without bus lock detection, activate the
DR6_BUS_LOCK bit (bit 11) conditionally in DR6_FIXED_1 bits.
The set/clear of DR6_BUS_LOCK is similar to the DR6_RTM in DR6
register. The processor clears DR6_BUS_LOCK when bus lock debug
exception is generated. (For all other #DB the processor sets this bit
to 1.) Software #DB handler should set this bit before returning to the
interrupted task.
For VM exit caused by debug exception, bit 11 of the exit qualification
is set to indicate that a bus lock debug exception condition was
detected. The VMM should emulate the exception by clearing bit 11 of the
guest DR6.
Please rename DR6_INIT to DR6_ACTIVE_LOW, and then a lot of changes
become simpler:
- dr6 |= DR6_BD | DR6_RTM;
+ dr6 |= DR6_BD | DR6_RTM | DR6_BUS_LOCK;
dr6 |= DR6_BD | DR6_ACTIVE_LOW;
ctxt->ops->set_dr(ctxt, 6, dr6);
return emulate_db(ctxt);
}
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cce0143a6f80..3d8a0e30314f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1860,7 +1860,7 @@ static void svm_sync_dirty_debug_regs(struct
kvm_vcpu *vcpu)
get_debugreg(vcpu->arch.db[2], 2);
get_debugreg(vcpu->arch.db[3], 3);
/*
- * We cannot reset svm->vmcb->save.dr6 to DR6_FIXED_1|DR6_RTM here,
+ * We cannot reset svm->vmcb->save.dr6 to
DR6_FIXED_1|DR6_RTM|DR6_BUS_LOCK here,
We cannot reset svm->vmcb->save.dr6 to DR6_ACTIVE_LOW
* because db_interception might need it. We can do it before
vmentry.
*/
vcpu->arch.dr6 = svm->vmcb->save.dr6;
@@ -1911,7 +1911,7 @@ static int db_interception(struct vcpu_svm *svm)
if (!(svm->vcpu.guest_debug &
(KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)) &&
!svm->nmi_singlestep) {
- u32 payload = (svm->vmcb->save.dr6 ^ DR6_RTM) & ~DR6_FIXED_1;
+ u32 payload = (svm->vmcb->save.dr6 ^ (DR6_RTM|DR6_BUS_LOCK))
& ~DR6_FIXED_1;
u32 payload = svm->vmcb->save.dr6 ^ DR6_ACTIVE_LOW;
kvm_queue_exception_p(&svm->vcpu, DB_VECTOR, payload);
return 1;
}
@@ -3778,7 +3778,7 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct
kvm_vcpu *vcpu)
if (unlikely(svm->vcpu.arch.switch_db_regs &
KVM_DEBUGREG_WONT_EXIT))
svm_set_dr6(svm, vcpu->arch.dr6);
else
- svm_set_dr6(svm, DR6_FIXED_1 | DR6_RTM);
+ svm_set_dr6(svm, DR6_FIXED_1 | DR6_RTM | DR6_BUS_LOCK);
svm_set_dr6(svm, DR6_ACTIVE_LOW);
clgi();
kvm_load_guest_xsave_state(vcpu);
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index e2f26564a12d..c5d71a9b3729 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -412,7 +412,7 @@ static int nested_vmx_check_exception(struct
kvm_vcpu *vcpu, unsigned long *exit
if (!has_payload) {
payload = vcpu->arch.dr6;
payload &= ~(DR6_FIXED_1 | DR6_BT);
- payload ^= DR6_RTM;
+ payload ^= DR6_RTM | DR6_BUS_LOCK;
payload &= ~DR6_BT;
payload ^= DR6_ACTIVE_LOW;
}
*exit_qual = payload;
} else
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3f7c1fc7a3ce..06de2b9e57f3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -483,19 +483,20 @@ void kvm_deliver_exception_payload(struct
kvm_vcpu *vcpu)
*/
vcpu->arch.dr6 &= ~DR_TRAP_BITS;
/*
- * DR6.RTM is set by all #DB exceptions that don't clear it.
+ * DR6.RTM and DR6.BUS_LOCK are set by all #DB exceptions
+ * that don't clear it.
*/
- vcpu->arch.dr6 |= DR6_RTM;
+ vcpu->arch.dr6 |= DR6_RTM | DR6_BUS_LOCK;
vcpu->arch.dr6 |= payload;
/*
- * Bit 16 should be set in the payload whenever the #DB
- * exception should clear DR6.RTM. This makes the payload
- * compatible with the pending debug exceptions under VMX.
- * Though not currently documented in the SDM, this also
- * makes the payload compatible with the exit qualification
- * for #DB exceptions under VMX.
+ * Bit 16/Bit 11 should be set in the payload whenever
+ * the #DB exception should clear DR6.RTM/DR6.BUS_LOCK.
+ * This makes the payload compatible with the pending debug
+ * exceptions under VMX. Though not currently documented in
+ * the SDM, this also makes the payload compatible with the
+ * exit qualification for #DB exceptions under VMX.
*/
- vcpu->arch.dr6 ^= payload & DR6_RTM;
+ vcpu->arch.dr6 ^= payload & (DR6_RTM | DR6_BUS_LOCK);
vcpu->arch.dr6 &= ~DR_TRAP_BITS;
vcpu->arch.dr6 |= DR6_ACTIVE_LOW;
vcpu->arch.dr6 |= payload;
vcpu->arch.dr6 ^= payload & DR6_ACTIVE_LOW;
(with comments :))
and so on.
Thanks!
Paolo
Will do the changes.
Thank
Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
On 08/01/21 07:49, Chenyi Qiang wrote:
To avoid breaking the CPUs without bus lock detection, activate the
DR6_BUS_LOCK bit (bit 11) conditionally in DR6_FIXED_1 bits.
The set/clear of DR6_BUS_LOCK is similar to the DR6_RTM in DR6
register. The processor clears DR6_BUS_LOCK when bus lock debug
exception is generated. (For all other #DB the processor sets this bit
to 1.) Software #DB handler should set this bit before returning to the
interrupted task.
For VM exit caused by debug exception, bit 11 of the exit qualification
is set to indicate that a bus lock debug exception condition was
detected. The VMM should emulate the exception by clearing bit 11 of the
guest DR6.
Please rename DR6_INIT to DR6_ACTIVE_LOW, and then a lot of changes
become simpler:
- dr6 |= DR6_BD | DR6_RTM;
+ dr6 |= DR6_BD | DR6_RTM | DR6_BUS_LOCK;
dr6 |= DR6_BD | DR6_ACTIVE_LOW;
ctxt->ops->set_dr(ctxt, 6, dr6);
return emulate_db(ctxt);
}
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cce0143a6f80..3d8a0e30314f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1860,7 +1860,7 @@ static void svm_sync_dirty_debug_regs(struct kvm_vcpu
*vcpu)
get_debugreg(vcpu->arch.db[2], 2);
get_debugreg(vcpu->arch.db[3], 3);
/*
-* We cannot reset svm->vmcb->save.dr6 to DR6_FIXED_1|DR6_RTM here,
+* We cannot reset svm->vmcb->save.dr6 to
DR6_FIXED_1|DR6_RTM|DR6_BUS_LOCK here,
We cannot reset svm->vmcb->save.dr6 to DR6_ACTIVE_LOW
* because db_interception might need it. We can do it before vmentry.
*/
vcpu->arch.dr6 = svm->vmcb->save.dr6;
@@ -1911,7 +1911,7 @@ static int db_interception(struct vcpu_svm *svm)
if (!(svm->vcpu.guest_debug &
(KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)) &&
!svm->nmi_singlestep) {
- u32 payload = (svm->vmcb->save.dr6 ^ DR6_RTM) & ~DR6_FIXED_1;
+ u32 payload = (svm->vmcb->save.dr6 ^ (DR6_RTM|DR6_BUS_LOCK)) &
~DR6_FIXED_1;
u32 payload = svm->vmcb->save.dr6 ^ DR6_ACTIVE_LOW;
kvm_queue_exception_p(&svm->vcpu, DB_VECTOR, payload);
return 1;
}
@@ -3778,7 +3778,7 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu
*vcpu)
if (unlikely(svm->vcpu.arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT))
svm_set_dr6(svm, vcpu->arch.dr6);
else
- svm_set_dr6(svm, DR6_FIXED_1 | DR6_RTM);
+ svm_set_dr6(svm, DR6_FIXED_1 | DR6_RTM | DR6_BUS_LOCK);
svm_set_dr6(svm, DR6_ACTIVE_LOW);
clgi();
kvm_load_guest_xsave_state(vcpu);
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index e2f26564a12d..c5d71a9b3729 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -412,7 +412,7 @@ static int nested_vmx_check_exception(struct kvm_vcpu
*vcpu, unsigned long *exit
if (!has_payload) {
payload = vcpu->arch.dr6;
payload &= ~(DR6_FIXED_1 | DR6_BT);
- payload ^= DR6_RTM;
+ payload ^= DR6_RTM | DR6_BUS_LOCK;
payload &= ~DR6_BT;
payload ^= DR6_ACTIVE_LOW;
}
*exit_qual = payload;
} else
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3f7c1fc7a3ce..06de2b9e57f3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -483,19 +483,20 @@ void kvm_deliver_exception_payload(struct kvm_vcpu *vcpu)
*/
vcpu->arch.dr6 &= ~DR_TRAP_BITS;
/*
-* DR6.RTM is set by all #DB exceptions that don't clear it.
+* DR6.RTM and DR6.BUS_LOCK are set by all #DB exceptions
+* that don't clear it.
*/
- vcpu->arch.dr6 |= DR6_RTM;
+ vcpu->arch.dr6 |= DR6_RTM | DR6_BUS_LOCK;
vcpu->arch.dr6 |= payload;
/*
-* Bit 16 should be set in the payload whenever the #DB
-* exception should clear DR6.RTM. This makes the payload
-* compatible with the pending debug exceptions under VMX.
-* Though not currently documented in the SDM, this also
-* makes the payload compatible with the exit qualification
-* for #DB exceptions under VMX.
+* Bit 16/Bit 11 should be set in the payload whenever
+* the #DB exception should clear DR6.RTM/DR6.BUS_LOCK.
+* This makes the payload compatible with the pending debug
+* exceptions under VMX. Though not currently documented in
+* the SDM, this also makes the payload compatible with the
+* exit qualification for #DB exceptions under VMX.
*/
- v
Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
Hi Chenyi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v5.11-rc2 next-20210108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Chenyi-Qiang/Add-KVM-support-for-bus-lock-debug-exception/20210108-144848
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: i386-randconfig-r015-20210108 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/b1bac74da16aca55f4fb7f49c18849a5e3f66ae7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Chenyi-Qiang/Add-KVM-support-for-bus-lock-debug-exception/20210108-144848
git checkout b1bac74da16aca55f4fb7f49c18849a5e3f66ae7
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:5,
from arch/x86/include/asm/bug.h:93,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/percpu.h:5,
from include/linux/context_tracking_state.h:5,
from include/linux/hardirq.h:5,
from include/linux/kvm_host.h:7,
from arch/x86/kvm/x86.c:19:
arch/x86/kvm/x86.c: In function 'kvm_dr6_fixed':
>> arch/x86/kvm/x86.c:1131:29: error: 'X86_FEATURE_BUS_LOCK_DETECT' undeclared
>> (first use in this function); did you mean 'X86_FEATURE_SPLIT_LOCK_DETECT'?
1131 | if (!guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
| ^~~
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) :
__trace_if_value(cond))
|^~~~
arch/x86/kvm/x86.c:1131:2: note: in expansion of macro 'if'
1131 | if (!guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
| ^~
arch/x86/kvm/x86.c:1131:29: note: each undeclared identifier is reported
only once for each function it appears in
1131 | if (!guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
| ^~~
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) :
__trace_if_value(cond))
|^~~~
arch/x86/kvm/x86.c:1131:2: note: in expansion of macro 'if'
1131 | if (!guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
| ^~
vim +1131 arch/x86/kvm/x86.c
1123
1124 static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu)
1125 {
1126 u64 fixed = DR6_FIXED_1;
1127
1128 if (!guest_cpuid_has(vcpu, X86_FEATURE_RTM))
1129 fixed |= DR6_RTM;
1130
> 1131 if (!guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
1132 fixed |= DR6_BUS_LOCK;
1133 return fixed;
1134 }
1135
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
