Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 26 Jul 2023 17:37:59 -0700
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
Message-ID: <20230727003759.GA2021422@ls.amr.corp.intel.com>
References: <cover.1689893403.git.isaku.yamahata@intel.com>
 <8c0b7babbdd777a33acd4f6b0f831ae838037806.1689893403.git.isaku.yamahata@intel.com>
 <ZLqbWFnm7jyB8JuY@google.com>
 <9e8a98ad-1f8d-a09c-3173-71c5c3ab5ed4@intel.com>
 <ZL/r6Vca8WkFVaic@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZL/r6Vca8WkFVaic@google.com>
Precedence: bulk

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>