Re: [PATCH v2 1/7] KVM: nVMX: Keep KVM updates to BNDCFGS ctrl bits across MSR write

Paolo Bonzini
2022-02-07T17:31:06Z

On 2/4/22 21:46, Oliver Upton wrote:
> Since commit 5f76f6f5ff96 ("KVM: nVMX: Do not expose MPX VMX controls
> when guest MPX disabled"), KVM has taken ownership of the "load
> IA32_BNDCFGS" and "clear IA32_BNDCFGS" VMX entry/exit controls. The ABI
> is that these bits must be set in the IA32_VMX_TRUE_{ENTRY,EXIT}_CTLS
> MSRs if the guest's CPUID supports MPX, and clear otherwise.
>
> However, KVM will only do so if userspace sets the CPUID before writing
> to the corresponding MSRs. Of course, there are no ordering requirements
> between these ioctls. Uphold the ABI regardless of ordering by
> reapplying KVMs tweaks to the VMX control MSRs after userspace has
> written to them.

I don't understand this patch. If you first write the CPUID and then
the MSR, the consistency is upheld by these checks:

if (!is_bitwise_subset(data, supported, GENMASK_ULL(31, 0)))
return -EINVAL;

if (!is_bitwise_subset(supported, data, GENMASK_ULL(63, 32)))
return -EINVAL;

If you're fixing a case where userspace first writes the MSR and then
sets CPUID, then I would expect that KVM_SET_CPUID2 redoes those checks
and, if they fail, it adjusts the MSRs.

The selftests confirm that I'm a bit confused, but in general
vmx_restore_control_msr is not the place where I was expecting the change.

Paolo

> Fixes: 5f76f6f5ff96 ("KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled")
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
> arch/x86/kvm/vmx/nested.c | 9 +++++++++
> arch/x86/kvm/vmx/vmx.c | 2 +-
> arch/x86/kvm/vmx/vmx.h | 2 ++
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ba34e94049c7..59164394569f 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1291,6 +1291,15 @@ vmx_restore_control_msr(struct vcpu_vmx *vmx, u32 msr_index, u64 data)
>
> *lowp = data;
> *highp = data >> 32;
> +
> + /*
> + * 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);
> +
> return 0;
> }
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index aca3ae2a02f3..d63d6dfbadbf 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7227,7 +7227,7 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
> #undef cr4_fixed1_update
> }
>
> -static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
> +void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 7f2c82e7f38f..e134e2763502 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -423,6 +423,8 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
>
> void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
>
> +void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu);
> +
> /*
> * Note, early Intel manuals have the write-low and read-high bitmap offsets
> * the wrong way round. The bitmaps control MSRs 0x00000000-0x00001fff and