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

Huang, Kai
2024-05-13T22:44:40Z

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@google.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:
>
> 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.

>
> [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:
>>
>> #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>?
>