diff mbox series

[v2,09/15] x86: Out-of-inline the policy<->featureset convertors

Message ID 20230404095222.1373721-10-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86: Merge cpuid and msr policy objects | expand

Commit Message

Andrew Cooper April 4, 2023, 9:52 a.m. UTC 
These are already getting over-large for being inline functions, and are only
going to grow more over time.  Out of line them, yielding the following net
delta from bloat-o-meter:

  add/remove: 2/0 grow/shrink: 0/4 up/down: 276/-1877 (-1601)

Switch to the newer cpu_policy terminology while doing so.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * New
---
 tools/libs/guest/xg_cpuid_x86.c      |  2 +-
 xen/arch/x86/cpuid.c                 | 28 +++++++--------
 xen/arch/x86/sysctl.c                |  2 +-
 xen/include/xen/lib/x86/cpu-policy.h | 52 ++++++----------------------
 xen/lib/x86/cpuid.c                  | 42 ++++++++++++++++++++++
 5 files changed, 68 insertions(+), 58 deletions(-)

Comments

Jan Beulich April 4, 2023, 3:01 p.m. UTC | #1 
On 04.04.2023 11:52, Andrew Cooper wrote:
> These are already getting over-large for being inline functions, and are only
> going to grow more over time.  Out of line them, yielding the following net
> delta from bloat-o-meter:
> 
>   add/remove: 2/0 grow/shrink: 0/4 up/down: 276/-1877 (-1601)
> 
> Switch to the newer cpu_policy terminology while doing so.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

I take it you have a reason to ...

> --- a/xen/lib/x86/cpuid.c
> +++ b/xen/lib/x86/cpuid.c
> @@ -60,6 +60,48 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor)
>      }
>  }
>  
> +void x86_cpu_policy_to_featureset(
> +    const struct cpu_policy *p, uint32_t fs[FEATURESET_NR_ENTRIES])
> +{
> +    fs[FEATURESET_1d]        = p->basic._1d;
> +    fs[FEATURESET_1c]        = p->basic._1c;
> +    fs[FEATURESET_e1d]       = p->extd.e1d;
> +    fs[FEATURESET_e1c]       = p->extd.e1c;
> +    fs[FEATURESET_Da1]       = p->xstate.Da1;
> +    fs[FEATURESET_7b0]       = p->feat._7b0;
> +    fs[FEATURESET_7c0]       = p->feat._7c0;
> +    fs[FEATURESET_e7d]       = p->extd.e7d;
> +    fs[FEATURESET_e8b]       = p->extd.e8b;
> +    fs[FEATURESET_7d0]       = p->feat._7d0;
> +    fs[FEATURESET_7a1]       = p->feat._7a1;
> +    fs[FEATURESET_e21a]      = p->extd.e21a;
> +    fs[FEATURESET_7b1]       = p->feat._7b1;
> +    fs[FEATURESET_7d2]       = p->feat._7d2;
> +    fs[FEATURESET_7c1]       = p->feat._7c1;
> +    fs[FEATURESET_7d1]       = p->feat._7d1;
> +}
> +
> +void x86_cpu_featureset_to_policy(
> +    const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpu_policy *p)
> +{
> +    p->basic._1d             = fs[FEATURESET_1d];
> +    p->basic._1c             = fs[FEATURESET_1c];
> +    p->extd.e1d              = fs[FEATURESET_e1d];
> +    p->extd.e1c              = fs[FEATURESET_e1c];
> +    p->xstate.Da1            = fs[FEATURESET_Da1];
> +    p->feat._7b0             = fs[FEATURESET_7b0];
> +    p->feat._7c0             = fs[FEATURESET_7c0];
> +    p->extd.e7d              = fs[FEATURESET_e7d];
> +    p->extd.e8b              = fs[FEATURESET_e8b];
> +    p->feat._7d0             = fs[FEATURESET_7d0];
> +    p->feat._7a1             = fs[FEATURESET_7a1];
> +    p->extd.e21a             = fs[FEATURESET_e21a];
> +    p->feat._7b1             = fs[FEATURESET_7b1];
> +    p->feat._7d2             = fs[FEATURESET_7d2];
> +    p->feat._7c1             = fs[FEATURESET_7c1];
> +    p->feat._7d1             = fs[FEATURESET_7d1];
> +}

