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: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 note you dodge the actually difficult question of what this wrapper function
would look like ;)

So maybe like:

static int kvm_vcpu_load(struct kvm_vcpu *vcpu)
{
	int err;

	err = kvm_arch_allow_vcpu_load(vcpu);
	if (err)
		return err;

	vcpu_load(vcpu);
	return 0;
}

?

And I do like that you'd actually gate the right thing, I feel you on that,
obviously since this kind of predicate is what I started out with.

But I'm also looking to do the smallest possible thing here that fits the series
and doesn't preface it with a 'change how core kvm does something'.

But if Oliver/Marc feel this is viable then sure can go with it.

(Also naming is hard, kvm_vcpu_load()? do_vcpu_load()? maybe_vcpu_load()?
checked_vcpu_load()? vcpu_load_checked()? :P)

>
> > 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.
>
> But look at it from an x86 perspective.  Pretty much everyone will look at this
> and expect:
>
>   bool kvm_arch_vcpu_allow_pre_fault_memory(struct kvm_vcpu *vcpu)
>   {
> 	return vcpu->kvm->arch.pre_fault_allowed;
>   }

I'm not sure I really get your point here at all? :)

Why would it matter what people who are too lazy to go check the implementation
assume about an arch hook?

>
> > > 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? :)
>
> Because it allows arch code to dererefence "struct kvm_vcpu" in kvm_host.h,
> i.e. allows "inlining" the check.

Yeah I mean, micro-optimising a path run on a costly startup operation seems a
little unnecessary? :)

It seems the convention is __weak but I'm not going to die on this hill.

(C type safety is something of a myth but I do prefer to try to have what
little protection it offers when possible :)

--
Cheers, Lorenzo