Re: [Xen-devel] [PATCH v10 08/25] x86: refactor psr: L3 CAT: implement get value flow.

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-04-05 09:51:44, Jan Beulich wrote:
> >>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -1455,25 +1455,37 @@ long arch_do_domctl(
> >              break;
> >  
> >          case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM:
> > -            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> > -                                 &domctl->u.psr_cat_op.data,
> > -                                 PSR_CBM_TYPE_L3);
> > +        {
> > +            uint32_t val;
> > +
> > +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +                              &val, PSR_CBM_TYPE_L3);
> > +            domctl->u.psr_cat_op.data = val;
> >              copyback = 1;
> >              break;
> > +        }
> >  
> >          case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE:
> > -            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> > -                                 &domctl->u.psr_cat_op.data,
> > -                                 PSR_CBM_TYPE_L3_CODE);
> > +        {
> > +            uint32_t val;
> > +
> > +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +                              &val, PSR_CBM_TYPE_L3_CODE);
> > +            domctl->u.psr_cat_op.data = val;
> >              copyback = 1;
> >              break;
> > +        }
> >  
> >          case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA:
> > -            ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target,
> > -                                 &domctl->u.psr_cat_op.data,
> > -                                 PSR_CBM_TYPE_L3_DATA);
> > +        {
> > +            uint32_t val;
> > +
> > +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +                              &val, PSR_CBM_TYPE_L3_DATA);
> > +            domctl->u.psr_cat_op.data = val;
> >              copyback = 1;
> >              break;
> > +        }
> 
> I think code would read better overall if you had a switch()-wide
> variable (then probably encoding its width in its name, e.g. val32).
> 
I thought this but the switch() also covers 'set' cases. Is that appropriate
to define a wide range variable but some cases do not use it?

> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -97,6 +97,10 @@ struct feat_node {
> >          /* get_feat_info is used to get feature HW info. */
> >          bool (*get_feat_info)(const struct feat_node *feat,
> >                                uint32_t data[], unsigned int array_len);
> > +
> > +        /* get_val is used to get feature COS register value. */
> > +        void (*get_val)(const struct feat_node *feat, unsigned int cos,
> > +                        uint32_t *val);
> >      } *props;
> >  
> >      uint32_t cos_reg_val[MAX_COS_REG_CNT];
> > @@ -265,10 +269,17 @@ static bool cat_get_feat_info(const struct feat_node 
> > *feat,
> >      return true;
> >  }
> >  
> > +static void cat_get_val(const struct feat_node *feat, unsigned int cos,
> > +                        uint32_t *val)
> > +{
> > +    *val = feat->cos_reg_val[cos];
> > +}
> 
> This can be done by the caller - there's nothing feature specific in
> here, so there's no need for a hook.
> 
Hmm, CDP's 'get_val' is different so that we need this hook. Do you mean I
should create this CAT's 'get_val' hook when implementing CDP patch?

> > @@ -494,24 +505,34 @@ 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 struct feat_node * psr_get_feat(unsigned int socket,
> 
> Stray blank after *.
> 
> > +                                       enum cbm_type type)
> >  {
> >      const struct psr_socket_info *info = get_socket_info(socket);
> > -    const struct feat_node *feat;
> >      enum psr_feat_type feat_type;
> >  
> >      if ( IS_ERR(info) )
> > -        return PTR_ERR(info);
> > +        return ERR_PTR(PTR_ERR(info));
> 
> Urgh. But yes, a cast would seem to be the worse alternative.
> 
Then, any suggestion for this? Shall I add a parameter into the function to
get this error number back?

> > @@ -521,9 +542,35 @@ int psr_get_info(unsigned int socket, enum cbm_type 
> > type,
> >      return -EINVAL;
> >  }
> >  
> > -int psr_get_l3_cbm(struct domain *d, unsigned int socket,
> > -                   uint64_t *cbm, enum cbm_type type)
> > +int psr_get_val(struct domain *d, unsigned int socket,
> > +                uint32_t *val, enum cbm_type type)
> >  {
> > +    const struct feat_node *feat;
> > +    unsigned int cos;
> > +
> > +    ASSERT(d && val);
> 
> I don't think we ever ASSERT() domain pointers to be non-NULL.
> 
Ok, will remove check to domain.

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

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