Re: [PATCH 1/4] x86/reboot: Unconditionally define cpu_emergency_virt_cb typedef

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 14/05/2024 10:44 am, Huang, Kai wrote:


On 14/05/2024 4:01 am, Sean Christopherson wrote:
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.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
---
  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:

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

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

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

Yeah.


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 confident 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 for this small series, I prefer to unconditionally define the typedef, as it requires no additional #ifdefs, and there are no meaningful downsides to letting the
typedef exist for all kernel builds.

Seems the final target is to remove those <linux/kvm_host.h> users, or I think a safe-once-for-all solution is to provide the stubs in <linux/kvm_host.h> with:

     #ifdef CONFIG_KVM
     ...
     #else
     #endif

In either way, my concerns is it seems modifying the <asm/reboot.h> is a temporary workaround.  And when we reach the final solution I suppose we will need to revert it back to the current way?

If so, how about manually add a temporary typedef in <asm/kvm_host.h> for now?

     #ifndef CONFIG_KVM
     typedef void (cpu_emergency_virt_cb)(void);
     #endif

Yes it's ugly, but it's KVM self-contained, and can be removed when ready.

Anyway, just my 2 cents.


A second thought:

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.

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






[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux