Received-SPF: pass (google.com: domain of linux-kernel+bounces-185401-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 21 May 2024 13:02:55 -0700
In-Reply-To: <1dbb09c5-35c7-49b9-8c6f-24b532511f0b@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240425233951.3344485-1-seanjc@google.com> <20240425233951.3344485-2-seanjc@google.com>
 <5dfc9eb860a587d1864371874bbf267fa0aa7922.camel@intel.com>
 <ZkI5WApAR6iqCgil@google.com> <6100e822-378b-422e-8ff8-f41b19785eea@intel.com>
 <1dbb09c5-35c7-49b9-8c6f-24b532511f0b@intel.com>
Message-ID: <Zkz97y9VVAFgqNJB@google.com>
Subject: Re: [PATCH 1/4] x86/reboot: Unconditionally define
 cpu_emergency_virt_cb typedef
From: Sean Christopherson <seanjc@google.com>
To: Kai Huang <kai.huang@intel.com>
Cc: "pbonzini@redhat.com" <pbonzini@redhat.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"

On Wed, May 15, 2024, Kai Huang wrote:
> How about we just make all emergency virtualization disable code
> unconditional but not guided by CONFIG_KVM_INTEL || CONFIG_KVM_AMD, i.e.,
> revert commit
> 
>    261cd5ed934e ("x86/reboot: Expose VMCS crash hooks if and only if
> KVM_{INTEL,AMD} is enabled")
> 
> It makes sense anyway from the perspective that it allows the out-of-tree
> kernel module hypervisor to use this mechanism w/o needing to have the
> kernel built with KVM enabled in Kconfig.  Otherwise, strictly speaking,
> IIUC, the kernel won't be able to support out-of-tree module hypervisor as
> there's no other way the module can intercept emergency reboot.

Practically speaking, no one is running an out-of-tree hypervisor without either
(a) KVM being enabled in the .config, or (b) non-trivial changes to the kernel.

Exposing/exporting select APIs and symbols if and only if KVM is enabled is a
a well-established pattern, and there are concrete benefits to doing so.  E.g.
it allows minimizing the kernel footprint for use cases that don't want/need KVM.

> This approach avoids the weirdness of the unconditional define for only
> cpu_emergency_virt_cb.

I genuinely don't understand why you find it weird to unconditionally define
cpu_emergency_virt_cb.  There are myriad examples throughout the kernel where a
typedef, struct, enum, etc. is declared/defined even though support for its sole
end consumer is disabled.  E.g. include/linux/mm_types.h declares "struct mem_cgroup"
for pretty much the exact same reason, even though the structure is only fully
defined if CONFIG_MEMCG=y.

The only oddity here is that the API that the #ifdef that guards the usage happens
to be right below the typedef, but it shouldn't take that much brain power to
figure out why a typedef exists outside of an #ifdef.