Re: [PATCH v2 1/3] KVM: x86: Reject nested CAP enablement if nested virtualization is disabled

[Huang, Kai]

On Tue, 2026-07-14 at 11:24 -0700, Sean Christopherson wrote:
> On Tue, Jul 14, 2026, Kai Huang wrote:
> > 
> > > but now KVM is over-reporting support for
> > > KVM_CAP_NESTED_STATE and KVM_CAP_HYPERV_ENLIGHTENED_VMCS.
> > > 
> > > 
> > [...]
> > 
> > > @@ -2346,7 +2346,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > >  			r &= ~KVM_X2APIC_ENABLE_SUPPRESS_EOI_BROADCAST;
> > >  		break;
> > >  	case KVM_CAP_NESTED_STATE:
> > > -		r = kvm_x86_ops.nested_ops->get_state ?
> > > +		r = kvm_x86_ops.nested_ops->enabled ?
> > >  			kvm_x86_ops.nested_ops->get_state(NULL, NULL, 0) : 0;
> > >  		break;
> > 
> > [...]
> > 
> > >  #ifdef CONFIG_KVM_HYPERV
> > > @@ -2354,7 +2354,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > >  		r = kvm_x86_ops.enable_l2_tlb_flush != NULL;
> > >  		break;
> > >  	case KVM_CAP_HYPERV_ENLIGHTENED_VMCS:
> > > -		r = kvm_x86_ops.nested_ops->enable_evmcs != NULL;
> > > +		r = kvm_x86_ops.nested_ops->enabled &&
> > > +		    kvm_x86_ops.nested_ops->enable_evmcs != NULL;
> > > 
> > 
> > IMHO an additional 'enabled' seems a bit redundant?  And it seems there's no
> > rule whether the common x86 code should check it before (checking and) making
> > the call or not.
> 
> Because in most cases, the hooks are reachable if and only if nested virtualization
> is actively being used.  
> 

Yeah.

> Generally speaking, the only flows that need to check
> "enabled" are those that enumerate support to userspace.

I see.

> 
> > How about letting VMX/SVM code to explicitly clear the function pointer in
> > xx_hardware_setup()?
> 
> The downside to that is approach is that all nested ops would become optional
> from the perspective of arch/x86/include/asm/kvm-x86-nested-ops.h, i.e. we'd lose
> the sanity checks provided by:
> 
> #define KVM_X86_NESTED_OP(func) \
> 	WARN_ON(!kvm_nested_ops.func); __KVM_X86_NESTED_OP(func)

Right.

> 
> We could probably come up with a way to make things work, but I think we'd end
> up with something a lot like the "enabled" flag.
> 
> > Perhaps as an ultimate goal, I am not sure whether VMX/SVM should provide a all-
> > NULL version of kvm_x86_nested_ops and the kvm_nested_ops_update() should treat
> > all as either OPTIONAL or OPTIONAL_RET0.  There will be still call sites which
> > needs to check the pointer is NULL or not and return -EXXX but I guess we can
> > live with that (kvm_x86_ops is similar too anyway) ?
> 
> I'm concerned this would actually be less robust, due to static calls being nops
> by default.  If KVM screws up and invokes a "mandatory" nested op that has been
> nullified, exactly what will happen is harder to predict, e.g. kvm_translate_gpa()
> would probably return stack garbage?

Oh ok.  So I guess when nested is truly enabled, you only want to use OPTIONAL
when it is truly optional, so that the KVM_X86_NESTED_OP() can catch if a NULL
pointer is provided by vendor code.

> 
> Any such bug is highly unlikely to occur, but I don't want to sacrifice
> defense-in-depth without a clear benefit, and IMO avoiding the "enabled" flag
> wouldn't provide much benefit in practice.

OK agreed.  Yeah being able to catch bug is more important :-)

I thought about in case of nested=false, and for the callbacks that can only be
called when nested=true, we can replace the NULL pointer with some stub function
which does WARN_ON_ONCE() first but maybe that is not worth doing.

Thanks for explanation.