... add quite a few padding blanks in here, unlike in the originals?

Jan
Andrew Cooper April 4, 2023, 3:26 p.m. UTC | #2 
On 04/04/2023 4:01 pm, Jan Beulich wrote:
> On 04.04.2023 11:52, Andrew Cooper wrote:
>> These are already getting over-large for being inline functions, and are only
>> going to grow more over time.  Out of line them, yielding the following net
>> delta from bloat-o-meter:
>>
>>   add/remove: 2/0 grow/shrink: 0/4 up/down: 276/-1877 (-1601)
>>
>> Switch to the newer cpu_policy terminology while doing so.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> I take it you have a reason to ...
>
>> --- a/xen/lib/x86/cpuid.c
>> +++ b/xen/lib/x86/cpuid.c
>> @@ -60,6 +60,48 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor)
>>      }
>>  }
>>  
>> +void x86_cpu_policy_to_featureset(
>> +    const struct cpu_policy *p, uint32_t fs[FEATURESET_NR_ENTRIES])
>> +{
>> +    fs[FEATURESET_1d]        = p->basic._1d;
>> +    fs[FEATURESET_1c]        = p->basic._1c;
>> +    fs[FEATURESET_e1d]       = p->extd.e1d;
>> +    fs[FEATURESET_e1c]       = p->extd.e1c;
>> +    fs[FEATURESET_Da1]       = p->xstate.Da1;
>> +    fs[FEATURESET_7b0]       = p->feat._7b0;
>> +    fs[FEATURESET_7c0]       = p->feat._7c0;
>> +    fs[FEATURESET_e7d]       = p->extd.e7d;
>> +    fs[FEATURESET_e8b]       = p->extd.e8b;
>> +    fs[FEATURESET_7d0]       = p->feat._7d0;
>> +    fs[FEATURESET_7a1]       = p->feat._7a1;
>> +    fs[FEATURESET_e21a]      = p->extd.e21a;
>> +    fs[FEATURESET_7b1]       = p->feat._7b1;
>> +    fs[FEATURESET_7d2]       = p->feat._7d2;
>> +    fs[FEATURESET_7c1]       = p->feat._7c1;
>> +    fs[FEATURESET_7d1]       = p->feat._7d1;
>> +}
>> +
>> +void x86_cpu_featureset_to_policy(
>> +    const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpu_policy *p)
>> +{
>> +    p->basic._1d             = fs[FEATURESET_1d];
>> +    p->basic._1c             = fs[FEATURESET_1c];
>> +    p->extd.e1d              = fs[FEATURESET_e1d];
>> +    p->extd.e1c              = fs[FEATURESET_e1c];
>> +    p->xstate.Da1            = fs[FEATURESET_Da1];
>> +    p->feat._7b0             = fs[FEATURESET_7b0];
>> +    p->feat._7c0             = fs[FEATURESET_7c0];
>> +    p->extd.e7d              = fs[FEATURESET_e7d];
>> +    p->extd.e8b              = fs[FEATURESET_e8b];
>> +    p->feat._7d0             = fs[FEATURESET_7d0];
>> +    p->feat._7a1             = fs[FEATURESET_7a1];
>> +    p->extd.e21a             = fs[FEATURESET_e21a];
>> +    p->feat._7b1             = fs[FEATURESET_7b1];
>> +    p->feat._7d2             = fs[FEATURESET_7d2];
>> +    p->feat._7c1             = fs[FEATURESET_7c1];
>> +    p->feat._7d1             = fs[FEATURESET_7d1];
>> +}
> ... add quite a few padding blanks in here, unlike in the originals?

Yeah.  There was already one misalignment, and I haven't quite decided
on the MSR syntax yet but it's going to be longer still.

Here specifically, we've got p->arch_caps.{a,d} at a minimum, so column
width is based on the MSR name.

This is just a guestimate of "plenty for now".

~Andrew
diff mbox series

Patch

diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 259029be8b36..33d366a8eb43 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -565,7 +565,7 @@  int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
             }
         }
 
