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

On Tue, Sep 22, 2026, Lorenzo Stoakes (ARM) wrote:

On Tue, Sep 22, 2026 at 11:07:53AM -0700, Oliver Upton wrote:

quoted

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

quoted

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

quoted

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