Re: [PATCH v11 14/18] KVM: arm64: Handle guest_memfd-backed guest page faults

[Gavin Shan <gshan@redhat.com>]

Hi Fuad,

On 6/9/25 5:04 PM, Fuad Tabba wrote: 
> On Mon, 9 Jun 2025 at 05:08, Gavin Shan <gshan@redhat.com> wrote:
>>
>> On 6/6/25 1:37 AM, Fuad Tabba wrote:
>>> Add arm64 support for handling guest page faults on guest_memfd backed
>>> memslots. Until guest_memfd supports huge pages, the fault granule is
>>> restricted to PAGE_SIZE.
>>>
>>> Signed-off-by: Fuad Tabba <tabba@google.com>
>>> ---
>>>    arch/arm64/kvm/mmu.c | 93 ++++++++++++++++++++++++++++++++++++++++++--
>>>    1 file changed, 90 insertions(+), 3 deletions(-)
>>>
>>
>> One comment below. Otherwise, it looks good to me.
>>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>>
>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>> index ce80be116a30..f14925fe6144 100644
>>> --- a/arch/arm64/kvm/mmu.c
>>> +++ b/arch/arm64/kvm/mmu.c
>>> @@ -1508,6 +1508,89 @@ static void adjust_nested_fault_perms(struct kvm_s2_trans *nested,
>>>        *prot |= kvm_encode_nested_level(nested);
>>>    }
>>>
>>> +#define KVM_PGTABLE_WALK_MEMABORT_FLAGS (KVM_PGTABLE_WALK_HANDLE_FAULT | KVM_PGTABLE_WALK_SHARED)
>>> +
>>> +static int gmem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>> +                   struct kvm_s2_trans *nested,
>>> +                   struct kvm_memory_slot *memslot, bool is_perm)
>>> +{
>>> +     bool logging, write_fault, exec_fault, writable;
>>> +     enum kvm_pgtable_walk_flags flags = KVM_PGTABLE_WALK_MEMABORT_FLAGS;
>>> +     enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R;
>>> +     struct kvm_pgtable *pgt = vcpu->arch.hw_mmu->pgt;
>>> +     struct page *page;
>>> +     struct kvm *kvm = vcpu->kvm;
>>> +     void *memcache;
>>> +     kvm_pfn_t pfn;
>>> +     gfn_t gfn;
>>> +     int ret;
>>> +
>>> +     ret = prepare_mmu_memcache(vcpu, !is_perm, &memcache);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (nested)
>>> +             gfn = kvm_s2_trans_output(nested) >> PAGE_SHIFT;
>>> +     else
>>> +             gfn = fault_ipa >> PAGE_SHIFT;
>>> +
>>> +     logging = memslot_is_logging(memslot);
>>> +     write_fault = kvm_is_write_fault(vcpu);
>>> +     exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu);
>>> +
>>> +     if (write_fault && exec_fault) {
>>> +             kvm_err("Simultaneous write and execution fault\n");
>>> +             return -EFAULT;
>>> +     }
>>> +
>>> +     if (is_perm && !write_fault && !exec_fault) {
>>> +             kvm_err("Unexpected L2 read permission error\n");
>>> +             return -EFAULT;
>>> +     }
>>> +
>>> +     ret = kvm_gmem_get_pfn(kvm, memslot, gfn, &pfn, &page, NULL);
>>> +     if (ret) {
>>> +             kvm_prepare_memory_fault_exit(vcpu, fault_ipa, PAGE_SIZE,
>>> +                                           write_fault, exec_fault, false);
>>> +             return ret;
>>> +     }
>>> +
>>
>> -EFAULT or -EHWPOISON shall be returned, as documented in virt/kvm/api.rst. Besides,
>> kvm_send_hwpoison_signal() should be executed when -EHWPOISON is returned from
>> kvm_gmem_get_pfn()? :-)
> 
> This is a bit different since we don't have a VMA. Refer to the discussion here:
> 
> https://lore.kernel.org/all/20250514212653.1011484-1-jthoughton@google.com/
> 

Thanks for the pointer. You're right that we don't have VMA here. To return the
'ret' to userspace seems the practical way to have here.

Thanks,
Gavin

>>
>>> +     writable = !(memslot->flags & KVM_MEM_READONLY) &&
>>> +                (!logging || write_fault);
>>> +
>>> +     if (nested)
>>> +             adjust_nested_fault_perms(nested, &prot, &writable);
>>> +
>>> +     if (writable)
>>> +             prot |= KVM_PGTABLE_PROT_W;
>>> +
>>> +     if (exec_fault ||
>>> +         (cpus_have_final_cap(ARM64_HAS_CACHE_DIC) &&
>>> +          (!nested || kvm_s2_trans_executable(nested))))
>>> +             prot |= KVM_PGTABLE_PROT_X;
>>> +
>>> +     kvm_fault_lock(kvm);
>>> +     if (is_perm) {
>>> +             /*
>>> +              * Drop the SW bits in favour of those stored in the
>>> +              * PTE, which will be preserved.
>>> +              */
>>> +             prot &= ~KVM_NV_GUEST_MAP_SZ;
>>> +             ret = KVM_PGT_FN(kvm_pgtable_stage2_relax_perms)(pgt, fault_ipa, prot, flags);
>>> +     } else {
>>> +             ret = KVM_PGT_FN(kvm_pgtable_stage2_map)(pgt, fault_ipa, PAGE_SIZE,
>>> +                                          __pfn_to_phys(pfn), prot,
>>> +                                          memcache, flags);
>>> +     }
>>> +     kvm_release_faultin_page(kvm, page, !!ret, writable);
>>> +     kvm_fault_unlock(kvm);
>>> +
>>> +     if (writable && !ret)
>>> +             mark_page_dirty_in_slot(kvm, memslot, gfn);
>>> +
>>> +     return ret != -EAGAIN ? ret : 0;
>>> +}
>>> +
>>>    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,
>>> @@ -1532,7 +1615,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>        enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R;
>>>        struct kvm_pgtable *pgt;
>>>        struct page *page;
>>> -     enum kvm_pgtable_walk_flags flags = KVM_PGTABLE_WALK_HANDLE_FAULT | KVM_PGTABLE_WALK_SHARED;
>>> +     enum kvm_pgtable_walk_flags flags = KVM_PGTABLE_WALK_MEMABORT_FLAGS;
>>>
>>>        if (fault_is_perm)
>>>                fault_granule = kvm_vcpu_trap_get_perm_fault_granule(vcpu);
>>> @@ -1959,8 +2042,12 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
>>>                goto out_unlock;
>>>        }
>>>
>>> -     ret = user_mem_abort(vcpu, fault_ipa, nested, memslot, hva,
>>> -                          esr_fsc_is_permission_fault(esr));
>>> +     if (kvm_slot_has_gmem(memslot))
>>> +             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));
>>>        if (ret == 0)
>>>                ret = 1;
>>>    out:
>>
>