| Message ID | b16802f5-13ae-47a0-b12d-604928f4cf7e@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/HVM: misc tidying | expand |
On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote: > ... or we fail to enable the functionality on the BSP for other reasons. > The only place where hardware announcing the feature is recorded is the > raw CPU policy/featureset. > > Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> > > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -2543,6 +2543,7 @@ const struct hvm_function_table * __init > > if ( _svm_cpu_up(true) ) > { > + setup_clear_cpu_cap(X86_FEATURE_SVM); > printk("SVM: failed to initialise.\n"); > return NULL; > } > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void) > > if ( !ret ) > register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); > + else > + { > + setup_clear_cpu_cap(X86_FEATURE_VMX); > + > + /* > + * _vmx_vcpu_up() may have made it past feature identification. > + * Make sure all dependent features are off as well. > + */ > + vmx_basic_msr = 0; > + vmx_pin_based_exec_control = 0; > + vmx_cpu_based_exec_control = 0; > + vmx_secondary_exec_control = 0; > + vmx_vmexit_control = 0; > + vmx_vmentry_control = 0; > + vmx_ept_vpid_cap = 0; > + vmx_vmfunc = 0; Are there really any usages of those variables if VMX is disabled in CPUID? Thanks, Roger.
On 21.11.2023 17:24, Roger Pau Monné wrote: > On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote: >> ... or we fail to enable the functionality on the BSP for other reasons. >> The only place where hardware announcing the feature is recorded is the >> raw CPU policy/featureset. >> >> Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void) >> >> if ( !ret ) >> register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); >> + else >> + { >> + setup_clear_cpu_cap(X86_FEATURE_VMX); >> + >> + /* >> + * _vmx_vcpu_up() may have made it past feature identification. >> + * Make sure all dependent features are off as well. >> + */ >> + vmx_basic_msr = 0; >> + vmx_pin_based_exec_control = 0; >> + vmx_cpu_based_exec_control = 0; >> + vmx_secondary_exec_control = 0; >> + vmx_vmexit_control = 0; >> + vmx_vmentry_control = 0; >> + vmx_ept_vpid_cap = 0; >> + vmx_vmfunc = 0; > > Are there really any usages of those variables if VMX is disabled in > CPUID? I wanted to be on the safe side, as to me the question was "Are there really _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I couldn't easily convince myself of this being the case, seeing how all of vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of arch/x86/hvm/vmx/). Jan
On 21/11/2023 5:27 pm, Jan Beulich wrote: > On 21.11.2023 17:24, Roger Pau Monné wrote: >> On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote: >>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void) >>> >>> if ( !ret ) >>> register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); >>> + else >>> + { >>> + setup_clear_cpu_cap(X86_FEATURE_VMX); >>> + >>> + /* >>> + * _vmx_vcpu_up() may have made it past feature identification. >>> + * Make sure all dependent features are off as well. >>> + */ >>> + vmx_basic_msr = 0; >>> + vmx_pin_based_exec_control = 0; >>> + vmx_cpu_based_exec_control = 0; >>> + vmx_secondary_exec_control = 0; >>> + vmx_vmexit_control = 0; >>> + vmx_vmentry_control = 0; >>> + vmx_ept_vpid_cap = 0; >>> + vmx_vmfunc = 0; >> Are there really any usages of those variables if VMX is disabled in >> CPUID? > I wanted to be on the safe side, as to me the question was "Are there really > _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I > couldn't easily convince myself of this being the case, seeing how all of > vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of > arch/x86/hvm/vmx/). Before you commit, are you sure that VT-d will continue to be happy using IOMMU superpages when the EPT features are cleared like this? That's the only linkage I'm aware of that might cause issues. ~Andrew
On 21.11.2023 18:31, Andrew Cooper wrote: > On 21/11/2023 5:27 pm, Jan Beulich wrote: >> On 21.11.2023 17:24, Roger Pau Monné wrote: >>> On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote: >>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void) >>>> >>>> if ( !ret ) >>>> register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); >>>> + else >>>> + { >>>> + setup_clear_cpu_cap(X86_FEATURE_VMX); >>>> + >>>> + /* >>>> + * _vmx_vcpu_up() may have made it past feature identification. >>>> + * Make sure all dependent features are off as well. >>>> + */ >>>> + vmx_basic_msr = 0; >>>> + vmx_pin_based_exec_control = 0; >>>> + vmx_cpu_based_exec_control = 0; >>>> + vmx_secondary_exec_control = 0; >>>> + vmx_vmexit_control = 0; >>>> + vmx_vmentry_control = 0; >>>> + vmx_ept_vpid_cap = 0; >>>> + vmx_vmfunc = 0; >>> Are there really any usages of those variables if VMX is disabled in >>> CPUID? >> I wanted to be on the safe side, as to me the question was "Are there really >> _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I >> couldn't easily convince myself of this being the case, seeing how all of >> vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of >> arch/x86/hvm/vmx/). > > Before you commit, are you sure that VT-d will continue to be happy > using IOMMU superpages when the EPT features are cleared like this? There aren't going to be HVM guests in the case the clearing above happens. And the only thing that happens when vtd_ept_page_compatible() returns false is that page table sharing is suppressed, which is relevant only for HVM guests. Jan
On Tue, Nov 21, 2023 at 06:27:02PM +0100, Jan Beulich wrote: > On 21.11.2023 17:24, Roger Pau Monné wrote: > > On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote: > >> ... or we fail to enable the functionality on the BSP for other reasons. > >> The only place where hardware announcing the feature is recorded is the > >> raw CPU policy/featureset. > >> > >> Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > >> --- a/xen/arch/x86/hvm/vmx/vmcs.c > >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c > >> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void) > >> > >> if ( !ret ) > >> register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); > >> + else > >> + { > >> + setup_clear_cpu_cap(X86_FEATURE_VMX); > >> + > >> + /* > >> + * _vmx_vcpu_up() may have made it past feature identification. > >> + * Make sure all dependent features are off as well. > >> + */ > >> + vmx_basic_msr = 0; > >> + vmx_pin_based_exec_control = 0; > >> + vmx_cpu_based_exec_control = 0; > >> + vmx_secondary_exec_control = 0; > >> + vmx_vmexit_control = 0; > >> + vmx_vmentry_control = 0; > >> + vmx_ept_vpid_cap = 0; > >> + vmx_vmfunc = 0; > > > > Are there really any usages of those variables if VMX is disabled in > > CPUID? > > I wanted to be on the safe side, as to me the question was "Are there really > _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I > couldn't easily convince myself of this being the case, seeing how all of > vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of > arch/x86/hvm/vmx/). Wouldn't that have exploded already if initialization of _vmx_cpu_up() failed? (regardless of whether the CPUID flag is cleared or not) My main concern is that it's very easy for the variables here getting out of sync with the ones used by vmx_init_vmcs_config(). It might have been nice to place all those fields in an array that we could just zero here without having to account for each individual variable. Thanks, Roger.
On 22.11.2023 09:22, Roger Pau Monné wrote: > On Tue, Nov 21, 2023 at 06:27:02PM +0100, Jan Beulich wrote: >> On 21.11.2023 17:24, Roger Pau Monné wrote: >>> On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote: >>>> ... or we fail to enable the functionality on the BSP for other reasons. >>>> The only place where hardware announcing the feature is recorded is the >>>> raw CPU policy/featureset. >>>> >>>> Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> Acked-by: Roger Pau Monné <roger.pau@citrix.com> >> >> Thanks. >> >>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void) >>>> >>>> if ( !ret ) >>>> register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); >>>> + else >>>> + { >>>> + setup_clear_cpu_cap(X86_FEATURE_VMX); >>>> + >>>> + /* >>>> + * _vmx_vcpu_up() may have made it past feature identification. >>>> + * Make sure all dependent features are off as well. >>>> + */ >>>> + vmx_basic_msr = 0; >>>> + vmx_pin_based_exec_control = 0; >>>> + vmx_cpu_based_exec_control = 0; >>>> + vmx_secondary_exec_control = 0; >>>> + vmx_vmexit_control = 0; >>>> + vmx_vmentry_control = 0; >>>> + vmx_ept_vpid_cap = 0; >>>> + vmx_vmfunc = 0; >>> >>> Are there really any usages of those variables if VMX is disabled in >>> CPUID? >> >> I wanted to be on the safe side, as to me the question was "Are there really >> _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I >> couldn't easily convince myself of this being the case, seeing how all of >> vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of >> arch/x86/hvm/vmx/). > > Wouldn't that have exploded already if initialization of _vmx_cpu_up() > failed? (regardless of whether the CPUID flag is cleared or not) Quite likely, or in other words the clearing added here likely was missing before already. > My main concern is that it's very easy for the variables here getting > out of sync with the ones used by vmx_init_vmcs_config(). > > It might have been nice to place all those fields in an array that we > could just zero here without having to account for each individual > variable. Yeah, that might (have been) better. Indeed I already need to remember to correctly deal with vmx_tertiary_exec_control either here or in the patch introducing it. I guess I should make a follow-on patch converting to a struct and at the same time moving to __ro_after_init. Jan
--- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -2543,6 +2543,7 @@ const struct hvm_function_table * __init if ( _svm_cpu_up(true) ) { + setup_clear_cpu_cap(X86_FEATURE_SVM); printk("SVM: failed to initialise.\n"); return NULL; } --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void) if ( !ret ) register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); + else + { + setup_clear_cpu_cap(X86_FEATURE_VMX); + + /* + * _vmx_vcpu_up() may have made it past feature identification. + * Make sure all dependent features are off as well. + */ + vmx_basic_msr = 0; + vmx_pin_based_exec_control = 0; + vmx_cpu_based_exec_control = 0; + vmx_secondary_exec_control = 0; + vmx_vmexit_control = 0; + vmx_vmentry_control = 0; + vmx_ept_vpid_cap = 0; + vmx_vmfunc = 0; + } return ret; }
... or we fail to enable the functionality on the BSP for other reasons. The only place where hardware announcing the feature is recorded is the raw CPU policy/featureset. Inspired by https://lore.kernel.org/all/20230921114940.957141-1-pbonzini@redhat.com/. Signed-off-by: Jan Beulich <jbeulich@suse.com>