| Message ID | 20220708144223.610080-23-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Support updated eVMCSv1 revision + use vmcs_config for L1 VMX MSRs | expand |
On Fri, 2022-07-08 at 16:42 +0200, Vitaly Kuznetsov wrote: > Similar to exit_ctls_low, entry_ctls_low, procbased_ctls_low, > pinbased_ctls_low should be set to PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR > and not host's MSR_IA32_VMX_PINBASED_CTLS value |= > PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR. > > The commit eabeaaccfca0 ("KVM: nVMX: Clean up and fix pin-based > execution controls") which introduced '|=' doesn't mention anything > about why this is needed, the change seems rather accidental. > > Note: normally, required-1 portion of MSR_IA32_VMX_PINBASED_CTLS should > be equal to PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR so no behavioral change > is expected, however, it is (in theory) possible to observe something > different there when e.g. KVM is running as a nested hypervisor. Hope > this doesn't happen in practice. > > Reported-by: Jim Mattson <jmattson@google.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/vmx/nested.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index dcf3ee645212..09654d5c2144 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -6586,7 +6586,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps) > rdmsr(MSR_IA32_VMX_PINBASED_CTLS, > msrs->pinbased_ctls_low, > msrs->pinbased_ctls_high); > - msrs->pinbased_ctls_low |= > + msrs->pinbased_ctls_low = > PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR; > msrs->pinbased_ctls_high &= > PIN_BASED_EXT_INTR_MASK | Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index dcf3ee645212..09654d5c2144 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -6586,7 +6586,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps) rdmsr(MSR_IA32_VMX_PINBASED_CTLS, msrs->pinbased_ctls_low, msrs->pinbased_ctls_high); - msrs->pinbased_ctls_low |= + msrs->pinbased_ctls_low = PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR; msrs->pinbased_ctls_high &= PIN_BASED_EXT_INTR_MASK |
Similar to exit_ctls_low, entry_ctls_low, procbased_ctls_low, pinbased_ctls_low should be set to PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR and not host's MSR_IA32_VMX_PINBASED_CTLS value |= PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR. The commit eabeaaccfca0 ("KVM: nVMX: Clean up and fix pin-based execution controls") which introduced '|=' doesn't mention anything about why this is needed, the change seems rather accidental. Note: normally, required-1 portion of MSR_IA32_VMX_PINBASED_CTLS should be equal to PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR so no behavioral change is expected, however, it is (in theory) possible to observe something different there when e.g. KVM is running as a nested hypervisor. Hope this doesn't happen in practice. Reported-by: Jim Mattson <jmattson@google.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx/nested.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)