From: Sean Christopherson <seanjc@google.com>
To: Michael Roth <michael.roth@amd.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 v15 19/20] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST NAE event
Date: Mon, 13 May 2024 16:48:25 -0700 [thread overview]
Message-ID: <ZkKmySIx_vn0W-k_@google.com> (raw)
In-Reply-To: <20240501085210.2213060-20-michael.roth@amd.com>
On Wed, May 01, 2024, Michael Roth wrote:
> Version 2 of GHCB specification added support for the SNP Extended Guest
> Request Message NAE event. This event serves a nearly identical purpose
> to the previously-added SNP_GUEST_REQUEST event, but allows for
> additional certificate data to be supplied via an additional
> guest-supplied buffer to be used mainly for verifying the signature of
> an attestation report as returned by firmware.
>
> This certificate data is supplied by userspace, so unlike with
> SNP_GUEST_REQUEST events, SNP_EXTENDED_GUEST_REQUEST events are first
> forwarded to userspace via a KVM_EXIT_VMGEXIT exit structure, and then
> the firmware request is made after the certificate data has been fetched
> from userspace.
>
> Since there is a potential for race conditions where the
> userspace-supplied certificate data may be out-of-sync relative to the
> reported TCB or VLEK that firmware will use when signing attestation
> reports, a hook is also provided so that userspace can be informed once
> the attestation request is actually completed. See the updates to
> Documentation/ for more details on these aspects.
>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
> Documentation/virt/kvm/api.rst | 87 ++++++++++++++++++++++++++++++++++
> arch/x86/kvm/svm/sev.c | 86 +++++++++++++++++++++++++++++++++
> arch/x86/kvm/svm/svm.h | 3 ++
> include/uapi/linux/kvm.h | 23 +++++++++
> 4 files changed, 199 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index f0b76ff5030d..f3780ac98d56 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -7060,6 +7060,93 @@ Please note that the kernel is allowed to use the kvm_run structure as the
> primary storage for certain register types. Therefore, the kernel may use the
> values in kvm_run even if the corresponding bit in kvm_dirty_regs is not set.
>
> +::
> +
> + /* KVM_EXIT_VMGEXIT */
> + struct kvm_user_vmgexit {
LOL, it looks dumb, but maybe kvm_vmgexit_exit to avoid confusing about whether
the struct refers to host userspace vs. guest userspace?
Actually, I vote to punt on naming until more exits need to be kicked to userspace,
and just do (see below for details on how I got here):
/* KVM_EXIT_VMGEXIT */
struct {
__u64 exit_code;
union {
struct {
__u64 data_gpa;
__u64 data_npages;
__u64 ret;
} req_certs;
};
} vmgexit;
> + #define KVM_USER_VMGEXIT_REQ_CERTS 1
> + __u32 type; /* KVM_USER_VMGEXIT_* type */
Regardless of whether or not requesting a certificate is vendor specific enough
to justify its own exit reason, I don't think KVM should have a #VMGEXIT that
adds its own layer. Structuring the user exit this way will make it weird and/or
difficult to handle #VMGEXITs that _do_ fit a generic pattern, e.g. a user might
wonder why PSC #VMGEXITs don't show up here.
And defining an exit reason that is, for all intents and purposes, a regurgitation
of the raw #VMGEXIT reason, but with a different value, is also confusing. E.g.
it wouldn't be unreasonable for a reader to expect that "type" matches the value
defined in the GHCB (or whever the values are defined).
Ah, you copied what KVM does for Hyper-V and Xen emulation. Hrm. But only
partially.
Assuming it's impractical to have a generic user exit for this, and we think
there is a high likelihood of needing to punt more #VMGEXITs to userspace, then
we should more closely (perhaps even exactly) follow the Hyper-V and Xen models.
I.e. for all values and whanot that are controlled/defined by a third party
(Hyper-V, Xen, the GHCB, etc.) #define those values in a header that is clearly
"owned" by the third party.
E.g. IIRC, include/xen/interface/xen.h is copied verbatim from Xen documentation
(source?). And include/asm-generic/hyperv-tlfs.h is the kernel's copy of the
TLFS, which dictates all of the Hyper-V hypercalls.
If we do that, then my concerns/objections largely go away, e.g. KVM isn't
defining magic values, there's less chance for confusion about what "type" holds,
etc.
Oh, and if we go that route, the sizes for all fields should follow the GHCB,
e.g. I believe the "type" should be a __u64.
> + union {
> + struct {
> + __u64 data_gpa;
> + __u64 data_npages;
> + #define KVM_USER_VMGEXIT_REQ_CERTS_ERROR_INVALID_LEN 1
> + #define KVM_USER_VMGEXIT_REQ_CERTS_ERROR_BUSY 2
> + #define KVM_USER_VMGEXIT_REQ_CERTS_ERROR_GENERIC (1 << 31)
Hopefully it won't matter, but are BUSY and GENERIC actually defined somewhere?
I don't see them in GHCB 2.0.
In a perfect world, it would be nice for KVM to not have to care about the error
codes. But KVM disallows KVM_{G,S}ET_REGS for guest with protected state, which
means it's not feasible for userspace to set registers, at least not in any sane
way.
Heh, we could abuse KVM_SYNC_X86_REGS to let userspace specify RBX, but (a) that's
gross, and (b) KVM_SYNC_X86_REGS and KVM_SYNC_X86_SREGS really ought to be rejected
if guest state is protected.
> + __u32 ret;
> + #define KVM_USER_VMGEXIT_REQ_CERTS_FLAGS_NOTIFY_DONE BIT(0)
This has no business being buried in a VMGEXIT_REQ_CERTS flags. Notifying
userspace that KVM completed its portion of a userspace exit is completely generic.
And aside from where the notification flag lives, _if_ we add a notification
mechanism, it belongs in a separate patch, because it's purely a performance
optimization. Userspace can use immediate_exit to force KVM to re-exit after
completing an exit.
Actually, I take that back, this isn't even an optimization, it's literally a
non-generic implementation of kvm_run.immediate_exit.
If this were an optimization, i.e. KVM truly notified userspace without exiting,
then it would need to be a lot more robust, e.g. to ensure userspace actually
received the notification before KVM moved on.
> + __u8 flags;
> + #define KVM_USER_VMGEXIT_REQ_CERTS_STATUS_PENDING 0
> + #define KVM_USER_VMGEXIT_REQ_CERTS_STATUS_DONE 1
This is also a weird reimplementation of generic functionality. KVM nullifies
vcpu->arch.complete_userspace_io _before_ invoking the callback. So if a callback
needs to run again on the next KVM_RUN, it can simply set complete_userspace_io
again. In other words, literally doing nothing will get you what you want :-)
> + __u8 status;
> + } req_certs;
> + };
> + };
next prev parent reply other threads:[~2024-05-13 23:48 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-01 8:51 [PATCH v15 00/20] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Michael Roth
2024-05-01 8:51 ` [PATCH v15 01/20] Revert "KVM: x86: Add gmem hook for determining max NPT mapping level" Michael Roth
2024-05-01 8:51 ` [PATCH v15 02/20] KVM: x86: Add hook for determining max NPT mapping level Michael Roth
2024-05-02 23:11 ` Isaku Yamahata
2024-05-07 17:48 ` Paolo Bonzini
2024-05-01 8:51 ` [PATCH v15 03/20] KVM: SEV: Select KVM_GENERIC_PRIVATE_MEM when CONFIG_KVM_AMD_SEV=y Michael Roth
2024-05-01 8:51 ` [PATCH v15 04/20] KVM: SEV: Add initial SEV-SNP support Michael Roth
2024-05-01 8:51 ` [PATCH v15 05/20] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command Michael Roth
2024-05-01 8:51 ` [PATCH v15 06/20] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_UPDATE command Michael Roth
2024-05-01 8:51 ` [PATCH v15 07/20] KVM: SEV: Add KVM_SEV_SNP_LAUNCH_FINISH command Michael Roth
2024-05-01 8:51 ` [PATCH v15 08/20] KVM: SEV: Add support to handle GHCB GPA register VMGEXIT Michael Roth
2024-05-01 8:51 ` [PATCH v15 09/20] KVM: SEV: Add support to handle MSR based Page State Change VMGEXIT Michael Roth
2024-05-16 8:28 ` Binbin Wu
2024-05-16 17:23 ` Paolo Bonzini
2024-05-21 0:49 ` Binbin Wu
2024-05-21 21:49 ` Michael Roth
2024-05-27 12:25 ` Binbin Wu
2024-05-28 10:39 ` Paolo Bonzini
2024-05-29 20:02 ` Sean Christopherson
2024-05-31 1:22 ` Binbin Wu
2024-05-31 13:10 ` Paolo Bonzini
2024-05-30 16:47 ` Zhi Wang
2024-05-01 8:52 ` [PATCH v15 10/20] KVM: SEV: Add support to handle " Michael Roth
2024-05-01 8:52 ` [PATCH v15 11/20] KVM: SEV: Add support to handle RMP nested page faults Michael Roth
2024-05-01 8:52 ` [PATCH v15 12/20] KVM: SEV: Support SEV-SNP AP Creation NAE event Michael Roth
2024-05-01 8:52 ` [PATCH v15 13/20] KVM: SEV: Implement gmem hook for initializing private pages Michael Roth
2024-05-20 10:16 ` Huang, Kai
2024-05-20 17:35 ` Sean Christopherson
2024-05-20 21:57 ` Huang, Kai
2024-05-20 23:15 ` Sean Christopherson
2024-05-20 23:41 ` Huang, Kai
2024-05-21 0:30 ` Sean Christopherson
2024-05-20 19:14 ` Isaku Yamahata
2024-05-01 8:52 ` [PATCH v15 14/20] KVM: SEV: Implement gmem hook for invalidating " Michael Roth
2024-05-01 8:52 ` [PATCH v15 15/20] KVM: x86: Implement hook for determining max NPT mapping level Michael Roth
2024-05-01 8:52 ` [PATCH v15 16/20] KVM: SEV: Avoid WBINVD for HVA-based MMU notifications for SNP Michael Roth
2024-05-01 8:52 ` [PATCH v15 17/20] KVM: SVM: Add module parameter to enable SEV-SNP Michael Roth
2024-05-01 8:52 ` [PATCH v15 18/20] KVM: SEV: Provide support for SNP_GUEST_REQUEST NAE event Michael Roth
2024-05-01 8:52 ` [PATCH v15 19/20] KVM: SEV: Provide support for SNP_EXTENDED_GUEST_REQUEST " Michael Roth
2024-05-13 23:48 ` Sean Christopherson [this message]
2024-05-14 2:51 ` Michael Roth
2024-05-14 14:36 ` Sean Christopherson
2024-05-15 1:25 ` [PATCH] KVM: SEV: Replace KVM_EXIT_VMGEXIT with KVM_EXIT_SNP_REQ_CERTS Michael Roth
2024-05-01 8:52 ` [PATCH v15 20/20] crypto: ccp: Add the SNP_VLEK_LOAD command Michael Roth
2024-05-07 18:04 ` [PATCH v15 00/20] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support Paolo Bonzini
2024-05-07 18:14 ` Michael Roth
2024-05-10 2:34 ` Michael Roth
2024-05-10 1:58 ` [PATCH v15 21/23] KVM: MMU: Disable fast path for private memslots Michael Roth
2024-05-10 1:58 ` [PATCH v15 22/23] KVM: SEV: Fix return code interpretation for RMP nested page faults Michael Roth
2024-05-10 13:58 ` Sean Christopherson
2024-05-10 15:36 ` Michael Roth
2024-05-10 16:01 ` Paolo Bonzini
2024-05-10 16:37 ` Michael Roth
2024-05-10 16:59 ` Paolo Bonzini
2024-05-10 17:25 ` Paolo Bonzini
2024-05-14 8:10 ` Borislav Petkov
2024-05-10 1:58 ` [PATCH v15 23/23] KVM: SEV: Fix PSC handling for SMASH/UNSMASH and partial update ops Michael Roth
2024-05-10 17:09 ` Paolo Bonzini
2024-05-10 19:08 ` Michael Roth
2024-05-10 13:47 ` [PATCH v15 21/23] KVM: MMU: Disable fast path for private memslots Sean Christopherson
2024-05-10 13:50 ` Paolo Bonzini
2024-05-10 15:27 ` Michael Roth
2024-05-10 15:59 ` Sean Christopherson
2024-05-10 17:47 ` Isaku Yamahata
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=ZkKmySIx_vn0W-k_@google.com \
--to=seanjc@google.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=michael.roth@amd.com \
--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=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).