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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.17 at 14:59, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> 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?

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).

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

Jan


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