diff mbox

[v3,03/15] x86: refactor psr: Remove 'struct psr_cat_cbm'.

Message ID 1477366863-5246-4-git-send-email-yi.y.sun@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yi Sun Oct. 25, 2016, 3:40 a.m. UTC 
'struct psr_cat_cbm' is defined for L3 CAT only. It is not
appropriate for other features. This patch replaces it with
a generic array, 'uint64_t cos_reg_val[MAX_COS_REG_NUM]',
to save values of COS registers. So 'temp_cos_to_cbm' is not
useful anymore, remove it.

Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 xen/arch/x86/psr.c | 93 ++++++++++++++++++++++++++----------------------------
 1 file changed, 45 insertions(+), 48 deletions(-)

Comments

Jan Beulich Nov. 25, 2016, 3:45 p.m. UTC | #1 
>>> On 25.10.16 at 05:40, <yi.y.sun@linux.intel.com> wrote:
>  struct psr_cat_socket_info {
>      unsigned int cbm_len;
>      unsigned int cos_max;
> -    struct psr_cat_cbm *cos_to_cbm;
> +    /*
> +     * Store the values of COS registers:
> +     * CAT uses 1 entry for one COS ID;
> +     * CDP uses 2 entries for one COS ID and DATA is the first one.
> +     */
> +    uint64_t cos_reg_val[MAX_COS_REG_NUM];

Same comment as on the previous patch regarding this fixed size.

> @@ -49,6 +44,21 @@ struct psr_cat_socket_info {
>      spinlock_t ref_lock;
>  };
>  
> +/*
> + * get_data - get DATA COS register value from input COS ID.
> + * @info:        the struct psr_cat_socket_info pointer.
> + * @cos:         the COS ID.
> + */
> +#define get_cdp_data(info, cos) \
> +        info->cos_reg_val[cos * 2]
> +/*
> + * get_cdp_code - get CODE COS register value from input COS ID.
> + * @info:        the struct psr_cat_socket_info pointer.
> + * @cos:         the COS ID.
> + */
> +#define get_cdp_code(info, cos) \
> +        info->cos_reg_val[cos * 2 + 1]

Both macros need to be properly parenthesized.

> @@ -306,6 +314,7 @@ int psr_get_l3_cbm(struct domain *d, unsigned int socket,
>  {
>      struct psr_cat_socket_info *info = get_cat_socket_info(socket);
>      bool_t cdp_enabled = cdp_is_enabled(socket);
> +    unsigned int cos = d->arch.psr_cos_ids[socket];
>  
>      if ( IS_ERR(info) )
>          return PTR_ERR(info);

You must not use socket as an array index before having passed
this check (or else you render the check pointless).

> @@ -417,9 +426,9 @@ static int find_cos(struct psr_cat_cbm *map, unsigned int *ref,
>      for ( cos = 0; cos <= cos_max; cos++ )
>      {
>          if ( (ref[cos] || cos == 0) &&
> -             ((!cdp_enabled && map[cos].cbm == cbm_code) ||
> -              (cdp_enabled && map[cos].code == cbm_code &&
> -                              map[cos].data == cbm_data)) )
> +             ((!cdp_enabled && info->cos_reg_val[cos] == cbm_code) ||
> +              (cdp_enabled && get_cdp_code(info, cos) == cbm_code &&
> +                              get_cdp_data(info, cos) == cbm_data)) )

ref[cos] and get_cdp_code(info, cos) reference different array
indexes, so one could at least suspect ref counting and values
have gone out of sync here (the halving of ->cos_max during
initialization means it is still correct, but that's non-obvious I would
say). If you want per-register ref counting, and you need ref
counts for pairs (or more generally groups) of registers, then I
think you need a better abstraction. For example, what about
adding a referral array, which - when not matching a certain
"unused" indicator - tells which ref[] index holds the ref count.
E.g. the code register would be the one with the ref count, and
the data register would refer to the code ones' array index.

Depending on how much of the above construct is really a
hardware induced requirement (instead of just how software
chooses to handle things), there may of course also a backref
array be necessary then. In which case there may be an
easier to handle alternative model ...

> @@ -583,10 +591,7 @@ static int cat_cpu_prepare(unsigned int cpu)
>      if ( !cat_socket_info )
>          return 0;
>  
> -    if ( temp_cos_to_cbm == NULL &&
> -         (temp_cos_to_cbm = xzalloc_array(struct psr_cat_cbm,
> -                                          MAX_COS_REG_NUM)) == NULL )
> -        return -ENOMEM;
> +    /* Keep this function for future usage. */

Unless you need it later in this series, please don't.

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index ee20389..a0342ce 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -26,20 +26,15 @@ 
 /* Per spec, the maximum COS register number is 128. */
 #define MAX_COS_REG_NUM  128
 
-struct psr_cat_cbm {
-    union {
-        uint64_t cbm;
-        struct {
-            uint64_t code;
-            uint64_t data;
-        };
-    };
-};
-
 struct psr_cat_socket_info {
     unsigned int cbm_len;
     unsigned int cos_max;
-    struct psr_cat_cbm *cos_to_cbm;
+    /*
+     * Store the values of COS registers:
+     * CAT uses 1 entry for one COS ID;
+     * CDP uses 2 entries for one COS ID and DATA is the first one.
+     */
+    uint64_t cos_reg_val[MAX_COS_REG_NUM];
     /*
      * Every entry of cos_ref is the reference count of a COS register.
      * One entry of cos_ref corresponds to one COS ID.
@@ -49,6 +44,21 @@  struct psr_cat_socket_info {
     spinlock_t ref_lock;
 };
 
+/*
+ * get_data - get DATA COS register value from input COS ID.
+ * @info:        the struct psr_cat_socket_info pointer.
+ * @cos:         the COS ID.
+ */
+#define get_cdp_data(info, cos) \
+        info->cos_reg_val[cos * 2]
+/*
+ * get_cdp_code - get CODE COS register value from input COS ID.
+ * @info:        the struct psr_cat_socket_info pointer.
+ * @cos:         the COS ID.
+ */
+#define get_cdp_code(info, cos) \
+        info->cos_reg_val[cos * 2 + 1]
+
 struct psr_assoc {
     uint64_t val;
     uint64_t cos_mask;
@@ -66,8 +76,6 @@  static unsigned int __read_mostly opt_cos_max = MAX_COS_REG_NUM - 1;
 static uint64_t rmid_mask;
 static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
 
-static struct psr_cat_cbm *temp_cos_to_cbm;
-
 static unsigned int get_socket_cpu(unsigned int socket)
 {
     if ( likely(socket < nr_sockets) )
@@ -306,6 +314,7 @@  int psr_get_l3_cbm(struct domain *d, unsigned int socket,
 {
     struct psr_cat_socket_info *info = get_cat_socket_info(socket);
     bool_t cdp_enabled = cdp_is_enabled(socket);
+    unsigned int cos = d->arch.psr_cos_ids[socket];
 
     if ( IS_ERR(info) )
         return PTR_ERR(info);
@@ -315,21 +324,21 @@  int psr_get_l3_cbm(struct domain *d, unsigned int socket,
     case PSR_CBM_TYPE_L3:
         if ( cdp_enabled )
             return -EXDEV;
-        *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].cbm;
+        *cbm = info->cos_reg_val[cos];
         break;
 
     case PSR_CBM_TYPE_L3_CODE:
         if ( !cdp_enabled )
-            *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].cbm;
+            *cbm = info->cos_reg_val[cos];
         else
-            *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].code;
+            *cbm = get_cdp_code(info, cos);
         break;
 
     case PSR_CBM_TYPE_L3_DATA:
         if ( !cdp_enabled )
-            *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].cbm;
+            *cbm = info->cos_reg_val[cos];
         else
-            *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].data;
+            *cbm = get_cdp_data(info, cos);
         break;
 
     default:
@@ -408,7 +417,7 @@  static int write_l3_cbm(unsigned int socket, unsigned int cos,
     return 0;
 }
 
-static int find_cos(struct psr_cat_cbm *map, unsigned int *ref,
+static int find_cos(struct psr_cat_socket_info *info, unsigned int *ref,
                     unsigned int cos_max,
                     uint64_t cbm_code, uint64_t cbm_data, bool_t cdp_enabled)
 {
@@ -417,9 +426,9 @@  static int find_cos(struct psr_cat_cbm *map, unsigned int *ref,
     for ( cos = 0; cos <= cos_max; cos++ )
     {
         if ( (ref[cos] || cos == 0) &&
-             ((!cdp_enabled && map[cos].cbm == cbm_code) ||
-              (cdp_enabled && map[cos].code == cbm_code &&
-                              map[cos].data == cbm_data)) )
+             ((!cdp_enabled && info->cos_reg_val[cos] == cbm_code) ||
+              (cdp_enabled && get_cdp_code(info, cos) == cbm_code &&
+                              get_cdp_data(info, cos) == cbm_data)) )
             return cos;
     }
 
@@ -450,7 +459,6 @@  int psr_set_l3_cbm(struct domain *d, unsigned int socket,
     int cos, ret;
     uint64_t cbm_data, cbm_code;
     bool_t cdp_enabled = cdp_is_enabled(socket);
-    struct psr_cat_cbm *map;
     struct psr_cat_socket_info *info = get_cat_socket_info(socket);
     unsigned int *ref;
 
@@ -466,7 +474,6 @@  int psr_set_l3_cbm(struct domain *d, unsigned int socket,
 
     cos_max = info->cos_max;
     old_cos = d->arch.psr_cos_ids[socket];
-    map = info->cos_to_cbm;
     ref = info->cos_ref;
 
     switch ( type )
@@ -478,11 +485,11 @@  int psr_set_l3_cbm(struct domain *d, unsigned int socket,
 
     case PSR_CBM_TYPE_L3_CODE:
         cbm_code = cbm;
-        cbm_data = map[old_cos].data;
+        cbm_data = get_cdp_data(info, old_cos);
         break;
 
     case PSR_CBM_TYPE_L3_DATA:
-        cbm_code = map[old_cos].code;
+        cbm_code = get_cdp_code(info, old_cos);
         cbm_data = cbm;
         break;
 
@@ -492,7 +499,7 @@  int psr_set_l3_cbm(struct domain *d, unsigned int socket,
     }
 
     spin_lock(&info->ref_lock);
-    cos = find_cos(map, ref, cos_max, cbm_code, cbm_data, cdp_enabled);
+    cos = find_cos(info, ref, cos_max, cbm_code, cbm_data, cdp_enabled);
     if ( cos >= 0 )
     {
         if ( cos == old_cos )
@@ -512,8 +519,9 @@  int psr_set_l3_cbm(struct domain *d, unsigned int socket,
 
         /* We try to avoid writing MSR. */
         if ( (cdp_enabled &&
-             (map[cos].code != cbm_code || map[cos].data != cbm_data)) ||
-             (!cdp_enabled && map[cos].cbm != cbm_code) )
+             (get_cdp_code(info, cos) != cbm_code ||
+              get_cdp_data(info, cos) != cbm_data)) ||
+             (!cdp_enabled && info->cos_reg_val[cos] != cbm_code) )
         {
             ret = write_l3_cbm(socket, cos, cbm_code, cbm_data, cdp_enabled);
             if ( ret )
@@ -521,8 +529,8 @@  int psr_set_l3_cbm(struct domain *d, unsigned int socket,
                 spin_unlock(&info->ref_lock);
                 return ret;
             }
-            map[cos].code = cbm_code;
-            map[cos].data = cbm_data;
+            get_cdp_code(info, cos) = cbm_code;
+            get_cdp_data(info, cos) = cbm_data;
         }
     }
 
@@ -583,10 +591,7 @@  static int cat_cpu_prepare(unsigned int cpu)
     if ( !cat_socket_info )
         return 0;
 
-    if ( temp_cos_to_cbm == NULL &&
-         (temp_cos_to_cbm = xzalloc_array(struct psr_cat_cbm,
-                                          MAX_COS_REG_NUM)) == NULL )
-        return -ENOMEM;
+    /* Keep this function for future usage. */
 
     return 0;
 }
@@ -615,10 +620,8 @@  static void cat_cpu_init(void)
         info->cbm_len = (eax & 0x1f) + 1;
         info->cos_max = min(opt_cos_max, edx & 0xffff);
 
-        info->cos_to_cbm = temp_cos_to_cbm;
-        temp_cos_to_cbm = NULL;
         /* cos=0 is reserved as default cbm(all ones). */
-        info->cos_to_cbm[0].cbm = (1ull << info->cbm_len) - 1;
+        info->cos_reg_val[0] = (1ull << info->cbm_len) - 1;
 
         spin_lock_init(&info->ref_lock);
 
@@ -627,8 +630,10 @@  static void cat_cpu_init(void)
         if ( (ecx & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) &&
              cdp_socket_enable && !test_bit(socket, cdp_socket_enable) )
         {
-            info->cos_to_cbm[0].code = (1ull << info->cbm_len) - 1;
-            info->cos_to_cbm[0].data = (1ull << info->cbm_len) - 1;
+            /* CODE */
+            get_cdp_code(info, 0) = (1ull << info->cbm_len) - 1;
+            /* DATA */
+            get_cdp_data(info, 0) = (1ull << info->cbm_len) - 1;
 
             /* We only write mask1 since mask0 is always all ones by default. */
             wrmsrl(MSR_IA32_PSR_L3_MASK(1), (1ull << info->cbm_len) - 1);
@@ -653,14 +658,6 @@  static void cat_cpu_fini(unsigned int cpu)
 
     if ( !socket_cpumask[socket] || cpumask_empty(socket_cpumask[socket]) )
     {
-        struct psr_cat_socket_info *info = cat_socket_info + socket;
-
-        if ( info->cos_to_cbm )
-        {
-            xfree(info->cos_to_cbm);
-            info->cos_to_cbm = NULL;
-        }
-
         if ( cdp_is_enabled(socket) )
             clear_bit(socket, cdp_socket_enable);