Thanks Jiří for this feedback, much appreciated.
To summarize the next steps:
1) I will submit a patch to modify the script sync_qemu_features_i386.py
The change aims to extract correctly the vmx-* features from the MSR
registers (and ONLY those MSR registers, other MSR registers
will be left untouched) that have
special logic (control bits in the 32 lower bits).
2) Once 1) has been approved and merged,
a) Run the script sync_qemu_features_i386.py to generate the x86
feature XML file.
b) Run tests/cputestdata/cpu-data.py diff
tests/cputestdata/x86_64-cpuid-*.json
c) VIR_TEST_REGENERATE_OUTPUT=1 tests/cputest
Submit a patch with the change resulting from the above actions.
What do you think ?
Best
Hector
On Thu, Nov 6, 2025 at 12:13 PM Jiří Denemark <[email protected]> wrote:
> On Wed, Nov 05, 2025 at 14:41:50 +0100, Jiri Denemark wrote:
> > On Sat, May 31, 2025 at 00:30:23 +0200, Hector Cao wrote:
> > > A model specific register (msr) is an 64-bit register.
> > > It can be read with the RDMSR instruction and the
> > > register value is returned via two registers EDX:EAX.
> > > EDX holds the 32 higher bits and EAX holds the 32 lower
> > > bits.
> > >
> > > In the x86_features.xml file, some vmx-* features are
> > > wrongly defined as bits in the EAX register. For example,
> > > for the MSR 0x48B, the feature vmx-apicv-xapic is currently
> > > specified as the first bit of the EAX register but in the
> > > Intel specification [1], this feature is represented by the
> > > first bit of the EDX register (higher 32 bits).
> > >
> > > This is the list of affected msrs that need to be fixed:
> > >
> > > 0x48B : IA32_VMX_PROCBASED_CTLS2
> > > 0x48D : IA32_VMX_TRUE_PINBASED_CTLS
> > > 0x48E : IA32_VMX_TRUE_PROCBASED_CTLS
> > > 0x48F : IA32_VMX_TRUE_EXIT_CTLS
> > > 0x490 : IA32_VMX_TRUE_ENTRY_CTLS
> > >
> > > [1] Appendix A.3
> > > Intel® 64 and IA-32 Architectures Software Developers Manual
> > > Volume 3 (3A, 3B, 3C & 3D): System Programming Guide
> >
> > So technically the values don't matter much as long as they are
> > distinct. They are only used for detecting host CPU capabilities found
> > in the capabilities XML, which is not used by libvirt for anything and
> > our documentation says the host CPU definition in the capabilities XML
> > is only marginally useful. For all other CPU model related things we use
> > feature names, translate the names to bits according to our map, do some
> > bit operations, and translate the bits back to names. In this case the
> > mapping has to be bijective, but the actual values are irrelevant.
> >
> > Nevertheless we should keep the values correct to avoid lying in the
> > capabilities XML more than we have to. Luckily since the values
> > are only used by a single process, we can change the values anytime
> > without any impact on backward compatibility.
> >
> > >
> > > Signed-off-by: Hector Cao <[email protected]>
> > > ---
> > > src/cpu_map/x86_features.xml | 136 +++++++++++++++++------------------
> > > 1 file changed, 68 insertions(+), 68 deletions(-)
> ...
> > Overall the changes look OK, but as you noticed in your reply, you need
> > to fix sync_qemu_features_i386.py script. The script should be changed
> > first in a separate patch and then followed by a patch created by
> > running the modified script. Just beware to only include changes to
> > existing features.
>
> And I completely forgot that after changing the feature definitions you
> should do what sync_qemu_features_i386.py suggests and run
>
> tests/cputestdata/cpu-data.py diff
> tests/cputestdata/x86_64-cpuid-*.json
>
> and if it generates any changes, you also need to regenerate cpu test
> data
>
> VIR_TEST_REGENERATE_OUTPUT=1 tests/cputest
>
> Jirka
>
>
--
Hector CAO
Software Engineer – Server Team / Virtualization
[email protected]
https://launc <https://launchpad.net/~hectorcao>hpad.net/~hectorcao
<https://launchpad.net/~hectorcao>
<https://launchpad.net/~hectorcao>
