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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.04.17 at 08:10, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> 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?

Yes of course - why would it not be? We also do so elsewhere.

>> > --- 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?

No, not really - doesn't the type-to-index mapping array (or
whichever way it ends up being) all take care of the feature
specific aspects here?

>> > @@ -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?

Once again, excuse me: Didn't my previous reply make clear that
while this looks ugly, I can't see a better alternative, and hence
it can remain as is unless someone comes up with a better
suggestion?

Jan

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