Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>, kvm@vger.kernel.org
Cc:     Anirudh Rayabharam <anrayabh@linux.microsoft.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>
Subject: Re: [PATCH v3 11/25] KVM: VMX: Get rid of eVMCS specific VMX
 controls sanitization
In-Reply-To: <f1d030d7db4aaf3075fe625799b99ae335fc9f60.camel@redhat.com>
References: <20220708144223.610080-1-vkuznets@redhat.com>
 <20220708144223.610080-12-vkuznets@redhat.com>
 <f1d030d7db4aaf3075fe625799b99ae335fc9f60.camel@redhat.com>
Date:   Tue, 12 Jul 2022 14:14:06 +0200
Message-ID: <877d4iplyp.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Fri, 2022-07-08 at 16:42 +0200, Vitaly Kuznetsov wrote:
>> With the updated eVMCSv1 definition, there's no known 'problematic'
>> controls which are exposed in VMX control MSRs but are not present in
>> eVMCSv1. Get rid of VMX control MSRs filtering for KVM on Hyper-V.
>
> If I understand correctly we are taking about running KVM as a nested gue=
st of Hyper-V here:
>
> Don't we need to check the new CPUID bit and only then use the new fields=
 of eVMCS,
> aka check that the 'cpu' supports the updated eVMCS version?
>

I've checked various Hyper-V versions available around and it seems
there's no need for that: these new features are exposed in VMX control
MSRs only when the updated eVMCS is supported.

We can, in theory, preserve the filtering for non-updated eVMCS verison
but I'd vote for putting a WARN_ON() or something around: we can
eventually get rid of it in case we don't get any reports.

> Best regards,
> 	Maxim Levitsky
>
>
>
>>=20
>> Note: VMX control MSRs filtering for Hyper-V on KVM
>> (nested_evmcs_filter_control_msr()) stays as even the updated eVMCSv1
>> definition doesn't have all the features implemented by KVM and some
>> fields are still missing. Moreover, nested_evmcs_filter_control_msr()
>> has to support the original eVMCSv1 version when VMM wishes so.
>>=20
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> =C2=A0arch/x86/kvm/vmx/evmcs.c | 13 -------------
>> =C2=A0arch/x86/kvm/vmx/evmcs.h |=C2=A0 1 -
>> =C2=A0arch/x86/kvm/vmx/vmx.c=C2=A0=C2=A0 |=C2=A0 5 -----
>> =C2=A03 files changed, 19 deletions(-)
>>=20
>> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
>> index 52a53debd806..b5cfbf7d487b 100644
>> --- a/arch/x86/kvm/vmx/evmcs.c
>> +++ b/arch/x86/kvm/vmx/evmcs.c
>> @@ -320,19 +320,6 @@ const struct evmcs_field vmcs_field_to_evmcs_1[] =
=3D {
>> =C2=A0};
>> =C2=A0const unsigned int nr_evmcs_1_fields =3D ARRAY_SIZE(vmcs_field_to_=
evmcs_1);
>> =C2=A0
>> -#if IS_ENABLED(CONFIG_HYPERV)
>> -__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf)
>> -{
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmcs_conf->cpu_based_exec_ctr=
l &=3D ~EVMCS1_UNSUPPORTED_EXEC_CTRL;
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmcs_conf->pin_based_exec_ctr=
l &=3D ~EVMCS1_UNSUPPORTED_PINCTRL;
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmcs_conf->cpu_based_2nd_exec=
_ctrl &=3D ~EVMCS1_UNSUPPORTED_2NDEXEC;
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmcs_conf->cpu_based_3rd_exec=
_ctrl =3D 0;
>> -
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmcs_conf->vmexit_ctrl &=3D ~=
EVMCS1_UNSUPPORTED_VMEXIT_CTRL;
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmcs_conf->vmentry_ctrl &=3D =
~EVMCS1_UNSUPPORTED_VMENTRY_CTRL;
>> -}
>> -#endif
>> -
>> =C2=A0bool nested_enlightened_vmentry(struct kvm_vcpu *vcpu, u64 *evmcs_=
gpa)
>> =C2=A0{
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct hv_vp_assist_page=
 assist_page;
>> diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
>> index 4b809c79ae63..0feac101cce4 100644
>> --- a/arch/x86/kvm/vmx/evmcs.h
>> +++ b/arch/x86/kvm/vmx/evmcs.h
>> @@ -203,7 +203,6 @@ static inline void evmcs_load(u64 phys_addr)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vp_ap->enlighten_vmentry=
 =3D 1;
>> =C2=A0}
>> =C2=A0
>> -__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf);
>> =C2=A0#else /* !IS_ENABLED(CONFIG_HYPERV) */
>> =C2=A0static __always_inline void evmcs_write64(unsigned long field, u64=
 value) {}
>> =C2=A0static inline void evmcs_write32(unsigned long field, u32 value) {}
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index b4915d841357..dd905ad72637 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -2689,11 +2689,6 @@ static __init int setup_vmcs_config(struct vmcs_c=
onfig *vmcs_conf,
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmcs_conf->vmexit_ctrl=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D _vmexit_control;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmcs_conf->vmentry_ctrl=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D _vmentry_control;
>> =C2=A0
>> -#if IS_ENABLED(CONFIG_HYPERV)
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (enlightened_vmcs)
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0evmcs_sanitize_exec_ctrls(vmcs_conf);
>> -#endif
>> -
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0;
>> =C2=A0}
>> =C2=A0
>
>

--=20
Vitaly