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

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

On 17-03-27 03:23:08, Jan Beulich wrote:
> >>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -1455,23 +1455,26 @@ 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);
> > +            domctl->u.psr_cat_op.data = 0;
> > +            ret = psr_get_val(d, domctl->u.psr_cat_op.target,
> > +                              (uint32_t *)&domctl->u.psr_cat_op.data,
> 
> This is exactly why I generally object to casts: The high half of
> the field will remain untouched, likely confusing the caller. You
> need to decide at what layer you want to do the extension from
> the internally used 32-bit type to the public interface induced
> 64-bit one.
> 
'psr_cat_op.data' is used as interface between tools/ and hyperviosr. We
defined it as 'uint64_t' to fulfill future requests because MSRs registers
are 64bit although the upper 32bit are not used yet.

Per your suggetion to use 'uint32_t' internally for CBM, I changed
psr_get_val/psr_set_val parameters type from 'uint64_t' to 'uint32_t'. That
is the reason to do cast here. Is this an appropriate choice?

> > @@ -504,21 +515,30 @@ 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 const struct feat_node * psr_get_feat(unsigned int socket,
> > +                                             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 NULL;
> 
> You're losing the error information here - is that intentional?
> 
This function returns the 'struct feat_node *' object. If error happens, it
returns NULL. The caller handles the error.

> > +    feat_type = psr_cbm_type_to_feat_type(type);
> > +    feat = info->features[feat_type];
> > +    return feat;
> 
> No need for at least the intermediate variable "feat". Instead
> please add a blank line before the final return statement.
> 
Sure. Thanks!

> > @@ -528,9 +548,33 @@ 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;
> > +
> > +    if ( !d || !val )
> > +        return -EINVAL;
> 
> Wouldn't this better be an ASSERT()? -EINVAL should generally
> indicate bad hypercall input, not things got wrong internally.
> 
Ok, thanks!

> Jan

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