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

[Michael Roth]

On Tue, Jul 18, 2023 at 01:29:30PM -0700, Isaku Yamahata wrote:
> 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;
> };

Ah, true, I didn't notice that. I guess I don't see issues with the
proposed format then. This flags field could allow for new fields to be
added over time without breaking old userspaces, so it seems like we
still have some flexibility in the future as well.

> 
> 
> 
> > 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?

Based on the above doesn't seem like we need to deprecate just yet. If
there's some major benefit we'd gain in doing so then maybe, but if
current approach still allows for backward-compatibility and future
extension then it doesn't seem like there's much to gain there.

-Mike

> -- 
> Isaku Yamahata <isaku.yamahata@xxxxxxxxx>