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:   Tue, 18 Jul 2023 13:29:30 -0700
From:   Isaku Yamahata <isaku.yamahata@gmail.com>
To:     Michael Roth <michael.roth@amd.com>
Cc:     isaku.yamahata@intel.com, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        isaku.yamahata@gmail.com, Kai Huang <kai.huang@intel.com>,
        Zhi Wang <zhi.wang.linux@gmail.com>,
        Xiaoyao Li <xiaoyao.li@intel.com>, chen.bo@intel.com,
        Sagi Shahar <sagis@google.com>,
        Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [RFC PATCH] KVM: x86: Make struct sev_cmd common for
 KVM_MEM_ENC_OP
Message-ID: <20230718202930.GB25699@ls.amr.corp.intel.com>
References: <2f296c5a255936b92cffde5e7d6414edfb93f660.1689645293.git.isaku.yamahata@intel.com>
 <20230718193918.rgluetgaiuib662g@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230718193918.rgluetgaiuib662g@amd.com>
Precedence: bulk

On Tue, Jul 18, 2023 at 02:39:18PM -0500,
Michael Roth <michael.roth@amd.com> wrote:

> On Mon, Jul 17, 2023 at 06:58:54PM -0700, isaku.yamahata@intel.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > TDX KVM will use KVM_MEM_ENC_OP.  Make struct sev_cmd common both for
> > vendor backend, SEV and TDX, with rename.  Make the struct common uABI for
> > KVM_MEM_ENC_OP.  TDX backend wants to return 64 bit error code instead of
> > 32 bit. To keep ABI for SEV backend, use union to accommodate 64 bit
> > member.
> > 
> > Some data structures for sub-commands could be common.  The current
> > candidate would be KVM_SEV{,_ES}_INIT, KVM_SEV_LAUNCH_FINISH,
> > KVM_SEV_LAUNCH_UPDATE_VMSA, KVM_SEV_DBG_DECRYPT, and KVM_SEV_DBG_ENCRYPT.
> > 
> > Only compile tested for SEV code.
> > 
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  2 +-
> >  arch/x86/include/uapi/asm/kvm.h | 22 +++++++++++
> >  arch/x86/kvm/svm/sev.c          | 68 ++++++++++++++++++---------------
> >  arch/x86/kvm/svm/svm.h          |  2 +-
> >  arch/x86/kvm/x86.c              | 16 +++++++-
> >  5 files changed, 76 insertions(+), 34 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 28bd38303d70..f14c8df707ac 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1706,7 +1706,7 @@ struct kvm_x86_ops {
> >  	void (*enable_smi_window)(struct kvm_vcpu *vcpu);
> >  #endif
> >  
> > -	int (*mem_enc_ioctl)(struct kvm *kvm, void __user *argp);
> > +	int (*mem_enc_ioctl)(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd);
> >  	int (*mem_enc_register_region)(struct kvm *kvm, struct kvm_enc_region *argp);
> >  	int (*mem_enc_unregister_region)(struct kvm *kvm, struct kvm_enc_region *argp);
> >  	int (*vm_copy_enc_context_from)(struct kvm *kvm, unsigned int source_fd);
> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index 1a6a1f987949..c458c38bb0cb 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -562,4 +562,26 @@ 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. */
> > +	__u32 flags;
> 
> struct kvm_sev_cmd doesn't have this flags field, so this would break for
> older userspaces that try to pass it in instead of the struct kvm_mem_enc_cmd
> proposed by this patch. Maybe move it to the end of the struct? Or
> make it part of a TDX-specific union field.

Please notice the padding. We don't have __packed attribute.

struct kvm_sev_cmd {
        __u32 id;
        <<<<< 32bit padding here
        __u64 data;
        __u32 error;
        __u32 sev_fd;
};


> But then you might also run into issues if you copy_to_user() with
> sizeof(struct kvm_mem_enc_cmd) instead of sizeof(struct kvm_sev_cmd),
> since the former might copy an additional 4 bytes more than what userspace
> allocated.
> 
> So maybe only common bits should be copy_to_user()'d by common KVM code,
> and the platform-specific fields in the union should be separately copied
> by platform code?
> 
> E.g.
> 
>   struct kvm_mem_enc_sev_cmd {
>       __u32 error;
>       __u32 sev_fd;
>   }
> 
>   struct kvm_mem_enc_tdx_cmd {
>       __u64 error;
>       __u32 flags;
>   }
> 
>   struct kvm_mem_enc_cmd {
>       __u32 id;
>       __u64 data;
>       union {
>         struct kvm_mem_enc_sev_cmd sev_cmd;
>         struct kvm_mem_enc_tdx_cmd tdx_cmd;
>       }
>   };
> 
> But then we'd need to copy_from_user() for common header, then for
> platform-specific sub-command metadata like sev_fd, then for the
> sub-command-specific parameters themselves.
> 
> Make me wonder if this warrants a KVM_MEM_ENC_OP2 (or whatever) that
> uses the new structure from the start so that legacy constaints aren't
> an issue.

I'm fine with a new ioctl and deprecating the existing one.  I'm looking
for the least painful way to avoid unnecessary divergence.
Not only for creation/attestation, but also for debug, migration, etc in near
future.  Thoughts?
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>