Message-ID: <CALMp9eRet6+v8Y1Q-i6mqPm4hUow_kJNhmVHfOV8tMfuSS=tVg@mail.gmail.com>
Date: Tue, 9 Jul 2024 21:15:02 -0700
From: Jim Mattson <jmattson@...gle.com>
To: Ravi Bangoria <ravi.bangoria@....com>
Cc: Sean Christopherson <seanjc@...gle.com>, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, pbonzini@...hat.com, thomas.lendacky@....com, 
	hpa@...or.com, rmk+kernel@...linux.org.uk, peterz@...radead.org, 
	james.morse@....com, lukas.bulwahn@...il.com, arjan@...ux.intel.com, 
	j.granados@...sung.com, sibs@...natelecom.cn, nik.borisov@...e.com, 
	michael.roth@....com, nikunj.dadhania@....com, babu.moger@....com, 
	x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	santosh.shukla@....com, ananth.narayan@....com, sandipan.das@....com, 
	manali.shukla@....com
Subject: Re: [PATCH 3/3] KVM SVM: Add Bus Lock Detect support

On Tue, Jul 9, 2024 at 7:25 PM Ravi Bangoria <ravi.bangoria@....com> wrote:
>
> Sean,
>
> Apologies for the delay. I was waiting for Bus Lock Threshold patches to be
> posted upstream:
>
>   https://lore.kernel.org/r/20240709175145.9986-1-manali.shukla@amd.com
>
> On 12-Jun-24 7:12 AM, Sean Christopherson wrote:
> > On Wed, Jun 05, 2024, Ravi Bangoria wrote:
> >> On 6/5/2024 8:38 PM, Sean Christopherson wrote:
> >>> Some of the problems on Intel were due to the awful FMS-based feature detection,
> >>> but those weren't the only hiccups.  E.g. IIRC, we never sorted out what should
> >>> happen if both the host and guest want bus-lock #DBs.
> >>
> >> I've to check about vcpu->guest_debug part, but keeping that aside, host and
> >> guest can use Bus Lock Detect in parallel because, DEBUG_CTL MSR and DR6
> >> register are save/restored in VMCB, hardware cause a VMEXIT_EXCEPTION_1 for
> >> guest #DB(when intercepted) and hardware raises #DB on host when it's for the
> >> host.
> >
> > I'm talking about the case where the host wants to do something in response to
> > bus locks that occurred in the guest.  E.g. if the host is taking punitive action,
> > say by stalling the vCPU, then the guest kernel could bypass that behavior by
> > enabling bus lock detect itself.
> >
> > Maybe it's moot point in practice, since it sounds like Bus Lock Threshold will
> > be available at the same time.
> >
> > Ugh, and if we wanted to let the host handle guest-induced #DBs, we'd need code
> > to keep Bus Lock Detect enabled in the guest since it resides in DEBUG_CTL.  Bah.
> >
> > So I guess if the vcpu->guest_debug part is fairly straightforward, it probably
> > makes to virtualize Bus Lock Detect because the only reason not to virtualize it
> > would actually require more work/code in KVM.
>
> KVM forwards #DB to Qemu when vcpu->guest_debug is set and it's Qemu's
> responsibility to re-inject exception when Bus Lock Trap is enabled
> inside the guest. I realized that it is broken so I've prepared a
> Qemu patch, embedding it at the end.
>
> > I'd still love to see Bus Lock Threshold support sooner than later though :-)
>
> With Bus Lock Threshold enabled, I assume the changes introduced by this
> patch plus Qemu fix are sufficient to support Bus Lock Trap inside the
> guest?

In any case, it seems that commit 76ea438b4afc ("KVM: X86: Expose bus
lock debug exception to guest") prematurely advertised the presence of
X86_FEATURE_BUS_LOCK to userspace on non-Intel platforms. We should
probably either accept these changes or fix up that commit. Either
way, something should be done for all active branches back to v5.15.

> Thanks,
> Ravi
>
> ---><---
> From 0b01859f99c4c5e18323e3f4ac19d1f3e5ad4aee Mon Sep 17 00:00:00 2001
> From: Ravi Bangoria <ravi.bangoria@....com>
> Date: Thu, 4 Jul 2024 06:48:24 +0000
> Subject: [PATCH] target/i386: Add Bus Lock Detect support
>
> Upcoming AMD uarch will support Bus Lock Detect (called Bus Lock Trap
> in AMD docs). Bus Lock Detect is enumerated with cpuid Fn0000_0007_ECX_x0
> bit [24 / BUSLOCKTRAP]. It can be enabled through MSR_IA32_DEBUGCTLMSR.
> When enabled, hardware clears DR6[11] and raises a #DB exception on
> occurrence of Bus Lock if CPL > 0. More detail about the feature can be
> found in AMD APM[1].
>
> Qemu supports remote debugging through host gdb (the "gdbstub" facility)
> where some of the remote debugging features like instruction and data
> breakpoints relies on the same hardware infrastructure (#DB, DR6 etc.)
> that Bus Lock Detect also uses. Instead of handling internally, KVM
> forwards #DB to Qemu when remote debugging is ON and #DB is being
> intercepted. It's Qemu's responsibility to re-inject the exception to
> guest when some of the exception source bits (in DR6) are not being
> handled by Qemu remote debug handler. Bus Lock Detect is one such case.
>
> [1]: AMD64 Architecture Programmer's Manual Pub. 40332, Rev. 4.07 - June
>      2023, Vol 2, 13.1.3.6 Bus Lock Trap
>      https://bugzilla.kernel.org/attachment.cgi?id=304653
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@....com>
> ---
>  target/i386/cpu.h     | 1 +
>  target/i386/kvm/kvm.c | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index c64ef0c1a2..89bcff2fa3 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -271,6 +271,7 @@ typedef enum X86Seg {
>                  | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK \
>                  | CR4_LAM_SUP_MASK))
>
> +#define DR6_BLD         (1 << 11)
>  #define DR6_BD          (1 << 13)
>  #define DR6_BS          (1 << 14)
>  #define DR6_BT          (1 << 15)
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 6c864e4611..d128d4e5ca 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5141,14 +5141,14 @@ static int kvm_handle_debug(X86CPU *cpu,
>      } else if (kvm_find_sw_breakpoint(cs, arch_info->pc)) {
>          ret = EXCP_DEBUG;
>      }
> -    if (ret == 0) {
> +    if (ret == 0 || !(arch_info->dr6 & DR6_BLD)) {
>          cpu_synchronize_state(cs);
>          assert(env->exception_nr == -1);
>
>          /* pass to guest */
>          kvm_queue_exception(env, arch_info->exception,
>                              arch_info->exception == EXCP01_DB,
> -                            arch_info->dr6);
> +                            ret == 0 ? arch_info->dr6 ^ DR6_BLD : DR6_BLD);
>          env->has_error_code = 0;
>      }
>
> --
> 2.34.1
>

Powered by blists - more mailing lists