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

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-03-27 07:34:43, Jan Beulich wrote:
> >>> On 27.03.17 at 14:59, <yi.y.sun@linux.intel.com> wrote:
> > On 17-03-27 03:23:08, Jan Beulich wrote:
> >> >>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> 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?
> 
> No, as said, it is not. The choice of types is fine, but you need to
> make this work without casts (i.e. presumably with some
> intermediate variable at one of the layers).
> 
Then, I have to use a local variable to do this.

> >> > @@ -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.
> 
> You didn't understand: Your callee has handed you error
> information (a -E... value encoded as a pointer), which you
> discard.
> 
Thanks for explanation! I will modify the type of 'psr_get_feat' to be able to
return such error back.

> Jan

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