Re: [PATCH kernel v5 5/6] KVM: SEV: Enable data breakpoints in SEV-ES

From: Alexey Kardashevskiy
Date: Tue May 30 2023 - 04:58:09 EST

On 27/5/23 00:39, Sean Christopherson wrote:
On Fri, May 26, 2023, Alexey Kardashevskiy wrote:

On 24/5/23 01:44, Sean Christopherson wrote:
On Tue, May 23, 2023, Alexey Kardashevskiy wrote:
Actually, can't disabling #DB interception for DebugSwap SEV-ES guests be a
separate patch? KVM can still inject #DBs for SEV-ES guests, no?

Sorry for my ignorance but what is the point of injecting #DB if there is no
way of changing the guest's DR7?

Well, _injecting_ the #DB is necessary for correctness from the guest's perspective.
"What's the point of _intercepting_ #DB" is the real question. And for SEV-ES guests
with DebugSwap, there is no point, which is why I agree that KVM should disable
interception in that case. What I'm calling out is that disabling #Db interception
isn't _necessary_ for correctness (unless I'm missing something), which means that
it can and should go in a separate patch.

About this. Instead of sev_es_init_vmcb(), I can toggle the #DB intercept
when toggling guest_debug, see below. This
kvm_x86_ops::update_exception_bitmap hook is called on vcpu reset and
kvm_arch_vcpu_ioctl_set_guest_debug (which skips this call if
guest_state_protected = true).

KVM also intercepts #DB when single-stepping over IRET to find an NMI window, so
you'd also have to factor in nmi_singlestep, and update svm_enable_nmi_window()
and disable_nmi_singlestep() to call svm_update_exception_bitmap().

Uff. New can of worms for me :-/

Is there any downside?

Complexity is the main one. The complexity is quite low, but the benefit to the
guest is likely even lower. A #DB in the guest isn't likely to be performance
sensitive. And on the flip side, opening an NMI window would be a tiny bit more
expensive, though I doubt that would be meaningful either.

All in all, I think it makes sense to just keep intercepting #DB for non-SEV-ES

Side topic, isn't there an existing bug regarding SEV-ES NMI windows? KVM can't
actually single-step an SEV-ES guest, but tries to set RFLAGS.TF anyways.

Why is it a "bug" and what does the patch fix? Sound to me as it is pointless and the guest won't do single stepping and instead will run till it exits somehow, what do I miss?

and suppressing EFER.SVME in efer_trap() is a bit gross,

Why suppressed? svm_set_efer() sets it eventually anyway.

but I suppose since the
GHCB doesn't allow for CLGI or STGI it's "fine".

GHCB does not mention this, instead these are always intercepted in init_vmcb().

E.g. shouldn't KVM do this?

It sure can and I am happy to include this into the series, the commit log is what I am struggling with :)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index ca32389f3c36..4e4a49031efe 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3784,6 +3784,16 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
if (svm_get_nmi_mask(vcpu) && !svm->awaiting_iret_completion)
return; /* IRET will cause a vm exit */
+ /*
+ * KVM can't single-step SEV-ES guests and instead assumes that IRET
+ * in the guest will always succeed,

It relies on GHCB's NMI_COMPLETE (which SVM than handles is it was IRET):

ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_IRET);

i.e. clears NMI masking on the
+ * next VM-Exit. Note, GIF is guaranteed to be '1' for SEV-ES guests
+ * as the GHCB doesn't allow for CLGI or STGI (and KVM suppresses
+ * EFER.SVME for good measure, see efer_trap()).

SVM KVM seems to not enforce EFER.SVME, the guest does what it wants and KVM is only told the new value via EFER_WRITE_TRAP. And "writes by SEV-ES guests to EFER.SVME are always ignored by hardware" says the APM. I must be missing the point here...

+ */
+ if (sev_es_guest(vcpu->kvm))
+ return;
if (!gif_set(svm)) {
if (vgif)
svm_set_intercept(svm, INTERCEPT_STGI);