From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f53.google.com (mail-lf1-f53.google.com [209.85.167.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2E9457A for ; Mon, 11 Jul 2022 14:05:22 +0000 (UTC) Received: by mail-lf1-f53.google.com with SMTP id a9so8851101lfk.11 for ; Mon, 11 Jul 2022 07:05:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=/q0quOUBTZl2gZ3/jAMDJM6IbzyCJBTglBZNVpNLFAg=; b=OrD3qzvjeeLGTBMraqT2BYvr0yduEBgTKuhSk/E0kxg3vkZv4jX3EyewouXwf6dCCq uAZcyX19ccZNhJbindes8nJi5AREvwTXp/PkgH0V5RLCa4wS04W6iJBQbwc1qikmv6Ta Oz1p8LhqQQO+Py50NrAt8cB689pHX13B3jg5rFLqNz8UpUvIApkpkukZUoeooUYiH5KF hK9vTeGXNomKC2l94M8dS1+yOkcyMfFW19eS4TJwdJ0hHjY5FpX0PrTLl2RXLSq61YEl hPeThgGZKETyFHA8MHZH6KiEG6SLlx/sbWBFsthhH1WHZBM0ngdsToPFTuVlYj2mfk+g CepQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=/q0quOUBTZl2gZ3/jAMDJM6IbzyCJBTglBZNVpNLFAg=; b=mCR5fsu1VPLV5BAK3ZvDV8cQcl1mcQ61xrhj0/s0nh1xloNKwLaiQq2Lj0hsSkHkoY gQhEdqVvfapdAqCBXZlDmLvKTU3Mcal6HvA8B8YUbS39NmuZyPUsrfmkvvL03SH7Hm55 Ifx2bNVVpTDECbpzCMpuScRKyA+kOVKv5FejMDq+w0vvnt6Cbx5tbi5HsGxItOYAvmad hP15HFOA2bNw8AUM+ljbmcbpxwVVynwXD9p0Bt39UCJV902KNuZDs1N+qLZFXjWp5CDQ 5NVrls6OpB+9GpSf82Nv6Onig8Es/KMxNH1yGZ2ufOrL29MTYceeTDE73WRKQ4b/3zGE mZfg== X-Gm-Message-State: AJIora+9Uq0DUkpjDJlF/5N1jhVbjHoRGmtGGOyq6hfUbH3roXG2N3Jb kngz2RpHSP06OPEqCEmNkDJx/Lr1PiGZbgfln/VXxw== X-Google-Smtp-Source: AGRyM1vr9Y62NuaJJYfkorIddGjcA9PSwfY28vd8Oaa2IQe6aGGNvRjDvJ+WHbbexoWMRT+ZRh4z37GhWYBnUZQ0IZo= X-Received: by 2002:a05:6512:32c5:b0:481:1822:c41f with SMTP id f5-20020a05651232c500b004811822c41fmr12289097lfg.373.1657548319052; Mon, 11 Jul 2022 07:05:19 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <6a513cf79bf71c479dbd72165faf1d804d77b3af.1655761627.git.ashish.kalra@amd.com> In-Reply-To: <6a513cf79bf71c479dbd72165faf1d804d77b3af.1655761627.git.ashish.kalra@amd.com> From: Peter Gonda Date: Mon, 11 Jul 2022 08:05:07 -0600 Message-ID: Subject: Re: [PATCH Part2 v6 28/49] KVM: SVM: Add KVM_SEV_SNP_LAUNCH_FINISH command To: Ashish Kalra Cc: "the arch/x86 maintainers" , LKML , kvm list , linux-coco@lists.linux.dev, Linux Memory Management List , Linux Crypto Mailing List , Thomas Gleixner , Ingo Molnar , Joerg Roedel , "Lendacky, Thomas" , "H. Peter Anvin" , Ard Biesheuvel , Paolo Bonzini , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Andy Lutomirski , Dave Hansen , Sergio Lopez , Peter Zijlstra , Srinivas Pandruvada , David Rientjes , Dov Murik , Tobin Feldman-Fitzthum , Borislav Petkov , Michael Roth , Vlastimil Babka , "Kirill A . Shutemov" , Andi Kleen , Tony Luck , Marc Orr , Sathyanarayanan Kuppuswamy , Alper Gun , "Dr. David Alan Gilbert" , jarkko@kernel.org Content-Type: text/plain; charset="UTF-8" On Mon, Jun 20, 2022 at 5:08 PM Ashish Kalra wrote: > > From: Brijesh Singh > > The KVM_SEV_SNP_LAUNCH_FINISH finalize the cryptographic digest and stores > it as the measurement of the guest at launch. > > While finalizing the launch flow, it also issues the LAUNCH_UPDATE command > to encrypt the VMSA pages. > > If its an SNP guest, then VMSA was added in the RMP entry as > a guest owned page and also removed from the kernel direct map > so flush it later after it is transitioned back to hypervisor > state and restored in the direct map. Given the guest uses the SNP NAE AP boot protocol we were expecting that there would be some option to add vCPUs to the VM but mark them as "pending AP boot creation protocol" state. This would allow the LaunchDigest of a VM doesn't change just because its vCPU count changes. Would it be possible to add a new add an argument to KVM_SNP_LAUNCH_FINISH to tell it which vCPUs to LAUNCH_UPDATE VMSA pages for or similarly a new argument for KVM_CREATE_VCPU? > > Signed-off-by: Brijesh Singh > Signed-off-by: Ashish Kalra > --- > .../virt/kvm/x86/amd-memory-encryption.rst | 22 ++++ > arch/x86/kvm/svm/sev.c | 119 ++++++++++++++++++ > include/uapi/linux/kvm.h | 14 +++ > 3 files changed, 155 insertions(+) > > diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst > index 62abd5c1f72b..750162cff87b 100644 > --- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst > +++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst > @@ -514,6 +514,28 @@ Returns: 0 on success, -negative on error > See the SEV-SNP spec for further details on how to build the VMPL permission > mask and page type. > > +21. KVM_SNP_LAUNCH_FINISH > +------------------------- > + > +After completion of the SNP guest launch flow, the KVM_SNP_LAUNCH_FINISH command can be > +issued to make the guest ready for the execution. > + > +Parameters (in): struct kvm_sev_snp_launch_finish > + > +Returns: 0 on success, -negative on error > + > +:: > + > + struct kvm_sev_snp_launch_finish { > + __u64 id_block_uaddr; > + __u64 id_auth_uaddr; > + __u8 id_block_en; > + __u8 auth_key_en; > + __u8 host_data[32]; > + }; > + > + > +See SEV-SNP specification for further details on launch finish input parameters. > > References > ========== > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index a9461d352eda..a5b90469683f 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2095,6 +2095,106 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp) > return ret; > } > > +static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > + struct sev_data_snp_launch_update data = {}; > + int i, ret; > + > + data.gctx_paddr = __psp_pa(sev->snp_context); > + data.page_type = SNP_PAGE_TYPE_VMSA; > + > + for (i = 0; i < kvm->created_vcpus; i++) { > + struct vcpu_svm *svm = to_svm(xa_load(&kvm->vcpu_array, i)); Why are we iterating over |created_vcpus| rather than using kvm_for_each_vcpu? > + u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT; > + > + /* Perform some pre-encryption checks against the VMSA */ > + ret = sev_es_sync_vmsa(svm); > + if (ret) > + return ret; Do we need to take the 'vcpu->mutex' lock before modifying the vcpu,like we do for SEV-ES in sev_launch_update_vmsa()? > + > + /* Transition the VMSA page to a firmware state. */ > + ret = rmp_make_private(pfn, -1, PG_LEVEL_4K, sev->asid, true); > + if (ret) > + return ret; > + > + /* Issue the SNP command to encrypt the VMSA */ > + data.address = __sme_pa(svm->sev_es.vmsa); > + ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_SNP_LAUNCH_UPDATE, > + &data, &argp->error); > + if (ret) { > + snp_page_reclaim(pfn); > + return ret; > + } > + > + svm->vcpu.arch.guest_state_protected = true; > + } > + > + return 0; > +} > + > +static int snp_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp) > +{ > + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > + struct sev_data_snp_launch_finish *data; > + void *id_block = NULL, *id_auth = NULL; > + struct kvm_sev_snp_launch_finish params; > + int ret; > + > + if (!sev_snp_guest(kvm)) > + return -ENOTTY; > + > + if (!sev->snp_context) > + return -EINVAL; > + > + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, sizeof(params))) > + return -EFAULT; > + > + /* Measure all vCPUs using LAUNCH_UPDATE before we finalize the launch flow. */ > + ret = snp_launch_update_vmsa(kvm, argp); > + if (ret) > + return ret; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL_ACCOUNT); > + if (!data) > + return -ENOMEM; > + > + if (params.id_block_en) { > + id_block = psp_copy_user_blob(params.id_block_uaddr, KVM_SEV_SNP_ID_BLOCK_SIZE); > + if (IS_ERR(id_block)) { > + ret = PTR_ERR(id_block); > + goto e_free; > + } > + > + data->id_block_en = 1; > + data->id_block_paddr = __sme_pa(id_block); > + } > + > + if (params.auth_key_en) { > + id_auth = psp_copy_user_blob(params.id_auth_uaddr, KVM_SEV_SNP_ID_AUTH_SIZE); > + if (IS_ERR(id_auth)) { > + ret = PTR_ERR(id_auth); > + goto e_free_id_block; > + } > + > + data->auth_key_en = 1; > + data->id_auth_paddr = __sme_pa(id_auth); > + } > + > + data->gctx_paddr = __psp_pa(sev->snp_context); > + ret = sev_issue_cmd(kvm, SEV_CMD_SNP_LAUNCH_FINISH, data, &argp->error); > + > + kfree(id_auth); > + > +e_free_id_block: > + kfree(id_block); > + > +e_free: > + kfree(data); > + > + return ret; > +} > + > int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp) > { > struct kvm_sev_cmd sev_cmd; > @@ -2191,6 +2291,9 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp) > case KVM_SEV_SNP_LAUNCH_UPDATE: > r = snp_launch_update(kvm, &sev_cmd); > break; > + case KVM_SEV_SNP_LAUNCH_FINISH: > + r = snp_launch_finish(kvm, &sev_cmd); > + break; > default: > r = -EINVAL; > goto out; > @@ -2696,11 +2799,27 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu) > > svm = to_svm(vcpu); > > + /* > + * If its an SNP guest, then VMSA was added in the RMP entry as > + * a guest owned page. Transition the page to hypervisor state > + * before releasing it back to the system. > + * Also the page is removed from the kernel direct map, so flush it > + * later after it is transitioned back to hypervisor state and > + * restored in the direct map. > + */ > + if (sev_snp_guest(vcpu->kvm)) { > + u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT; > + > + if (host_rmp_make_shared(pfn, PG_LEVEL_4K, false)) > + goto skip_vmsa_free; Why not call host_rmp_make_shared with leak==true? This old VMSA page is now unusable IIUC. > + } > + > if (vcpu->arch.guest_state_protected) > sev_flush_encrypted_page(vcpu, svm->sev_es.vmsa); > > __free_page(virt_to_page(svm->sev_es.vmsa)); > > +skip_vmsa_free: > if (svm->sev_es.ghcb_sa_free) > kvfree(svm->sev_es.ghcb_sa); > } > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index 9b36b07414ea..5a4662716b6a 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -1814,6 +1814,7 @@ enum sev_cmd_id { > KVM_SEV_SNP_INIT, > KVM_SEV_SNP_LAUNCH_START, > KVM_SEV_SNP_LAUNCH_UPDATE, > + KVM_SEV_SNP_LAUNCH_FINISH, > > KVM_SEV_NR_MAX, > }; > @@ -1948,6 +1949,19 @@ struct kvm_sev_snp_launch_update { > __u8 vmpl1_perms; > }; > > +#define KVM_SEV_SNP_ID_BLOCK_SIZE 96 > +#define KVM_SEV_SNP_ID_AUTH_SIZE 4096 > +#define KVM_SEV_SNP_FINISH_DATA_SIZE 32 > + > +struct kvm_sev_snp_launch_finish { > + __u64 id_block_uaddr; > + __u64 id_auth_uaddr; > + __u8 id_block_en; > + __u8 auth_key_en; > + __u8 host_data[KVM_SEV_SNP_FINISH_DATA_SIZE]; > + __u8 pad[6]; > +}; > + > #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.25.1 >