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