-        cpuid_featureset_to_policy(feat, p);
+        x86_cpu_featureset_to_policy(feat, p);
     }
     else
     {
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index df3e503ced9d..5eb5f1893516 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -368,7 +368,7 @@  static void __init calculate_host_policy(void)
     p->extd.max_leaf = 0x80000000 | min_t(uint32_t, max_extd_leaf & 0xffff,
                                           ARRAY_SIZE(p->extd.raw) - 1);
 
-    cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
+    x86_cpu_featureset_to_policy(boot_cpu_data.x86_capability, p);
     recalculate_xstate(p);
     recalculate_misc(p);
 
@@ -450,7 +450,7 @@  static void __init calculate_pv_max_policy(void)
     unsigned int i;
 
     *p = host_cpu_policy;
-    cpuid_policy_to_featureset(p, pv_featureset);
+    x86_cpu_policy_to_featureset(p, pv_featureset);
 
     for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
         pv_featureset[i] &= pv_max_featuremask[i];
@@ -468,7 +468,7 @@  static void __init calculate_pv_max_policy(void)
     guest_common_feature_adjustments(pv_featureset);
 
     sanitise_featureset(pv_featureset);
-    cpuid_featureset_to_policy(pv_featureset, p);
+    x86_cpu_featureset_to_policy(pv_featureset, p);
     recalculate_xstate(p);
 
     p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
@@ -481,7 +481,7 @@  static void __init calculate_pv_def_policy(void)
     unsigned int i;
 
     *p = pv_max_cpu_policy;
-    cpuid_policy_to_featureset(p, pv_featureset);
+    x86_cpu_policy_to_featureset(p, pv_featureset);
 
     for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
         pv_featureset[i] &= pv_def_featuremask[i];
@@ -490,7 +490,7 @@  static void __init calculate_pv_def_policy(void)
     guest_common_default_feature_adjustments(pv_featureset);
 
     sanitise_featureset(pv_featureset);
-    cpuid_featureset_to_policy(pv_featureset, p);
+    x86_cpu_featureset_to_policy(pv_featureset, p);
     recalculate_xstate(p);
 }
 
@@ -502,7 +502,7 @@  static void __init calculate_hvm_max_policy(void)
     const uint32_t *hvm_featuremask;
 
     *p = host_cpu_policy;
-    cpuid_policy_to_featureset(p, hvm_featureset);
+    x86_cpu_policy_to_featureset(p, hvm_featureset);
 
     hvm_featuremask = hvm_hap_supported() ?
         hvm_hap_max_featuremask : hvm_shadow_max_featuremask;
@@ -581,7 +581,7 @@  static void __init calculate_hvm_max_policy(void)
     guest_common_feature_adjustments(hvm_featureset);
 
     sanitise_featureset(hvm_featureset);
-    cpuid_featureset_to_policy(hvm_featureset, p);
+    x86_cpu_featureset_to_policy(hvm_featureset, p);
     recalculate_xstate(p);
 }
 
@@ -593,7 +593,7 @@  static void __init calculate_hvm_def_policy(void)
     const uint32_t *hvm_featuremask;
 
     *p = hvm_max_cpu_policy;
-    cpuid_policy_to_featureset(p, hvm_featureset);
+    x86_cpu_policy_to_featureset(p, hvm_featureset);
 
     hvm_featuremask = hvm_hap_supported() ?
         hvm_hap_def_featuremask : hvm_shadow_def_featuremask;
@@ -612,7 +612,7 @@  static void __init calculate_hvm_def_policy(void)
         __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
 
     sanitise_featureset(hvm_featureset);
-    cpuid_featureset_to_policy(hvm_featureset, p);
+    x86_cpu_featureset_to_policy(hvm_featureset, p);
     recalculate_xstate(p);
 }
 
@@ -682,8 +682,8 @@  void recalculate_cpuid_policy(struct domain *d)
                                             ? CPUID_GUEST_NR_EXTD_AMD
                                             : CPUID_GUEST_NR_EXTD_INTEL) - 1);
 
