Re: [PATCH v4 1/3] KVM: arm64: Add pre_fault_memory implementation

["Thomson, Jack" <jackabt.amazon@gmail.com>]

Hi Marc,

On 18/01/2026 10:29 am, Marc Zyngier wrote:
> On Fri, 16 Jan 2026 14:33:42 +0000,
> "Thomson, Jack" <jackabt.amazon@gmail.com> wrote:
>>
>>
>> Hey Marc,
>>
>> Thanks for the review.
>>
>> On 15/01/2026 9:51 am, Marc Zyngier wrote:
>>> [+ Vladimir, who was also looking at this patch]
>>>
>>> On Tue, 13 Jan 2026 15:26:40 +0000,
>>> Jack Thomson <jackabt.amazon@gmail.com> wrote:
>>>>
>>>> From: Jack Thomson <jackabt@amazon.com>
>>>>
>>>> Add kvm_arch_vcpu_pre_fault_memory() for arm64. The implementation hands
>>>> off the stage-2 faulting logic to either gmem_abort() or
>>>> user_mem_abort().
>>>>
>>>> Add an optional page_size output parameter to user_mem_abort() to
>>>> return the VMA page size, which is needed when pre-faulting.
>>>>
>>>> Update the documentation to clarify x86 specific behaviour.
>>>>
>>>> Signed-off-by: Jack Thomson <jackabt@amazon.com>
>>>> ---
>>>>    Documentation/virt/kvm/api.rst |  3 +-
>>>>    arch/arm64/kvm/Kconfig         |  1 +
>>>>    arch/arm64/kvm/arm.c           |  1 +
>>>>    arch/arm64/kvm/mmu.c           | 79 ++++++++++++++++++++++++++++++++--
>>>>    4 files changed, 79 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>>>> index 01a3abef8abb..44cfd9e736bb 100644
>>>> --- a/Documentation/virt/kvm/api.rst
>>>> +++ b/Documentation/virt/kvm/api.rst
>>>> @@ -6493,7 +6493,8 @@ Errors:
>>>>    KVM_PRE_FAULT_MEMORY populates KVM's stage-2 page tables used to map memory
>>>>    for the current vCPU state.  KVM maps memory as if the vCPU generated a
>>>>    stage-2 read page fault, e.g. faults in memory as needed, but doesn't break
>>>> -CoW.  However, KVM does not mark any newly created stage-2 PTE as Accessed.
>>>> +CoW.  However, on x86, KVM does not mark any newly created stage-2 PTE as
>>>> +Accessed.
>>>>      In the case of confidential VM types where there is an initial
>>>> set up of
>>>>    private guest memory before the guest is 'finalized'/measured, this ioctl
>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
>>>> index 4f803fd1c99a..6872aaabe16c 100644
>>>> --- a/arch/arm64/kvm/Kconfig
>>>> +++ b/arch/arm64/kvm/Kconfig
>>>> @@ -25,6 +25,7 @@ menuconfig KVM
>>>>    	select HAVE_KVM_CPU_RELAX_INTERCEPT
>>>>    	select KVM_MMIO
>>>>    	select KVM_GENERIC_DIRTYLOG_READ_PROTECT
>>>> +	select KVM_GENERIC_PRE_FAULT_MEMORY
>>>>    	select VIRT_XFER_TO_GUEST_WORK
>>>>    	select KVM_VFIO
>>>>    	select HAVE_KVM_DIRTY_RING_ACQ_REL
>>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>>> index 4f80da0c0d1d..19bac68f737f 100644
>>>> --- a/arch/arm64/kvm/arm.c
>>>> +++ b/arch/arm64/kvm/arm.c
>>>> @@ -332,6 +332,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>>    	case KVM_CAP_COUNTER_OFFSET:
>>>>    	case KVM_CAP_ARM_WRITABLE_IMP_ID_REGS:
>>>>    	case KVM_CAP_ARM_SEA_TO_USER:
>>>> +	case KVM_CAP_PRE_FAULT_MEMORY:
>>>>    		r = 1;
>>>>    		break;
>>>>    	case KVM_CAP_SET_GUEST_DEBUG2:
>>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>>> index 48d7c372a4cd..499b131f794e 100644
>>>> --- a/arch/arm64/kvm/mmu.c
>>>> +++ b/arch/arm64/kvm/mmu.c
>>>> @@ -1642,8 +1642,8 @@ static int gmem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>      static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t
>>>> fault_ipa,
>>>>    			  struct kvm_s2_trans *nested,
>>>> -			  struct kvm_memory_slot *memslot, unsigned long hva,
>>>> -			  bool fault_is_perm)
>>>> +			  struct kvm_memory_slot *memslot, unsigned long *page_size,
>>>> +			  unsigned long hva, bool fault_is_perm)
>>>>    {
>>>>    	int ret = 0;
>>>>    	bool topup_memcache;
>>>> @@ -1923,6 +1923,9 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>    	kvm_release_faultin_page(kvm, page, !!ret, writable);
>>>>    	kvm_fault_unlock(kvm);
>>>>    +	if (page_size)
>>>> +		*page_size = vma_pagesize;
>>>> +
>>>>    	/* Mark the page dirty only if the fault is handled successfully */
>>>>    	if (writable && !ret)
>>>>    		mark_page_dirty_in_slot(kvm, memslot, gfn);
>>>> @@ -2196,8 +2199,8 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
>>>>    		ret = gmem_abort(vcpu, fault_ipa, nested, memslot,
>>>>    				 esr_fsc_is_permission_fault(esr));
>>>>    	else
>>>> -		ret = user_mem_abort(vcpu, fault_ipa, nested, memslot, hva,
>>>> -				     esr_fsc_is_permission_fault(esr));
>>>> +		ret = user_mem_abort(vcpu, fault_ipa, nested, memslot, NULL,
>>>> +				     hva, esr_fsc_is_permission_fault(esr));
>>>>    	if (ret == 0)
>>>>    		ret = 1;
>>>>    out:
>>>> @@ -2573,3 +2576,71 @@ void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled)
>>>>      	trace_kvm_toggle_cache(*vcpu_pc(vcpu), was_enabled,
>>>> now_enabled);
>>>>    }
>>>> +
>>>> +long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
>>>> +				    struct kvm_pre_fault_memory *range)
>>>> +{
>>>> +	struct kvm_vcpu_fault_info *fault_info = &vcpu->arch.fault;
>>>> +	struct kvm_s2_trans nested_trans, *nested = NULL;
>>>> +	unsigned long page_size = PAGE_SIZE;
>>>> +	struct kvm_memory_slot *memslot;
>>>> +	phys_addr_t ipa = range->gpa;
>>>> +	phys_addr_t end;
>>>> +	hva_t hva;
>>>> +	gfn_t gfn;
>>>> +	int ret;
>>>> +
>>>> +	if (vcpu_is_protected(vcpu))
>>>> +		return -EOPNOTSUPP;
>>>
>>> This feels pretty odd. If you have advertised the capability, then
>>> saying "not supported" at this stage is not on.
>>>
>>
>> Thanks good point, I think I can actually just drop this completely since
>> kvm_pvm_ext_allowed() would already exclude this as a capacility.
>>
> 
> I think you still need some runtime handling, just in case userspace
> is acting silly.
> 

Yeah makes sense I'll put something in.

>>>> +
>>>> +	/*
>>>> +	 * We may prefault on a shadow stage 2 page table if we are
>>>> +	 * running a nested guest.  In this case, we have to resolve the L2
>>>> +	 * IPA to the L1 IPA first, before knowing what kind of memory should
>>>> +	 * back the L1 IPA.
>>>> +	 *
>>>> +	 * If the shadow stage 2 page table walk faults, then we return
>>>> +	 * -EFAULT
>>>> +	 */
>>>> +	if (kvm_is_nested_s2_mmu(vcpu->kvm, vcpu->arch.hw_mmu) &&
>>>> +	    vcpu->arch.hw_mmu->nested_stage2_enabled) {
>>>> +		ret = kvm_walk_nested_s2(vcpu, ipa, &nested_trans);
>>>> +		if (ret)
>>>> +			return -EFAULT;
>>>
>>> And then what? Userspace is completely screwed here, with no way to
>>> make any forward progress, because the L1 is in charge of that S2, and
>>> L1 is not running. What's the outcome? Light a candle and pray?
>>>
>>> Also, the IPA you are passing as a parameter means absolutely nothing
>>> in the context of L2. Userspace doesn't have the faintest clue about
>>> the memory map presented to L2, as that's L1 business. L1 can
>>> absolutely present to L2 a memory map that doesn't have a single
>>> address in common with its own.
>>>
>>> So this really doesn't work at all.
>>>
>> Would just returning -EOPNOTSUPP in this case like:
> 
> Absolutely *not*. Userspace has no idea what the guest is doing, and
> cannot influence it (other than disabling nesting altogether). This is
> just as bad a -EFAULT.
> 
>>
>>    if (kvm_is_nested_s2_mmu(vcpu->kvm, vcpu->arch.hw_mmu) &&
>>        vcpu->arch.hw_mmu->nested_stage2_enabled)
>>      return -EOPNOTSUPP;
>>
>> be the best way to continue for now?
> 
> We both know that what you actually mean is "this doesn't match my use
> case, let someone else deal with it". To which my answer is that you
> either fully support pre-faulting, or you don't at all. There is no
> middle ground.
> 

Sorry if it came across as a cop-out, I think I just misunderstood your
earlier comment about taking a step back and looking at an easier
approach. If this is required I'll definitely look at full support for
pre-faulting.

>>>> +
>>>> +		ipa = kvm_s2_trans_output(&nested_trans);
>>>> +		nested = &nested_trans;
>>>> +	}
>>>> +
>>>> +	if (ipa >= kvm_phys_size(vcpu->arch.hw_mmu))
>>>> +		return -ENOENT;
>>>> +
>>>> +	/* Generate a synthetic abort for the pre-fault address */
>>>> +	fault_info->esr_el2 = (ESR_ELx_EC_DABT_LOW << ESR_ELx_EC_SHIFT) |
>>>> +		ESR_ELx_FSC_FAULT_L(KVM_PGTABLE_LAST_LEVEL);
>>>
>>> Why level 3? You must present a fault that matches the level at which
>>> the emulated fault would actually occur, because the rest of the
>>> infrastructure relies on that (at least on the permission path, and
>>> more to come).
>>>
>>
>> Ack, thanks I was relying on the fact `fault_is_perm` was hardcoded to
>> false. I'll replace with something like:
>>
>>    pgt = vcpu->arch.hw_mmu->pgt;
>>    ret = kvm_pgtable_get_leaf(pgt, gpa, &pte, &level);
>>    if (ret)
>>      return ret;
>>
>>    fault_info->esr_el2 = (ESR_ELx_EC_DABT_LOW << ESR_ELx_EC_SHIFT) |
>>      ESR_ELx_FSC_FAULT_L(level);
>>    fault_info->hpfar_el2 = HPFAR_EL2_NS |
>>      FIELD_PREP(HPFAR_EL2_FIPA, gpa >> 12);
> 
> If a mapping exists, you probably don't want to replay the fault. And
> this needs to occur while the mmu_lock is held.
> 

Got you thanks.

>>
>>> Taking a step back on all this, 90% of the problems are there because
>>> you are trying to support prefaulting a guest that is already running.
>>> If you limited this to actually *pre*-faulting the guest, it would be
>>> the easiest thing ever, and wouldn't suffer from any of the above (you
>>> can't be in a nested context if you haven't run).
>>>
>>> What prevents you from doing so? I'm perfectly happy to make this a
>>> separate API if this contradicts other implementations. Or are you
>>> relying on other side effects of the "already running" state?
>>
>> We would need this to work on an already running guest.
> 
> Then you need to fully support pre-faulting the guest even when it is
> in a nested context, without resorting to coping-out in situations
> that do not match your narrow use-case. Which means populating the
> canonical s2_mmu even when you're not in that particular context.

Will do, I'll work on a next revision with this case.

Thanks again for looking.

Jack