On Wed, Mar 02, 2022 at 10:22:43PM +0100, Paolo Bonzini wrote: > On 3/2/22 21:51, Oliver Upton wrote: > > On Wed, Mar 02, 2022 at 01:21:23PM +0100, Paolo Bonzini wrote: > > > On 3/1/22 19:43, Oliver Upton wrote: > > > > Right, a 1-setting of '{load,clear} IA32_BNDCFGS' should really be the > > > > responsibility of userspace. My issue is that the commit message in > > > > commit 5f76f6f5ff96 ("KVM: nVMX: Do not expose MPX VMX controls when > > > > guest MPX disabled") suggests that userspace can expect these bits to be > > > > configured based on guest CPUID. Furthermore, before commit aedbaf4f6afd > > > > ("KVM: x86: Extract kvm_update_cpuid_runtime() from > > > > kvm_update_cpuid()"), if userspace clears these bits, KVM will continue > > > > to set them based on CPUID. > > > > > > > > What is the userspace expectation here? If we are saying that changes to > > > > IA32_VMX_TRUE_{ENTRY,EXIT}_CTLS after userspace writes these MSRs is a > > > > bug, then I agree aedbaf4f6afd is in fact a bugfix. But, the commit > > > > message in 5f76f6f5ff96 seems to indicate that userspace wants KVM to > > > > configure these bits based on guest CPUID. > > > > > > Yes, but I think it's reasonable that userspace wants to override them. It > > > has to do that after KVM_SET_CPUID2, but that's okay too. > > > > > > > In that case, I can rework the tests at the end of this series to ensure > > userspace's ability to override w/o a quirk. Sorry for the toil, > > aedbaf4f6afd caused some breakage for us internally, but really is just > > a userspace bug. > > How did vanadium break? Maybe I can redirect you to a test case to highlight a possible regression in KVM, as seen by userspace ;-) from Patch 7/8: > + /* > + * Test that KVM will set these bits regardless of userspace if the > + * guest CPUID exposes a supporting vPMU. > + */ > + test_vmx_control_msr(vm, MSR_IA32_VMX_TRUE_ENTRY_CTLS, > + 0, /* set */ > + VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL, /* clear */ > + VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL, /* exp_set */ > + 0); /* exp_clear */ > + test_vmx_control_msr(vm, MSR_IA32_VMX_TRUE_EXIT_CTLS, > + 0, /* set */ > + VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL, /* clear */ > + VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL, /* exp_set */ > + 0); /* exp_clear */ Same goes for the "{load,clear} IA32_BNDCFGS" bits too. -- Thanks, Oliver