-    cpuid_policy_to_featureset(p, fs);
-    cpuid_policy_to_featureset(max, max_fs);
+    x86_cpu_policy_to_featureset(p, fs);
+    x86_cpu_policy_to_featureset(max, max_fs);
 
     if ( is_hvm_domain(d) )
     {
@@ -740,7 +740,7 @@  void recalculate_cpuid_policy(struct domain *d)
                            (cpufeat_mask(X86_FEATURE_FDP_EXCP_ONLY) |
                             cpufeat_mask(X86_FEATURE_NO_FPU_SEL)));
 
-    cpuid_featureset_to_policy(fs, p);
+    x86_cpu_featureset_to_policy(fs, p);
 
     /* Pass host cacheline size through to guests. */
     p->basic.clflush_size = max->basic.clflush_size;
@@ -806,7 +806,7 @@  void __init init_dom0_cpuid_policy(struct domain *d)
         uint32_t fs[FSCAPINTS];
         unsigned int i;
 
-        cpuid_policy_to_featureset(p, fs);
+        x86_cpu_policy_to_featureset(p, fs);
 
         for ( i = 0; i < ARRAY_SIZE(fs); ++i )
         {
@@ -814,7 +814,7 @@  void __init init_dom0_cpuid_policy(struct domain *d)
             fs[i] &= ~dom0_disable_feat[i];
         }
 
