[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 08/25] x86: refactor psr: L3 CAT: implement get value flow.
On 17-03-27 07:34:43, Jan Beulich wrote: > >>> 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). > 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |