Re: [Xen-devel] [PATCH v12 15/23] x86: refactor psr: CDP: implement set value callback function.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/14/17 3:26 AM >>>
> This patch implements L3 CDP set value related callback function.
> 
> With this patch, 'psr-cat-cbm-set' command can work for L3 CDP.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> ---
> v12:
>     - add comment to explain how to deal with the case that user set new val
>       for both DATA and CODE at same time.
>     - add parameter for 'psr_cbm_type_to_feat_type' to return the feature type
>       according to it.
>     - use the feature type returned by 'psr_cbm_type_to_feat_type' to check
>       if we need insert the new value into all items of the feature value 
> array.
>     - use conditional expression for wrmsrl.
>       (suggested by Jan Beulich)
> ---
>  xen/arch/x86/psr.c | 36 +++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index aee6e3e..91b2122 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -209,7 +209,8 @@ static void free_socket_resources(unsigned int socket)
>      bitmap_zero(info->dom_set, DOMID_IDLE + 1);
>  }
>  
> -static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type)
> +static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type,
> +                                                    bool strict)
>  {
>      enum psr_feat_type feat_type = PSR_SOCKET_FEAT_UNKNOWN;
>  
> @@ -222,7 +223,7 @@ static enum psr_feat_type psr_cbm_type_to_feat_type(enum 
> cbm_type type)
>           * If type is L3 CAT but we cannot find it in feat_props array,
>           * try CDP.
>           */
> -        if ( !feat_props[feat_type] )
> +        if ( !feat_props[feat_type] && !strict )
>              feat_type = PSR_SOCKET_L3_CDP;
>  
>          break;
> @@ -358,11 +359,20 @@ static bool l3_cdp_get_feat_info(const struct feat_node 
> *feat,
>      return true;
>  }
>  
> +static void l3_cdp_write_msr(unsigned int cos, uint32_t val, enum cbm_type 
> type)
> +{
> +    wrmsrl(((type == PSR_CBM_TYPE_L3_DATA) ?
> +            MSR_IA32_PSR_L3_MASK_DATA(cos) :
> +            MSR_IA32_PSR_L3_MASK_CODE(cos)),
> +           val);
> +}
> +
>  static const struct feat_props l3_cdp_props = {
>      .cos_num = 2,
>      .type[0] = PSR_CBM_TYPE_L3_DATA,
>      .type[1] = PSR_CBM_TYPE_L3_CODE,
>      .get_feat_info = l3_cdp_get_feat_info,
> +    .write_msr = l3_cdp_write_msr,

Adding this hook only now means the earlier CDP patches must not be applied on
their own. You should state this prominently (in the patch introducing
l3_cdp_props) for whoever is going to eventually commit (parts of) this series.

> @@ -805,17 +816,24 @@ static int insert_val_into_array(uint32_t val[],
>      if ( !psr_check_cbm(feat->cbm_len, new_val) )
>          return -EINVAL;
>  
> -    /* Value setting position is same as feature array. */
> +    /*
> +     * Value setting position is same as feature array.
> +     * For CDP, user may set both DATA and CODE to same value. For such case,
> +     * user input 'PSR_CBM_TYPE_L3' as type. The strict feature type of
> +     * 'PSR_CBM_TYPE_L3' is L3 CAT. So, we should set new_val to both of DATA
> +     * and CODE under such case.
> +     */
>      for ( i = 0; i < props->cos_num; i++ )
>      {
> -        if ( type == props->type[i] )
> +        if ( type == props->type[i] ||
> +             feat_type != psr_cbm_type_to_feat_type(type, true) )

While I think it is correct (at least up to the L2 CAT additions), it still
seems fragile to me to use != here (effectively allowing any other type to
come back). Couldn't props gain a field indicating the permitted alternative
type?

>          {
>              val[i] = new_val;
> -            return 0;
> +            ret = 0;
>          }

Wouldn't it be better to return -EINVAL in a to be added else branch here
and ...

>      }
>  
> -    return -EINVAL;
> +    return ret;
>  }

... to return zero here?

Jan

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