Received-SPF: pass (google.com: domain of linux-kernel+bounces-177796-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 13 May 2024 09:01:28 -0700
In-Reply-To: <5dfc9eb860a587d1864371874bbf267fa0aa7922.camel@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>
Message-ID: <ZkI5WApAR6iqCgil@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="utf-8"
Content-Transfer-Encoding: quoted-printable

On Mon, May 13, 2024, Kai Huang wrote:
> On Thu, 2024-04-25 at 16:39 -0700, Sean Christopherson wrote:
> > Define cpu_emergency_virt_cb even if the kernel is being built without =
KVM
> > support so that KVM can reference the typedef in asm/kvm_host.h without
> > needing yet more #ifdefs.
> >=20
> > No functional change intended.
> >=20
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/include/asm/reboot.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >=20
> > diff --git a/arch/x86/include/asm/reboot.h b/arch/x86/include/asm/reboo=
t.h
> > index 6536873f8fc0..d0ef2a678d66 100644
> > --- a/arch/x86/include/asm/reboot.h
> > +++ b/arch/x86/include/asm/reboot.h
> > @@ -25,8 +25,8 @@ void __noreturn machine_real_restart(unsigned int typ=
e);
> >  #define MRR_BIOS	0
> >  #define MRR_APM		1
> > =20
> > -#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
> >  typedef void (cpu_emergency_virt_cb)(void);
> > +#if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
> >  void cpu_emergency_register_virt_callback(cpu_emergency_virt_cb *callb=
ack);
> >  void cpu_emergency_unregister_virt_callback(cpu_emergency_virt_cb *cal=
lback);
> >  void cpu_emergency_disable_virtualization(void);
>=20
> It looks a little it weird.  If other file wants to include
> <asm/kvm_host.h> (directly or via <linux/kvm_host.h>) unconditionally the=
n
> in general I think <asm/kvm_host.h> or <linux/kvm_host.h> should
> have=C2=A0something like:
>=20
> 	#ifdef CONFIG_KVM
>=20
> 	void func(void);
> 	...
>=20
> 	#else
>=20
> 	static inline void func(void) {}
>=20
> 	#endif
>=20
> But it seems neither <asm/kvm_host.h> nor <linux/kvm_host.h> has this
> pattern.
>=20
> I tried to build with !CONFIG_KVM with patch 2 in this series, and I got
> below error:

Well, yeah.

> In file included from ./include/linux/kvm_host.h:45,
>                  from arch/x86/events/intel/core.c:17:
> ./arch/x86/include/asm/kvm_host.h:1617:9: error: unknown type name
> =E2=80=98cpu_emergency_virt_cb=E2=80=99
>  1617 |         cpu_emergency_virt_cb *emergency_disable;
>       |         ^~~~~~~~~~~~~~~~~~~~~
>=20
>=20
> Looking at the code, it seems it is because intel_guest_get_msrs() needs
> 'struct kvm_pmu' (e.g., it accesses the members of 'struct kvm_pmu').  Bu=
t
> it doesn't look the relevant code should be compiled when !CONFIG_KVM.=C2=
=A0
>=20
> So looks a better way is to explicitly use #ifdef CONFIG_KVM around the
> relevant code in the arch/x86/events/intel/core.c?

Eh, there's no right or wrong way to handle code that is conditionally comp=
iled.
There are always tradeoffs and pros/cons, e.g. the number of #ifdefs, the a=
mount
of effective code validation for all configs, readability, etc.

E.g. if there is only one user of a function that conditionally exists, the=
n
having the caller handle the situation might be cleaner.  But if there are
multiple callers, then providing a stub is usually preferable.

IMO, the real problem is that perf pokes into KVM _at all_.  Same for VFIO.
The perf usage is especially egregious, as there is zero reason perf should=
 need
KVM internals[1].  VFIO requires a bit more effort, but I'm fairly confiden=
t that
Jason's file-based approach[2] will yield clean, robust code that minimizes=
 the
number of #ifdefs required.

I'm planning/hoping to get back to that series in the next few weeks.  As f=
or
this small series, I prefer to unconditionally define the typedef, as it re=
quires
no additional #ifdefs, and there are no meaningful downsides to letting the
typedef exist for all kernel builds.

[1] https://lore.kernel.org/all/20230916003118.2540661-21-seanjc@google.com
[2] https://lore.kernel.org/all/ZXkVSKULLivrMkBl@google.com

> And it seems vfio does it in vfio_main.c:
>=20
> 	#if IS_ENABLED(CONFIG_KVM)
> 	#include <linux/kvm_host.h>
> 	#endif
>=20
> 	#if IS_ENABLED(CONFIG_KVM)
> 	void vfio_device_get_kvm_safe(struct vfio_device *device,=C2=A0
> 			struct kvm *kvm)
> 	{
> 		...
> 	}
> 	...
> 	#endif
>=20
>=20
> The only remaining weird thing is 'struct kvm *kvm' is still used
> unconditionally in vfio_main.c, but I think the reason it builds fine wit=
h
> !CONFIG_KVM is because <linux/vfio.h> declares it explicitly:
>=20
> 	struct kvm;
> 	struct iommufd_ctx;
> 	...
>=20
> So it seems to me that this patch around 'cpu_emergency_virt_cb' is more
> like a workaround of existing non-perfect <linux/kvm_host.h> and/or
> <asm/kvm_host.h>?