From: Isaku Yamahata <isaku.yamahata@gmail.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Xiaoyao Li <xiaoyao.li@intel.com>,
isaku.yamahata@intel.com, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
Michael Roth <michael.roth@amd.com>,
Paolo Bonzini <pbonzini@redhat.com>,
erdemaktas@google.com, Sagi Shahar <sagis@google.com>,
David Matlack <dmatlack@google.com>,
Kai Huang <kai.huang@intel.com>,
Zhi Wang <zhi.wang.linux@gmail.com>,
chen.bo@intel.com, linux-coco@lists.linux.dev,
Chao Peng <chao.p.peng@linux.intel.com>,
Ackerley Tng <ackerleytng@google.com>,
Vishal Annapurve <vannapurve@google.com>,
Yuan Yao <yuan.yao@linux.intel.com>
Subject: Re: [RFC PATCH v4 09/10] KVM: x86: Make struct sev_cmd common for KVM_MEM_ENC_OP
Date: Wed, 26 Jul 2023 17:37:59 -0700 [thread overview]
Message-ID: <20230727003759.GA2021422@ls.amr.corp.intel.com> (raw)
In-Reply-To: <ZL/r6Vca8WkFVaic@google.com>
On Tue, Jul 25, 2023 at 08:36:09AM -0700,
Sean Christopherson <seanjc@google.com> wrote:
> On Tue, Jul 25, 2023, Xiaoyao Li wrote:
> > On 7/21/2023 10:51 PM, Sean Christopherson wrote:
> > > On Thu, Jul 20, 2023, isaku.yamahata@intel.com wrote:
> > > > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > > > index aa7a56a47564..32883e520b00 100644
> > > > --- a/arch/x86/include/uapi/asm/kvm.h
> > > > +++ b/arch/x86/include/uapi/asm/kvm.h
> > > > @@ -562,6 +562,39 @@ struct kvm_pmu_event_filter {
> > > > /* x86-specific KVM_EXIT_HYPERCALL flags. */
> > > > #define KVM_EXIT_HYPERCALL_LONG_MODE BIT(0)
> > > > +struct kvm_mem_enc_cmd {
> > > > + /* sub-command id of KVM_MEM_ENC_OP. */
> > > > + __u32 id;
> > > > + /*
> > > > + * Auxiliary flags for sub-command. If sub-command doesn't use it,
> > > > + * set zero.
> > > > + */
> > > > + __u32 flags;
> > > > + /*
> > > > + * Data for sub-command. An immediate or a pointer to the actual
> > > > + * data in process virtual address. If sub-command doesn't use it,
> > > > + * set zero.
> > > > + */
> > > > + __u64 data;
> > > > + /*
> > > > + * Supplemental error code in the case of error.
> > > > + * SEV error code from the PSP or TDX SEAMCALL status code.
> > > > + * The caller should set zero.
> > > > + */
> > > > + union {
> > > > + struct {
> > > > + __u32 error;
> > > > + /*
> > > > + * KVM_SEV_LAUNCH_START and KVM_SEV_RECEIVE_START
> > > > + * require extra data. Not included in struct
> > > > + * kvm_sev_launch_start or struct kvm_sev_receive_start.
> > > > + */
> > > > + __u32 sev_fd;
> > > > + };
> > > > + __u64 error64;
> > > > + };
> > > > +};
> > >
> > > Eww. Why not just use an entirely different struct for TDX? I don't see what
> > > benefit this provides other than a warm fuzzy feeling that TDX and SEV share a
> > > struct. Practically speaking, KVM will likely take on more work to forcefully
> > > smush the two together than if they're separate things.
> >
> > generalizing the struct of KVM_MEM_ENC_OP should be the first step.
>
> It's not just the one structure though. The "data" field is a pointer to yet
> another layer of commands, and SEV has a rather big pile of those. Making
> kvm_mem_enc_cmd common is just putting lipstick on a pig since the vast majority
> of the structures associated with the ioctl() would still be vendor specific.
> struct kvm_sev_launch_start
> struct kvm_sev_launch_update_data
> struct kvm_sev_launch_secret
> struct kvm_sev_launch_measure
> struct kvm_sev_guest_status
> struct kvm_sev_dbg
> struct kvm_sev_attestation_report
> struct kvm_sev_send_start
> struct kvm_sev_send_update_data
> struct kvm_sev_receive_start
> struct kvm_sev_receive_update_data
>
> FWIW, I really dislike KVM's uAPI for KVM_MEM_ENC_OP. The above structures are
> all basically copied verbatim from PSP firmware structures, i.e. the commands and
> their payloads are tightly coupled to "hardware" and essentially have no abstraction
> whatsoever. But that ship has already sailed, and practically speaking trying to
> provide a layer of abstraction might not of worked very well anyways.
>
> In other words, unless there's an obvious and easy way path to convergence, I
> recommend you don't spend much time/effort on trying to share code with TDX.
I think we can easily unify vcpu initialization, populating/measure initial
memory, completing guest creation, and guest memory access for debug.
KVM_SEV_LAUNCH_UPDATE_VMSA <-> KVM_TDX_INIT_VCPU
KVM_SEV_LAUNCH_UPDATE_DATA and KVM_SEV_LAUNCH_MEASURE <-> KVM_INIT_MEM_REGION
KVM_SEV_LAUNCH_FINISH <-> KVM_TDX_FINALIZE_VM
KVM_SEV_DBG_DECRYPT, KVM_SEV_DBG_ENCRYPT: KVM common API for access protected guest memory
Here's my assessment. For now I don't address migration.
For creating confidential guest:
- Get the capability of underlying platform
KVM_TDX_CAPABILITY: no sev correspondence.
- Initialize VM as confidential VM
struct kvm_sev_launch_start
KVM_SEV{,_ES}_INIT, and KVM_SEV_LAUNCH_START:
KVM_TDX_INIT_VM
They take vendor specific data.
- Initialize vcpu
KVM_SEV_LAUNCH_UPDATE_VMSA: no extra argument
KVM_TDX_INIT_VCPU: no extra argument
- populate initial memory + measurement
KVM_SEV_LAUNCH_UPDATE_DATA and KVM_SEV_LAUNCH_MEASURE,
struct kvm_sev_launch_update_data {
__u64 uaddr;
__u32 len;
};
struct kvm_sev_launch_measure {
__u64 uaddr;
__u32 len;
};
=> GPA is calculated from uaddr.
KVM_INIT_MEM_REGION:
struct kvm_tdx_init_mem_region {
__u64 source_addr; // uaddr
__u64 gpa;
__u64 nr_pages;
};
I think those can same structure. Or prefault or prepopulating
e.g.
struct {
__u64 uaddr;
__u64 gpa;
__u64 len;
#define FLAG_MEASURE BIT(0)
#define FLAG_GPA BIT(1) // GPA is valid or calculated from uaddr
__u64 flags;
};
- Complete initialization. Make the guest ready to run vcpu
KVM_SEV_LAUNCH_FINISH: no argument
KVM_TDX_FINALIZE_VM: no argument
- KVM_SEV_LAUNCH_SECRET: no TDX correspondence
struct kvm_sev_launch_secret
For guest debug
- KVM_SEV_DBG_DECRYPT, KVM_SEV_DBG_ENCRYPT: struct kvm_sev_dbg
This is to read/write guest memory for debug. We can easily have a common
API.
- KVM_SEV_GUEST_STATUS
struct kvm_sev_guest_status
No TDX correspondence
Thanks,
--
Isaku Yamahata <isaku.yamahata@gmail.com>
next prev parent reply other threads:[~2023-07-27 0:38 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-20 23:32 [RFC PATCH v4 00/10] KVM: guest_memfd(), X86: Common base for SNP and TDX (was KVM: guest memory: Misc enhancement) isaku.yamahata
2023-07-20 23:32 ` [RFC PATCH v4 01/10] KVM: x86: Add is_vm_type_supported callback isaku.yamahata
2023-07-20 23:32 ` [RFC PATCH v4 02/10] KVM: x86/mmu: Guard against collision with KVM-defined PFERR_IMPLICIT_ACCESS isaku.yamahata
2023-07-20 23:32 ` [RFC PATCH v4 03/10] KVM: x86/mmu: Pass around full 64-bit error code for the KVM page fault isaku.yamahata
2023-07-20 23:32 ` [RFC PATCH v4 04/10] KVM: x86: Introduce PFERR_GUEST_ENC_MASK to indicate fault is private isaku.yamahata
2023-07-21 14:11 ` Sean Christopherson
2023-07-22 0:52 ` Isaku Yamahata
2024-02-22 2:05 ` Sean Christopherson
2023-07-20 23:32 ` [RFC PATCH v4 05/10] KVM: Add new members to struct kvm_gfn_range to operate on isaku.yamahata
2023-07-20 23:32 ` [RFC PATCH v4 06/10] KVM: x86: Export the kvm_zap_gfn_range() for the SNP use isaku.yamahata
2023-07-20 23:32 ` [RFC PATCH v4 07/10] KVM: x86: Add gmem hook for initializing private memory isaku.yamahata
2023-07-21 14:25 ` Sean Christopherson
2023-07-22 0:34 ` Michael Roth
2023-08-18 22:27 ` Sean Christopherson
2023-08-26 0:59 ` Michael Roth
2023-08-29 13:27 ` Michael Roth
2023-09-08 23:57 ` Sean Christopherson
2023-07-20 23:32 ` [RFC PATCH v4 08/10] KVM: x86: Add gmem hook for invalidating " isaku.yamahata
2023-07-20 23:32 ` [RFC PATCH v4 09/10] KVM: x86: Make struct sev_cmd common for KVM_MEM_ENC_OP isaku.yamahata
2023-07-21 14:51 ` Sean Christopherson
2023-07-21 18:43 ` Isaku Yamahata
2023-07-25 9:07 ` Xiaoyao Li
2023-07-25 15:36 ` Sean Christopherson
2023-07-27 0:37 ` Isaku Yamahata [this message]
2023-07-20 23:32 ` [RFC PATCH v4 10/10] KVM: X86: KVM_MEM_ENC_OP check if unused field (flags, error) is zero 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=20230727003759.GA2021422@ls.amr.corp.intel.com \
--to=isaku.yamahata@gmail.com \
--cc=ackerleytng@google.com \
--cc=chao.p.peng@linux.intel.com \
--cc=chen.bo@intel.com \
--cc=dmatlack@google.com \
--cc=erdemaktas@google.com \
--cc=isaku.yamahata@intel.com \
--cc=kai.huang@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=michael.roth@amd.com \
--cc=pbonzini@redhat.com \
--cc=sagis@google.com \
--cc=seanjc@google.com \
--cc=vannapurve@google.com \
--cc=xiaoyao.li@intel.com \
--cc=yuan.yao@linux.intel.com \
--cc=zhi.wang.linux@gmail.com \
/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 NNTP newsgroup(s).