Messages in this thread | | | Date | Fri, 2 Jun 2023 09:31:05 +1000 | Subject | Re: [PATCH kernel v5 5/6] KVM: SEV: Enable data breakpoints in SEV-ES | From | Alexey Kardashevskiy <> |
| |
Sean, ping?
I wonder if this sev-es-not-singlestepping is a showstopper or it is alright to repost this patchset without it? Thanks,
On 30/5/23 18:57, Alexey Kardashevskiy wrote: > > > 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 >> guests. >> >> 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? > >> Blech, >> 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): > > case SVM_VMGEXIT_NMI_COMPLETE: > ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_IRET); > break; > > >> 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); >
-- Alexey
| |