From: Michael Roth <michael.roth@amd.com>
To: Sean Christopherson <seanjc@google.com>
Cc: <kvm@vger.kernel.org>, <linux-coco@lists.linux.dev>,
<linux-mm@kvack.org>, <linux-crypto@vger.kernel.org>,
<x86@kernel.org>, <linux-kernel@vger.kernel.org>,
<tglx@linutronix.de>, <mingo@redhat.com>, <jroedel@suse.de>,
<thomas.lendacky@amd.com>, <hpa@zytor.com>, <ardb@kernel.org>,
<pbonzini@redhat.com>, <vkuznets@redhat.com>,
<jmattson@google.com>, <luto@kernel.org>,
<dave.hansen@linux.intel.com>, <slp@redhat.com>,
<pgonda@google.com>, <peterz@infradead.org>,
<srinivas.pandruvada@linux.intel.com>, <rientjes@google.com>,
<dovmurik@linux.ibm.com>, <tobin@ibm.com>, <bp@alien8.de>,
<vbabka@suse.cz>, <kirill@shutemov.name>, <ak@linux.intel.com>,
<tony.luck@intel.com>,
<sathyanarayanan.kuppuswamy@linux.intel.com>,
<alpergun@google.com>, <jarkko@kernel.org>,
<ashish.kalra@amd.com>, <nikunj.dadhania@amd.com>,
<pankaj.gupta@amd.com>, <liam.merwick@oracle.com>
Subject: Re: [PATCH v14 22/22] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST NAE event
Date: Fri, 26 Apr 2024 12:57:43 -0500 [thread overview]
Message-ID: <20240426175743.t4cocerwwhaevieh@amd.com> (raw)
In-Reply-To: <Zimfdyhq3U2HVX0N@google.com>
On Wed, Apr 24, 2024 at 05:10:31PM -0700, Sean Christopherson wrote:
> On Sun, Apr 21, 2024, Michael Roth wrote:
> > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > index 85099198a10f..6cf186ed8f66 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -7066,6 +7066,7 @@ values in kvm_run even if the corresponding bit in kvm_dirty_regs is not set.
> > struct kvm_user_vmgexit {
> > #define KVM_USER_VMGEXIT_PSC_MSR 1
> > #define KVM_USER_VMGEXIT_PSC 2
> > + #define KVM_USER_VMGEXIT_EXT_GUEST_REQ 3
>
> Assuming we can't get massage this into a vendor agnostic exit, there's gotta be
> a better name than EXT_GUEST_REQ, which is completely meaningless to me and probably
> most other people that aren't intimately familar with the specs. Request what?
EXT_CERT_REQ maybe? That's effectively all it boils down to, fetching
the GHCB-defined certificate blob from userspace.
>
> > __u32 type; /* KVM_USER_VMGEXIT_* type */
> > union {
> > struct {
> > @@ -3812,6 +3813,84 @@ static void snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp
> > ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, SNP_GUEST_ERR(vmm_ret, fw_err));
> > }
> >
> > +static int snp_complete_ext_guest_req(struct kvm_vcpu *vcpu)
> > +{
> > + struct vcpu_svm *svm = to_svm(vcpu);
> > + struct vmcb_control_area *control;
> > + struct kvm *kvm = vcpu->kvm;
> > + sev_ret_code fw_err = 0;
> > + int vmm_ret;
> > +
> > + vmm_ret = vcpu->run->vmgexit.ext_guest_req.ret;
> > + if (vmm_ret) {
> > + if (vmm_ret == SNP_GUEST_VMM_ERR_INVALID_LEN)
> > + vcpu->arch.regs[VCPU_REGS_RBX] =
> > + vcpu->run->vmgexit.ext_guest_req.data_npages;
> > + goto abort_request;
> > + }
> > +
> > + control = &svm->vmcb->control;
> > +
> > + /*
> > + * To avoid the message sequence number getting out of sync between the
> > + * actual value seen by firmware verses the value expected by the guest,
> > + * make sure attestations can't get paused on the write-side at this
> > + * point by holding the lock for the entire duration of the firmware
> > + * request so that there is no situation where SNP_GUEST_VMM_ERR_BUSY
> > + * would need to be returned after firmware sees the request.
> > + */
> > + mutex_lock(&snp_pause_attestation_lock);
>
> Why is this in KVM? IIUC, KVM only needs to get involved to translate GFNs to
> PFNs, the rest can go in the sev-dev driver, no? The whole split is weird,
> seemingly due to KVM "needing" to take this lock. E.g. why is core kernel code
> in arch/x86/virt/svm/sev.c at all dealing with attestation goo, when pretty much
> all of the actual usage is (or can be) in sev-dev.c
It would be very tempting to have all this locking tucked away in sev-dev
driver, but KVM is a part of that sequence because it needs to handle fetching
the certificate that will be returned to the guest as part of the
attestation request. The transaction ID updates from PAUSE/RESUME
commands is technically enough satisfy this requirement, in which case
KVM wouldn't need to take the lock directly, only check that the
transaction ID isn't stale and report -EBUSY/-EAGAIN to the guest if it
is.
But if the request actually gets sent to firmware, and then we decide
after that the transaction is stale and thus the attestation response
and certificate might not be in sync, then reporting -EBUSY/-EAGAIN will
permanently hose the guest because the message seq fields will be out of
sync between firmware and guest kernel. That's why we need to hold the
lock to actually block a transaction ID update from occuring once we've
committed to sending the attestation request to firmware.
>
> > +
> > + if (__snp_transaction_is_stale(svm->snp_transaction_id))
> > + vmm_ret = SNP_GUEST_VMM_ERR_BUSY;
> > + else if (!__snp_handle_guest_req(kvm, control->exit_info_1,
> > + control->exit_info_2, &fw_err))
> > + vmm_ret = SNP_GUEST_VMM_ERR_GENERIC;
> > +
> > + mutex_unlock(&snp_pause_attestation_lock);
> > +
> > +abort_request:
> > + ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, SNP_GUEST_ERR(vmm_ret, fw_err));
> > +
> > + return 1; /* resume guest */
> > +}
> > +
> > +static int snp_begin_ext_guest_req(struct kvm_vcpu *vcpu)
> > +{
> > + int vmm_ret = SNP_GUEST_VMM_ERR_GENERIC;
> > + struct vcpu_svm *svm = to_svm(vcpu);
> > + unsigned long data_npages;
> > + sev_ret_code fw_err;
> > + gpa_t data_gpa;
> > +
> > + if (!sev_snp_guest(vcpu->kvm))
> > + goto abort_request;
> > +
> > + data_gpa = vcpu->arch.regs[VCPU_REGS_RAX];
> > + data_npages = vcpu->arch.regs[VCPU_REGS_RBX];
> > +
> > + if (!IS_ALIGNED(data_gpa, PAGE_SIZE))
> > + goto abort_request;
> > +
> > + svm->snp_transaction_id = snp_transaction_get_id();
> > + if (snp_transaction_is_stale(svm->snp_transaction_id)) {
>
> Why bother? I assume this is rare, so why not handle it on the backend, i.e.
> after userspace does its thing? Then KVM doesn't even have to care about
> checking for stale IDs, I think?
That's true, it's little more than a very minor performance optimization
to do it here rather than on the return path, so could definitely be
dropped.
>
> None of this makes much sense to my eyes, e.g. AFAICT, the only thing that can
> pause attestation is userspace, yet the kernel is responsible for tracking whether
> or not a transaction is fresh?
Pausing is essentially the start of an update transaction initiated by
userspace. So the kernel is tracking whether there's a potential
transaction that has been started or completed since it first started
servicing the attestation request, otherwise there could be a mismatch
between the endorsement key used for the attestation report versus the
certificate for the endorsement key that was fetched from userspace.
-Mike
next prev parent reply other threads:[~2024-04-26 17:58 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-21 18:01 [PATCH v14 00/22] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Michael Roth
2024-04-21 18:01 ` [PATCH v14 01/22] KVM: SEV: Select KVM_GENERIC_PRIVATE_MEM when CONFIG_KVM_AMD_SEV=y Michael Roth
2024-04-21 18:01 ` [PATCH v14 02/22] KVM: SEV: Add support to handle AP reset MSR protocol Michael Roth
2024-04-21 18:01 ` [PATCH v14 03/22] KVM: SEV: Add GHCB handling for Hypervisor Feature Support requests Michael Roth
2024-04-24 20:21 ` Sean Christopherson
2024-04-25 20:52 ` Michael Roth
2024-04-25 21:55 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 04/22] KVM: SEV: Add initial SEV-SNP support Michael Roth
2024-04-21 18:01 ` [PATCH v14 05/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command Michael Roth
2024-04-24 23:26 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 06/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command Michael Roth
2024-04-24 23:58 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 07/22] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_FINISH command Michael Roth
2024-04-21 18:01 ` [PATCH v14 08/22] KVM: SEV: Add support to handle GHCB GPA register VMGEXIT Michael Roth
2024-04-21 18:01 ` [PATCH v14 09/22] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT Michael Roth
2024-04-24 20:59 ` Sean Christopherson
2024-04-25 22:00 ` Michael Roth
2024-04-25 22:13 ` Sean Christopherson
2024-04-26 17:16 ` Michael Roth
2024-04-26 20:14 ` Sean Christopherson
2024-04-26 22:24 ` Michael Roth
2024-04-26 22:48 ` Michael Roth
2024-04-21 18:01 ` [PATCH v14 10/22] KVM: SEV: Add support to handle " Michael Roth
2024-04-21 18:01 ` [PATCH v14 11/22] KVM: SEV: Add support to handle RMP nested page faults Michael Roth
2024-04-21 18:01 ` [PATCH v14 12/22] KVM: SEV: Support SEV-SNP AP Creation NAE event Michael Roth
2024-04-21 18:01 ` [PATCH v14 13/22] KVM: SEV: Add support for GHCB-based termination requests Michael Roth
2024-04-21 18:01 ` [PATCH v14 14/22] KVM: SEV: Implement gmem hook for initializing private pages Michael Roth
2024-04-21 18:01 ` [PATCH v14 15/22] KVM: SEV: Implement gmem hook for invalidating " Michael Roth
2024-04-21 18:01 ` [PATCH v14 16/22] KVM: x86: Implement gmem hook for determining max NPT mapping level Michael Roth
2024-04-25 0:45 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 17/22] KVM: SEV: Avoid WBINVD for HVA-based MMU notifications for SNP Michael Roth
2024-04-25 0:17 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 18/22] KVM: SVM: Add module parameter to enable SEV-SNP Michael Roth
2024-04-21 18:01 ` [PATCH v14 19/22] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event Michael Roth
2024-04-21 18:01 ` [PATCH v14 20/22] crypto: ccp: Add the SNP_VLEK_LOAD command Michael Roth
2024-04-21 18:01 ` [PATCH v14 21/22] crypto: ccp: Add the SNP_{PAUSE,RESUME}_ATTESTATION commands Michael Roth
2024-04-25 0:15 ` Sean Christopherson
2024-04-26 17:35 ` Michael Roth
2024-04-26 19:57 ` Sean Christopherson
2024-04-26 21:46 ` Michael Roth
2024-04-27 0:10 ` Sean Christopherson
2024-04-27 1:32 ` Michael Roth
2024-04-29 14:27 ` Sean Christopherson
2024-04-21 18:01 ` [PATCH v14 22/22] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST NAE event Michael Roth
2024-04-25 0:10 ` Sean Christopherson
2024-04-26 17:57 ` Michael Roth [this message]
2024-04-23 16:21 ` [PATCH v14 23/22] [SQUASH] KVM: SEV: Add support to handle GHCB GPA register VMGEXIT Michael Roth
2024-04-23 16:21 ` [PATCH v14 24/22] [SQUASH] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT Michael Roth
2024-04-23 16:21 ` [PATCH v14 25/22] [SQUASH] KVM: SEV: Add support to handle " Michael Roth
2024-04-23 16:21 ` [PATCH v14 26/22] [SQUASH] KVM: SEV: Add support for GHCB-based termination requests Michael Roth
2024-04-23 16:21 ` [PATCH v14 27/22] [SQUASH] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event Michael Roth
2024-04-23 16:21 ` [PATCH v14 28/22] [SQUASH] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST " Michael Roth
2024-04-23 21:36 ` Jarkko Sakkinen
2024-04-23 16:21 ` [PATCH v14 29/22] [SQUASH] KVM: SEV: Support SEV-SNP AP Creation " Michael Roth
2024-04-23 16:31 ` [PATCH v14 00/22] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Michael Roth
2024-04-24 16:51 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240426175743.t4cocerwwhaevieh@amd.com \
--to=michael.roth@amd.com \
--cc=ak@linux.intel.com \
--cc=alpergun@google.com \
--cc=ardb@kernel.org \
--cc=ashish.kalra@amd.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=dovmurik@linux.ibm.com \
--cc=hpa@zytor.com \
--cc=jarkko@kernel.org \
--cc=jmattson@google.com \
--cc=jroedel@suse.de \
--cc=kirill@shutemov.name \
--cc=kvm@vger.kernel.org \
--cc=liam.merwick@oracle.com \
--cc=linux-coco@lists.linux.dev \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=nikunj.dadhania@amd.com \
--cc=pankaj.gupta@amd.com \
--cc=pbonzini@redhat.com \
--cc=peterz@infradead.org \
--cc=pgonda@google.com \
--cc=rientjes@google.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=seanjc@google.com \
--cc=slp@redhat.com \
--cc=srinivas.pandruvada@linux.intel.com \
--cc=tglx@linutronix.de \
--cc=thomas.lendacky@amd.com \
--cc=tobin@ibm.com \
--cc=tony.luck@intel.com \
--cc=vbabka@suse.cz \
--cc=vkuznets@redhat.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).