linux-kernel - Re: [GIT PULL] KVM/x86 changes for Linux 6.12

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] [day] [month] [year] [list]

Message-ID: <34b68d1e-7982-48e0-8d8a-5d3e0737ab42@redhat.com>
Date: Mon, 30 Sep 2024 19:17:50 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
 Kai Huang <kai.huang@...el.com>, Chao Gao <chao.gao@...el.com>,
 Farrah Chen <farrah.chen@...el.com>, linux-kernel@...r.kernel.org,
 kvm@...r.kernel.org
Subject: Re: [GIT PULL] KVM/x86 changes for Linux 6.12

On 9/30/24 18:53, Sean Christopherson wrote:
> On Mon, Sep 30, 2024, Paolo Bonzini wrote:
>> On Sun, Sep 29, 2024 at 7:36 PM Linus Torvalds <torvalds@...ux-foundation.org> wrote:
>>> The culprit is commit 590b09b1d88e ("KVM: x86: Register "emergency
>>> disable" callbacks when virt is enabled"), and the reason seems to be
>>> this:
>>>
>>>    #if IS_ENABLED(CONFIG_KVM_INTEL) || IS_ENABLED(CONFIG_KVM_AMD)
>>>    void cpu_emergency_register_virt_callback(cpu_emergency_virt_cb *callback);
>>>    ...
>>>
>>> ie if you have a config with KVM enabled, but neither KVM_INTEL nor
>>> KVM_AMD set, you don't get that callback thing.
>>>
>>> The fix may be something like the attached.
>>
>> Yeah, there was an attempt in commit 6d55a94222db ("x86/reboot:
>> Unconditionally define cpu_emergency_virt_cb typedef") but that only
>> covers the headers and the !CONFIG_KVM case; not the !CONFIG_KVM_INTEL
>> && !CONFIG_KVM_AMD one that you stumbled upon.
>>
>> Your fix is not wrong, but there's no point in compiling kvm.ko if
>> nobody is using it.
>>
>> This is what I'll test more and submit:
>>
>> ------------------ 8< ------------------
>> From: Paolo Bonzini <pbonzini@...hat.com>
>> Subject: [PATCH] KVM: x86: leave kvm.ko out of the build if no vendor module is requested
>> kvm.ko is nothing but library code shared by kvm-intel.ko and kvm-amd.ko.
>> It provides no functionality on its own and it is unnecessary unless one
>> of the vendor-specific module is compiled.  In particular, /dev/kvm is
>> not created until one of kvm-intel.ko or kvm-amd.ko is loaded.
>> Use CONFIG_KVM to decide if it is built-in or a module, but use the
>> vendor-specific modules for the actual decision on whether to build it.
>> This also fixes a build failure when CONFIG_KVM_INTEL and CONFIG_KVM_AMD
>> are both disabled.  The cpu_emergency_register_virt_callback() function
>> is called from kvm.ko, but it is only defined if at least one of
>> CONFIG_KVM_INTEL and CONFIG_KVM_AMD is provided.
>>
>> Reported-by: Linus Torvalds <torvalds@...ux-foundation.org>
>> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
>>
>> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
>> index 4287a8071a3a..aee054a91031 100644
>> --- a/arch/x86/kvm/Kconfig
>> +++ b/arch/x86/kvm/Kconfig
>> @@ -17,8 +17,8 @@ menuconfig VIRTUALIZATION
>>   if VIRTUALIZATION
>> -config KVM
>> -	tristate "Kernel-based Virtual Machine (KVM) support"
>> +config KVM_X86_COMMON
>> +	def_tristate KVM if KVM_INTEL || KVM_AMD
>>   	depends on HIGH_RES_TIMERS
>>   	depends on X86_LOCAL_APIC
>>   	select KVM_COMMON
>> @@ -46,6 +47,9 @@ config KVM
>>   	select KVM_GENERIC_HARDWARE_ENABLING
>>   	select KVM_GENERIC_PRE_FAULT_MEMORY
>>   	select KVM_WERROR if WERROR
>> +
>> +config KVM
>> +	tristate "Kernel-based Virtual Machine (KVM) support"
> 
> I like the idea, but allowing users to select KVM=m|y but not building any parts
> of KVM seems like it will lead to confusion.  What if we hide KVM entirely, and
> autoselect m/y/n based on the vendor modules?  AFAICT, this behaves as expected.

Showing the KVM option is useful anyway as a grouping for other options 
(e.g. SW-protected VMs, Xen, etc.).  I can play with reordering 
everything and using "select" to group these options, but I doubt it
will be better/more user-friendly than the above minimal change.

And also...

> Not having documentation for kvm.ko is unfortunate, but explaining how kvm.ko
> interacts with kvm-{amd,intel}.ko probably belongs in Documentation/virt/kvm/?
> anyways.

... documentation changes can wait for 6.13 anyway, unlike fixing
the build (even if in a rare config that would mostly be hit by
randconfig).

> If you haven't already, can you also kill off KVM_COMMON?  The only usage is in
> scripts/gdb/linux/constants.py.in, to print Intel's posted interrupt IRQs in
> scripts/gdb/linux/interrupts.py, which is quite ridiculous.

Sure.

Paolo