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-06 02:40:01, Jan Beulich wrote:
> >>> 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/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?
>
For CDP case, the value getting depends on type. If we don't have this hook,
we have to do special handling in main flow.

Still use 'fits_cos_max' as example:
    /* For CDP, DATA is the first item in val[], CODE is the second. */
    for ( j = 0; j < feat->props->cos_num; j++ )                       
    {                                                                  
        feat->props->get_val(feat, 0, feat->props->type[j],            
                             &default_val);                          
        if ( val[j] != default_val )                              
            return false;                                       
    } 

We want to get CDP DATA and CODE one by one to compare with val[] as the order.
If we do not have 'get_val', how can we handle this case? Getting DATA is
different with getting CODE which is shown below. Even we have type-to-index,
we still need the hook to help I think. So far, I cannot figure out a generic
way.
Get data: (feat)->cos_reg_val[(cos) * 2]
Get code: (feat)->cos_reg_val[(cos) * 2 + 1]

Furthermore, 'get_val' is straightforward. I think it loses the pithiness if we
remove the function.

> >> > @@ -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?
> 
Oh, sorry, I mis-understood you. I thought you cannot accept current codes.
But I was wrong.

> 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