diff mbox

[20/27] x86/cpuid: Drop the temporary linear feature bitmap from struct cpuid_policy

Message ID 1483533584-8015-21-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper Jan. 4, 2017, 12:39 p.m. UTC 
With most uses of the *_featureset API removed, the remaining uses are only
during XEN_SYSCTL_get_cpu_featureset, init_guest_cpuid(), and
recalculate_cpuid_policy(), none of which are hot paths.

Drop the temporary infrastructure, and have the current users recreate the
linear bitmap using cpuid_policy_to_featureset().  This avoids storing
duplicated information in struct cpuid_policy.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpuid.c        | 19 ++++++++++---------
 xen/arch/x86/sysctl.c       | 21 ++++++++++++---------
 xen/include/asm-x86/cpuid.h |  9 ---------
 3 files changed, 22 insertions(+), 27 deletions(-)

Comments

Jan Beulich Jan. 5, 2017, 1:07 p.m. UTC | #1 
>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> wrote:
>  static void __init calculate_pv_max_policy(void)
>  {
>      struct cpuid_policy *p = &pv_max_policy;
> +    uint32_t pv_featureset[FSCAPINTS], host_featureset[FSCAPINTS];
>      unsigned int i;
>  
> +    cpuid_policy_to_featureset(&host_policy, host_featureset);
> +
>      for ( i = 0; i < FSCAPINTS; ++i )
>          pv_featureset[i] = host_featureset[i] & pv_featuremask[i];

While at init time we shouldn't be tight on stack space, it would still
feel better if you didn't put two such (growing in the future) arrays
on the stack. Would you consider it unreasonable to do

    cpuid_policy_to_featureset(&host_policy, pv_featureset);
    for ( i = 0; i < FSCAPINTS; ++i )
        pv_featureset[i] &= pv_featuremask[i];

(and then similarly for HVM)? Either way
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper Jan. 5, 2017, 1:12 p.m. UTC | #2 
On 05/01/17 13:07, Jan Beulich wrote:
>>>> On 04.01.17 at 13:39, <andrew.cooper3@citrix.com> wrote:
>>  static void __init calculate_pv_max_policy(void)
>>  {
>>      struct cpuid_policy *p = &pv_max_policy;
>> +    uint32_t pv_featureset[FSCAPINTS], host_featureset[FSCAPINTS];
>>      unsigned int i;
>>  
>> +    cpuid_policy_to_featureset(&host_policy, host_featureset);
>> +
>>      for ( i = 0; i < FSCAPINTS; ++i )
>>          pv_featureset[i] = host_featureset[i] & pv_featuremask[i];
> While at init time we shouldn't be tight on stack space, it would still
> feel better if you didn't put two such (growing in the future) arrays
> on the stack. Would you consider it unreasonable to do
>
>     cpuid_policy_to_featureset(&host_policy, pv_featureset);
>     for ( i = 0; i < FSCAPINTS; ++i )
>         pv_featureset[i] &= pv_featuremask[i];
>
> (and then similarly for HVM)?

The following patch does (basically) this, and drops one of the arrays.

~Andrew
diff mbox

Patch

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 86f598f..a261843 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -126,24 +126,23 @@  static void __init calculate_raw_policy(void)
     for ( i = 1; i < min(ARRAY_SIZE(p->extd.raw),
                          p->extd.max_leaf + 1 - 0x80000000ul); ++i )
         cpuid_leaf(0x80000000 + i, &p->extd.raw[i]);
-
-    cpuid_policy_to_featureset(p, p->fs);
 }
 
 static void __init calculate_host_policy(void)
 {
     struct cpuid_policy *p = &host_policy;
 
-    memcpy(p->fs, boot_cpu_data.x86_capability, sizeof(p->fs));
-
-    cpuid_featureset_to_policy(host_featureset, p);
+    cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
 }
 
 static void __init calculate_pv_max_policy(void)
 {
     struct cpuid_policy *p = &pv_max_policy;
+    uint32_t pv_featureset[FSCAPINTS], host_featureset[FSCAPINTS];
     unsigned int i;
 
+    cpuid_policy_to_featureset(&host_policy, host_featureset);
+
     for ( i = 0; i < FSCAPINTS; ++i )
         pv_featureset[i] = host_featureset[i] & pv_featuremask[i];
 
@@ -165,12 +164,15 @@  static void __init calculate_pv_max_policy(void)
 static void __init calculate_hvm_max_policy(void)
 {
     struct cpuid_policy *p = &hvm_max_policy;
+    uint32_t hvm_featureset[FSCAPINTS], host_featureset[FSCAPINTS];
     unsigned int i;
     const uint32_t *hvm_featuremask;
 
     if ( !hvm_enabled )
         return;
 
+    cpuid_policy_to_featureset(&host_policy, host_featureset);
+
     hvm_featuremask = hvm_funcs.hap_supported ?
         hvm_hap_featuremask : hvm_shadow_featuremask;
 
@@ -199,8 +201,7 @@  static void __init calculate_hvm_max_policy(void)
      * long mode (and init_amd() has cleared it out of host capabilities), but
      * HVM guests are able if running in protected mode.
      */
-    if ( (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
-         test_bit(X86_FEATURE_SEP, raw_featureset) )
+    if ( (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && raw_policy.basic.sep )
         __set_bit(X86_FEATURE_SEP, hvm_featureset);
 
     /*
@@ -267,7 +268,7 @@  void recalculate_cpuid_policy(struct domain *d)
     unsigned int i;
 
     cpuid_policy_to_featureset(p, fs);
-    memcpy(max_fs, max->fs, sizeof(max_fs));
+    cpuid_policy_to_featureset(max, max_fs);
 
     /* Allow a toolstack to possibly select ITSC... */
     if ( cpu_has_itsc )
@@ -295,7 +296,7 @@  void recalculate_cpuid_policy(struct domain *d)
 
     /* Fold host's FDP_EXCP_ONLY and NO_FPU_SEL into guest's view. */
     fs[FEATURESET_7b0] &= ~special_features[FEATURESET_7b0];
-    fs[FEATURESET_7b0] |= (host_featureset[FEATURESET_7b0] &
+    fs[FEATURESET_7b0] |= (host_policy.feat._7b0 &
                            special_features[FEATURESET_7b0]);
 
     sanitise_featureset(fs);
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 14e7dc7..87da541 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -199,13 +199,14 @@  long arch_do_sysctl(
 
     case XEN_SYSCTL_get_cpu_featureset:
     {
-        static const uint32_t *const featureset_table[] = {
-            [XEN_SYSCTL_cpu_featureset_raw]  = raw_featureset,
-            [XEN_SYSCTL_cpu_featureset_host] = host_featureset,
-            [XEN_SYSCTL_cpu_featureset_pv]   = pv_featureset,
-            [XEN_SYSCTL_cpu_featureset_hvm]  = hvm_featureset,
+        static const struct cpuid_policy *const policy_table[] = {
+            [XEN_SYSCTL_cpu_featureset_raw]  = &raw_policy,
+            [XEN_SYSCTL_cpu_featureset_host] = &host_policy,
+            [XEN_SYSCTL_cpu_featureset_pv]   = &pv_max_policy,
+            [XEN_SYSCTL_cpu_featureset_hvm]  = &hvm_max_policy,
         };
-        const uint32_t *featureset = NULL;
+        const struct cpuid_policy *p = NULL;
+        uint32_t featureset[FSCAPINTS];
         unsigned int nr;
 
         /* Request for maximum number of features? */
@@ -223,13 +224,15 @@  long arch_do_sysctl(
                    FSCAPINTS);
 
         /* Look up requested featureset. */
-        if ( sysctl->u.cpu_featureset.index < ARRAY_SIZE(featureset_table) )
-            featureset = featureset_table[sysctl->u.cpu_featureset.index];
+        if ( sysctl->u.cpu_featureset.index < ARRAY_SIZE(policy_table) )
+            p = policy_table[sysctl->u.cpu_featureset.index];
 
         /* Bad featureset index? */
-        if ( !featureset )
+        if ( !p )
             ret = -EINVAL;
 
+        cpuid_policy_to_featureset(p, featureset);
+
         /* Copy the requested featureset into place. */
         if ( !ret && copy_to_guest(sysctl->u.cpu_featureset.features,
                                    featureset, nr) )
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 0371e6e..9788cac 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -253,9 +253,6 @@  struct cpuid_policy
             };
         };
     } extd;
-
-    /* Temporary featureset bitmap. */
-    uint32_t fs[FSCAPINTS];
 };
 
 /* Fill in a featureset bitmap from a CPUID policy. */
@@ -293,12 +290,6 @@  static inline void cpuid_featureset_to_policy(
 extern struct cpuid_policy raw_policy, host_policy, pv_max_policy,
     hvm_max_policy;
 
-/* Temporary compatibility defines. */
-#define raw_featureset raw_policy.fs
-#define host_featureset host_policy.fs
-#define pv_featureset pv_max_policy.fs
-#define hvm_featureset hvm_max_policy.fs
-
 /* Allocate and initialise a CPUID policy suitable for the domain. */
 int init_domain_cpuid_policy(struct domain *d);