diff mbox series

[3/5] x86/cpu: Renumber X86_VENDOR_* to form a bitmap

Message ID 1554409592-28572-4-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/cpu: Rework of X86_VENDOR_* constants | expand

Commit Message

Andrew Cooper April 4, 2019, 8:26 p.m. UTC 
CPUs from different vendors sometimes share characteristics.  All users of
X86_VENDOR_* are now direct equal/not-equal comparisons.  By expressing the
X86_VENDOR_* constants in a bitmap fashon, we can more concicely and
efficiently test whether a vendor is one of a group.

Update all parts of the code which can already benefit from this improvement.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: David Wang <davidwang@zhaoxin.com>
CC: Pu Wen <puwen@hygon.cn>

The 3 locations which are INTEL | CENTAUR are all to do with SYSENTER MSR
handling, which I expect should include SHANGHAI.  I expect this is also true
of !hvm_long_mode_active() handling of X86_FEATURE_SYSCALL.
---
 xen/arch/x86/acpi/cpufreq/cpufreq.c |  3 +--
 xen/arch/x86/acpi/suspend.c         |  6 ++----
 xen/arch/x86/pv/emul-priv-op.c      |  3 +--
 xen/arch/x86/x86_64/traps.c         |  3 +--
 xen/include/asm-x86/x86-vendors.h   | 13 +++++++------
 5 files changed, 12 insertions(+), 16 deletions(-)

Comments

Jan Beulich April 5, 2019, 9:03 a.m. UTC | #1 
>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/x86-vendors.h
> +++ b/xen/include/asm-x86/x86-vendors.h
> @@ -4,28 +4,29 @@
>  /*
>   * CPU vendor IDs
>   *
> - * - X86_VENDOR_* are Xen-internal identifiers.  Values and order are
> - *   arbitrary.
> + * - X86_VENDOR_* are Xen-internal identifiers.  The order is arbitrary, but
> + *   values form a bitmap so vendor checks can be made against multiple
> + *   vendors at once.
>   * - X86_VENDOR_*_E?X are architectural information from CPUID leaf 0
>   */
>  #define X86_VENDOR_UNKNOWN 0
>  
> -#define X86_VENDOR_INTEL 1
> +#define X86_VENDOR_INTEL (1 << 1)

Any reason you start from a shift count of 1, instead of 0, when
"unknown" simply is no bit set? We're not about to run out of bits,
but preferably with this corrected
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper April 5, 2019, 9:28 a.m. UTC | #2 
On 05/04/2019 10:03, Jan Beulich wrote:
>>>> On 04.04.19 at 22:26, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/asm-x86/x86-vendors.h
>> +++ b/xen/include/asm-x86/x86-vendors.h
>> @@ -4,28 +4,29 @@
>>  /*
>>   * CPU vendor IDs
>>   *
>> - * - X86_VENDOR_* are Xen-internal identifiers.  Values and order are
>> - *   arbitrary.
>> + * - X86_VENDOR_* are Xen-internal identifiers.  The order is arbitrary, but
>> + *   values form a bitmap so vendor checks can be made against multiple
>> + *   vendors at once.
>>   * - X86_VENDOR_*_E?X are architectural information from CPUID leaf 0
>>   */
>>  #define X86_VENDOR_UNKNOWN 0
>>  
>> -#define X86_VENDOR_INTEL 1
>> +#define X86_VENDOR_INTEL (1 << 1)
> Any reason you start from a shift count of 1, instead of 0, when
> "unknown" simply is no bit set? We're not about to run out of bits,
> but preferably with this corrected
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

No - that wasn't intentional.  Will fix.

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index 844ab85..f4e13e1 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -661,8 +661,7 @@  int cpufreq_cpu_init(unsigned int cpuid)
     int ret;
 
     /* Currently we only handle Intel and AMD processor */
-    if ( (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) ||
-         (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) )
+    if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_AMD) )
         ret = cpufreq_add_cpu(cpuid);
     else
         ret = -EFAULT;
diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index 00e6012..9e69bf2 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -27,8 +27,7 @@  void save_rest_processor_state(void)
     rdmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
     rdmsrl(MSR_CSTAR, saved_cstar);
     rdmsrl(MSR_LSTAR, saved_lstar);
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-         boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR )
+    if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) )
     {
         rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
         rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
@@ -52,8 +51,7 @@  void restore_rest_processor_state(void)
     wrgsbase(saved_gs_base);
     wrmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
 
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-         boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR )
+    if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) )
     {
         /* Recover sysenter MSRs */
         wrmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 84ce67c..f594065 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1066,8 +1066,7 @@  static int write_msr(unsigned int reg, uint64_t val,
 
     case MSR_IA32_MPERF:
     case MSR_IA32_APERF:
-        if ( (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) &&
-             (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) )
+        if ( !(boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_AMD)) )
             break;
         if ( likely(!is_cpufreq_controller(currd)) ||
              wrmsr_safe(reg, val) == 0 )
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index bf7870e..44af765 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -334,8 +334,7 @@  void subarch_percpu_traps_init(void)
                                    (unsigned long)lstar_enter);
     stub_va += offset;
 
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-         boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR )
+    if ( boot_cpu_data.x86_vendor & (X86_VENDOR_INTEL | X86_VENDOR_CENTAUR) )
     {
         /* SYSENTER entry. */
         wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom);
diff --git a/xen/include/asm-x86/x86-vendors.h b/xen/include/asm-x86/x86-vendors.h
index fca7396..1ecb934 100644
--- a/xen/include/asm-x86/x86-vendors.h
+++ b/xen/include/asm-x86/x86-vendors.h
@@ -4,28 +4,29 @@ 
 /*
  * CPU vendor IDs
  *
- * - X86_VENDOR_* are Xen-internal identifiers.  Values and order are
- *   arbitrary.
+ * - X86_VENDOR_* are Xen-internal identifiers.  The order is arbitrary, but
+ *   values form a bitmap so vendor checks can be made against multiple
+ *   vendors at once.
  * - X86_VENDOR_*_E?X are architectural information from CPUID leaf 0
  */
 #define X86_VENDOR_UNKNOWN 0
 
-#define X86_VENDOR_INTEL 1
+#define X86_VENDOR_INTEL (1 << 1)
 #define X86_VENDOR_INTEL_EBX 0x756e6547U /* "GenuineIntel" */
 #define X86_VENDOR_INTEL_ECX 0x6c65746eU
 #define X86_VENDOR_INTEL_EDX 0x49656e69U
 
-#define X86_VENDOR_AMD 2
+#define X86_VENDOR_AMD (1 << 2)
 #define X86_VENDOR_AMD_EBX 0x68747541U /* "AuthenticAMD" */
 #define X86_VENDOR_AMD_ECX 0x444d4163U
 #define X86_VENDOR_AMD_EDX 0x69746e65U
 
-#define X86_VENDOR_CENTAUR 3
+#define X86_VENDOR_CENTAUR (1 << 3)
 #define X86_VENDOR_CENTAUR_EBX 0x746e6543U /* "CentaurHauls" */
 #define X86_VENDOR_CENTAUR_ECX 0x736c7561U
 #define X86_VENDOR_CENTAUR_EDX 0x48727561U
 
-#define X86_VENDOR_SHANGHAI 4
+#define X86_VENDOR_SHANGHAI (1 << 4)
 #define X86_VENDOR_SHANGHAI_EBX 0x68532020U /* "  Shanghai  " */
 #define X86_VENDOR_SHANGHAI_ECX 0x20206961U
 #define X86_VENDOR_SHANGHAI_EDX 0x68676e61U