Re: [PATCH 02/10] KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3

[Sean Christopherson <seanjc@google.com>]

On Mon, Feb 24, 2025, Tom Lendacky wrote:
> On 2/18/25 19:26, Sean Christopherson wrote:
> > Never rely on the CPU to restore/load host DR0..DR3 values, even if the
> > CPU supports DebugSwap, as there are no guarantees that SNP guests will
> > actually enable DebugSwap on APs.  E.g. if KVM were to rely on the CPU to
> > load DR0..DR3 and skipped them during hw_breakpoint_restore(), KVM would
> > run with clobbered-to-zero DRs if an SNP guest created APs without
> > DebugSwap enabled.
> > 
> > Update the comment to explain the dangers, and hopefully prevent breaking
> > KVM in the future.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> 
> See comment below about the Type-A vs Type-B thing, but functionally:
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
> > ---
> >  arch/x86/kvm/svm/sev.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index e3606d072735..6c6d45e13858 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -4594,18 +4594,21 @@ void sev_es_prepare_switch_to_guest(struct vcpu_svm *svm, struct sev_es_save_are
> >  	/*
> >  	 * If DebugSwap is enabled, debug registers are loaded but NOT saved by
> >  	 * the CPU (Type-B). If DebugSwap is disabled/unsupported, the CPU both
> > -	 * saves and loads debug registers (Type-A).  Sadly, on CPUs without
> > -	 * ALLOWED_SEV_FEATURES, KVM can't prevent SNP guests from enabling
> > -	 * DebugSwap on secondary vCPUs without KVM's knowledge via "AP Create",
> > -	 * and so KVM must save DRs if DebugSwap is supported to prevent DRs
> > -	 * from being clobbered by a misbehaving guest.
> > +	 * saves and loads debug registers (Type-A).  Sadly, KVM can't prevent
> 
> This mention of Type-A was bothering me, so I did some investigation on
> this. If DebugSwap (DebugVirtualization in the latest APM) is
> disabled/unsupported, DR0-3 and DR0-3 Mask registers are left alone and
> the guest sees the host values, they are not fully restored and fully
> saved. When DebugVirtualization is enabled, at that point the registers
> become Type-B.

Good catch.  I completely glossed over that; I'm pretty sure my subconcious simply
rejected that statement as wrong.

> I'm not sure whether it is best to update the comment here or in the
> first patch.

Probably first patch.