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

On Tue, Sep 22, 2026 at 10:36:49AM -0700, Sean Christopherson wrote:

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

quoted

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?

quoted

quoted

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