Re: [PATCH v5 2/5] KVM: arm64: Add pre_fault_memory implementation

Hi Jack,

On Fri, Aug 21, 2026 at 02:52:17PM +0100, Thomson, Jack wrote:
> 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.

Ok, I see, VCPU exits to userspace due to MMIO, userspace calls
KVM_PRE_FAULT_MEMORY, userspace resumes VCPU and uses fault_info from
KVM_PRE_FAULT_MEMORY. Nicely spotted.

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

My point was that since there's no serialization between memslot changes and
KVM_PRE_FAULT_MEMORY, the condition can become true immediately after the if
statement. Doesn't really matter though anyway.

> 
> > 
> > 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!

I think there's a word missing there, I meant to say that it would be useful to
explain *why* the x86 implementation does not mark new PTEs as accessed, while
arm64 marks them as accessed, and arm64 also does that for existing PTEs.

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

Ah, I missed that, thanks for explaining!

I was wondering if you have any plans about posting an updated series.

Thanks,
Alex