Hi Alex,
On 10/07/2026 5:03 pm, Alexandru Elisei wrote:
+ unsigned long *page_size;
As far as I know, fault handling was reworked to use struct
kvm_s2_fault_desc to store the fault information that user_mem_abort()
needs to handle the fault. Adding a 'page_size' field, that represents the
result of the gpa mapping process, might not be desirable.
Yeah agreed, Vincent suggested a separate output struct that
kvm_s2_fault_map() fills with what was actually mapped. I'll do that in
v6 thanks!
+ struct kvm_vcpu_fault_info *fault_info = &vcpu->arch.fault;
+ struct kvm_vcpu_fault_info fault_backup = *fault_info;
I'm not sure you need to make a backup here. vcpu->arch.fault is populated
each time the CPU takes a fault.
So this was actually flagged up by sashiko when I ran it before
submission, it suggested handling this for the case in which the vCPU
exited for MMIO, the next KVM_RUN calls kvm_handle_mmio_return which
uses the vcu->arch.fault.esr_el2. I'll add some comments to this maybe
to explain why this would be needed.
+ if (memslot->flags & KVM_MEMSLOT_INVALID) {
I don't think that's something we should care about, the flag can be set
immediately after the check as the function doesn't take kvm->slots_lock.
kvm_vcpu_prefault_memory() takes the srcu lock in read mode, so the
function is safe to run even if userspace does something silly like
deleting a memslot at the same time that it's prefaulting the guest memory
it represents.
I was there not as much for safety, but rather to pick up that error.
This way rather than userspace getting an -EFAULT it can just retry.
This is the same way it is handled in x86 as well.
It might not be obvious, but taking kvm->mmu_lock in read mode does not
guarantee that gpa will be mapped when kvm_pgtable_get_leaf() returns.
That's because a concurrent kvm_pgtable_stage2_map() for a different gpa
can destroy the mapping of the current gpa.
[ ... ]
The kvm->mmu_lock is dropped here, which means that it is possible for a
MMU notifier callback to have just unmapped the entire stage 2 for the VM.
Yeah as we mentioned in the other thread, this is best effort and we
can't guarantee that is survives the return. We do this for forward
progression, reporting the correct advance size for ranges which are
already mapped, instead of repeating the full dance for every page of them.
handle_access_fault() will fail if the mapping is gone. But I guess that's
fine if the ioctl does not guarantee that memory is still mapped after it
completes.
Yeah I think it's fine and will degrade to a no-op in this case.
Also, the documentation that this patch adds says: 'On arm64, newly created
stage-2 PTEs are marked Accessed'. Does not say anything about marking
**existing** ptes as accessed. Would be useful to explain the code does it.
Ack, will do thanks!
+ hva = gfn_to_hva_memslot_prot(memslot, gfn, NULL);
There's gfn_to_hva_memslot(memslot, gfn), is that what you are looking for?
I think gfn_to_hva_memslot() resolves the hva for write, so would fail
on KVM_MEM_READONLY slots, and prefualting is a read. Also this mirrors
the guest abort path so matches the way the run path resolves the HVA.
--
Thanks,
Jack
