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

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

On Tue, Sep 22, 2026 at 10:23:43AM -0700, Sean Christopherson wrote:
> On Tue, Sep 22, 2026, Oliver Upton wrote:
> > Hi Lorenzo,
> >
> > On Tue, Sep 22, 2026 at 03:17:55PM +0100, Lorenzo Stoakes (ARM) wrote:
> > > +bool __weak kvm_arch_vcpu_allow_pre_fault_memory(struct kvm_vcpu *vcpu)
> > > +{
> > > +	return true;
> > > +}
> > > +
> > >  void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
> > >  {
> > >  	int nr_vcpus, start, i, idx, yielded;
> > > @@ -4365,6 +4370,9 @@ static int kvm_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
> > >  	    range->gpa + range->size <= range->gpa)
> > >  		return -EINVAL;
> > >
> > > +	if (!kvm_arch_vcpu_allow_pre_fault_memory(vcpu))
> > > +		return -ENOEXEC;
> > > +
> >
> > nit: it'd be better to let the arch hook return an error of its choosing
> > but in reality this is only going to be used by arm64.
>
> Heh, except x86 already has something similar.
>
> 	if (!vcpu->kvm->arch.pre_fault_allowed)
> 		return -EOPNOTSUPP;
>
> As does s390:
>
> 	if (kvm_is_ucontrol(vcpu->kvm))
> 		return -EINVAL;

Yeah but they're all for different reasons I think :)

>
> I also don't like that this is subtly about avoiding vcpu_load(); it will be all
> too easy to overlook that detail in the future.

Well you see there's a problem here...

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

But you're still stuck with the problem of where exactly you put this check.

So then do you put that check in a wrapper around it?

Instead you can make the predicate 'don't prefault on a not-yet-initialised
vCPU' which is pretty sensible I think, have a specific place to put it and
all's well with the world.

(And adding that makes sense in the pre-fault series too...)

>
> I'd also be tempted to say it can be a macro, not a __weak function.  E.g.

Yeah it can be many things but why would you want a macro if you could possibly
avoid it? :)

Macros make the already-basically-pretend C type system into something even
worse.

Also it seems the convention for 'arches might not specify this' is the __weak
route AFAICT.

>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 27fe0cd5b2d7..99613df254cf 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1533,6 +1533,7 @@ static inline bool __vcpu_has_feature(const struct kvm_arch *ka, int feature)
>  #define vcpu_has_feature(v, f)	__vcpu_has_feature(&(v)->kvm->arch, (f))
>
>  #define kvm_vcpu_initialized(v) vcpu_get_flag(v, VCPU_INITIALIZED)
> +#define kvm_is_vcpu_loadable kvm_vcpu_initialized
>
>  int kvm_trng_call(struct kvm_vcpu *vcpu);
>  #ifdef CONFIG_KVM
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 3dd04605f2e5..02401b080507 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1053,6 +1053,9 @@ int kvm_trylock_all_vcpus(struct kvm *kvm);
>  int kvm_lock_all_vcpus(struct kvm *kvm);
>  void kvm_unlock_all_vcpus(struct kvm *kvm);
>
> +#ifndef kvm_is_vcpu_loadable
> +#define kvm_is_vcpu_loadable(v) true
> +#endif
>  void vcpu_load(struct kvm_vcpu *vcpu);
>  void vcpu_put(struct kvm_vcpu *vcpu);

--
Cheers, Lorenzo