Re: [PATCH 3/3] x86: short-circuit certain cpu_has_* when x86-64-v{2,3} are in effect

From: Jan Beulich <jbeulich@suse.com>
To: Jason Andryuk <jandryuk@gmail.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH 3/3] x86: short-circuit certain cpu_has_* when x86-64-v{2,3} are in effect
Date: Mon, 17 Jul 2023 15:38:29 +0200	[thread overview]
Message-ID: <40039335-8df7-cda6-c69c-cc94e80cb164@suse.com> (raw)
In-Reply-To: <CAKf6xpt9RTA7sef_QBM=y2gcZURcGzCpU9itUwCD5EPDTtukqA@mail.gmail.com>

On 17.07.2023 14:35, Jason Andryuk wrote:
> On Wed, Jul 12, 2023 at 8:36 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Certain fallback code can be made subject to DCE this way. Note that
>> CX16 has no compiler provided manifest constant, so CONFIG_* are used
>> there instead. Note also that we don't have cpu_has_movbe nor
>> cpu_has_lzcnt (aka cpu_has_abm).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Thanks.

>> --- a/xen/arch/x86/include/asm/cpufeature.h
>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>> @@ -76,13 +76,19 @@ static inline bool boot_cpu_has(unsigned
>>  #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)
>> -#define cpu_has_cx16            boot_cpu_has(X86_FEATURE_CX16)
>> +#define cpu_has_cx16            (IS_ENABLED(CONFIG_X86_64_V2) || \
>> +                                 IS_ENABLED(CONFIG_X86_64_V3) || \
>> +                                 boot_cpu_has(X86_FEATURE_CX16))
> 
> If you think there may be more ABI selections in the future, it might
> be better to express the "V$N" numerically and check >= 2.  Or you can
> add a Kconfig CONFIG_X86_64_CX16 and select that as appropriate.  But
> if there aren't going to be more of these, then this is fine.

I was thinking this same way: If more appear (which aren't SIMD-only,
like v4 is), we can use a numeric CONFIG_*, but for now it's good
enough (and slightly simpler) this way.

Jan