| Message ID | 20240510015822.503071-1-michael.roth@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand |
On Thu, May 09, 2024, Michael Roth wrote: > --- > arch/x86/kvm/mmu/mmu.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 62ad38b2a8c9..cecd8360378f 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -3296,7 +3296,7 @@ static int kvm_handle_noslot_fault(struct kvm_vcpu *vcpu, > return RET_PF_CONTINUE; > } > > -static bool page_fault_can_be_fast(struct kvm_page_fault *fault) > +static bool page_fault_can_be_fast(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) > { > /* > * Page faults with reserved bits set, i.e. faults on MMIO SPTEs, only > @@ -3307,6 +3307,32 @@ static bool page_fault_can_be_fast(struct kvm_page_fault *fault) > if (fault->rsvd) > return false; > > + /* > + * For hardware-protected VMs, certain conditions like attempting to > + * perform a write to a page which is not in the state that the guest > + * expects it to be in can result in a nested/extended #PF. In this > + * case, the below code might misconstrue this situation as being the > + * result of a write-protected access, and treat it as a spurious case > + * rather than taking any action to satisfy the real source of the #PF > + * such as generating a KVM_EXIT_MEMORY_FAULT. This can lead to the > + * guest spinning on a #PF indefinitely. > + * > + * For now, just skip the fast path for hardware-protected VMs since > + * they don't currently utilize any of this machinery anyway. In the > + * future, these considerations will need to be taken into account if > + * there's any need/desire to re-enable the fast path for > + * hardware-protected VMs. > + * > + * Since software-protected VMs don't have a notion of a shared vs. > + * private that's separate from what KVM is tracking, the above > + * KVM_EXIT_MEMORY_FAULT condition wouldn't occur, so avoid the > + * special handling for that case for now. Very technically, it can occur if userspace _just_ modified the attributes. And as I've said multiple times, at least for now, I want to avoid special casing SW-protected VMs unless it is *absolutely* necessary, because their sole purpose is to allow testing flows that are impossible to excercise without SNP/TDX hardware. > + */ > + if (kvm_slot_can_be_private(fault->slot) && > + !(IS_ENABLED(CONFIG_KVM_SW_PROTECTED_VM) && > + vcpu->kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM)) Heh, !(x && y) kills me, I misread this like 4 times. Anyways, I don't like the heuristic. It doesn't tie the restriction back to the cause in any reasonable way. Can't this simply be? if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn) return false; Which is much, much more self-explanatory.
On Thu, May 09, 2024, Michael Roth wrote: > The intended logic when handling #NPFs with the RMP bit set (31) is to > first check to see if the #NPF requires a shared<->private transition > and, if so, to go ahead and let the corresponding KVM_EXIT_MEMORY_FAULT > get forwarded on to userspace before proceeding with any handling of > other potential RMP fault conditions like needing to PSMASH the RMP > entry/etc (which will be done later if the guest still re-faults after > the KVM_EXIT_MEMORY_FAULT is processed by userspace). > > The determination of whether any userspace handling of > KVM_EXIT_MEMORY_FAULT is needed is done by interpreting the return code > of kvm_mmu_page_fault(). However, the current code misinterprets the > return code, expecting 0 to indicate a userspace exit rather than less > than 0 (-EFAULT). This leads to the following unexpected behavior: > > - for KVM_EXIT_MEMORY_FAULTs resulting for implicit shared->private > conversions, warnings get printed from sev_handle_rmp_fault() > because it does not expect to be called for GPAs where > KVM_MEMORY_ATTRIBUTE_PRIVATE is not set. Standard linux guests don't > generally do this, but it is allowed and should be handled > similarly to private->shared conversions rather than triggering any > sort of warnings > > - if gmem support for 2MB folios is enabled (via currently out-of-tree > code), implicit shared<->private conversions will always result in > a PSMASH being attempted, even if it's not actually needed to > resolve the RMP fault. This doesn't cause any harm, but results in a > needless PSMASH and zapping of the sPTE > > Resolve these issues by calling sev_handle_rmp_fault() only when > kvm_mmu_page_fault()'s return code is greater than or equal to 0, > indicating a KVM_MEMORY_EXIT_FAULT/-EFAULT isn't needed. While here, > simplify the code slightly and fix up the associated comments for better > clarity. > > Fixes: ccc9d836c5c3 ("KVM: SEV: Add support to handle RMP nested page faults") > > Signed-off-by: Michael Roth <michael.roth@amd.com> > --- > arch/x86/kvm/svm/svm.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index 426ad49325d7..9431ce74c7d4 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -2070,14 +2070,12 @@ static int npf_interception(struct kvm_vcpu *vcpu) > svm->vmcb->control.insn_len); > > /* > - * rc == 0 indicates a userspace exit is needed to handle page > - * transitions, so do that first before updating the RMP table. > + * rc < 0 indicates a userspace exit may be needed to handle page > + * attribute updates, so deal with that first before handling other > + * potential RMP fault conditions. > */ > - if (error_code & PFERR_GUEST_RMP_MASK) { > - if (rc == 0) > - return rc; > + if (rc >= 0 && error_code & PFERR_GUEST_RMP_MASK) This isn't correct either. A return of '0' also indiciates "exit to userspace", it just doesn't happen with SNP because '0' is returned only when KVM attempts emulation, and that too gets short-circuited by svm_check_emulate_instruction(). And I would honestly drop the comment, KVM's less-than-pleasant 1/0/-errno return values overload is ubiquitous enough that it should be relatively self-explanatory. Or if you prefer to keep a comment, drop the part that specifically calls out attributes updates, because that incorrectly implies that's the _only_ reason why KVM checks the return. But my vote is to drop the comment, because it essentially becomes "don't proceed to step 2 if step 1 failed", which kind of makes the reader go "well, yeah".
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 62ad38b2a8c9..cecd8360378f 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3296,7 +3296,7 @@ static int kvm_handle_noslot_fault(struct kvm_vcpu *vcpu, return RET_PF_CONTINUE; } -static bool page_fault_can_be_fast(struct kvm_page_fault *fault) +static bool page_fault_can_be_fast(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) { /* * Page faults with reserved bits set, i.e. faults on MMIO SPTEs, only @@ -3307,6 +3307,32 @@ static bool page_fault_can_be_fast(struct kvm_page_fault *fault) if (fault->rsvd) return false; + /* + * For hardware-protected VMs, certain conditions like attempting to + * perform a write to a page which is not in the state that the guest + * expects it to be in can result in a nested/extended #PF. In this + * case, the below code might misconstrue this situation as being the + * result of a write-protected access, and treat it as a spurious case + * rather than taking any action to satisfy the real source of the #PF + * such as generating a KVM_EXIT_MEMORY_FAULT. This can lead to the + * guest spinning on a #PF indefinitely. + * + * For now, just skip the fast path for hardware-protected VMs since + * they don't currently utilize any of this machinery anyway. In the + * future, these considerations will need to be taken into account if + * there's any need/desire to re-enable the fast path for + * hardware-protected VMs. + * + * Since software-protected VMs don't have a notion of a shared vs. + * private that's separate from what KVM is tracking, the above + * KVM_EXIT_MEMORY_FAULT condition wouldn't occur, so avoid the + * special handling for that case for now. + */ + if (kvm_slot_can_be_private(fault->slot) && + !(IS_ENABLED(CONFIG_KVM_SW_PROTECTED_VM) && + vcpu->kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM)) + return false; + /* * #PF can be fast if: * @@ -3407,7 +3433,7 @@ static int fast_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault) u64 *sptep; uint retry_count = 0; - if (!page_fault_can_be_fast(fault)) + if (!page_fault_can_be_fast(vcpu, fault)) return ret; walk_shadow_page_lockless_begin(vcpu);
For hardware-protected VMs like SEV-SNP guests, certain conditions like attempting to perform a write to a page which is not in the state that the guest expects it to be in can result in a nested/extended #PF which can only be satisfied by the host performing an implicit page state change to transition the page into the expected shared/private state. This is generally handled by generating a KVM_EXIT_MEMORY_FAULT event that gets forwarded to userspace to handle via KVM_SET_MEMORY_ATTRIBUTES. However, the fast_page_fault() code might misconstrue this situation as being the result of a write-protected access, and treat it as a spurious case when it sees that writes are already allowed for the sPTE. This results in the KVM MMU trying to resume the guest rather than taking any action to satisfy the real source of the #PF such as generating a KVM_EXIT_MEMORY_FAULT, resulting in the guest spinning on nested #PFs. For now, just skip the fast path for hardware-protected VMs since they don't currently utilize any of this access-tracking machinery anyway. In the future, these considerations will need to be taken into account if there's any need/desire to re-enable the fast path for hardware-protected VMs. Since software-protected VMs don't have a notion of a shared vs. private that's separate from what KVM is tracking, the above KVM_EXIT_MEMORY_FAULT condition wouldn't occur, so avoid the special handling for that case for now. Cc: Isaku Yamahata <isaku.yamahata@intel.com> Signed-off-by: Michael Roth <michael.roth@amd.com> --- arch/x86/kvm/mmu/mmu.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)