Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Cc:     brijesh.singh@amd.com, 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 <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Ard Biesheuvel <ardb@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Sergio Lopez <slp@redhat.com>, Peter Gonda <pgonda@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        David Rientjes <rientjes@google.com>,
        Dov Murik <dovmurik@linux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@ibm.com>,
        Borislav Petkov <bp@alien8.de>,
        Michael Roth <michael.roth@amd.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Andi Kleen <ak@linux.intel.com>, 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
To:     Sean Christopherson <seanjc@google.com>
References: <20210820155918.7518-1-brijesh.singh@amd.com>
 <20210820155918.7518-40-brijesh.singh@amd.com> <YWYm/Gw8PbaAKBF0@google.com>
 <94128da2-c9f7-d990-2508-5a56f6cf16e7@amd.com> <YWc9KL8gghEiI48h@google.com>
From:   Brijesh Singh <brijesh.singh@amd.com>
Message-ID: <a7a541ba-129a-1083-3517-c30e9f9cd7c7@amd.com>
Date:   Wed, 13 Oct 2021 16:49:28 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.14.0
In-Reply-To: <YWc9KL8gghEiI48h@google.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?amdDZHNxYWhQUGNJRFpqT2xMWW4rVUVLYTNKZXhoVkh1K2Rod3RBTm9VaFZZ?=
 =?utf-8?B?VExoU2kwZVNzT3UxWkxMUTdvYW1jVGR0UkFOc0MxSzFvbGFoQ3drV2VkdGp1?=
 =?utf-8?B?SUlXTml6cVlFMHZaOHF4bldSeGtXcVd3NjA4M0YramF3VXRORGltVk5mUFpZ?=
 =?utf-8?B?NjhneTdDRkMrS2NNd3EzTlRGcENNZVVQa05XYkRFL3B3ekV2ZU03UWpNcmhi?=
 =?utf-8?B?TGZFWjQ4YUhVSzF6RGQybnJCTXF3UkFDanJvNjJ3aDJFYnFna2ZveWJadjFC?=
 =?utf-8?B?Y3A3NkdjbXN5MWhoU3ZYZGhzcFM3ajYwUHBmSnVmQVFLLzdyVnRzVmxtYTcw?=
 =?utf-8?B?cWRWQ1ZVaHcvUmdiek5BMGkyTXgxUFdqeGFPWFpScS82VFh4aVdia3FnK29i?=
 =?utf-8?B?ZTM5T1Z3SmgwTWxFN2J1QWFYVVdJZTR3bXNUR0phMDVDTTArL05jc3JlbHBt?=
 =?utf-8?B?QUpWWVd0Y1NEbkJqbHY2UzlaR1M3UGN4RzM5L2ZVa05YQzA3YTluNnR0M1VB?=
 =?utf-8?B?eWVQQVV1cGUwSkNqaDY4N05rVUdUQ0crSFJlVzNuVzltcTIrVGxqazFnYzQ0?=
 =?utf-8?B?RDQxUzlLakZ1VEtSWUNMd3pBOXhPTDZOSTVzYnJPQkN3WEo3ZkNGWmxnV1NQ?=
 =?utf-8?B?cnJIdTNYaWNjb0o1citoOXJOTzVvREQwdDI1TnR3bWdzUVVFOWIvaE5CaGg0?=
 =?utf-8?B?dTlrdzZHc3Rsclpzd0dURHpRWTZURmtGa0NKZ2tpdmQ3ZDZKWHdwT2pWMHBX?=
 =?utf-8?B?REJDUXIyZUlFYkprekg0THh4WlRiVmVEZHkrOWR6NVJjSUx5a2tzejEzeHlq?=
 =?utf-8?B?OHNiN2hSRUNuekNRNUhPUWZsZGdRMFI0T1huZnlpZ2NOMVBMQmtXdVo1emt6?=
 =?utf-8?B?WS9Ob3YvT1p4QzQ1RGt5YXpvV3l5QkE2QmUzTUIrZGI0djBsWUZ2M3ljZW5a?=
 =?utf-8?B?MnFpTU5DQnkzbG1raG01Sk5KNjlwRllMSE1BbUFVV1NQTXp0ZmE4cFpES3FQ?=
 =?utf-8?B?RFI1OWxYKzVUZkdDbGhDMXYxNEN6bmVyZHBVVElMNUlsNnd4WUt5NEs5RXlQ?=
 =?utf-8?B?VnpYYWUrQkYzbzB3VUQrUGsrQzNwOWF0VVF1UzlvRVVmdU9LU1pqVFRDbmw2?=
 =?utf-8?B?cWRtRWZIM2dESzA3Uk1hYWVRQTVFdUgzUk1NKzNkUWliQWIwbWJqK0NndHBX?=
 =?utf-8?B?akR3UDFsV3FuWHBrV0hWTVFRa1B5NUVZQWNWa3M2OXNqOHB6U3paeWk5Z3Fs?=
 =?utf-8?B?S0VXS3NYOVZ6M0Z2MGFrVjgyck5qM1ExRlY0d2JxTDA4R2NaZlhXaDdtVmZu?=
 =?utf-8?B?eSs0STFHcGVpaUJPQTNaM0o0b0E5Ty90VUhQYkVidGlPSXo3RDVEQXQvMWRH?=
 =?utf-8?B?SHlyUEZ3dkJocjlESVoyMHpPYVg5RUVtTjBTK1Iyb3NIWGRVL29XNmFCZGRn?=
 =?utf-8?B?TGdMekZHN0o0MFFKMWhHdzBMdlJMQ29IVHBDSjRPeEFrNXhIa2RmUU1WM1ZO?=
 =?utf-8?B?UkRrNVR0ZnBvelRDaEU2M2MveTB0N210VHN3RWpvVGVXc082OG5tTmZyNzAr?=
 =?utf-8?B?SW5Lbllqb0dBSnZnVVM5RHUrMzNQREgwcmpPc1JBMU15WVZoa2oyQzgzd2Za?=
 =?utf-8?B?L004SmFuSElvR1ltS05tR3lrU0N4RWhTZ0V1UGt6bHdBN3EvV0RTQXRhK3Fv?=
 =?utf-8?B?aWl1ZEJWa3hkbllkL0tQYzhqcG9acm0rUDREellkOG9tdE84VkZqSUJkU0NR?=
 =?utf-8?Q?Vkd+biKzs6m98LzVYU66KXcHDYM91R9THOFTQi3?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b577bf6-0cde-423e-6c36-08d98e9356f0
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2021 21:49:32.1538
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 53G9PMVlkiTPx6+1xFOGQ1GESEBx4l7SFxs8e0vZh2eFXHNnomgNIVHOBsxKlB0Gp4fE4WmQWCP2V/mBQpkTsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR12MB4750
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org


