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(-)
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)
--
2.25.1On 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
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
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
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>
© 2016 - 2024 Red Hat, Inc.