On Thu, Sep 18, 2025 at 02:13:57AM +0200, Hector Cao wrote:
On recent Intel CPUs, some of the CPU features (e.g. vmx-* sub-features) are listed and controlled via the MSRs (Model Specific Registers) instead of the traditional CPUID instruction method.
To be able to read the MSR's values, the kernel module msr has to be loaded and the values can be read via /dev/cpu/*/msr.
This commit introduces following changes:
- install modules-load.d file for msr for x86 and when qemu driver is enabled. by default, this option is enabled but user can disable it via a build option: meson ... -Dmsr_module_load=false ... build
- modify virt-host-validate to check of /dev/cpu/*/msr is available.
Hint: if you need to list the things that your patch does in the commit message, that's usually a pretty good indication that you should have split it into separate patches instead ;)
+++ b/meson_options.txt @@ -135,3 +135,4 @@ option('sysctl_config', type: 'feature', value: 'auto', description: 'Whether to # dep:sysctl_config option('userfaultfd_sysctl', type: 'feature', value: 'auto', description: 'Whether to install sysctl config for enabling unprivileged userfaultfd') option('tls_priority', type: 'string', value: 'NORMAL', description: 'set the default TLS session priority string') +option('msr_module_load', type: 'boolean', value: true, description: 'Install modules-load.d file for msr module on x86')
I think this should be a feature rather than a boolean, with default value "auto", i.e. "do the right thing".
diff --git a/src/util/modules-load.d/msr.conf b/src/util/modules-load.d/msr.conf new file mode 100644 index 0000000000..3e5ee7fa15 --- /dev/null +++ b/src/util/modules-load.d/msr.conf @@ -0,0 +1 @@ +msr
The name of the installed file should probably be /etc/modules-load.d/libvirt-msr.conf to make its origin self-explanatory to the admin. Relatedly, the name of the source file should probably be src/util/libvirt-msr.modules-load.conf to follow the existing pattern: see for example src/qemu/libvirt-qemu.sysusers.conf src/remote/libvirtd.sysctl src/remote/libvirtd.policy.in and so on.
+++ b/src/util/meson.build @@ -225,3 +225,22 @@ if conf.has('WITH_LIBVIRTD') endif
util_inc_dir = include_directories('.') + +# Install the modules-load.d file to load the MSR module +# NB: For the file to taken into account and the module load to happen, users must trigger +# the modules-load.d reload (by reboot or restart the systemd-modules-load service). +msr_module_load = get_option('msr_module_load') +if (msr_module_load and + get_option('driver_qemu').enabled() and
I don't think this should be conditional on whether the QEMU driver is enabled. The function that needs the kernel module to be loaded is in the generic utility code and it's invoked by the x86 CPU driver, so we should install the file unconditionally.
+ (host_machine.cpu_family() == 'x86' or host_machine.cpu_family() == 'x86_64')) + message('msr_module_load: Enabling installation of modules-load.d/msr.conf for x86') + install_data( + 'modules-load.d/msr.conf', + install_dir: join_paths(sysconfdir, 'modules-load.d') + ) +elif msr_module_load + message('msr_module_load: option enabled, but not applicable : skipping') +else + message('msr_module_load: option disabled: skipping') +endif
We don't really use message() in libvirt so let's not start doing that now. On the other hand, we *do* include information about whether optional files have been installed in the meson summary, so please make sure that happens. See for example how the sysctl_config option is handled. We also usually try to perform fairly strict validation for meson arguments, erroring out when unreasonable things are being requested. That's considered preferrable to silently failing to comply with the user's explicit requests. Putting it all together, this is what the check should look like: if not get_option('msr_module_load').disabled() if host_machine.system() != 'linux' if get_option('msr_module_load').enabled() error('msr module loading can be enabled on Linux only') endif endif if host_machine.cpu_family() not in [ 'x86', 'x86_64' ] if get_option('msr_module_load').enabled() error('msr module loading can be enabled on x86 arches only') endif endif conf.set('WITH_MSR_MODULE_LOAD', 1) endif And the installation part: if conf.has('WITH_MSR_MODULE_LOAD') install_data( 'libvirt-msr.modules-load.conf', install_dir: sysconfdir / 'modules-load.d', rename: 'libvirt-msr.conf', ) endif Note the use of '/' instead of join_paths() and the trailing comma for all lines, including the last one. Again, this follows the established patterns instead of diverging from them.
+++ b/tools/virt-host-validate-qemu.c @@ -95,6 +95,13 @@ int virHostValidateQEMU(void) virValidatePass(); }
+ if (arch == VIR_ARCH_X86_64) { + if (virHostValidateDeviceExists("QEMU", "/dev/cpu/0/msr", + VIR_HOST_VALIDATE_WARN, + _("Load the 'msr' module for better x86 CPU features detection")) < 0) + ret = -1; + }
On the meson side, the file gets installed for both i386 and x86_64, so it seems to me that we should run the check on the /dev node for both architectures here as well. You can use the ARCH_IS_X86(arch) helper. Also it's just VIR_VALIDATE_WARN now. And as hinted at the very start this should be a separate patch. Finally, there's the issue of the spec file not having been updated after adding this file. Currently that results in the following failure when building the RPM: Installed (but unpackaged) file(s) found: /etc/modules-load.d/msr.conf which is quite expected. We could take two roads here. One is to ensure that we explicitly pass -Dmsr_module_load=enabled when building on x86 arches, -Dmsr_module_load=disabled otherwise (including MinGW builds), then make the file installation conditional. The other option is to rely on the fact that Fedora and RHEL have the msr module as built-in and always pass -Dmsr_module_load=disabled, avoiding a bunch of complexity. I'm not 100% sure which one I like better, to be honest. The attached patch implements the former, as well as the other review comments. -- Andrea Bolognani / Red Hat / Virtualization