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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6E420C433FE for ; Wed, 13 Oct 2021 20:10:24 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D6D006101D for ; Wed, 13 Oct 2021 20:10:23 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D6D006101D Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 317A16B006C; Wed, 13 Oct 2021 16:10:23 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2C7376B0071; Wed, 13 Oct 2021 16:10:23 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 169A0900002; Wed, 13 Oct 2021 16:10:23 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0051.hostedemail.com [216.40.44.51]) by kanga.kvack.org (Postfix) with ESMTP id 01C6B6B006C for ; Wed, 13 Oct 2021 16:10:22 -0400 (EDT) Received: from smtpin18.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id B612B39B81 for ; Wed, 13 Oct 2021 20:10:22 +0000 (UTC) X-FDA: 78692506284.18.0E3E527 Received: from mail-pj1-f46.google.com (mail-pj1-f46.google.com [209.85.216.46]) by imf15.hostedemail.com (Postfix) with ESMTP id F36AFD000097 for ; Wed, 13 Oct 2021 20:10:21 +0000 (UTC) Received: by mail-pj1-f46.google.com with SMTP id k23-20020a17090a591700b001976d2db364so3142872pji.2 for ; Wed, 13 Oct 2021 13:10:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=x1LbBin+3giaISRnECjj9ym7k3KwVa5JtFi86EOVr/U=; b=QDCwnlgrMzfzTJO2pUJLtu7nG00gr1hza14ldZPiMdaKHgFwF4SXNeVU3mY98HXsC9 OU26/0D3oks/kHUGsc3RYZWTSkdbVJMH3cqGsg6S/kOLm5uGSlbA7uGRO9InDb8rJOp/ zZfaVSpfjeMg6nZbHv1OH1ZzJ8Kk0q0yEs616ejBQv6HQdTG8KQolJJMZVNxGy8nFDAm INdYjQnMguxFHMCCQSXWtn/GZC7cbbuz/iVozreHj2rq5G3TmpjIZEf9A233O1t6TRlX 4SwX0YAYtJPOvzVN5QrMQ10zrnlQK0gvhrWH1H6fXXQf5HuB8G5naQ115F+0a9umJB9e LXdQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=x1LbBin+3giaISRnECjj9ym7k3KwVa5JtFi86EOVr/U=; b=apWkGeP3lsREFtaZ0cUFv+pzs4mQ4PabdvmyunJUmv8TZBIqRMUhbCYn9D+U+09k49 9BcNdD1EPvUaTNUUkD+qiDGIg90ubsuGib9h1bjmE9Nq7fDPgqJzIYWjrFdNnxoCYGbB a8IMXdratipOG9AtlYDJ3UwHNm9rQR27tEGPoD91TUarIpSeyxl1Y8a2Jo51875rZuxd 4521GEaJtvuUc2Ostre+7hAt5v1G9GNF0JPvUh5nGmDZTOpPPwWQPog//JVrFpDwVHyK h/u6Mx4uKAZRT4/IKu06bdnUaq3sWKE4hMGh2y825+omYnGWnLwpxiQXj9xhWN1icZ+C yQIQ== X-Gm-Message-State: AOAM531IpHjYcQFwFuPUi8V4oIM8E8vGrER/7YmBMBwI/auTtMurOjDf Cf6i4MqZT51cVAaubAQhm76MEw== X-Google-Smtp-Source: ABdhPJwrMztjMpB1VLcp74m0YwjBj8njZMHQfsLZuHu+FNwvxHkPE/McM+Eqb0ufZNt4AIE6TMKqNg== X-Received: by 2002:a17:90a:d190:: with SMTP id fu16mr1564938pjb.14.1634155821020; Wed, 13 Oct 2021 13:10:21 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id kb15sm330965pjb.43.2021.10.13.13.10.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Oct 2021 13:10:20 -0700 (PDT) Date: Wed, 13 Oct 2021 20:10:16 +0000 From: Sean Christopherson To: Brijesh Singh Cc: x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, linux-coco@lists.linux.dev, linux-mm@kvack.org, linux-crypto@vger.kernel.org, Thomas Gleixner , Ingo Molnar , Joerg Roedel , Tom Lendacky , "H. Peter Anvin" , Ard Biesheuvel , Paolo Bonzini , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Andy Lutomirski , Dave Hansen , Sergio Lopez , Peter Gonda , Peter Zijlstra , Srinivas Pandruvada , David Rientjes , Dov Murik , Tobin Feldman-Fitzthum , Borislav Petkov , Michael Roth , Vlastimil Babka , "Kirill A . Shutemov" , Andi Kleen , tony.luck@intel.com, marcorr@google.com, sathyanarayanan.kuppuswamy@linux.intel.com Subject: Re: [PATCH Part2 v5 39/45] KVM: SVM: Introduce ops for the post gfn map and unmap Message-ID: References: <20210820155918.7518-1-brijesh.singh@amd.com> <20210820155918.7518-40-brijesh.singh@amd.com> <94128da2-c9f7-d990-2508-5a56f6cf16e7@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <94128da2-c9f7-d990-2508-5a56f6cf16e7@amd.com> X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: F36AFD000097 X-Stat-Signature: 9yi3rptsrkhr3qgfryfu96cojt44rntu Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=QDCwnlgr; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of seanjc@google.com designates 209.85.216.46 as permitted sender) smtp.mailfrom=seanjc@google.com X-HE-Tag: 1634155821-417929 Content-Transfer-Encoding: quoted-printable 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 Wed, Oct 13, 2021, Brijesh Singh wrote: >=20 > On 10/12/21 5:23 PM, Sean Christopherson wrote: > > On Fri, Aug 20, 2021, Brijesh Singh wrote: > >> When SEV-SNP is enabled in the guest VM, the guest memory pages can > >> either be a private or shared. A write from the hypervisor goes thro= ugh > >> the RMP checks. If hardware sees that hypervisor is attempting to wr= ite > >> to a guest private page, then it triggers an RMP violation #PF. > >> > >> To avoid the RMP violation, add post_{map,unmap}_gfn() ops that can = be > >> used to verify that its safe to map a given guest page. Use the SRCU= to > >> protect against the page state change for existing mapped pages. > > SRCU isn't protecting anything. The synchronize_srcu_expedited() in = the PSC code > > forces it to wait for existing maps to go away, but it doesn't preven= t new maps > > from being created while the actual RMP updates are in-flight. Most = telling is > > that the RMP updates happen _after_ the synchronize_srcu_expedited() = call. > > > > This also doesn't handle kvm_{read,write}_guest_cached(). >=20 > Hmm, I thought the kvm_{read_write}_guest_cached() uses the > copy_{to,from}_user(). Writing to the private will cause a #PF and will > fail the copy_to_user(). Am I missing something? Doh, right you are. I was thinking they cached the kmap, but it's just t= he gpa->hva that gets cached. > > I can't help but note that the private memslots idea[*] would handle = this gracefully, > > e.g. the memslot lookup would fail, and any change in private memslot= s would > > invalidate the cache due to a generation mismatch. > > > > KSM is another mess that would Just Work. > > > > I'm not saying that SNP should be blocked on support for unmapping gu= est private > > memory, but I do think we should strongly consider focusing on that e= ffort rather > > than trying to fix things piecemeal throughout KVM. I don't think it= 's too absurd > > to say that it might actually be faster overall. And I 100% think th= at having a > > cohesive design and uABI for SNP and TDX would be hugely beneficial t= o KVM. > > > > [*] https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F= %2Flkml.kernel.org%2Fr%2F20210824005248.200037-1-seanjc%40google.com&= data=3D04%7C01%7Cbrijesh.singh%40amd.com%7Cd1717d511a1f473cedc408d98ddfb0= 27%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637696814148744591%7CUnkn= own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ= XVCI6Mn0%3D%7C1000&sdata=3D3LF77%2BcqmpUdiP6YAk7LpImisBzjRGUzdI3raqjJ= xl0%3D&reserved=3D0 > > > >> +int sev_post_map_gfn(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, int= *token) > >> +{ > >> + struct kvm_sev_info *sev =3D &to_kvm_svm(kvm)->sev_info; > >> + int level; > >> + > >> + if (!sev_snp_guest(kvm)) > >> + return 0; > >> + > >> + *token =3D srcu_read_lock(&sev->psc_srcu); > >> + > >> + /* If pfn is not added as private then fail */ > > This comment and the pr_err() are backwards, and confused the heck ou= t of me. > > snp_lookup_rmpentry() returns '1' if the pfn is assigned, a.k.a. priv= ate. That > > means this code throws an error if the page is private, i.e. requires= the page > > to be shared. Which makes sense given the use cases, it's just incre= dibly > > confusing. > Actually I followed your recommendation from the previous feedback that > snp_lookup_rmpentry() should return 1 for the assigned page, 0 for the > shared and -negative for invalid. I can clarify it here=A0 again. > > >> + if (snp_lookup_rmpentry(pfn, &level) =3D=3D 1) { > > Any reason not to provide e.g. rmp_is_shared() and rmp_is_private() s= o that > > callers don't have to care as much about the return values? The -err= no/0/1 > > semantics are all but guarantee to bite us in the rear at some point. >=20 > If we look at the previous series, I had a macro rmp_is_assigned() for > exactly the same purpose but the feedback was to drop those macros and > return the state indirectly through the snp_lookup_rmpentry(). I can > certainly add a helper rmp_is_{shared,private}() if it makes code more > readable. Ah rats. Bad communication on my side. I didn't intended to have non-RM= P code directly consume snp_lookup_rmpentry() for simple checks. The goal behin= d the helper was to bury "struct rmpentry" so that it wasn't visible to the ker= nel at large. I.e. my objection was that rmp_assigned() was looking directly at= a non-architectural struct. My full thought for snp_lookup_rmpentry() was that it _could_ be consumed= directly without exposing "struct rmpentry", but that simple, common use cases wou= ld provide wrappers, e.g. static inline snp_is_rmp_private(...) { return snp_lookup_rmpentry(...) =3D=3D 1; } static inline snp_is_rmp_shared(...) { return snp_lookup_rmpentry(...) < 1; } Side topic, what do you think about s/assigned/private for the "public" A= PIs, as suggested/implied above? I actually like the terminology when talking sp= ecifically about the RMP, but it doesn't fit the abstractions that tend to be used w= hen talking about these things in other contexts, e.g. in KVM.