On Tue, Sep 22, 2026 at 11:46:18AM -0700, Sean Christopherson wrote:
> On Tue, Sep 22, 2026, Lorenzo Stoakes (ARM) wrote:
> > 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.
>
> Oh, they definitely can. I'm a-ok with using kvm_arch_pre_fault_allowed() on
> s390 and x86, the only option I am against is adding
> kvm_arch_pre_fault_allowed()
> but then not using it on architectures that obviously perform that exact
> check.
>
> > > --- 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;
> > > +}
>
> There should be no need for a __weak placeholder since this code is guarded by
> CONFIG_KVM_GENERIC_PRE_FAULT_MEMORY=y. I.e. force architectures to define the
> API. I don't think it's a coincidence that all of arm64, s390, and x86 ended
> up
> with restrictions; pre-faulting is far from a simple operation.
>
> Actually, that's an argument for a dedicated kvm_arch_pre_fault_allowed()
> versus
> a generic kvm_is_vcpu_loadable(): it helps force future architectures to
> actually
> think about when exactly pre-faulting is safe.
Ack on all and I agree __weak is not necessary in this case and probably quite
useful to make arches define it.
Will take this approach on respin thanks all!
--
Cheers, Lorenzo
