Re: [PATCH v3 01/14] KVM: Allow architectures to disallow pre-fault
From: "Lorenzo Stoakes (ARM)" <ljs@kernel.org>
Date: 2026-09-22 18:35:15
Also in:
kvm, kvmarm, linux-doc, linux-kselftest, lkml
On Tue, Sep 22, 2026 at 11:07:53AM -0700, Oliver Upton wrote:
On Tue, Sep 22, 2026 at 10:36:49AM -0700, Sean Christopherson wrote:quoted
On Tue, Sep 22, 2026, Lorenzo Stoakes (ARM) wrote:quoted
On Tue, Sep 22, 2026 at 10:23:43AM -0700, Sean Christopherson wrote:quoted
Rather than have kvm_arch_vcpu_allow_pre_fault_memory(), what if we add a more generic kvm_is_vcpu_loadable()? That way we don't need to worry as much about the return value, the connection to vcpu_load() is obvious, and we don't need to add another pre-check if future (or cleaned-up existing?) ioctls want to do vcpu_load() in common code....this is exactly what I started out with. But then you are in a pickle, because _really_ you need to do that check in vcpu_load(). Which is a void function. Which is called by every single architecture all over the place. So you'd have actually no way of signalling the error back. Of course those places are arch code and you could say 'arches should know better and if they call it it's fine not to call the arch 'can you load' function.Yes, that's my vote. It'd be easy enough to clarify that "rule" with a comment in linux/kvm_host.h.I feel like trying to make this generic will wind up under-documenting the single example we have with the pre fault ioctl. Putting the comment into a header practically guarantees that nobody will read it either. I'd favor doing something like below and sticking the comment inline in the ioctl handler. Unless I'm missing something blatantly obvious, I don't see why the x86 or s390 pre-conditions can't be tested early too. But I don't care enough to bikeshed this any further.
Haha yup :) this is eminately bikesheddable territory.
Thanks, Oliver
I'm fine with the below if x86/s390 people are. The inline comment is a cheeky trick that should help clarify intent (I think perhaps Sean that's what you meant re: people assuming it would check some local state?) Anyway if people think that's sane I can do on respin and we can settle on the lovely shade of purple or whatever the shed looks like now ;)
quoted hunk ↗ jump to hunk
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 8b080804bc90..396e64875fe7 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c@@ -1852,6 +1852,14 @@ static int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu, return __kvm_arm_vcpu_set_events(vcpu, events); } +int kvm_arch_pre_fault_allowed(struct kvm_vcpu *vcpu) +{ + if (!kvm_vcpu_initialized(vcpu)) + return -ENOEXEC; + + return 0; +} + long kvm_arch_vcpu_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg) {diff --git a/arch/s390/kvm/s390/s390.c b/arch/s390/kvm/s390/s390.c index 5c73f43782a7..47fe032444f4 100644 --- a/arch/s390/kvm/s390/s390.c +++ b/arch/s390/kvm/s390/s390.c@@ -5784,6 +5784,14 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *old, s390_kvm_mmu_commit_memory_region(kvm, old, new, change); } +int kvm_arch_pre_fault_allowed(struct kvm_vcpu *vcpu) +{ + if (kvm_is_ucontrol(vcpu->kvm)) + return -EINVAL; + + return 0; +} + /** * kvm_arch_vcpu_pre_fault_memory() -- pre-fault and link gmap dat tables * @vcpu: the vcpu that shall appear to have generated the fault-in.@@ -5810,9 +5818,6 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, struct kvm_pre_fault_ gpa_t end; int rc; - if (kvm_is_ucontrol(vcpu->kvm)) - return -EINVAL; - rc = kvm_s390_faultin_gfn(vcpu, NULL, &f); if (rc == PGM_ADDRESSING) return -ENOENT;diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 064ecc33b926..c35fd2868c20 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c@@ -5086,6 +5086,14 @@ static int kvm_tdp_page_prefault(struct kvm_vcpu *vcpu, gpa_t gpa, } } +int kvm_arch_pre_fault_allowed(struct kvm_vcpu *vcpu) +{ + if (!vcpu->kvm->arch.pre_fault_allowed) + return -EOPNOTSUPP; + + return 0; +} + long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, struct kvm_pre_fault_memory *range) {@@ -5095,9 +5103,6 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, u64 end; int r; - if (!vcpu->kvm->arch.pre_fault_allowed) - return -EOPNOTSUPP; - if (kvm_is_gfn_alias(vcpu->kvm, gpa_to_gfn(range->gpa))) return -EINVAL;diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 03bfc92864b6..bff842548c04 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h@@ -1693,6 +1693,7 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu); bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu); bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu); bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu); +int kvm_arch_pre_fault_allowed(struct kvm_vcpu *vcpu); void kvm_arch_pre_destroy_vm(struct kvm *kvm); void kvm_arch_create_vm_debugfs(struct kvm *kvm);diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 65eb26a0520d..07f2ce7a3cb3 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c@@ -3961,6 +3961,11 @@ bool __weak kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu) return false; } +int __weak kvm_arch_pre_fault_allowed(struct kvm_vcpu *vcpu) +{ + return 0; +} + void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode) { int nr_vcpus, start, i, idx, yielded;@@ -4353,7 +4358,7 @@ static int kvm_vcpu_ioctl_get_stats_fd(struct kvm_vcpu *vcpu) static int kvm_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, struct kvm_pre_fault_memory *range) { - int idx; + int idx, ret; long r; u64 full_size;@@ -4365,6 +4370,14 @@ static int kvm_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, range->gpa + range->size <= range->gpa) return -EINVAL; + /* + * Certain architectures (e.g. arm64) need to reject the ioctl 'early' + * before vcpu_load(). + */ + ret = kvm_arch_pre_fault_allowed(vcpu); + if (ret) + return ret; + vcpu_load(vcpu); idx = srcu_read_lock(&vcpu->kvm->srcu);Thanks, Oliver
-- Cheers, Lorenzo