Re: [PATCH Part2 RFC v4 23/40] KVM: SVM: Add KVM_SEV_SNP_LAUNCH_START command
From: Sean Christopherson <seanjc@google.com>
Date: 2021-07-16 19:43:27
Also in:
kvm, linux-coco, linux-crypto, linux-efi, lkml, platform-driver-x86
On Wed, Jul 07, 2021, Brijesh Singh wrote:
quoted hunk ↗ jump to hunk
@@ -1527,6 +1530,100 @@ static int sev_receive_finish(struct kvm *kvm, struct kvm_sev_cmd *argp) return sev_issue_cmd(kvm, SEV_CMD_RECEIVE_FINISH, &data, &argp->error); } +static void *snp_context_create(struct kvm *kvm, struct kvm_sev_cmd *argp) +{ + struct sev_data_snp_gctx_create data = {}; + void *context; + int rc; + + /* Allocate memory for context page */
Eh, I'd drop this comment. It's quite obvious that a page is being allocated and that it's being assigned to the context.
+ context = snp_alloc_firmware_page(GFP_KERNEL_ACCOUNT);
+ if (!context)
+ return NULL;
+
+ data.gctx_paddr = __psp_pa(context);
+ rc = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_GCTX_CREATE, &data, &argp->error);
+ if (rc) {
+ snp_free_firmware_page(context);
+ return NULL;
+ }
+
+ return context;
+}
+
+static int snp_bind_asid(struct kvm *kvm, int *error)
+{
+ struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+ struct sev_data_snp_activate data = {};
+ int asid = sev_get_asid(kvm);
+ int ret, retry_count = 0;
+
+ /* Activate ASID on the given context */
+ data.gctx_paddr = __psp_pa(sev->snp_context);
+ data.asid = asid;
+again:
+ ret = sev_issue_cmd(kvm, SEV_CMD_SNP_ACTIVATE, &data, error);
+
+ /* Check if the DF_FLUSH is required, and try again */Please provide more info on why this may be necessary. I can see from the code that it does a flush and retries, but I have no idea why a flush would be required in the first place, e.g. why can't KVM guarantee that everything is in the proper state before attempting to bind an ASID?
+ if (ret && (*error == SEV_RET_DFFLUSH_REQUIRED) && (!retry_count)) {
+ /* Guard DEACTIVATE against WBINVD/DF_FLUSH used in ASID recycling */
+ down_read(&sev_deactivate_lock);
+ wbinvd_on_all_cpus();
+ ret = snp_guest_df_flush(error);
+ up_read(&sev_deactivate_lock);
+
+ if (ret)
+ return ret;
+
+ /* only one retry */Again, please explain why. Is this arbitrary? Is retrying more than once guaranteed to be useless?
+ retry_count = 1; + + goto again; + } + + return ret; +}
...
quoted hunk ↗ jump to hunk
void sev_vm_destroy(struct kvm *kvm) { struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;@@ -1847,7 +1969,15 @@ void sev_vm_destroy(struct kvm *kvm) mutex_unlock(&kvm->lock); - sev_unbind_asid(kvm, sev->handle); + if (sev_snp_guest(kvm)) { + if (snp_decommission_context(kvm)) { + pr_err("Failed to free SNP guest context, leaking asid!\n");
I agree with Peter that this likely warrants a WARN. If a WARN isn't justified, e.g. this can happen without a KVM/CPU bug, then there absolutely needs to be a massive comment explaining why we have code that result in memory leaks.
quoted hunk ↗ jump to hunk
+ return; + } + } else { + sev_unbind_asid(kvm, sev->handle); + } + sev_asid_free(sev); }diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index b9ea99f8579e..bc5582b44356 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h@@ -67,6 +67,7 @@ struct kvm_sev_info { u64 ap_jump_table; /* SEV-ES AP Jump Table address */ struct kvm *enc_context_owner; /* Owner of copied encryption context */ struct misc_cg *misc_cg; /* For misc cgroup accounting */ + void *snp_context; /* SNP guest context page */ }; struct kvm_svm {diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 989a64aa1ae5..dbd05179d8fa 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h@@ -1680,6 +1680,7 @@ enum sev_cmd_id { /* SNP specific commands */ KVM_SEV_SNP_INIT = 256, + KVM_SEV_SNP_LAUNCH_START, KVM_SEV_NR_MAX, };@@ -1781,6 +1782,14 @@ struct kvm_snp_init { __u64 flags; }; +struct kvm_sev_snp_launch_start { + __u64 policy; + __u64 ma_uaddr; + __u8 ma_en; + __u8 imi_en; + __u8 gosvw[16];
Hmm, I'd prefer to pad this out to be 8-byte sized.
+}; + #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0) #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1) #define KVM_DEV_ASSIGN_MASK_INTX (1 << 2) -- 2.17.1