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

[Sean Christopherson]

 On Fri, Jun 02, 2023, Alexey Kardashevskiy wrote:
> Sean, ping?
> 
> I wonder if this sev-es-not-singlestepping is a showstopper or it is alright
> to repost this patchset without it? Thanks,

Ah, shoot, I completely lost this in my inbox.  Sorry :-/

> > > 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?

The bug is benign in the end, but it's still a bug.  I'm not worried about fixing
any behavior, but I dislike having dead, misleading code, especially for something
like this where both NMI virtualization and SEV-ES are already crazy complex and
subtle.  I think it's safe to say that I've spent more time digging through SEV-ES
and NMI virtualization than most KVM developers, and as evidenced by the number of
things I got wrong below, I'm still struggling to keep track of the bigger picture.
Developers that are new to all of this need as much help as they can get.

> > > Blech, and suppressing EFER.SVME in efer_trap() is a bit gross,
> > 
> > Why suppressed? svm_set_efer() sets it eventually anyway.

svm_set_efer() sets SVME in hardware, but KVM's view of the guest's value that's
stored in vcpu->arch.efer doesn't have SVME set.  E.g. from the guest's perspective,
EFER.SVME will have "Reserved Read As Zero" semantics.

> > > 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().

Right, I'm calling out that the absense of protocol support for requesting CLGI
or STGI emulation means dropping the guest's EFER.SVME is ok (though gross :-) ).

> > > 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;

Ah, right, better to say that the guest is responsible for signaling that it's
ready to accept NMIs, which KVM handles by "emulating" 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.

Ahhh, that blurb in the APM is what I'm missing.

Actually, there's a real bug here.  KVM doesn't immediately unmask NMIs in response
to NMI_COMPLETE, and instead goes through the whole awaiting_iret_completion =>
svm_complete_interrupts(), which means that KVM doesn't unmask NMIs until the
*next* VM-Exit.  Theoretically, that could be never, e.g. if the host is tickless
and the guest is configured to busy wait idle CPUs.

Attached patches are compile tested only.