From: Isaku Yamahata <[email protected]>
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 <[email protected]>
---
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;
+ /* Data for sub-command. Typically __user pointer to actual parameter. */
+ __u64 data;
+ /* Supplemental error code in error case. */
+ 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;
+ };
+};
+
#endif /* _ASM_X86_KVM_H */
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 07756b7348ae..94e13bb49c86 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1835,30 +1835,39 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
return ret;
}
-int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
+int sev_mem_enc_ioctl(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd)
{
- struct kvm_sev_cmd sev_cmd;
+ struct kvm_sev_cmd *sev_cmd = (struct kvm_sev_cmd *)cmd;
int r;
+ /* TODO: replace struct kvm_sev_cmd with kvm_mem_enc_cmd. */
+ BUILD_BUG_ON(sizeof(*sev_cmd) != sizeof(*cmd));
+ BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, id) !=
+ offsetof(struct kvm_mem_enc_cmd, id));
+ BUILD_BUG_ON(sizeof(sev_cmd->id) != sizeof(cmd->id));
+ BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, data) !=
+ offsetof(struct kvm_mem_enc_cmd, data));
+ BUILD_BUG_ON(sizeof(sev_cmd->data) != sizeof(cmd->data));
+ BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, error) !=
+ offsetof(struct kvm_mem_enc_cmd, error));
+ BUILD_BUG_ON(sizeof(sev_cmd->error) != sizeof(cmd->error));
+ BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, sev_fd) !=
+ offsetof(struct kvm_mem_enc_cmd, sev_fd));
+ BUILD_BUG_ON(sizeof(sev_cmd->sev_fd) != sizeof(cmd->sev_fd));
+
if (!sev_enabled)
return -ENOTTY;
- if (!argp)
- return 0;
-
- if (copy_from_user(&sev_cmd, argp, sizeof(struct kvm_sev_cmd)))
- return -EFAULT;
-
mutex_lock(&kvm->lock);
/* Only the enc_context_owner handles some memory enc operations. */
if (is_mirroring_enc_context(kvm) &&
- !is_cmd_allowed_from_mirror(sev_cmd.id)) {
+ !is_cmd_allowed_from_mirror(sev_cmd->id)) {
r = -EINVAL;
goto out;
}
- switch (sev_cmd.id) {
+ switch (sev_cmd->id) {
case KVM_SEV_ES_INIT:
if (!sev_es_enabled) {
r = -ENOTTY;
@@ -1866,67 +1875,64 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
}
fallthrough;
case KVM_SEV_INIT:
- r = sev_guest_init(kvm, &sev_cmd);
+ r = sev_guest_init(kvm, sev_cmd);
break;
case KVM_SEV_LAUNCH_START:
- r = sev_launch_start(kvm, &sev_cmd);
+ r = sev_launch_start(kvm, sev_cmd);
break;
case KVM_SEV_LAUNCH_UPDATE_DATA:
- r = sev_launch_update_data(kvm, &sev_cmd);
+ r = sev_launch_update_data(kvm, sev_cmd);
break;
case KVM_SEV_LAUNCH_UPDATE_VMSA:
- r = sev_launch_update_vmsa(kvm, &sev_cmd);
+ r = sev_launch_update_vmsa(kvm, sev_cmd);
break;
case KVM_SEV_LAUNCH_MEASURE:
- r = sev_launch_measure(kvm, &sev_cmd);
+ r = sev_launch_measure(kvm, sev_cmd);
break;
case KVM_SEV_LAUNCH_FINISH:
- r = sev_launch_finish(kvm, &sev_cmd);
+ r = sev_launch_finish(kvm, sev_cmd);
break;
case KVM_SEV_GUEST_STATUS:
- r = sev_guest_status(kvm, &sev_cmd);
+ r = sev_guest_status(kvm, sev_cmd);
break;
case KVM_SEV_DBG_DECRYPT:
- r = sev_dbg_crypt(kvm, &sev_cmd, true);
+ r = sev_dbg_crypt(kvm, sev_cmd, true);
break;
case KVM_SEV_DBG_ENCRYPT:
- r = sev_dbg_crypt(kvm, &sev_cmd, false);
+ r = sev_dbg_crypt(kvm, sev_cmd, false);
break;
case KVM_SEV_LAUNCH_SECRET:
- r = sev_launch_secret(kvm, &sev_cmd);
+ r = sev_launch_secret(kvm, sev_cmd);
break;
case KVM_SEV_GET_ATTESTATION_REPORT:
- r = sev_get_attestation_report(kvm, &sev_cmd);
+ r = sev_get_attestation_report(kvm, sev_cmd);
break;
case KVM_SEV_SEND_START:
- r = sev_send_start(kvm, &sev_cmd);
+ r = sev_send_start(kvm, sev_cmd);
break;
case KVM_SEV_SEND_UPDATE_DATA:
- r = sev_send_update_data(kvm, &sev_cmd);
+ r = sev_send_update_data(kvm, sev_cmd);
break;
case KVM_SEV_SEND_FINISH:
- r = sev_send_finish(kvm, &sev_cmd);
+ r = sev_send_finish(kvm, sev_cmd);
break;
case KVM_SEV_SEND_CANCEL:
- r = sev_send_cancel(kvm, &sev_cmd);
+ r = sev_send_cancel(kvm, sev_cmd);
break;
case KVM_SEV_RECEIVE_START:
- r = sev_receive_start(kvm, &sev_cmd);
+ r = sev_receive_start(kvm, sev_cmd);
break;
case KVM_SEV_RECEIVE_UPDATE_DATA:
- r = sev_receive_update_data(kvm, &sev_cmd);
+ r = sev_receive_update_data(kvm, sev_cmd);
break;
case KVM_SEV_RECEIVE_FINISH:
- r = sev_receive_finish(kvm, &sev_cmd);
+ r = sev_receive_finish(kvm, sev_cmd);
break;
default:
r = -EINVAL;
goto out;
}
- if (copy_to_user(argp, &sev_cmd, sizeof(struct kvm_sev_cmd)))
- r = -EFAULT;
-
out:
mutex_unlock(&kvm->lock);
return r;
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 18af7e712a5a..74ecab20c24b 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -716,7 +716,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
extern unsigned int max_sev_asid;
void sev_vm_destroy(struct kvm *kvm);
-int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp);
+int sev_mem_enc_ioctl(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd);
int sev_mem_enc_register_region(struct kvm *kvm,
struct kvm_enc_region *range);
int sev_mem_enc_unregister_region(struct kvm *kvm,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a6b9bea62fb8..14cfbc3266dd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7018,11 +7018,25 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
goto out;
}
case KVM_MEMORY_ENCRYPT_OP: {
+ struct kvm_mem_enc_cmd cmd;
+
r = -ENOTTY;
if (!kvm_x86_ops.mem_enc_ioctl)
goto out;
- r = static_call(kvm_x86_mem_enc_ioctl)(kvm, argp);
+ if (!argp) {
+ r = 0;
+ goto out;
+ }
+
+ if (copy_from_user(&cmd, argp, sizeof(cmd))) {
+ r = -EFAULT;
+ goto out;
+ }
+ r = static_call(kvm_x86_mem_enc_ioctl)(kvm, &cmd);
+ if (copy_to_user(argp, &cmd, sizeof(cmd)))
+ r = -EFAULT;
+
break;
}
case KVM_MEMORY_ENCRYPT_REG_REGION: {
base-commit: 831fe284d8275987596b7d640518dddba5735f61
--
2.25.1
On Mon, Jul 17, 2023 at 06:58:54PM -0700, [email protected] wrote:
> From: Isaku Yamahata <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
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.
-Mike
> + /* Data for sub-command. Typically __user pointer to actual parameter. */
> + __u64 data;
> + /* Supplemental error code in error case. */
> + 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;
> + };
> +};
> +
> #endif /* _ASM_X86_KVM_H */
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 07756b7348ae..94e13bb49c86 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1835,30 +1835,39 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
> return ret;
> }
>
> -int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> +int sev_mem_enc_ioctl(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd)
> {
> - struct kvm_sev_cmd sev_cmd;
> + struct kvm_sev_cmd *sev_cmd = (struct kvm_sev_cmd *)cmd;
> int r;
>
> + /* TODO: replace struct kvm_sev_cmd with kvm_mem_enc_cmd. */
> + BUILD_BUG_ON(sizeof(*sev_cmd) != sizeof(*cmd));
> + BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, id) !=
> + offsetof(struct kvm_mem_enc_cmd, id));
> + BUILD_BUG_ON(sizeof(sev_cmd->id) != sizeof(cmd->id));
> + BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, data) !=
> + offsetof(struct kvm_mem_enc_cmd, data));
> + BUILD_BUG_ON(sizeof(sev_cmd->data) != sizeof(cmd->data));
> + BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, error) !=
> + offsetof(struct kvm_mem_enc_cmd, error));
> + BUILD_BUG_ON(sizeof(sev_cmd->error) != sizeof(cmd->error));
> + BUILD_BUG_ON(offsetof(struct kvm_sev_cmd, sev_fd) !=
> + offsetof(struct kvm_mem_enc_cmd, sev_fd));
> + BUILD_BUG_ON(sizeof(sev_cmd->sev_fd) != sizeof(cmd->sev_fd));
> +
> if (!sev_enabled)
> return -ENOTTY;
>
> - if (!argp)
> - return 0;
> -
> - if (copy_from_user(&sev_cmd, argp, sizeof(struct kvm_sev_cmd)))
> - return -EFAULT;
> -
> mutex_lock(&kvm->lock);
>
> /* Only the enc_context_owner handles some memory enc operations. */
> if (is_mirroring_enc_context(kvm) &&
> - !is_cmd_allowed_from_mirror(sev_cmd.id)) {
> + !is_cmd_allowed_from_mirror(sev_cmd->id)) {
> r = -EINVAL;
> goto out;
> }
>
> - switch (sev_cmd.id) {
> + switch (sev_cmd->id) {
> case KVM_SEV_ES_INIT:
> if (!sev_es_enabled) {
> r = -ENOTTY;
> @@ -1866,67 +1875,64 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> }
> fallthrough;
> case KVM_SEV_INIT:
> - r = sev_guest_init(kvm, &sev_cmd);
> + r = sev_guest_init(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_START:
> - r = sev_launch_start(kvm, &sev_cmd);
> + r = sev_launch_start(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_UPDATE_DATA:
> - r = sev_launch_update_data(kvm, &sev_cmd);
> + r = sev_launch_update_data(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_UPDATE_VMSA:
> - r = sev_launch_update_vmsa(kvm, &sev_cmd);
> + r = sev_launch_update_vmsa(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_MEASURE:
> - r = sev_launch_measure(kvm, &sev_cmd);
> + r = sev_launch_measure(kvm, sev_cmd);
> break;
> case KVM_SEV_LAUNCH_FINISH:
> - r = sev_launch_finish(kvm, &sev_cmd);
> + r = sev_launch_finish(kvm, sev_cmd);
> break;
> case KVM_SEV_GUEST_STATUS:
> - r = sev_guest_status(kvm, &sev_cmd);
> + r = sev_guest_status(kvm, sev_cmd);
> break;
> case KVM_SEV_DBG_DECRYPT:
> - r = sev_dbg_crypt(kvm, &sev_cmd, true);
> + r = sev_dbg_crypt(kvm, sev_cmd, true);
> break;
> case KVM_SEV_DBG_ENCRYPT:
> - r = sev_dbg_crypt(kvm, &sev_cmd, false);
> + r = sev_dbg_crypt(kvm, sev_cmd, false);
> break;
> case KVM_SEV_LAUNCH_SECRET:
> - r = sev_launch_secret(kvm, &sev_cmd);
> + r = sev_launch_secret(kvm, sev_cmd);
> break;
> case KVM_SEV_GET_ATTESTATION_REPORT:
> - r = sev_get_attestation_report(kvm, &sev_cmd);
> + r = sev_get_attestation_report(kvm, sev_cmd);
> break;
> case KVM_SEV_SEND_START:
> - r = sev_send_start(kvm, &sev_cmd);
> + r = sev_send_start(kvm, sev_cmd);
> break;
> case KVM_SEV_SEND_UPDATE_DATA:
> - r = sev_send_update_data(kvm, &sev_cmd);
> + r = sev_send_update_data(kvm, sev_cmd);
> break;
> case KVM_SEV_SEND_FINISH:
> - r = sev_send_finish(kvm, &sev_cmd);
> + r = sev_send_finish(kvm, sev_cmd);
> break;
> case KVM_SEV_SEND_CANCEL:
> - r = sev_send_cancel(kvm, &sev_cmd);
> + r = sev_send_cancel(kvm, sev_cmd);
> break;
> case KVM_SEV_RECEIVE_START:
> - r = sev_receive_start(kvm, &sev_cmd);
> + r = sev_receive_start(kvm, sev_cmd);
> break;
> case KVM_SEV_RECEIVE_UPDATE_DATA:
> - r = sev_receive_update_data(kvm, &sev_cmd);
> + r = sev_receive_update_data(kvm, sev_cmd);
> break;
> case KVM_SEV_RECEIVE_FINISH:
> - r = sev_receive_finish(kvm, &sev_cmd);
> + r = sev_receive_finish(kvm, sev_cmd);
> break;
> default:
> r = -EINVAL;
> goto out;
> }
>
> - if (copy_to_user(argp, &sev_cmd, sizeof(struct kvm_sev_cmd)))
> - r = -EFAULT;
> -
> out:
> mutex_unlock(&kvm->lock);
> return r;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 18af7e712a5a..74ecab20c24b 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -716,7 +716,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
> extern unsigned int max_sev_asid;
>
> void sev_vm_destroy(struct kvm *kvm);
> -int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp);
> +int sev_mem_enc_ioctl(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd);
> int sev_mem_enc_register_region(struct kvm *kvm,
> struct kvm_enc_region *range);
> int sev_mem_enc_unregister_region(struct kvm *kvm,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a6b9bea62fb8..14cfbc3266dd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7018,11 +7018,25 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> goto out;
> }
> case KVM_MEMORY_ENCRYPT_OP: {
> + struct kvm_mem_enc_cmd cmd;
> +
> r = -ENOTTY;
> if (!kvm_x86_ops.mem_enc_ioctl)
> goto out;
>
> - r = static_call(kvm_x86_mem_enc_ioctl)(kvm, argp);
> + if (!argp) {
> + r = 0;
> + goto out;
> + }
> +
> + if (copy_from_user(&cmd, argp, sizeof(cmd))) {
> + r = -EFAULT;
> + goto out;
> + }
> + r = static_call(kvm_x86_mem_enc_ioctl)(kvm, &cmd);
> + if (copy_to_user(argp, &cmd, sizeof(cmd)))
> + r = -EFAULT;
> +
> break;
> }
> case KVM_MEMORY_ENCRYPT_REG_REGION: {
>
> base-commit: 831fe284d8275987596b7d640518dddba5735f61
> --
> 2.25.1
>
On Tue, Jul 18, 2023 at 02:39:18PM -0500,
Michael Roth <[email protected]> wrote:
> On Mon, Jul 17, 2023 at 06:58:54PM -0700, [email protected] wrote:
> > From: Isaku Yamahata <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]> wrote:
>
> > On Mon, Jul 17, 2023 at 06:58:54PM -0700, [email protected] wrote:
> > > From: Isaku Yamahata <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>