From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 16265C6FA93 for ; Mon, 26 Sep 2022 15:19:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9394F8E0065; Mon, 26 Sep 2022 11:19:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8C36A6B00F6; Mon, 26 Sep 2022 11:19:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7152E8E0065; Mon, 26 Sep 2022 11:19:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 5BB726B00F5 for ; Mon, 26 Sep 2022 11:19:31 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 2F14AAAFE0 for ; Mon, 26 Sep 2022 15:19:31 +0000 (UTC) X-FDA: 79954595742.25.F1617EF Received: from mail-lj1-f174.google.com (mail-lj1-f174.google.com [209.85.208.174]) by imf15.hostedemail.com (Postfix) with ESMTP id D8349A000E for ; Mon, 26 Sep 2022 15:19:29 +0000 (UTC) Received: by mail-lj1-f174.google.com with SMTP id q17so7802155lji.11 for ; Mon, 26 Sep 2022 08:19:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=5+t0kf8gFUiFFnh/Q7NI1yZ0x97Y2cLvwCVBzXkQC40=; b=Y5alOsje+V17Mvu/UEe7LvpZijxhzaul2BWGDT5bztSMyS9dMJlmavzjn+2deaq5tw XvNm10CbZ4ZLITBQuIdLXCkRvPCo3ttVoMGyxzaON3gVItEvPjupvJPPUWnuX4u3O3tn co73Ml8h2TBj2tZ3Q+cEwbmMhaCQyZnb/jmJORcXwiChwx+Tu1zvYoIOQ+pro7NVexxo vWL6d8nrSBsIC6Mcn75pHch+puY3c+/i3igKsdBO2b6YYYhHXs9n3RQDStIPYiKWQRyr lGrB/fhsUdaZ7+jLdk8uF+Qc0ur1rup4v+UwMl6mX5qWWYXjsMM4rzFwlIKmKZM9pxrk cvoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=5+t0kf8gFUiFFnh/Q7NI1yZ0x97Y2cLvwCVBzXkQC40=; b=Wj2z0fD6/+sE7I2bnk22N9AN46/2jtq7Mkp/GTo2ui4PZUhBty5JvhZtClOam1z9jW kHMx/ZV9DOYpdK76haJgOO3ZOmzwawUY/h8U8Vw9ZltKvLODTIn0KADnTSRHimbiuSvO m5Tb3dBLKny6Oh9zdPIfVRyLYgoVpvZZ4kZlryCqCabEPHowy6OvkdXgCcj+HluyG4q5 tiUYY5TCDZxh+DgQe/NH7xvXyOqS2dY7A3zqjp3Wi0143rWtIgkCzsmrt6sZQzJcTBRb Pu4uIDj3h9OliTBzp+j//LhYDLe7ZoXeh56NnN8d1zftaf3RgAInqwlTl604l7a1bMbI 0/Yg== X-Gm-Message-State: ACrzQf1IGdJVt2D3W9YdpPyhkr1g/r8kHxZAmKyLdnwlWMNGYmKshdV2 enDIX2tneIPbisqBU4KQhR6HVsqoOEuGjANC3yYOTQ== X-Google-Smtp-Source: AMsMyM6+y6FU6UQ6FRNs597KPkJSz9uCZoF/CXoGwAr/mTWlHtClwgzOToRRC2voZPY2KOrU2AZgoD/DEnDfu92c2EM= X-Received: by 2002:a2e:983:0:b0:26c:5b0e:f5e4 with SMTP id 125-20020a2e0983000000b0026c5b0ef5e4mr7720323ljj.502.1664205567769; Mon, 26 Sep 2022 08:19:27 -0700 (PDT) MIME-Version: 1.0 References: <78e30b5a25c926fcfdcaafea3d484f1bb25f20b9.1655761627.git.ashish.kalra@amd.com> <0716365f-3572-638b-e841-fcce7d30571a@amd.com> <8113b5d4-31c6-012c-fc0c-78a9bdbb1e69@amd.com> <31c1b2bb-b43a-709a-2b7e-0e945b9e8bb7@amd.com> In-Reply-To: <31c1b2bb-b43a-709a-2b7e-0e945b9e8bb7@amd.com> From: Peter Gonda Date: Mon, 26 Sep 2022 09:19:15 -0600 Message-ID: Subject: Re: [PATCH Part2 v6 37/49] KVM: SVM: Add support to handle MSR based Page State Change VMGEXIT To: Ashish Kalra Cc: Tom Lendacky , Alper Gun , Ashish Kalra , "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 , "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 , "Dr. David Alan Gilbert" , jarkko@kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1664205569; a=rsa-sha256; cv=none; b=pnskD3SVEVS26qsThh5XK5fTMihr/iHaqYWrq0dLej2cZj1h2dx/CoRSh9dMsEox2ymnLE 4ECRHlKjqAs+5N++BBXBBFusRMMdbiBSXEEJkx5Kdxq05MAUT1Rk410mA74RO8KizgBjAK e3tL8/0HxyUkIvDh5RXyX2WPvoZLAw0= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Y5alOsje; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of pgonda@google.com designates 209.85.208.174 as permitted sender) smtp.mailfrom=pgonda@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1664205569; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5+t0kf8gFUiFFnh/Q7NI1yZ0x97Y2cLvwCVBzXkQC40=; b=YRmQJGA0Vq9/iIP6gypXCPXhh/ZSL+KgWIEFB4wZl9oSk9HQM+3akF6Iw8A8kNcC+sHugh 3Lr2+4unN8CFxh2pqYnzV2R0kARLZIwLl7b4CbtTqH/LkK3ErC4iBWSnRXx7De3f5Y7vli 8a521Qq3vIAFZA/ycN2ek0tQECzNbak= X-Rspamd-Server: rspam03 X-Rspam-User: Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Y5alOsje; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of pgonda@google.com designates 209.85.208.174 as permitted sender) smtp.mailfrom=pgonda@google.com X-Stat-Signature: hmrhf7xisix7sierg3nan4au8sgearit X-Rspamd-Queue-Id: D8349A000E X-HE-Tag: 1664205569-920837 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Sep 19, 2022 at 5:47 PM Ashish Kalra wrote: > > > On 9/19/22 22:18, Tom Lendacky wrote: > > On 9/19/22 17:02, Alper Gun wrote: > >> On Mon, Sep 19, 2022 at 2:38 PM Tom Lendacky > >> wrote: > >>> > >>> On 9/19/22 12:53, Alper Gun wrote: > >>>> On Fri, Aug 19, 2022 at 9:54 AM Peter Gonda wrote: > >>>>> > >>>>>> + > >>>>>> +static int __snp_handle_page_state_change(struct kvm_vcpu *vcpu, > >>>>>> enum psc_op op, gpa_t gpa, > >>>>>> + int level) > >>>>>> +{ > >>>>>> + struct kvm_sev_info *sev = &to_kvm_svm(vcpu->kvm)->sev_info; > >>>>>> + struct kvm *kvm = vcpu->kvm; > >>>>>> + int rc, npt_level; > >>>>>> + kvm_pfn_t pfn; > >>>>>> + gpa_t gpa_end; > >>>>>> + > >>>>>> + gpa_end = gpa + page_level_size(level); > >>>>>> + > >>>>>> + while (gpa < gpa_end) { > >>>>>> + /* > >>>>>> + * If the gpa is not present in the NPT then > >>>>>> build the NPT. > >>>>>> + */ > >>>>>> + rc = snp_check_and_build_npt(vcpu, gpa, level); > >>>>>> + if (rc) > >>>>>> + return -EINVAL; > >>>>>> + > >>>>>> + if (op == SNP_PAGE_STATE_PRIVATE) { > >>>>>> + hva_t hva; > >>>>>> + > >>>>>> + if (snp_gpa_to_hva(kvm, gpa, &hva)) > >>>>>> + return -EINVAL; > >>>>>> + > >>>>>> + /* > >>>>>> + * Verify that the hva range is > >>>>>> registered. This enforcement is > >>>>>> + * required to avoid the cases where a > >>>>>> page is marked private > >>>>>> + * in the RMP table but never gets > >>>>>> cleanup during the VM > >>>>>> + * termination path. > >>>>>> + */ > >>>>>> + mutex_lock(&kvm->lock); > >>>>>> + rc = is_hva_registered(kvm, hva, > >>>>>> page_level_size(level)); > >>>>>> + mutex_unlock(&kvm->lock); > >>>>>> + if (!rc) > >>>>>> + return -EINVAL; > >>>>>> + > >>>>>> + /* > >>>>>> + * Mark the userspace range unmerable > >>>>>> before adding the pages > >>>>>> + * in the RMP table. > >>>>>> + */ > >>>>>> + mmap_write_lock(kvm->mm); > >>>>>> + rc = snp_mark_unmergable(kvm, hva, > >>>>>> page_level_size(level)); > >>>>>> + mmap_write_unlock(kvm->mm); > >>>>>> + if (rc) > >>>>>> + return -EINVAL; > >>>>>> + } > >>>>>> + > >>>>>> + write_lock(&kvm->mmu_lock); > >>>>>> + > >>>>>> + rc = kvm_mmu_get_tdp_walk(vcpu, gpa, &pfn, > >>>>>> &npt_level); > >>>>>> + if (!rc) { > >>>>>> + /* > >>>>>> + * This may happen if another vCPU > >>>>>> unmapped the page > >>>>>> + * before we acquire the lock. Retry the > >>>>>> PSC. > >>>>>> + */ > >>>>>> + write_unlock(&kvm->mmu_lock); > >>>>>> + return 0; > >>>>>> + } > >>>>> > >>>>> I think we want to return -EAGAIN or similar if we want the caller to > >>>>> retry, right? I think returning 0 here hides the error. > >>>>> > >>>> > >>>> The problem here is that the caller(linux guest kernel) doesn't retry > >>>> if PSC fails. The current implementation in the guest kernel is that > >>>> if a page state change request fails, it terminates the VM with > >>>> GHCB_TERM_PSC reason. > >>>> Returning 0 here is not a good option because it will fail the PSC > >>>> silently and will probably cause a nested RMP fault later. Returning > >>> > >>> Returning 0 here is ok because the PSC current index into the PSC > >>> structure will not be updated and the guest will then retry (see the > >>> loop > >>> in vmgexit_psc() in arch/x86/kernel/sev.c). > >>> > >>> Thanks, > >>> Tom > >> > >> But the host code updates the index. It doesn't leave the loop because > >> rc is 0. The guest will think that it is successful. > >> rc = __snp_handle_page_state_change(vcpu, op, gpa, level); > >> if (rc) > >> goto out; > >> > >> Also the page state change request with MSR is not retried. It > >> terminates the VM if the MSR request fails. > > > > Ah, right. I see what you mean. It should probably return a -EAGAIN > > instead of 0 and then the if (rc) check should be modified to > > specifically look for -EAGAIN and goto out after setting rc to 0. > > > > But that does leave the MSR protocol open to the problem that you > > mention, so, yes, retry logic in snp_handle_page_state_change() for a > > -EAGAIN seems reasonable. > > > > Thanks, > > Tom > > I believe it makes more sense to add the retry logic within > __snp_handle_page_state_change() itself, as that will make it work for > both the GHCB MSR protocol and the GHCB VMGEXIT requests. You are suggesting we just retry 'kvm_mmu_get_tdp_walk' inside of __snp_handle_page_state_change()? That should work but how many times do we retry? If we return EAGAIN or error we can leave it up to the caller > > Thanks, Ashish > > > > >> > >>> > >>>> an error also terminates the guest immediately with current guest > >>>> implementation. I think the best approach here is adding a retry logic > >>>> to this function. Retrying without returning an error should help it > >>>> work because snp_check_and_build_npt will be called again and in the > >>>> second attempt this should work. > >>>> > >>>>>> + > >>>>>> + /* > >>>>>> + * Adjust the level so that we don't go higher > >>>>>> than the backing > >>>>>> + * page level. > >>>>>> + */ > >>>>>> + level = min_t(size_t, level, npt_level); > >>>>>> + > >>>>>> + trace_kvm_snp_psc(vcpu->vcpu_id, pfn, gpa, op, > >>>>>> level); > >>>>>> + > >>>>>> + switch (op) { > >>>>>> + case SNP_PAGE_STATE_SHARED: > >>>>>> + rc = snp_make_page_shared(kvm, gpa, pfn, > >>>>>> level); > >>>>>> + break; > >>>>>> + case SNP_PAGE_STATE_PRIVATE: > >>>>>> + rc = rmp_make_private(pfn, gpa, level, > >>>>>> sev->asid, false); > >>>>>> + break; > >>>>>> + default: > >>>>>> + rc = -EINVAL; > >>>>>> + break; > >>>>>> + } > >>>>>> + > >>>>>> + write_unlock(&kvm->mmu_lock); > >>>>>> + > >>>>>> + if (rc) { > >>>>>> + pr_err_ratelimited("Error op %d gpa %llx > >>>>>> pfn %llx level %d rc %d\n", > >>>>>> + op, gpa, pfn, level, rc); > >>>>>> + return rc; > >>>>>> + } > >>>>>> + > >>>>>> + gpa = gpa + page_level_size(level); > >>>>>> + } > >>>>>> + > >>>>>> + return 0; > >>>>>> +} > >>>>>> +