-        cpuid_featureset_to_policy(fs, p);
+        x86_cpu_featureset_to_policy(fs, p);
 
         recalculate_cpuid_policy(d);
     }
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 43a241f2090f..c107f40c6283 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -338,7 +338,7 @@  long arch_do_sysctl(
             ret = -EINVAL;
 
         if ( !ret )
-            cpuid_policy_to_featureset(p, featureset);
+            x86_cpu_policy_to_featureset(p, featureset);
 
         /* Copy the requested featureset into place. */
         if ( !ret && copy_to_guest(sysctl->u.cpu_featureset.features,
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index 8b27a0725b8e..57b4633c861e 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -387,49 +387,17 @@  struct cpu_policy_errors
 
 #define INIT_CPU_POLICY_ERRORS { -1, -1, -1 }
 
-/* Fill in a featureset bitmap from a CPUID policy. */
-static inline void cpuid_policy_to_featureset(
-    const struct cpuid_policy *p, uint32_t fs[FEATURESET_NR_ENTRIES])
-{
-    fs[FEATURESET_1d]  = p->basic._1d;
-    fs[FEATURESET_1c]  = p->basic._1c;
-    fs[FEATURESET_e1d] = p->extd.e1d;
-    fs[FEATURESET_e1c] = p->extd.e1c;
-    fs[FEATURESET_Da1] = p->xstate.Da1;
-    fs[FEATURESET_7b0] = p->feat._7b0;
-    fs[FEATURESET_7c0] = p->feat._7c0;
-    fs[FEATURESET_e7d] = p->extd.e7d;
-    fs[FEATURESET_e8b] = p->extd.e8b;
-    fs[FEATURESET_7d0] = p->feat._7d0;
-    fs[FEATURESET_7a1] = p->feat._7a1;
-    fs[FEATURESET_e21a] = p->extd.e21a;
-    fs[FEATURESET_7b1] = p->feat._7b1;
-    fs[FEATURESET_7d2] = p->feat._7d2;
-    fs[FEATURESET_7c1] = p->feat._7c1;
-    fs[FEATURESET_7d1] = p->feat._7d1;
-}
+/**
+ * Copy the featureset words out of a cpu_policy object.
+ */
+void x86_cpu_policy_to_featureset(const struct cpu_policy *p,
+                                  uint32_t fs[FEATURESET_NR_ENTRIES]);
 
-/* Fill in a CPUID policy from a featureset bitmap. */
-static inline void cpuid_featureset_to_policy(
-    const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpuid_policy *p)
-{
-    p->basic._1d  = fs[FEATURESET_1d];
-    p->basic._1c  = fs[FEATURESET_1c];
-    p->extd.e1d   = fs[FEATURESET_e1d];
-    p->extd.e1c   = fs[FEATURESET_e1c];
-    p->xstate.Da1 = fs[FEATURESET_Da1];
-    p->feat._7b0  = fs[FEATURESET_7b0];
-    p->feat._7c0  = fs[FEATURESET_7c0];
-    p->extd.e7d   = fs[FEATURESET_e7d];
-    p->extd.e8b   = fs[FEATURESET_e8b];
-    p->feat._7d0  = fs[FEATURESET_7d0];
-    p->feat._7a1  = fs[FEATURESET_7a1];
-    p->extd.e21a  = fs[FEATURESET_e21a];
-    p->feat._7b1  = fs[FEATURESET_7b1];
-    p->feat._7d2  = fs[FEATURESET_7d2];
-    p->feat._7c1  = fs[FEATURESET_7c1];
-    p->feat._7d1  = fs[FEATURESET_7d1];
-}
+/**
+ * Copy the featureset words back into a cpu_policy object.
+ */
+void x86_cpu_featureset_to_policy(const uint32_t fs[FEATURESET_NR_ENTRIES],
+                                  struct cpu_policy *p);
 
 static inline uint64_t cpuid_policy_xcr0_max(const struct cpuid_policy *p)
 {
diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
index e81f76c779c0..734e90823a63 100644
--- a/xen/lib/x86/cpuid.c
+++ b/xen/lib/x86/cpuid.c
@@ -60,6 +60,48 @@  const char *x86_cpuid_vendor_to_str(unsigned int vendor)
     }
 }
 
+void x86_cpu_policy_to_featureset(
+    const struct cpu_policy *p, uint32_t fs[FEATURESET_NR_ENTRIES])
+{
+    fs[FEATURESET_1d]        = p->basic._1d;
+    fs[FEATURESET_1c]        = p->basic._1c;
+    fs[FEATURESET_e1d]       = p->extd.e1d;
+    fs[FEATURESET_e1c]       = p->extd.e1c;
+    fs[FEATURESET_Da1]       = p->xstate.Da1;
+    fs[FEATURESET_7b0]       = p->feat._7b0;
+    fs[FEATURESET_7c0]       = p->feat._7c0;
+    fs[FEATURESET_e7d]       = p->extd.e7d;
+    fs[FEATURESET_e8b]       = p->extd.e8b;
+    fs[FEATURESET_7d0]       = p->feat._7d0;
+    fs[FEATURESET_7a1]       = p->feat._7a1;
+    fs[FEATURESET_e21a]      = p->extd.e21a;
+    fs[FEATURESET_7b1]       = p->feat._7b1;
+    fs[FEATURESET_7d2]       = p->feat._7d2;
+    fs[FEATURESET_7c1]       = p->feat._7c1;
+    fs[FEATURESET_7d1]       = p->feat._7d1;
+}
+
+void x86_cpu_featureset_to_policy(
+    const uint32_t fs[FEATURESET_NR_ENTRIES], struct cpu_policy *p)
+{
+    p->basic._1d             = fs[FEATURESET_1d];
+    p->basic._1c             = fs[FEATURESET_1c];
+    p->extd.e1d              = fs[FEATURESET_e1d];
+    p->extd.e1c              = fs[FEATURESET_e1c];
+    p->xstate.Da1            = fs[FEATURESET_Da1];
+    p->feat._7b0             = fs[FEATURESET_7b0];
+    p->feat._7c0             = fs[FEATURESET_7c0];
+    p->extd.e7d              = fs[FEATURESET_e7d];
+    p->extd.e8b              = fs[FEATURESET_e8b];
+    p->feat._7d0             = fs[FEATURESET_7d0];
+    p->feat._7a1             = fs[FEATURESET_7a1];
+    p->extd.e21a             = fs[FEATURESET_e21a];
+    p->feat._7b1             = fs[FEATURESET_7b1];
+    p->feat._7d2             = fs[FEATURESET_7d2];
+    p->feat._7c1             = fs[FEATURESET_7c1];
+    p->feat._7d1             = fs[FEATURESET_7d1];
+}
+
 void x86_cpuid_policy_recalc_synth(struct cpuid_policy *p)
 {
     p->x86_vendor = x86_cpuid_lookup_vendor(