* [RESEND PATCH 0/2] Add KVM support for bus lock debug exception
@ 2021-01-08 6:49 Chenyi Qiang
2021-01-08 6:49 ` [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit Chenyi Qiang
2021-01-08 6:49 ` [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest Chenyi Qiang
0 siblings, 2 replies; 15+ messages in thread
From: Chenyi Qiang @ 2021-01-08 6:49 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Xiaoyao Li
Cc: kvm, linux-kernel
Hi all,
Any comment on this rebased version? I'd appreciate it if anyone has
time to review this short series.
---
A bus lock is acquired either through split locked access to writeback
(WB) memory or by using locks to uncacheable (UC) memory. This is
typically > 1000 cycles slower than atomic opertaion within a cache
line. It also disrupts performance on other cores.
Bus lock debug exception is a sub-feature of bus lock detection. It is
an ability to notify the kernel by an #DB trap after the instruction
acquires a bus lock when CPL>0. This allows the kernel to enforce user
application throttling or mitigatioins.
Expose the bus lock debug exception to guest by the enumeration of
CPUID.(EAX=7,ECX=0).ECX[24]. Software in guest can enable these
exceptions by setting the DEBUGCTLMSR_BUS_LOCK_DETECT(bit 2) of
MSR_IA32_DEBUTCTL.
The bus lock #DB exception can also be intercepted by the VMM and
identified through the bit 11 of the exit qualification at VM exit. The
bit 11 (DR6_BUS_LOCK) of DR6 register is introduced to indicate a bus
lock #DB exception. DR6_BUS_LOCK has formerly always been 1 and delivery
of a bus lock #DB clears it. The VMM should emulate the exceptions by
clearing the bit 11 of the guest DR6.
The kernel support patches for bus lock debug exception is available at
https://lore.kernel.org/lkml/20201124205245.4164633-1-fenghua.yu@intel.com/
Document for Bus Lock Detection is now available at the latest "Intel
Architecture Instruction Set Extensions Programming Reference".
Document Link:
https://software.intel.com/content/www/us/en/develop/download/intel-architecture-instruction-set-extensions-programming-reference.html
---
Changelogs
RFC->v1:
- rebase on top of v5.11-rc1, no difference compared with the last version
- v1:https://lore.kernel.org/lkml/20201119092957.16940-1-chenyi.qiang@intel.com/
Chenyi Qiang (2):
KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
KVM: X86: Expose bus lock debug exception to guest
arch/x86/include/asm/kvm_host.h | 5 ++--
arch/x86/kvm/cpuid.c | 3 ++-
arch/x86/kvm/emulate.c | 2 +-
arch/x86/kvm/svm/svm.c | 6 ++---
arch/x86/kvm/vmx/nested.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 29 +++++++++++++++++++---
arch/x86/kvm/x86.c | 44 ++++++++++++++-------------------
arch/x86/kvm/x86.h | 2 ++
8 files changed, 56 insertions(+), 37 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-08 6:49 [RESEND PATCH 0/2] Add KVM support for bus lock debug exception Chenyi Qiang
@ 2021-01-08 6:49 ` Chenyi Qiang
2021-01-08 16:38 ` kernel test robot
2021-01-26 16:31 ` Paolo Bonzini
2021-01-08 6:49 ` [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest Chenyi Qiang
1 sibling, 2 replies; 15+ messages in thread
From: Chenyi Qiang @ 2021-01-08 6:49 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Xiaoyao Li
Cc: kvm, linux-kernel
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.
Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
---
arch/x86/include/asm/kvm_host.h | 5 +++--
arch/x86/kvm/emulate.c | 2 +-
arch/x86/kvm/svm/svm.c | 6 +++---
arch/x86/kvm/vmx/nested.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 6 ++++--
arch/x86/kvm/x86.c | 28 +++++++++++++++++-----------
6 files changed, 29 insertions(+), 20 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3ab7b46087b7..826fd1e87352 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -196,13 +196,14 @@ enum x86_intercept_stage;
#define KVM_NR_DB_REGS 4
+#define DR6_BUS_LOCK (1 << 11)
#define DR6_BD (1 << 13)
#define DR6_BS (1 << 14)
#define DR6_BT (1 << 15)
#define DR6_RTM (1 << 16)
-#define DR6_FIXED_1 0xfffe0ff0
+#define DR6_FIXED_1 0xfffe07f0
#define DR6_INIT 0xffff0ff0
-#define DR6_VOLATILE 0x0001e00f
+#define DR6_VOLATILE 0x0001e80f
#define DR7_BP_EN_MASK 0x000000ff
#define DR7_GE (1 << 9)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 56cae1ff9e3f..882598da20f0 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4327,7 +4327,7 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt)
ctxt->ops->get_dr(ctxt, 6, &dr6);
dr6 &= ~DR_TRAP_BITS;
- dr6 |= DR6_BD | DR6_RTM;
+ dr6 |= DR6_BD | DR6_RTM | DR6_BUS_LOCK;
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,
* 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;
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);
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;
}
*exit_qual = payload;
} else
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 75c9c6a0a3a4..13c9bcc4d9d9 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4824,7 +4824,8 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
kvm_queue_exception_p(vcpu, DB_VECTOR, dr6);
return 1;
}
- kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM;
+ kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM |
+ DR6_BUS_LOCK;
kvm_run->debug.arch.dr7 = vmcs_readl(GUEST_DR7);
fallthrough;
case BP_VECTOR:
@@ -5068,7 +5069,8 @@ static int handle_dr(struct kvm_vcpu *vcpu)
* guest debugging itself.
*/
if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
- vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1;
+ vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1 |
+ DR6_BUS_LOCK;
vcpu->run->debug.arch.dr7 = dr7;
vcpu->run->debug.arch.pc = kvm_get_linear_rip(vcpu);
vcpu->run->debug.arch.exception = DB_VECTOR;
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);
/*
* The #DB payload is defined as compatible with the 'pending
@@ -1126,6 +1127,9 @@ static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu)
if (!guest_cpuid_has(vcpu, X86_FEATURE_RTM))
fixed |= DR6_RTM;
+
+ if (!guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
+ fixed |= DR6_BUS_LOCK;
return fixed;
}
@@ -7197,7 +7201,8 @@ static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu)
struct kvm_run *kvm_run = vcpu->run;
if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
- kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | DR6_RTM;
+ kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | DR6_RTM |
+ DR6_BUS_LOCK;
kvm_run->debug.arch.pc = kvm_get_linear_rip(vcpu);
kvm_run->debug.arch.exception = DB_VECTOR;
kvm_run->exit_reason = KVM_EXIT_DEBUG;
@@ -7241,7 +7246,8 @@ static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
vcpu->arch.eff_db);
if (dr6 != 0) {
- kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM;
+ kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM |
+ DR6_BUS_LOCK;
kvm_run->debug.arch.pc = eip;
kvm_run->debug.arch.exception = DB_VECTOR;
kvm_run->exit_reason = KVM_EXIT_DEBUG;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest
2021-01-08 6:49 [RESEND PATCH 0/2] Add KVM support for bus lock debug exception Chenyi Qiang
2021-01-08 6:49 ` [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit Chenyi Qiang
@ 2021-01-08 6:49 ` Chenyi Qiang
2021-01-08 18:16 ` kernel test robot
1 sibling, 1 reply; 15+ messages in thread
From: Chenyi Qiang @ 2021-01-08 6:49 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Xiaoyao Li
Cc: kvm, linux-kernel
Bus lock debug exception is an ability to notify the kernel by an #DB
trap after the instruction acquires a bus lock and is executed when
CPL>0. This allows the kernel to enforce user application throttling or
mitigations.
Existence of bus lock debug exception is enumerated via
CPUID.(EAX=7,ECX=0).ECX[24]. Software can enable these exceptions by
setting bit 2 of the MSR_IA32_DEBUGCTL. Expose the CPUID to guest and
emulate the MSR handling when guest enables it.
Since SVM already has specific handlers of MSR_IA32_DEBUGMSR in the
svm_get/set_msr, move x86 commmon part to VMX and add the bus lock debug
exception support.
Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
---
arch/x86/kvm/cpuid.c | 3 ++-
arch/x86/kvm/vmx/vmx.c | 23 +++++++++++++++++++++--
arch/x86/kvm/x86.c | 16 ++--------------
arch/x86/kvm/x86.h | 2 ++
4 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 13036cf0b912..ea7c593794d2 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -402,7 +402,8 @@ void kvm_set_cpu_caps(void)
F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) |
F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
- F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/
+ F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/ |
+ F(BUS_LOCK_DETECT)
);
/* Set LA57 based on hardware capability. */
if (cpuid_ecx(7) & F(LA57))
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 13c9bcc4d9d9..39f28f90bbb4 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -147,6 +147,9 @@ module_param(allow_smaller_maxphyaddr, bool, S_IRUGO);
RTIT_STATUS_ERROR | RTIT_STATUS_STOPPED | \
RTIT_STATUS_BYTECNT))
+#define MSR_VMX_SUPPORTED_DEBUGCTL (DEBUGCTLMSR_LBR | \
+ DEBUGCTLMSR_BTF | DEBUGCTLMSR_BUS_LOCK_DETECT)
+
/*
* List of MSRs that can be directly passed to the guest.
* In addition to these x2apic and PT MSRs are handled specially.
@@ -1924,6 +1927,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
!guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
return 1;
goto find_uret_msr;
+ case MSR_IA32_DEBUGCTLMSR:
+ msr_info->data = vmcs_read64(GUEST_IA32_DEBUGCTL);
+ break;
default:
find_uret_msr:
msr = vmx_find_uret_msr(vmx, msr_info->index);
@@ -2002,9 +2008,22 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
VM_EXIT_SAVE_DEBUG_CONTROLS)
get_vmcs12(vcpu)->guest_ia32_debugctl = data;
- ret = kvm_set_msr_common(vcpu, msr_info);
- break;
+ if (data & ~MSR_VMX_SUPPORTED_DEBUGCTL)
+ return 1;
+ if (!msr_info->host_initiated &&
+ (data & DEBUGCTLMSR_BUS_LOCK_DETECT) &&
+ !guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
+ return 1;
+
+ if (data & (DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR)) {
+ if (report_ignored_msrs)
+ vcpu_unimpl(vcpu, "%s: BTF|LBR in IA32_DEBUGCTLMSR 0x%llx, nop\n",
+ __func__, data);
+ data &= ~(DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR);
+ }
+ vmcs_write64(GUEST_IA32_DEBUGCTL, data);
+ return 0;
case MSR_IA32_BNDCFGS:
if (!kvm_mpx_supported() ||
(!msr_info->host_initiated &&
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 06de2b9e57f3..d4a601482794 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -116,8 +116,9 @@ EXPORT_SYMBOL_GPL(kvm_x86_ops);
static bool __read_mostly ignore_msrs = 0;
module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR);
-static bool __read_mostly report_ignored_msrs = true;
+bool __read_mostly report_ignored_msrs = true;
module_param(report_ignored_msrs, bool, S_IRUGO | S_IWUSR);
+EXPORT_SYMBOL_GPL(report_ignored_msrs);
unsigned int min_timer_period_us = 200;
module_param(min_timer_period_us, uint, S_IRUGO | S_IWUSR);
@@ -3067,18 +3068,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
return 1;
}
break;
- case MSR_IA32_DEBUGCTLMSR:
- if (!data) {
- /* We support the non-activated case already */
- break;
- } else if (data & ~(DEBUGCTLMSR_LBR | DEBUGCTLMSR_BTF)) {
- /* Values other than LBR and BTF are vendor-specific,
- thus reserved and should throw a #GP */
- return 1;
- } else if (report_ignored_msrs)
- vcpu_unimpl(vcpu, "%s: MSR_IA32_DEBUGCTLMSR 0x%llx, nop\n",
- __func__, data);
- break;
case 0x200 ... 0x2ff:
return kvm_mtrr_set_msr(vcpu, msr, data);
case MSR_IA32_APICBASE:
@@ -3351,7 +3340,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
switch (msr_info->index) {
case MSR_IA32_PLATFORM_ID:
case MSR_IA32_EBL_CR_POWERON:
- case MSR_IA32_DEBUGCTLMSR:
case MSR_IA32_LASTBRANCHFROMIP:
case MSR_IA32_LASTBRANCHTOIP:
case MSR_IA32_LASTINTFROMIP:
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index c5ee0f5ce0f1..f13aa386e536 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -296,6 +296,8 @@ extern int pi_inject_timer;
extern struct static_key kvm_no_apic_vcpu;
+extern bool report_ignored_msrs;
+
static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec)
{
return pvclock_scale_delta(nsec, vcpu->arch.virtual_tsc_mult,
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-08 6:49 ` [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit Chenyi Qiang
@ 2021-01-08 16:38 ` kernel test robot
2021-01-26 16:31 ` Paolo Bonzini
1 sibling, 0 replies; 15+ messages in thread
From: kernel test robot @ 2021-01-08 16:38 UTC (permalink / raw)
To: Chenyi Qiang, Paolo Bonzini, Sean Christopherson,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
Xiaoyao Li
Cc: kbuild-all, kvm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]
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 <lkp@intel.com>
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-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32672 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest
2021-01-08 6:49 ` [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest Chenyi Qiang
@ 2021-01-08 18:16 ` kernel test robot
2021-01-26 16:33 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: kernel test robot @ 2021-01-08 18:16 UTC (permalink / raw)
To: Chenyi Qiang, Paolo Bonzini, Sean Christopherson,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
Xiaoyao Li
Cc: kbuild-all, kvm, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5378 bytes --]
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/fd0365cd4650a48a76379269b7cebd40ee38e52c
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 fd0365cd4650a48a76379269b7cebd40ee38e52c
# 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 <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from arch/x86/kvm/cpuid.c:21:
arch/x86/kvm/cpuid.c: In function 'kvm_set_cpu_caps':
>> arch/x86/kvm/cpuid.h:98:42: error: 'X86_FEATURE_BUS_LOCK_DETECT' undeclared (first use in this function); did you mean 'X86_FEATURE_SPLIT_LOCK_DETECT'?
98 | #define feature_bit(name) __feature_bit(X86_FEATURE_##name)
| ^~~~~~~~~~~~
arch/x86/kvm/cpuid.c:55:11: note: in expansion of macro 'feature_bit'
55 | #define F feature_bit
| ^~~~~~~~~~~
arch/x86/kvm/cpuid.c:406:3: note: in expansion of macro 'F'
406 | F(BUS_LOCK_DETECT)
| ^
arch/x86/kvm/cpuid.h:98:42: note: each undeclared identifier is reported only once for each function it appears in
98 | #define feature_bit(name) __feature_bit(X86_FEATURE_##name)
| ^~~~~~~~~~~~
arch/x86/kvm/cpuid.c:55:11: note: in expansion of macro 'feature_bit'
55 | #define F feature_bit
| ^~~~~~~~~~~
arch/x86/kvm/cpuid.c:406:3: note: in expansion of macro 'F'
406 | F(BUS_LOCK_DETECT)
| ^
--
In file included from include/linux/export.h:43,
from include/linux/linkage.h:7,
from include/linux/fs.h:5,
from include/linux/highmem.h:5,
from arch/x86/kvm/vmx/vmx.c:16:
arch/x86/kvm/vmx/vmx.c: In function 'vmx_set_msr':
>> arch/x86/kvm/vmx/vmx.c:151:20: error: 'DEBUGCTLMSR_BUS_LOCK_DETECT' undeclared (first use in this function)
151 | DEBUGCTLMSR_BTF | DEBUGCTLMSR_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/vmx/vmx.c:2011:3: note: in expansion of macro 'if'
2011 | if (data & ~MSR_VMX_SUPPORTED_DEBUGCTL)
| ^~
arch/x86/kvm/vmx/vmx.c:2011:15: note: in expansion of macro 'MSR_VMX_SUPPORTED_DEBUGCTL'
2011 | if (data & ~MSR_VMX_SUPPORTED_DEBUGCTL)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/kvm/vmx/vmx.c:151:20: note: each undeclared identifier is reported only once for each function it appears in
151 | DEBUGCTLMSR_BTF | DEBUGCTLMSR_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/vmx/vmx.c:2011:3: note: in expansion of macro 'if'
2011 | if (data & ~MSR_VMX_SUPPORTED_DEBUGCTL)
| ^~
arch/x86/kvm/vmx/vmx.c:2011:15: note: in expansion of macro 'MSR_VMX_SUPPORTED_DEBUGCTL'
2011 | if (data & ~MSR_VMX_SUPPORTED_DEBUGCTL)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> arch/x86/kvm/vmx/vmx.c:2016:30: error: 'X86_FEATURE_BUS_LOCK_DETECT' undeclared (first use in this function); did you mean 'X86_FEATURE_SPLIT_LOCK_DETECT'?
2016 | !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/vmx/vmx.c:2014:3: note: in expansion of macro 'if'
2014 | if (!msr_info->host_initiated &&
| ^~
vim +98 arch/x86/kvm/cpuid.h
a0a2260c12d8658 Sean Christopherson 2019-12-17 97
87382003e355592 Sean Christopherson 2019-12-17 @98 #define feature_bit(name) __feature_bit(X86_FEATURE_##name)
87382003e355592 Sean Christopherson 2019-12-17 99
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32672 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-08 6:49 ` [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit Chenyi Qiang
2021-01-08 16:38 ` kernel test robot
@ 2021-01-26 16:31 ` Paolo Bonzini
2021-01-27 1:48 ` Chenyi Qiang
2021-01-27 3:41 ` Xiaoyao Li
1 sibling, 2 replies; 15+ messages in thread
From: Paolo Bonzini @ 2021-01-26 16:31 UTC (permalink / raw)
To: Chenyi Qiang, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Xiaoyao Li
Cc: kvm, linux-kernel
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
>
> /*
> * The #DB payload is defined as compatible with the 'pending
> @@ -1126,6 +1127,9 @@ static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu)
>
> if (!guest_cpuid_has(vcpu, X86_FEATURE_RTM))
> fixed |= DR6_RTM;
> +
> + if (!guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
> + fixed |= DR6_BUS_LOCK;
> return fixed;
> }
>
> @@ -7197,7 +7201,8 @@ static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu)
> struct kvm_run *kvm_run = vcpu->run;
>
> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> - kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | DR6_RTM;
> + kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | DR6_RTM |
> + DR6_BUS_LOCK;
> kvm_run->debug.arch.pc = kvm_get_linear_rip(vcpu);
> kvm_run->debug.arch.exception = DB_VECTOR;
> kvm_run->exit_reason = KVM_EXIT_DEBUG;
> @@ -7241,7 +7246,8 @@ static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
> vcpu->arch.eff_db);
>
> if (dr6 != 0) {
> - kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM;
> + kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM |
> + DR6_BUS_LOCK;
> kvm_run->debug.arch.pc = eip;
> kvm_run->debug.arch.exception = DB_VECTOR;
> kvm_run->exit_reason = KVM_EXIT_DEBUG;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest
2021-01-08 18:16 ` kernel test robot
@ 2021-01-26 16:33 ` Paolo Bonzini
2021-01-27 0:57 ` Chenyi Qiang
0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-01-26 16:33 UTC (permalink / raw)
To: kernel test robot, Chenyi Qiang, Sean Christopherson,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
Xiaoyao Li
Cc: kbuild-all, kvm, linux-kernel
On 08/01/21 19:16, kernel test robot wrote:
> 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]
What is the status of the patch to introduce X86_FEATURE_BUS_LOCK_DETECT
(I saw
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2389369.html)?
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest
2021-01-26 16:33 ` Paolo Bonzini
@ 2021-01-27 0:57 ` Chenyi Qiang
2021-01-27 12:31 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Chenyi Qiang @ 2021-01-27 0:57 UTC (permalink / raw)
To: Paolo Bonzini, kernel test robot, Sean Christopherson,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
Xiaoyao Li
Cc: kbuild-all, kvm, linux-kernel
On 1/27/2021 12:33 AM, Paolo Bonzini wrote:
> On 08/01/21 19:16, kernel test robot wrote:
>> 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]
>
> What is the status of the patch to introduce X86_FEATURE_BUS_LOCK_DETECT
> (I saw
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2389369.html)?
>
> Paolo
Fenghua sent the v4 patch and pinged x86 maintainers, but still no feedback.
https://lore.kernel.org/lkml/YA8bkmYjShKwmyXx@otcwcpicx3.sc.intel.com/
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-26 16:31 ` Paolo Bonzini
@ 2021-01-27 1:48 ` Chenyi Qiang
2021-01-27 3:41 ` Xiaoyao Li
1 sibling, 0 replies; 15+ messages in thread
From: Chenyi Qiang @ 2021-01-27 1:48 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel, Xiaoyao Li
Cc: kvm, linux-kernel
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.
Thanks!
Chenyi
>> /*
>> * The #DB payload is defined as compatible with the 'pending
>> @@ -1126,6 +1127,9 @@ static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu)
>> if (!guest_cpuid_has(vcpu, X86_FEATURE_RTM))
>> fixed |= DR6_RTM;
>> +
>> + if (!guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
>> + fixed |= DR6_BUS_LOCK;
>> return fixed;
>> }
>> @@ -7197,7 +7201,8 @@ static int kvm_vcpu_do_singlestep(struct
>> kvm_vcpu *vcpu)
>> struct kvm_run *kvm_run = vcpu->run;
>> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
>> - kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | DR6_RTM;
>> + kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1 | DR6_RTM |
>> + DR6_BUS_LOCK;
>> kvm_run->debug.arch.pc = kvm_get_linear_rip(vcpu);
>> kvm_run->debug.arch.exception = DB_VECTOR;
>> kvm_run->exit_reason = KVM_EXIT_DEBUG;
>> @@ -7241,7 +7246,8 @@ static bool kvm_vcpu_check_breakpoint(struct
>> kvm_vcpu *vcpu, int *r)
>> vcpu->arch.eff_db);
>> if (dr6 != 0) {
>> - kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM;
>> + kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1 | DR6_RTM |
>> + DR6_BUS_LOCK;
>> kvm_run->debug.arch.pc = eip;
>> kvm_run->debug.arch.exception = DB_VECTOR;
>> kvm_run->exit_reason = KVM_EXIT_DEBUG;
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-26 16:31 ` Paolo Bonzini
2021-01-27 1:48 ` Chenyi Qiang
@ 2021-01-27 3:41 ` Xiaoyao Li
2021-01-27 10:04 ` Paolo Bonzini
1 sibling, 1 reply; 15+ messages in thread
From: Xiaoyao Li @ 2021-01-27 3:41 UTC (permalink / raw)
To: Paolo Bonzini, Chenyi Qiang, Sean Christopherson,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel
Cc: kvm, linux-kernel
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;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-27 3:41 ` Xiaoyao Li
@ 2021-01-27 10:04 ` Paolo Bonzini
2021-01-28 7:17 ` Xiaoyao Li
0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-01-27 10:04 UTC (permalink / raw)
To: Xiaoyao Li, Chenyi Qiang, Sean Christopherson, Vitaly Kuznetsov,
Wanpeng Li, Jim Mattson, Joerg Roedel
Cc: kvm, linux-kernel
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;
>>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest
2021-01-27 0:57 ` Chenyi Qiang
@ 2021-01-27 12:31 ` Paolo Bonzini
0 siblings, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2021-01-27 12:31 UTC (permalink / raw)
To: Chenyi Qiang, kernel test robot, Sean Christopherson,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
Xiaoyao Li
Cc: kbuild-all, kvm, linux-kernel
On 27/01/21 01:57, Chenyi Qiang wrote:
>>
>> What is the status of the patch to introduce
>> X86_FEATURE_BUS_LOCK_DETECT (I saw
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2389369.html)?
>>
>>
>> Paolo
>
> Fenghua sent the v4 patch and pinged x86 maintainers, but still no
> feedback.
> https://lore.kernel.org/lkml/YA8bkmYjShKwmyXx@otcwcpicx3.sc.intel.com/
Ok, please include it when you repost. Thanks!
Paolo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-27 10:04 ` Paolo Bonzini
@ 2021-01-28 7:17 ` Xiaoyao Li
2021-01-28 7:25 ` Paolo Bonzini
0 siblings, 1 reply; 15+ messages in thread
From: Xiaoyao Li @ 2021-01-28 7:17 UTC (permalink / raw)
To: Paolo Bonzini, Chenyi Qiang, Sean Christopherson,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel
Cc: kvm, linux-kernel
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;
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-28 7:17 ` Xiaoyao Li
@ 2021-01-28 7:25 ` Paolo Bonzini
2021-01-28 7:41 ` Xiaoyao Li
0 siblings, 1 reply; 15+ messages in thread
From: Paolo Bonzini @ 2021-01-28 7:25 UTC (permalink / raw)
To: Xiaoyao Li, Chenyi Qiang, Sean Christopherson, Vitaly Kuznetsov,
Wanpeng Li, Jim Mattson, Joerg Roedel
Cc: kvm, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit
2021-01-28 7:25 ` Paolo Bonzini
@ 2021-01-28 7:41 ` Xiaoyao Li
0 siblings, 0 replies; 15+ messages in thread
From: Xiaoyao Li @ 2021-01-28 7:41 UTC (permalink / raw)
To: Paolo Bonzini, Chenyi Qiang, Sean Christopherson,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel
Cc: kvm, linux-kernel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-01-28 7:44 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-08 6:49 [RESEND PATCH 0/2] Add KVM support for bus lock debug exception Chenyi Qiang
2021-01-08 6:49 ` [RESEND PATCH 1/2] KVM: X86: Add support for the emulation of DR6_BUS_LOCK bit Chenyi Qiang
2021-01-08 16:38 ` kernel test robot
2021-01-26 16:31 ` Paolo Bonzini
2021-01-27 1:48 ` Chenyi Qiang
2021-01-27 3:41 ` Xiaoyao Li
2021-01-27 10:04 ` Paolo Bonzini
2021-01-28 7:17 ` Xiaoyao Li
2021-01-28 7:25 ` Paolo Bonzini
2021-01-28 7:41 ` Xiaoyao Li
2021-01-08 6:49 ` [RESEND PATCH 2/2] KVM: X86: Expose bus lock debug exception to guest Chenyi Qiang
2021-01-08 18:16 ` kernel test robot
2021-01-26 16:33 ` Paolo Bonzini
2021-01-27 0:57 ` Chenyi Qiang
2021-01-27 12:31 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).