Hi Paolo,
On Tue, Mar 01, 2022 at 07:00:57PM +0100, Paolo Bonzini wrote:
> On 3/1/22 07:03, Oliver Upton wrote:
> > +
> > + /*
> > + * Ensure KVM fiddling with these MSRs is preserved after userspace
> > + * write.
> > + */
> > + if (msr_index == MSR_IA32_VMX_TRUE_ENTRY_CTLS ||
> > + msr_index == MSR_IA32_VMX_TRUE_EXIT_CTLS)
> > + nested_vmx_entry_exit_ctls_update(&vmx->vcpu);
> > +
>
> I still don't understand this patch. You say:
>
> > Now, the BNDCFGS bits are only ever
> > updated after a KVM_SET_CPUID/KVM_SET_CPUID2 ioctl, meaning that a
> > subsequent MSR write from userspace will clobber these values.
>
> but I don't understand what's wrong with that. If you can (if so inclined)
> define a VM without LOAD_BNDCFGS or CLEAR_BNDCFGS even if MPX enabled,
> commit aedbaf4f6afd counts as a bugfix.
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.
Given that there were previous userspace expectations, I attempted to
restore the old behavior of KVM (ignore userspace writes) and add a
quirk to fully back out of the mess. All this logic also applies to
Patch 2 as well.
--
Oliver
