linux-kernel - Re: [PATCH 1/4] x86/reboot: Unconditionally define cpu_emergency_virt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5dfc9eb860a587d1864371874bbf267fa0aa7922.camel@intel.com>
Date: Mon, 13 May 2024 12:50:45 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "pbonzini@...hat.com" <pbonzini@...hat.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] x86/reboot: Unconditionally define
 cpu_emergency_virt_cb typedef

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.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/include/asm/reboot.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/reboot.h b/arch/x86/include/asm/reboot.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 type);
>  #define MRR_BIOS	0
>  #define MRR_APM		1
>  
> -#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 *callback);
>  void cpu_emergency_unregister_virt_callback(cpu_emergency_virt_cb *callback);
>  void cpu_emergency_disable_virtualization(void);

It looks a little it weird.  If other file wants to include
<asm/kvm_host.h> (directly or via <linux/kvm_host.h>) unconditionally then
in general I think <asm/kvm_host.h> or <linux/kvm_host.h> should
have something like:

	#ifdef CONFIG_KVM

	void func(void);
	...

	#else

	static inline void func(void) {}

	#endif

But it seems neither <asm/kvm_host.h> nor <linux/kvm_host.h> has this
pattern.

I tried to build with !CONFIG_KVM with patch 2 in this series, and I got
below error:

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
‘cpu_emergency_virt_cb’
 1617 |         cpu_emergency_virt_cb *emergency_disable;
      |         ^~~~~~~~~~~~~~~~~~~~~


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').  But
it doesn't look the relevant code should be compiled when !CONFIG_KVM. 

So looks a better way is to explicitly use #ifdef CONFIG_KVM around the
relevant code in the arch/x86/events/intel/core.c?

And it seems vfio does it in vfio_main.c:

	#if IS_ENABLED(CONFIG_KVM)
	#include <linux/kvm_host.h>
	#endif

	#if IS_ENABLED(CONFIG_KVM)
	void vfio_device_get_kvm_safe(struct vfio_device *device, 
			struct kvm *kvm)
	{
		...
	}
	...
	#endif


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 with
!CONFIG_KVM is because <linux/vfio.h> declares it explicitly:

	struct kvm;
	struct iommufd_ctx;
	...

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