Re: [Xen-devel] [PATCH v8 20/24] x86: L2 CAT: implement set value flow.

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Feb 15, 2017 at 04:49:35PM +0800, Yi Sun wrote:
> This patch implements L2 CAT set value related callback functions
> and domctl interface.
> 
> Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> ---
> v8:
>     - modify 'l2_cat_write_msr' to 'void'.
> ---
>  xen/arch/x86/domctl.c           |  6 +++
>  xen/arch/x86/psr.c              | 95 
> +++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/msr-index.h |  1 +
>  xen/include/public/domctl.h     |  1 +
>  4 files changed, 103 insertions(+)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 68c2d60..38dc087 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1439,6 +1439,12 @@ long arch_do_domctl(
>                                PSR_CBM_TYPE_L3_DATA);
>              break;
>  
> +        case XEN_DOMCTL_PSR_CAT_OP_SET_L2_CBM:
> +            ret = psr_set_val(d, domctl->u.psr_cat_op.target,
> +                              domctl->u.psr_cat_op.data,
> +                              PSR_CBM_TYPE_L2);
> +            break;
> +
>          case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
>              ret = psr_get_val(d, domctl->u.psr_cat_op.target,
>                                &domctl->u.psr_cat_op.data,
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 2adf62c..5604e03 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -755,10 +755,105 @@ static bool l2_cat_get_val(const struct feat_node 
> *feat, unsigned int cos,
>      return true;
>  }
>  
> +static unsigned int l2_cat_get_cos_num(const struct feat_node *feat)
> +{
> +    /* L2 CAT uses one COS. */
> +    return 1;
> +}
> +
> +static int l2_cat_get_old_val(uint64_t val[],
> +                              const struct feat_node *feat,
> +                              unsigned int old_cos)
> +{
> +    if ( old_cos > feat->info.l2_cat_info.cos_max )
> +        /* Use default value. */
> +        old_cos = 0;
> +
> +    val[0] = feat->cos_reg_val[old_cos];
> +
> +    return 0;
> +}
> +
> +static int l2_cat_set_new_val(uint64_t val[],
> +                              const struct feat_node *feat,
> +                              enum cbm_type type,
> +                              uint64_t m)
> +{
> +    if ( !psr_check_cbm(feat->info.l2_cat_info.cbm_len, m) )
> +        return -EINVAL;
> +
> +    val[0] = m;
> +
> +    return 0;
> +}
> +
> +static int l2_cat_compare_val(const uint64_t val[],
> +                              const struct feat_node *feat,
> +                              unsigned int cos, bool *found)
> +{
> +    uint64_t l2_def_cbm;
> +
> +    l2_def_cbm = (1ull << feat->info.l2_cat_info.cbm_len) - 1;
> +
> +    if ( cos > feat->info.l2_cat_info.cos_max )
> +    {
> +        if ( val[0] != l2_def_cbm )
> +        {
> +            *found = false;
> +            return -ENOENT;
> +        }
> +        *found = true;
> +    }
> +    else
> +        *found = (val[0] == feat->cos_reg_val[cos]);
> +
> +    return 0;

The logic of this function is kind of weird IMHO, you seem to be able to return
an error, and also a parameter that indicates success ("found"). Can't this be
simplified to simply return an error code, and the found parameter removed?

Roger.

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