| Message ID | 09f1336974c8fd2f788fe8e1d3ca5fee91da5a81.1715761386.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: make cpu virtualization support configurable | expand |
On Wed, 15 May 2024, Sergiy Kibrik wrote: > As we now have SVM/VMX config options for enabling/disabling these features > completely in the build, it may be feasible to add build-time checks to > cpu_has_{svm,vmx} macros. These are used extensively thoughout HVM code, so > we won't have to add extra #ifdef-s to check whether svm/vmx has been enabled, > while DCE cleans up calls to vmx/svm functions, if their code not being built. > > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > CC: Jan Beulich <jbeulich@suse.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 15.05.2024 11:12, Sergiy Kibrik wrote: > --- a/xen/arch/x86/include/asm/cpufeature.h > +++ b/xen/arch/x86/include/asm/cpufeature.h > @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat) > #define cpu_has_sse3 boot_cpu_has(X86_FEATURE_SSE3) > #define cpu_has_pclmulqdq boot_cpu_has(X86_FEATURE_PCLMULQDQ) > #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR) > -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX) > +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \ > + boot_cpu_has(X86_FEATURE_VMX)) > #define cpu_has_eist boot_cpu_has(X86_FEATURE_EIST) > #define cpu_has_ssse3 boot_cpu_has(X86_FEATURE_SSSE3) > #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA) > @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat) > > /* CPUID level 0x80000001.ecx */ > #define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) > -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM) > +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \ > + boot_cpu_has(X86_FEATURE_SVM)) > #define cpu_has_sse4a boot_cpu_has(X86_FEATURE_SSE4A) > #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP) > #define cpu_has_skinit boot_cpu_has(X86_FEATURE_SKINIT) Hmm, leaving aside the style issue (stray blanks after opening parentheses, and as a result one-off indentation on the wrapped lines) I'm not really certain we can do this. The description goes into detail why we would want this, but it doesn't cover at all why it is safe for all present (and ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks just to derive further knowledge from that, without them being directly related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(), for example. While it looks to be okay there, it may give you an idea of what I mean. Things might become better separated if instead for such checks we used host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But that's still pretty far out, I'm afraid. Jan
16.05.24 14:12, Jan Beulich: > On 15.05.2024 11:12, Sergiy Kibrik wrote: >> --- a/xen/arch/x86/include/asm/cpufeature.h >> +++ b/xen/arch/x86/include/asm/cpufeature.h >> @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat) >> #define cpu_has_sse3 boot_cpu_has(X86_FEATURE_SSE3) >> #define cpu_has_pclmulqdq boot_cpu_has(X86_FEATURE_PCLMULQDQ) >> #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR) >> -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX) >> +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \ >> + boot_cpu_has(X86_FEATURE_VMX)) >> #define cpu_has_eist boot_cpu_has(X86_FEATURE_EIST) >> #define cpu_has_ssse3 boot_cpu_has(X86_FEATURE_SSSE3) >> #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA) >> @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat) >> >> /* CPUID level 0x80000001.ecx */ >> #define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) >> -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM) >> +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \ >> + boot_cpu_has(X86_FEATURE_SVM)) >> #define cpu_has_sse4a boot_cpu_has(X86_FEATURE_SSE4A) >> #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP) >> #define cpu_has_skinit boot_cpu_has(X86_FEATURE_SKINIT) > > Hmm, leaving aside the style issue (stray blanks after opening parentheses, > and as a result one-off indentation on the wrapped lines) I'm not really > certain we can do this. The description goes into detail why we would want > this, but it doesn't cover at all why it is safe for all present (and > ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks > just to derive further knowledge from that, without them being directly > related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(), > for example. While it looks to be okay there, it may give you an idea of > what I mean. > > Things might become better separated if instead for such checks we used > host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But > that's still pretty far out, I'm afraid. > I've followed a suggestion you made for patch in previous series: https://lore.kernel.org/xen-devel/8fbd604e-5e5d-410c-880f-2ad257bbe08a@suse.com/ yet if this approach can potentially be unsafe (I'm not completely sure it's safe), should we instead fallback to the way it was done in v1 series? I.e. guard calls to vmx/svm-specific calls where needed, like in these 3 patches: 1) https://lore.kernel.org/xen-devel/20240416063328.3469386-1-Sergiy_Kibrik@epam.com/ 2) https://lore.kernel.org/xen-devel/20240416063740.3469592-1-Sergiy_Kibrik@epam.com/ 3) https://lore.kernel.org/xen-devel/20240416063947.3469718-1-Sergiy_Kibrik@epam.com/ -Sergiy
On 23.05.2024 15:07, Sergiy Kibrik wrote: > 16.05.24 14:12, Jan Beulich: >> On 15.05.2024 11:12, Sergiy Kibrik wrote: >>> --- a/xen/arch/x86/include/asm/cpufeature.h >>> +++ b/xen/arch/x86/include/asm/cpufeature.h >>> @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat) >>> #define cpu_has_sse3 boot_cpu_has(X86_FEATURE_SSE3) >>> #define cpu_has_pclmulqdq boot_cpu_has(X86_FEATURE_PCLMULQDQ) >>> #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR) >>> -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX) >>> +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \ >>> + boot_cpu_has(X86_FEATURE_VMX)) >>> #define cpu_has_eist boot_cpu_has(X86_FEATURE_EIST) >>> #define cpu_has_ssse3 boot_cpu_has(X86_FEATURE_SSSE3) >>> #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA) >>> @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat) >>> >>> /* CPUID level 0x80000001.ecx */ >>> #define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) >>> -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM) >>> +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \ >>> + boot_cpu_has(X86_FEATURE_SVM)) >>> #define cpu_has_sse4a boot_cpu_has(X86_FEATURE_SSE4A) >>> #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP) >>> #define cpu_has_skinit boot_cpu_has(X86_FEATURE_SKINIT) >> >> Hmm, leaving aside the style issue (stray blanks after opening parentheses, >> and as a result one-off indentation on the wrapped lines) I'm not really >> certain we can do this. The description goes into detail why we would want >> this, but it doesn't cover at all why it is safe for all present (and >> ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks >> just to derive further knowledge from that, without them being directly >> related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(), >> for example. While it looks to be okay there, it may give you an idea of >> what I mean. >> >> Things might become better separated if instead for such checks we used >> host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But >> that's still pretty far out, I'm afraid. > > I've followed a suggestion you made for patch in previous series: > > https://lore.kernel.org/xen-devel/8fbd604e-5e5d-410c-880f-2ad257bbe08a@suse.com/ See the "If not, ..." that I had put there. Doing the change just mechanically isn't enough, you also need to make clear (in the description) that you verified it's safe to have this way. > yet if this approach can potentially be unsafe (I'm not completely sure > it's safe), should we instead fallback to the way it was done in v1 > series? I.e. guard calls to vmx/svm-specific calls where needed, like in > these 3 patches: > > 1) > https://lore.kernel.org/xen-devel/20240416063328.3469386-1-Sergiy_Kibrik@epam.com/ > > 2) > https://lore.kernel.org/xen-devel/20240416063740.3469592-1-Sergiy_Kibrik@epam.com/ > > 3) > https://lore.kernel.org/xen-devel/20240416063947.3469718-1-Sergiy_Kibrik@epam.com/ I don't like this sprinkling around of IS_ENABLED() very much. Maybe we want to have two new helpers (say using_svm() and using_vmx()), to be used in place of most but possibly not all cpu_has_{svm,vmx}? Doing such a transformation would then kind of implicitly answer the safety question above, as at every use site you'd need to judge whether the replacement is correct. If it's correct everywhere, the construct(s) as proposed in this version could then be considered to be used in this very shape (instead of introducing the two new helpers). But of course the transition could also be done gradually then, touching only those uses that previously you touched in 1), 2), and 3). Jan
On Thu, 23 May 2024, Jan Beulich wrote: > On 23.05.2024 15:07, Sergiy Kibrik wrote: > > 16.05.24 14:12, Jan Beulich: > >> On 15.05.2024 11:12, Sergiy Kibrik wrote: > >>> --- a/xen/arch/x86/include/asm/cpufeature.h > >>> +++ b/xen/arch/x86/include/asm/cpufeature.h > >>> @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat) > >>> #define cpu_has_sse3 boot_cpu_has(X86_FEATURE_SSE3) > >>> #define cpu_has_pclmulqdq boot_cpu_has(X86_FEATURE_PCLMULQDQ) > >>> #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR) > >>> -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX) > >>> +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \ > >>> + boot_cpu_has(X86_FEATURE_VMX)) > >>> #define cpu_has_eist boot_cpu_has(X86_FEATURE_EIST) > >>> #define cpu_has_ssse3 boot_cpu_has(X86_FEATURE_SSSE3) > >>> #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA) > >>> @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat) > >>> > >>> /* CPUID level 0x80000001.ecx */ > >>> #define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) > >>> -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM) > >>> +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \ > >>> + boot_cpu_has(X86_FEATURE_SVM)) > >>> #define cpu_has_sse4a boot_cpu_has(X86_FEATURE_SSE4A) > >>> #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP) > >>> #define cpu_has_skinit boot_cpu_has(X86_FEATURE_SKINIT) > >> > >> Hmm, leaving aside the style issue (stray blanks after opening parentheses, > >> and as a result one-off indentation on the wrapped lines) I'm not really > >> certain we can do this. The description goes into detail why we would want > >> this, but it doesn't cover at all why it is safe for all present (and > >> ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks > >> just to derive further knowledge from that, without them being directly > >> related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(), > >> for example. While it looks to be okay there, it may give you an idea of > >> what I mean. > >> > >> Things might become better separated if instead for such checks we used > >> host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But > >> that's still pretty far out, I'm afraid. > > > > I've followed a suggestion you made for patch in previous series: > > > > https://lore.kernel.org/xen-devel/8fbd604e-5e5d-410c-880f-2ad257bbe08a@suse.com/ > > See the "If not, ..." that I had put there. Doing the change just mechanically > isn't enough, you also need to make clear (in the description) that you > verified it's safe to have this way. What does it mean to "verified it's safe to have this way"? "Safe" in what way? > > yet if this approach can potentially be unsafe (I'm not completely sure > > it's safe), should we instead fallback to the way it was done in v1 > > series? I.e. guard calls to vmx/svm-specific calls where needed, like in > > these 3 patches: > > > > 1) > > https://lore.kernel.org/xen-devel/20240416063328.3469386-1-Sergiy_Kibrik@epam.com/ > > > > 2) > > https://lore.kernel.org/xen-devel/20240416063740.3469592-1-Sergiy_Kibrik@epam.com/ > > > > 3) > > https://lore.kernel.org/xen-devel/20240416063947.3469718-1-Sergiy_Kibrik@epam.com/ > > I don't like this sprinkling around of IS_ENABLED() very much. Maybe we want > to have two new helpers (say using_svm() and using_vmx()), to be used in place > of most but possibly not all cpu_has_{svm,vmx}? Doing such a transformation > would then kind of implicitly answer the safety question above, as at every > use site you'd need to judge whether the replacement is correct. If it's > correct everywhere, the construct(s) as proposed in this version could then be > considered to be used in this very shape (instead of introducing the two new > helpers). But of course the transition could also be done gradually then, > touching only those uses that previously you touched in 1), 2), and 3).
On 24.05.2024 01:36, Stefano Stabellini wrote: > On Thu, 23 May 2024, Jan Beulich wrote: >> On 23.05.2024 15:07, Sergiy Kibrik wrote: >>> 16.05.24 14:12, Jan Beulich: >>>> On 15.05.2024 11:12, Sergiy Kibrik wrote: >>>>> --- a/xen/arch/x86/include/asm/cpufeature.h >>>>> +++ b/xen/arch/x86/include/asm/cpufeature.h >>>>> @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat) >>>>> #define cpu_has_sse3 boot_cpu_has(X86_FEATURE_SSE3) >>>>> #define cpu_has_pclmulqdq boot_cpu_has(X86_FEATURE_PCLMULQDQ) >>>>> #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR) >>>>> -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX) >>>>> +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \ >>>>> + boot_cpu_has(X86_FEATURE_VMX)) >>>>> #define cpu_has_eist boot_cpu_has(X86_FEATURE_EIST) >>>>> #define cpu_has_ssse3 boot_cpu_has(X86_FEATURE_SSSE3) >>>>> #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA) >>>>> @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat) >>>>> >>>>> /* CPUID level 0x80000001.ecx */ >>>>> #define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) >>>>> -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM) >>>>> +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \ >>>>> + boot_cpu_has(X86_FEATURE_SVM)) >>>>> #define cpu_has_sse4a boot_cpu_has(X86_FEATURE_SSE4A) >>>>> #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP) >>>>> #define cpu_has_skinit boot_cpu_has(X86_FEATURE_SKINIT) >>>> >>>> Hmm, leaving aside the style issue (stray blanks after opening parentheses, >>>> and as a result one-off indentation on the wrapped lines) I'm not really >>>> certain we can do this. The description goes into detail why we would want >>>> this, but it doesn't cover at all why it is safe for all present (and >>>> ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks >>>> just to derive further knowledge from that, without them being directly >>>> related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(), >>>> for example. While it looks to be okay there, it may give you an idea of >>>> what I mean. >>>> >>>> Things might become better separated if instead for such checks we used >>>> host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But >>>> that's still pretty far out, I'm afraid. >>> >>> I've followed a suggestion you made for patch in previous series: >>> >>> https://lore.kernel.org/xen-devel/8fbd604e-5e5d-410c-880f-2ad257bbe08a@suse.com/ >> >> See the "If not, ..." that I had put there. Doing the change just mechanically >> isn't enough, you also need to make clear (in the description) that you >> verified it's safe to have this way. > > What does it mean to "verified it's safe to have this way"? "Safe" in > what way? "Safe" as in "not breaking existing logic", anywhere. Jan
23.05.24 17:50, Jan Beulich: > On 23.05.2024 15:07, Sergiy Kibrik wrote: >> 16.05.24 14:12, Jan Beulich: >>> On 15.05.2024 11:12, Sergiy Kibrik wrote: >>>> --- a/xen/arch/x86/include/asm/cpufeature.h >>>> +++ b/xen/arch/x86/include/asm/cpufeature.h >>>> @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat) >>>> #define cpu_has_sse3 boot_cpu_has(X86_FEATURE_SSE3) >>>> #define cpu_has_pclmulqdq boot_cpu_has(X86_FEATURE_PCLMULQDQ) >>>> #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR) >>>> -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX) >>>> +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \ >>>> + boot_cpu_has(X86_FEATURE_VMX)) >>>> #define cpu_has_eist boot_cpu_has(X86_FEATURE_EIST) >>>> #define cpu_has_ssse3 boot_cpu_has(X86_FEATURE_SSSE3) >>>> #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA) >>>> @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat) >>>> >>>> /* CPUID level 0x80000001.ecx */ >>>> #define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) >>>> -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM) >>>> +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \ >>>> + boot_cpu_has(X86_FEATURE_SVM)) >>>> #define cpu_has_sse4a boot_cpu_has(X86_FEATURE_SSE4A) >>>> #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP) >>>> #define cpu_has_skinit boot_cpu_has(X86_FEATURE_SKINIT) >>> >>> Hmm, leaving aside the style issue (stray blanks after opening parentheses, >>> and as a result one-off indentation on the wrapped lines) I'm not really >>> certain we can do this. The description goes into detail why we would want >>> this, but it doesn't cover at all why it is safe for all present (and >>> ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks >>> just to derive further knowledge from that, without them being directly >>> related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(), >>> for example. While it looks to be okay there, it may give you an idea of >>> what I mean. >>> >>> Things might become better separated if instead for such checks we used >>> host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But >>> that's still pretty far out, I'm afraid. >> >> I've followed a suggestion you made for patch in previous series: >> [..] > > See the "If not, ..." that I had put there. Doing the change just mechanically > isn't enough, you also need to make clear (in the description) that you > verified it's safe to have this way. > >> yet if this approach can potentially be unsafe (I'm not completely sure >> it's safe), should we instead fallback to the way it was done in v1 >> series? I.e. guard calls to vmx/svm-specific calls where needed, like in >> these 3 patches: >> [..] > > I don't like this sprinkling around of IS_ENABLED() very much. Maybe we want > to have two new helpers (say using_svm() and using_vmx()), to be used in place > of most but possibly not all cpu_has_{svm,vmx}? Doing such a transformation > would then kind of implicitly answer the safety question above, as at every > use site you'd need to judge whether the replacement is correct. If it's > correct everywhere, the construct(s) as proposed in this version could then be > considered to be used in this very shape (instead of introducing the two new > helpers). But of course the transition could also be done gradually then, > touching only those uses that previously you touched in 1), 2), and 3). > now I might be seeing your concerns, if I understood correctly, situation is the following. As an example of cpu_has_vmx macro, it can be used to prove either of following two statements: 1) VMX features can be used or 2) CPU provides VMX features. Currently they're the same for Xen, yet after this patch series they're not, as the situation possible when non-vmx build would be able to get executed on vmx-enabled machine. E.g. the case of PV guest, or (if that makes any sense) at least hypervisor's code is still able to run until HVM guest has to be created. Changes in this patch makes indistinguishable for a user whether VMX support is absent in code or in hardware -- hence we may need two separate macros for these. Still the question remains whether a separate macro to check if CPU provides VMX/SVM is really needed at all at this point. I've counted only 16 uses of cpu_has_vmx in the code, not that much to check every one of them, so I did that. Most of uses are obviously checks before using vmx features, so logic not broken. As for the others, the surrounding context presumes that HVM domain required there or had already been created. But non-vmx build can't create HVM VMX domain anyway, so the logic not broken either. As for cpu_has_svm only 8 uses I've counted, all but one also don't seem to break logic as described above. One check of cpu_has_svm in init_speculation_mitigations(), where default speculation control flag gets set, not uses SVM features directly. Yet from the comment I can assume that it's also related to running HVM domain and usage of VMX features at later time. With all above, at the moment there doesn't seem to be uses of cpu_has_{svm,vmx} macros without subsequent usage of svm/vmx features, so this patch should be quite safe. Please let me know whether the above reasoning makes sense. -Sergiy
On 27.05.2024 12:27, Sergiy Kibrik wrote: > 23.05.24 17:50, Jan Beulich: >> On 23.05.2024 15:07, Sergiy Kibrik wrote: >>> 16.05.24 14:12, Jan Beulich: >>>> On 15.05.2024 11:12, Sergiy Kibrik wrote: >>>>> --- a/xen/arch/x86/include/asm/cpufeature.h >>>>> +++ b/xen/arch/x86/include/asm/cpufeature.h >>>>> @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat) >>>>> #define cpu_has_sse3 boot_cpu_has(X86_FEATURE_SSE3) >>>>> #define cpu_has_pclmulqdq boot_cpu_has(X86_FEATURE_PCLMULQDQ) >>>>> #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR) >>>>> -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX) >>>>> +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \ >>>>> + boot_cpu_has(X86_FEATURE_VMX)) >>>>> #define cpu_has_eist boot_cpu_has(X86_FEATURE_EIST) >>>>> #define cpu_has_ssse3 boot_cpu_has(X86_FEATURE_SSSE3) >>>>> #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA) >>>>> @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat) >>>>> >>>>> /* CPUID level 0x80000001.ecx */ >>>>> #define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) >>>>> -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM) >>>>> +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \ >>>>> + boot_cpu_has(X86_FEATURE_SVM)) >>>>> #define cpu_has_sse4a boot_cpu_has(X86_FEATURE_SSE4A) >>>>> #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP) >>>>> #define cpu_has_skinit boot_cpu_has(X86_FEATURE_SKINIT) >>>> >>>> Hmm, leaving aside the style issue (stray blanks after opening parentheses, >>>> and as a result one-off indentation on the wrapped lines) I'm not really >>>> certain we can do this. The description goes into detail why we would want >>>> this, but it doesn't cover at all why it is safe for all present (and >>>> ideally also future) uses. I wouldn't be surprised if we had VMX/SVM checks >>>> just to derive further knowledge from that, without them being directly >>>> related to the use of VMX/SVM. Take a look at calculate_hvm_max_policy(), >>>> for example. While it looks to be okay there, it may give you an idea of >>>> what I mean. >>>> >>>> Things might become better separated if instead for such checks we used >>>> host and raw CPU policies instead of cpuinfo_x86.x86_capability[]. But >>>> that's still pretty far out, I'm afraid. >>> >>> I've followed a suggestion you made for patch in previous series: >>> > [..] >> >> See the "If not, ..." that I had put there. Doing the change just mechanically >> isn't enough, you also need to make clear (in the description) that you >> verified it's safe to have this way. >> >>> yet if this approach can potentially be unsafe (I'm not completely sure >>> it's safe), should we instead fallback to the way it was done in v1 >>> series? I.e. guard calls to vmx/svm-specific calls where needed, like in >>> these 3 patches: >>> > [..] >> >> I don't like this sprinkling around of IS_ENABLED() very much. Maybe we want >> to have two new helpers (say using_svm() and using_vmx()), to be used in place >> of most but possibly not all cpu_has_{svm,vmx}? Doing such a transformation >> would then kind of implicitly answer the safety question above, as at every >> use site you'd need to judge whether the replacement is correct. If it's >> correct everywhere, the construct(s) as proposed in this version could then be >> considered to be used in this very shape (instead of introducing the two new >> helpers). But of course the transition could also be done gradually then, >> touching only those uses that previously you touched in 1), 2), and 3). >> > > now I might be seeing your concerns, if I understood correctly, > situation is the following. > > As an example of cpu_has_vmx macro, it can be used to prove either of > following two statements: 1) VMX features can be used or 2) CPU provides > VMX features. > Currently they're the same for Xen, yet after this patch series they're > not, as the situation possible when non-vmx build would be able to get > executed on vmx-enabled machine. E.g. the case of PV guest, or (if that > makes any sense) at least hypervisor's code is still able to run until > HVM guest has to be created. Changes in this patch makes > indistinguishable for a user whether VMX support is absent in code or in > hardware -- hence we may need two separate macros for these. > > Still the question remains whether a separate macro to check if CPU > provides VMX/SVM is really needed at all at this point. > > I've counted only 16 uses of cpu_has_vmx in the code, not that much to > check every one of them, so I did that. > Most of uses are obviously checks before using vmx features, so logic > not broken. > As for the others, the surrounding context presumes that HVM domain > required there or had already been created. But non-vmx build can't > create HVM VMX domain anyway, so the logic not broken either. > > As for cpu_has_svm only 8 uses I've counted, all but one also don't seem > to break logic as described above. One check of cpu_has_svm in > init_speculation_mitigations(), where default speculation control flag > gets set, not uses SVM features directly. Yet from the comment I can > assume that it's also related to running HVM domain and usage of VMX > features at later time. Note how to comment (necessarily) mixes SVM and VT-x. The !cpu_have_svm really looks to mean cpu_has_vmx there (i.e. absence of SVM on a HVM-only path implying VT-x). This would be broken if cpu_has_svm had IS_ENABLED() added to it. You'll want to consult the commit introducing the construct as well as perhaps Andrew directly as to whether switching to cpu_has_vmx here would be appropriate. I'm pretty sure there was a reason why this was written using the negative (!SVM) form. > With all above, at the moment there doesn't seem to be uses of > cpu_has_{svm,vmx} macros without subsequent usage of svm/vmx features, > so this patch should be quite safe. > > Please let me know whether the above reasoning makes sense. Fundamentally it does, with said one exception. Yet even in the absence of that exception the question would then remain whether we really want to close the road to one of the two possible uses of these constructs. Jan
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h index 9bc553681f..17f5aed000 100644 --- a/xen/arch/x86/include/asm/cpufeature.h +++ b/xen/arch/x86/include/asm/cpufeature.h @@ -81,7 +81,8 @@ static inline bool boot_cpu_has(unsigned int feat) #define cpu_has_sse3 boot_cpu_has(X86_FEATURE_SSE3) #define cpu_has_pclmulqdq boot_cpu_has(X86_FEATURE_PCLMULQDQ) #define cpu_has_monitor boot_cpu_has(X86_FEATURE_MONITOR) -#define cpu_has_vmx boot_cpu_has(X86_FEATURE_VMX) +#define cpu_has_vmx ( IS_ENABLED(CONFIG_VMX) && \ + boot_cpu_has(X86_FEATURE_VMX)) #define cpu_has_eist boot_cpu_has(X86_FEATURE_EIST) #define cpu_has_ssse3 boot_cpu_has(X86_FEATURE_SSSE3) #define cpu_has_fma boot_cpu_has(X86_FEATURE_FMA) @@ -109,7 +110,8 @@ static inline bool boot_cpu_has(unsigned int feat) /* CPUID level 0x80000001.ecx */ #define cpu_has_cmp_legacy boot_cpu_has(X86_FEATURE_CMP_LEGACY) -#define cpu_has_svm boot_cpu_has(X86_FEATURE_SVM) +#define cpu_has_svm ( IS_ENABLED(CONFIG_SVM) && \ + boot_cpu_has(X86_FEATURE_SVM)) #define cpu_has_sse4a boot_cpu_has(X86_FEATURE_SSE4A) #define cpu_has_xop boot_cpu_has(X86_FEATURE_XOP) #define cpu_has_skinit boot_cpu_has(X86_FEATURE_SKINIT)
As we now have SVM/VMX config options for enabling/disabling these features completely in the build, it may be feasible to add build-time checks to cpu_has_{svm,vmx} macros. These are used extensively thoughout HVM code, so we won't have to add extra #ifdef-s to check whether svm/vmx has been enabled, while DCE cleans up calls to vmx/svm functions, if their code not being built. Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> CC: Jan Beulich <jbeulich@suse.com> --- xen/arch/x86/include/asm/cpufeature.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)