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>
