Re: [RFC PATCH v4 09/10] KVM: x86: Make struct sev_cmd common for KVM_MEM_ENC_OP

[Sean Christopherson <seanjc@google.com>]

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.