Kernel KVM virtualization development
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Kai Huang <kai.huang@intel.com>
Cc: "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"vkuznets@redhat.com" <vkuznets@redhat.com>,
	 "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/3] KVM: x86: Reject nested CAP enablement if nested virtualization is disabled
Date: Tue, 14 Jul 2026 11:24:08 -0700	[thread overview]
Message-ID: <alZ-yMvZb2I2tJOf@google.com> (raw)
In-Reply-To: <6d8588f8fb1b68fee59d4a0c8cfcfd7a58d25f79.camel@intel.com>

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.  Generally speaking, the only flows that need to check
"enabled" are those that enumerate support to userspace.

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

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?

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.

  reply	other threads:[~2026-07-14 18:24 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-30 20:28 [PATCH v2 0/3] KVM: x86: Convert nested ops to static calls Sean Christopherson
2026-06-30 20:28 ` [PATCH v2 1/3] KVM: x86: Reject nested CAP enablement if nested virtualization is disabled Sean Christopherson
2026-07-10 12:56   ` Vitaly Kuznetsov
2026-07-14  3:46   ` Huang, Kai
2026-07-14 18:24     ` Sean Christopherson [this message]
2026-07-14 22:21       ` Huang, Kai
2026-06-30 20:28 ` [PATCH v2 2/3] KVM: x86: Add static calls for nested virtualization ops Sean Christopherson
2026-06-30 20:28 ` [PATCH v2 3/3] KVM: x86: Move nested_ops out of kvm_x86_ops, to global kvm_nested_ops Sean Christopherson
2026-07-14 18:41 ` [PATCH v2 0/3] KVM: x86: Convert nested ops to static calls Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alZ-yMvZb2I2tJOf@google.com \
    --to=seanjc@google.com \
    --cc=kai.huang@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=vkuznets@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox