Re: [Xen-devel] [PATCH v3 03/15] x86: refactor psr: Remove 'struct psr_cat_cbm'.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.10.16 at 05:40, <yi.y.sun@xxxxxxxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel