Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Mar 2023 09:39:02 -0700
In-Reply-To: <3b3a9ebc-b02e-a365-7f68-3da9189d062a@amd.com>
Mime-Version: 1.0
References: <20230120031047.628097-1-aik@amd.com> <20230120031047.628097-3-aik@amd.com>
 <Y9nL8iqhiL5+ALa2@google.com> <3b3a9ebc-b02e-a365-7f68-3da9189d062a@amd.com>
Message-ID: <ZByAptUXE+VMpn2x@google.com>
Subject: Re: [PATCH kernel v3 2/3] KVM: SEV: Enable data breakpoints in SEV-ES
From:   Sean Christopherson <seanjc@google.com>
To:     Alexey Kardashevskiy <aik@amd.com>
Cc:     kvm@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org,
        Yury Norov <yury.norov@gmail.com>,
        Venu Busireddy <venu.busireddy@oracle.com>,
        Tony Luck <tony.luck@intel.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Sandipan Das <sandipan.das@amd.com>,
        Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Michael Roth <michael.roth@amd.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        Kim Phillips <kim.phillips@amd.com>,
        Kees Cook <keescook@chromium.org>,
        Juergen Gross <jgross@suse.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Daniel Sneddon <daniel.sneddon@linux.intel.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Borislav Petkov <bp@alien8.de>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        "H. Peter Anvin" <hpa@zytor.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Fri, Feb 03, 2023, Alexey Kardashevskiy wrote:
> > Follow-up question: does KVM _have_ to wait until KVM_SEV_LAUNCH_UPDATE_VMSA to
> > set the flag?
> 
> Nope. Will repost soon as a reply to this mail.

Please, please do not post new versions In-Reply-To the previous version, and
especially not In-Reply-To a random mail buried deep in the thread.  b4, which
is imperfect but makes my life sooo much easier, gets confused by all the threading
and partial rerolls.  The next version also buries _this_ reply, which is partly
why I haven't responded until now.  I simply missed this the below questions because
I saw v4 and assumed all my feedback was addressed, i.e. that I could handle this
in the context of 6.4 and not earlier.

Continuing on that topic, please do not post a new version until open questions
from the previous version are resolved.  Posting a new version when there are
unresolved questions might feel like it helps things move faster, but more often
than not it has the comlete opposite effect.

> > > +/* enable/disable SEV-ES DebugSwap support */
> > > +static bool sev_es_debug_swap_enabled = true;
> > > +module_param_named(debug_swap, sev_es_debug_swap_enabled, bool, 0644);
> > 
> > Module param needs 0444 permissions, i.e. shouldn't be writable after KVM is
> > loaded.  Though I don't know that providing a module param is warranted in this
> > case.
> > KVM provides module params for SEV and SEV-ES because there are legitimate
> > reasons to turn them off, but at a glance, I don't see why we'd want that for this
> > feature.
> 
> 
> /me confused. You suggested this in the first place for (I think) for the
> case if the feature is found to be broken later on so admins can disable it.

Hrm, so I did.  Right, IIUC, this has guest visible effects, i.e. guest can
read/write DRs, and so the admin might want the ability to disable the feature.

Speaking of past me, no one answered my question about how this will interact
with SNP, where the VM can maniuplate the VMSA.

  : > @@ -604,6 +607,9 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
  : >       save->xss  = svm->vcpu.arch.ia32_xss;
  : >       save->dr6  = svm->vcpu.arch.dr6;
  : > 
  : > +     if (sev->debug_swap)
  : > +             save->sev_features |= SVM_SEV_FEAT_DEBUG_SWAP;
  : 
  : Resurrecting my objection to "AP Creation NAE event"[*], what happens if a hypervisor
  : supports GHCB_HV_FT_SNP_AP_CREATION but not DebugSwap?  IIUC, a guest can corrupt
  : host DRs by enabling DebugSwap in the VMSA of an AP vCPU, e.g. the CPU will load
  : zeros on VM-Exit if the host hasn't stuffed the host save area fields.
  : 
  : KVM can obviously just make sure to save its DRs if hardware supports DebugSwap,
  : but what if DebugSwap is buggy and needs to be disabled?  And what about the next
  : feature that can apparently be enabled by the guest?
  : 
  : [*] https://lore.kernel.org/all/YWnbfCet84Vup6q9@google.com

> And I was using it to verify "x86/debug: Fix stack recursion caused by DR7
> accesses" which is convenient but it is a minor thing.

...

> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index 60c7c880266b..6c54a3c9d442 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -1190,7 +1190,8 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
> > >   	set_exception_intercept(svm, UD_VECTOR);
> > >   	set_exception_intercept(svm, MC_VECTOR);
> > >   	set_exception_intercept(svm, AC_VECTOR);
> > > -	set_exception_intercept(svm, DB_VECTOR);
> > > +	if (!sev_es_is_debug_swap_enabled())
> > > +		set_exception_intercept(svm, DB_VECTOR);
> > 
> > This is wrong.  KVM needs to intercept #DBs when debugging non-SEV-ES VMs.
> 
> Sorry, not following. The #DB intercept for non-SEV-ES is enabled here.

The helper in this version (v3) just queries whether or not the feature is enabled,
it doesn't differentiate between SEV-ES and other VM types.  I.e. loading KVM with
SEV-ES and DebugSwap enabled would break non-SEV-ES VMs running on the same host.

 +bool sev_es_is_debug_swap_enabled(void)
 +{
 +     return sev_es_debug_swap_enabled;
 +}

Looks like this was fixed in v4.

> > This _could_ be tied to X86_FEATURE_NO_NESTED_DATA_BP, but the KVM would need to
> > toggle the intercept depending on whether or not userspace wants to debug the
> > guest.
> > 
> > Similar to the DR7 interception, can this check sev_features directly?