Re: [PATCH v3 01/14] KVM: Allow architectures to disallow pre-fault

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

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:
> > On Tue, Sep 22, 2026, Lorenzo Stoakes (ARM) wrote:
> > > On Tue, Sep 22, 2026 at 10:23:43AM -0700, Sean Christopherson wrote:
> > > > 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 ;)

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