On Thu, Sep 11, 2025 at 02:46:45PM +0100, Jack Thomson wrote:
@@ -1607,7 +1611,7 @@ static int __user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, struct kvm_s2_trans *nested, struct kvm_memory_slot *memslot, long *page_size, unsigned long hva,
bool fault_is_perm)
bool fault_is_perm, bool pre_fault){ int ret = 0; bool topup_memcache; @@ -1631,10 +1635,13 @@ static int __user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, vm_flags_t vm_flags; enum kvm_pgtable_walk_flags flags = KVM_PGTABLE_WALK_MEMABORT_FLAGS;
- if (pre_fault)
flags |= KVM_PGTABLE_WALK_PRE_FAULT;- if (fault_is_perm) fault_granule = kvm_vcpu_trap_get_perm_fault_granule(vcpu);
- write_fault = kvm_is_write_fault(vcpu);
- exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
- write_fault = !pre_fault && kvm_is_write_fault(vcpu);
- exec_fault = !pre_fault && kvm_vcpu_trap_is_exec_fault(vcpu);
I'm not a fan of this. While user_mem_abort() is already a sloppy mess, one thing we could reliably assume is the presence of a valid fault context. Now we need to remember to special-case our interpretation of a fault on whether or not we're getting invoked for a pre-fault.
I'd rather see the pre-fault infrastructure compose a synthetic fault context (HPFAR_EL2, ESR_EL2, etc.). It places the complexity where it belongs and the rest of the abort handling code should 'just work'.
+long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
struct kvm_pre_fault_memory *range)+{
- int r;
- hva_t hva;
- phys_addr_t end;
- long page_size;
- struct kvm_memory_slot *memslot;
- phys_addr_t ipa = range->gpa;
- gfn_t gfn = gpa_to_gfn(range->gpa);
- while (true) {
page_size = PAGE_SIZE;memslot = gfn_to_memslot(vcpu->kvm, gfn);if (!memslot)return -ENOENT;if (kvm_slot_has_gmem(memslot)) {r = __gmem_abort(vcpu, ipa, NULL, memslot, false, true);} else {hva = gfn_to_hva_memslot_prot(memslot, gfn, NULL);if (kvm_is_error_hva(hva))return -EFAULT;r = __user_mem_abort(vcpu, ipa, NULL, memslot, &page_size, hva, false,true);}if (r != -EAGAIN)break;if (signal_pending(current))return -EINTR;if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu))return -EIO;cond_resched();- };
Why do we need another retry loop? Looks like we've already got one in the arch-generic code.
Thanks, Oliver