On 7/19/21 6:30 PM, Sean Christopherson wrote: ...>
NAK on converting RMP entries in response to guest accesses. Corrupting guest data (due to dropping the "validated" flag) on a rogue/incorrect guest emulation request or misconfigured PV feature is double ungood. The potential kernel panic below isn't much better.
I also debated myself whether its okay to transition the page state to shared to complete the write operation. I am good with removing the converting RMP entries from the patch, and that will also remove the kernel panic code.
And I also don't think we need this heavyweight flow for user access, e.g. __copy_to_user(), just eat the RMP violation #PF like all other #PFs and exit to userspace with -EFAULT.
Yes, we could improve the __copy_to_user() to eat the RMP violation. I was tempted to go on that path but struggled to find a strong reason for it and was not sure if that accepted. I can add that support in next rev.
kvm_vcpu_map() and friends might need the manual lookup, at least initially,
Yes, the enhancement to the __copy_to_user() does not solve this problem and for it we need to do the manually lookup.
but
in an ideal world that would be naturally handled by gup(), e.g. by unmapping guest private memory or whatever approach TDX ends up employing to avoid #MCs.
+ */ + if (rmpentry_assigned(e)) { + pr_err("SEV-SNP: write to guest private gfn %llx\n", gfn); + rc = snp_make_page_shared(kvm_get_vcpu(kvm, 0), + gfn << PAGE_SHIFT, pfn, PG_LEVEL_4K); + BUG_ON(rc != 0); + } +}...+void kvm_arch_write_gfn_begin(struct kvm *kvm, struct kvm_memory_slot *slot, gfn_t gfn) +{ + update_gfn_track(slot, gfn, KVM_PAGE_TRACK_WRITE, 1);Tracking only writes isn't correct, as KVM reads to guest private memory will return garbage. Pulling the rug out from under KVM reads won't fail as spectacularly as writes (at least not right away), but they'll still fail. I'm actually ok reading garbage if the guest screws up, but KVM needs consistent semantics. Good news is that per-gfn tracking is probably overkill anyways. As mentioned above, user access don't need extra magic, they either fail or they don't. For kvm_vcpu_map(), one thought would be to add a "short-term" map variant that is not allowed to be retained across VM-Entry, and then use e.g. SRCU to block PSC requests until there are no consumers.
Sounds good to me, i will add the support in the next rev. thanks
