Re: [Xen-devel] [PATCH v8 07/24] x86: refactor psr: implement get value flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> @@ -260,9 +263,22 @@ static bool l3_cat_get_feat_info(const struct feat_node 
> *feat,
>      return true;
>  }
>  
> +static bool l3_cat_get_val(const struct feat_node *feat, unsigned int cos,
> +                           enum cbm_type type, uint64_t *val)
> +{
> +    if ( cos > feat->info.l3_cat_info.cos_max )
> +        /* Use default value. */
> +        cos = 0;
> +
> +    *val = feat->cos_reg_val[cos];
> +
> +    return true;

This one never failing I wonder whether the same will apply to the
later ones. If so, there's little point in returning a boolean here, but
instead you could return the value instead of using indirection.

>  static void __init parse_psr_bool(char *s, char *value, char *feature,
> @@ -482,12 +498,14 @@ static struct psr_socket_info *get_socket_info(unsigned 
> int socket)
>      return socket_info + socket;
>  }
>  
> -int psr_get_info(unsigned int socket, enum cbm_type type,
> -                 uint32_t data[], unsigned int array_len)
> +static int psr_get(unsigned int socket, enum cbm_type type,

The immediately preceding patch introduced thus function, and
now you're changing its name. Please give it the intended final
name right away.

> +                   uint32_t data[], unsigned int array_len,
> +                   struct domain *d, uint64_t *val)

const struct domain *, but I'm not even sure that's an appropriate
parameter here:

> @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type type,
>          if ( feat->feature != feat_type )
>              continue;
>  
> +        if ( d )
> +        {
> +            cos = d->arch.psr_cos_ids[socket];

You could equally well pass a more constrained pointer, like
psr_cos_ids[] on its own. But of course much depends on whether
you'll need d for other things in this function in later patches.

> +            if ( feat->ops.get_val(feat, cos, type, val) )
> +                return 0;
> +            else
> +                break;
> +        }
> +
>          if ( feat->ops.get_feat_info(feat, data, array_len) )
>              return 0;
>          else

Looking at the context here - is it really a good idea to overload the
function in this way, rather than creating a second one? Your
only complicating the live of the callers, as can be seen e.g. ...

> @@ -507,10 +534,16 @@ int psr_get_info(unsigned int socket, enum cbm_type 
> type,
>      return -ENOENT;
>  }
>  
> -int psr_get_l3_cbm(struct domain *d, unsigned int socket,
> -                   uint64_t *cbm, enum cbm_type type)
> +int psr_get_info(unsigned int socket, enum cbm_type type,
> +                 uint32_t data[], unsigned int array_len)
> +{
> +    return psr_get(socket, type, data, array_len, NULL, NULL);

... here and ...

> +}
> +
> +int psr_get_val(struct domain *d, unsigned int socket,
> +                uint64_t *val, enum cbm_type type)
>  {
> -    return 0;
> +    return psr_get(socket, type, NULL, 0, d, val);
>  }

... here (it is a bad sign that both pass NULL on either side).

Jan


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