On Tue, Jul 25, 2023 at 08:36:09AM -0700,
Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxx 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@xxxxxxxxx>
