Re: [RFC PATCH] KVM: x86: Make struct sev_cmd common for KVM_MEM_ENC_OP

[Isaku Yamahata]

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

> On Mon, Jul 17, 2023 at 06:58:54PM -0700, isaku.yamahata@xxxxxxxxx wrote:
> > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > 
> > 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@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxx>