Re: [PATCH Part2 v5 39/45] KVM: SVM: Introduce ops for the post gfn map and unmap

On 10/13/21 1:10 PM, Sean Christopherson wrote:

On Wed, Oct 13, 2021, Brijesh Singh wrote:

quoted

On 10/12/21 5:23 PM, Sean Christopherson wrote:

quoted

On Fri, Aug 20, 2021, Brijesh Singh wrote:

quoted

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.

quoted

quoted

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&data=04%7C01%7Cbrijesh.singh%40amd.com%7C0f1a3f5f63074b60d21b08d98e857daf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637697526304105177%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=w6xS3DcG4fcTweC5i4%2BuB4jhn3Xcj2a44BkoATVcSgQ%3D&reserved=0

quoted

+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.

quoted

quoted

+	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