On 10/13/21 1:10 PM, Sean Christopherson wrote:
> On Wed, Oct 13, 2021, Brijesh Singh wrote:
>> 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 through
>>>> the RMP checks. If hardware sees that hypervisor is attempting to write
>>>> 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 prevent 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().
>> 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 the
> 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 memslots 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 guest private
>>> memory, but I do think we should strongly consider focusing on that effort 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 that having a
>>> cohesive design and uABI for SNP and TDX would be hugely beneficial to KVM.
>>>
>>> [*] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2F20210824005248.200037-1-seanjc%40google.com&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C0f1a3f5f63074b60d21b08d98e857daf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637697526304105177%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=w6xS3DcG4fcTweC5i4%2BuB4jhn3Xcj2a44BkoATVcSgQ%3D&amp;reserved=0
>>>
>>>> +int sev_post_map_gfn(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, int *token)
>>>> +{
>>>> +	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>> +	int level;
>>>> +
>>>> +	if (!sev_snp_guest(kvm))
>>>> +		return 0;
>>>> +
>>>> +	*token = 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 out of me.
>>> snp_lookup_rmpentry() returns '1' if the pfn is assigned, a.k.a. private.  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 incredibly
>>> 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  again.
>>
>>>> +	if (snp_lookup_rmpentry(pfn, &level) == 1) {
>>> Any reason not to provide e.g. rmp_is_shared() and rmp_is_private() so that
>>> callers don't have to care as much about the return values?  The -errno/0/1
>>> semantics are all but guarantee to bite us in the rear at some point.
>> 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-RMP code
> directly consume snp_lookup_rmpentry() for simple checks.  The goal behind the
> helper was to bury "struct rmpentry" so that it wasn't visible to the kernel 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 would provide
> wrappers, e.g.
>
> static inline snp_is_rmp_private(...)
> {
> 	return snp_lookup_rmpentry(...) == 1;
> }
>
> static inline snp_is_rmp_shared(...)
> {
> 	return snp_lookup_rmpentry(...) < 1;
> }

Yep, that what I was going to do for the helper.


>
> Side topic, what do you think about s/assigned/private for the "public" APIs, as
> suggested/implied above?  I actually like the terminology when talking specifically
> about the RMP, but it doesn't fit the abstractions that tend to be used when talking
> about these things in other contexts, e.g. in KVM.

I can float the idea to see if docs folks is okay with the changes but
generally speaking we all have been referring the assigned == private in
the Linux SNP support patch.

thanks