| Message ID | 1491054836-30488-9-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1455,25 +1455,37 @@ 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); > + { > + uint32_t val; > + > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, > + &val, PSR_CBM_TYPE_L3); > + domctl->u.psr_cat_op.data = val; > copyback = 1; > break; > + } > > case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE: > - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, > - &domctl->u.psr_cat_op.data, > - PSR_CBM_TYPE_L3_CODE); > + { > + uint32_t val; > + > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, > + &val, PSR_CBM_TYPE_L3_CODE); > + domctl->u.psr_cat_op.data = val; > copyback = 1; > break; > + } > > case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA: > - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, > - &domctl->u.psr_cat_op.data, > - PSR_CBM_TYPE_L3_DATA); > + { > + uint32_t val; > + > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, > + &val, PSR_CBM_TYPE_L3_DATA); > + domctl->u.psr_cat_op.data = val; > copyback = 1; > break; > + } I think code would read better overall if you had a switch()-wide variable (then probably encoding its width in its name, e.g. val32). > --- 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. > @@ -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. > @@ -521,9 +542,35 @@ 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; > + > + ASSERT(d && val); I don't think we ever ASSERT() domain pointers to be non-NULL. Jan
On 17-04-05 09:51:44, Jan Beulich wrote: > >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: > > --- a/xen/arch/x86/domctl.c > > +++ b/xen/arch/x86/domctl.c > > @@ -1455,25 +1455,37 @@ 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); > > + { > > + uint32_t val; > > + > > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, > > + &val, PSR_CBM_TYPE_L3); > > + domctl->u.psr_cat_op.data = val; > > copyback = 1; > > break; > > + } > > > > case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE: > > - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, > > - &domctl->u.psr_cat_op.data, > > - PSR_CBM_TYPE_L3_CODE); > > + { > > + uint32_t val; > > + > > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, > > + &val, PSR_CBM_TYPE_L3_CODE); > > + domctl->u.psr_cat_op.data = val; > > copyback = 1; > > break; > > + } > > > > case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA: > > - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, > > - &domctl->u.psr_cat_op.data, > > - PSR_CBM_TYPE_L3_DATA); > > + { > > + uint32_t val; > > + > > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, > > + &val, PSR_CBM_TYPE_L3_DATA); > > + domctl->u.psr_cat_op.data = val; > > copyback = 1; > > break; > > + } > > I think code would read better overall if you had a switch()-wide > variable (then probably encoding its width in its name, e.g. val32). > I thought this but the switch() also covers 'set' cases. Is that appropriate to define a wide range variable but some cases do not use it? > > --- 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? > > @@ -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? > > @@ -521,9 +542,35 @@ 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; > > + > > + ASSERT(d && val); > > I don't think we ever ASSERT() domain pointers to be non-NULL. > Ok, will remove check to domain. > Jan > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 06.04.17 at 08:10, <yi.y.sun@linux.intel.com> wrote: > On 17-04-05 09:51:44, Jan Beulich wrote: >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> wrote: >> > --- a/xen/arch/x86/domctl.c >> > +++ b/xen/arch/x86/domctl.c >> > @@ -1455,25 +1455,37 @@ 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); >> > + { >> > + uint32_t val; >> > + >> > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, >> > + &val, PSR_CBM_TYPE_L3); >> > + domctl->u.psr_cat_op.data = val; >> > copyback = 1; >> > break; >> > + } >> > >> > case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE: >> > - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, >> > - &domctl->u.psr_cat_op.data, >> > - PSR_CBM_TYPE_L3_CODE); >> > + { >> > + uint32_t val; >> > + >> > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, >> > + &val, PSR_CBM_TYPE_L3_CODE); >> > + domctl->u.psr_cat_op.data = val; >> > copyback = 1; >> > break; >> > + } >> > >> > case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA: >> > - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, >> > - &domctl->u.psr_cat_op.data, >> > - PSR_CBM_TYPE_L3_DATA); >> > + { >> > + uint32_t val; >> > + >> > + ret = psr_get_val(d, domctl->u.psr_cat_op.target, >> > + &val, PSR_CBM_TYPE_L3_DATA); >> > + domctl->u.psr_cat_op.data = val; >> > copyback = 1; >> > break; >> > + } >> >> I think code would read better overall if you had a switch()-wide >> variable (then probably encoding its width in its name, e.g. val32). >> > I thought this but the switch() also covers 'set' cases. Is that appropriate > to define a wide range variable but some cases do not use it? Yes of course - why would it not be? We also do so elsewhere. >> > --- 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? >> > @@ -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? Jan
On 17-04-06 02:40:01, Jan Beulich wrote: > >>> On 06.04.17 at 08:10, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-05 09:51:44, Jan Beulich wrote: > >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> 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@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 06.04.17 at 13:13, <yi.y.sun@linux.intel.com> wrote: > On 17-04-06 02:40:01, Jan Beulich wrote: >> >>> On 06.04.17 at 08:10, <yi.y.sun@linux.intel.com> wrote: >> > On 17-04-05 09:51:44, Jan Beulich wrote: >> >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> 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] Which makes it for ( i = 0; i < props->cos_num; ++i ) val[i] = feat->cos_reg_val[cos * props->cos_num + i]; ? Jan
On 17-04-06 08:08:11, Jan Beulich wrote: > >>> On 06.04.17 at 13:13, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-06 02:40:01, Jan Beulich wrote: > >> >>> On 06.04.17 at 08:10, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-04-05 09:51:44, Jan Beulich wrote: > >> >> >>> On 01.04.17 at 15:53, <yi.y.sun@linux.intel.com> 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] > > Which makes it > > for ( i = 0; i < props->cos_num; ++i ) > val[i] = feat->cos_reg_val[cos * props->cos_num + i]; > > ? > Great, thanks! Then, I will remove 'get_val' hook. BRs, Sun Yi
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 02b48e8..dc213a7 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1455,25 +1455,37 @@ 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); + { + uint32_t val; + + ret = psr_get_val(d, domctl->u.psr_cat_op.target, + &val, PSR_CBM_TYPE_L3); + domctl->u.psr_cat_op.data = val; copyback = 1; break; + } case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CODE: - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, - &domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3_CODE); + { + uint32_t val; + + ret = psr_get_val(d, domctl->u.psr_cat_op.target, + &val, PSR_CBM_TYPE_L3_CODE); + domctl->u.psr_cat_op.data = val; copyback = 1; break; + } case XEN_DOMCTL_PSR_CAT_OP_GET_L3_DATA: - ret = psr_get_l3_cbm(d, domctl->u.psr_cat_op.target, - &domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3_DATA); + { + uint32_t val; + + ret = psr_get_val(d, domctl->u.psr_cat_op.target, + &val, PSR_CBM_TYPE_L3_DATA); + domctl->u.psr_cat_op.data = val; copyback = 1; break; + } default: ret = -EOPNOTSUPP; diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 36ade48..25fcd21 100644 --- 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]; +} + /* L3 CAT ops */ static struct feat_props l3_cat_props = { .cos_num = 1, .get_feat_info = cat_get_feat_info, + .get_val = cat_get_val, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -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, + 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)); + + feat_type = psr_cbm_type_to_feat_type(type); + if ( feat_type > ARRAY_SIZE(info->features) ) + return NULL; + + return info->features[feat_type]; +} + +int psr_get_info(unsigned int socket, enum cbm_type type, + uint32_t data[], unsigned int array_len) +{ + const struct feat_node *feat; if ( !data ) return -EINVAL; - feat_type = psr_cbm_type_to_feat_type(type); - if ( feat_type > ARRAY_SIZE(info->features) ) - return -ENOENT; + feat = psr_get_feat(socket, type); + if ( IS_ERR(feat) ) + return PTR_ERR(feat); - feat = info->features[feat_type]; if ( !feat ) return -ENOENT; @@ -521,9 +542,35 @@ 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; + + ASSERT(d && val); + + feat = psr_get_feat(socket, type); + if ( IS_ERR(feat) ) + return PTR_ERR(feat); + + if ( !feat ) + return -ENOENT; + + cos = d->arch.psr_cos_ids[socket]; + /* + * If input cos exceeds current feature's cos_max, we should return its + * default value which is stored in cos 0. This case only happens + * when more than two features enabled concurrently and at least one + * features's cos_max is bigger than others. When a domain's working cos + * id is bigger than some features' cos_max, HW automatically works as + * default value for those features which cos_max is smaller. + */ + if ( cos > feat->props->cos_max ) + cos = 0; + + feat->props->get_val(feat, cos, val); + return 0; } diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h index af3a465..7c6d38a 100644 --- a/xen/include/asm-x86/psr.h +++ b/xen/include/asm-x86/psr.h @@ -71,8 +71,8 @@ void psr_ctxt_switch_to(struct domain *d); int psr_get_info(unsigned int socket, enum cbm_type type, uint32_t data[], unsigned int array_len); -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); int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm, enum cbm_type type);
There is an interface in user space to show feature value of domains. This patch implements get value flow in hypervisor including L3 CAT callback function. It also changes domctl interface to make it more general. With this patch, 'psr-cat-show' can work for L3 CAT but not for L3 code/data which is implemented in patch "x86: refactor psr: implement get value flow for CDP.". Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v10: - use an intermediate variable to get value and avoid cast in domctl. (suggested by Jan Beulich) - remove 'type' in 'get_val' parameters and will add it back when implementing CDP. (suggested by Jan Beulich) - remove unnecessary variable and return error about 'info' in 'psr_get_feat'. (suggested by Jan Beulich) - use 'ASSERT' to check input parameter in 'psr_get_val'. (suggested by Jan Beulich) - changes about 'feat_props'. (suggested by Jan Beulich) v9: - add commit message to explain there is an user space interface. - rename 'l3_cat_get_val' to 'cat_get_val' to cover all L3/L2 CAT features. (suggested by Roger Pau) - replace feature list handling to feature array handling. (suggested by Roger Pau) - change parameter of 'psr_get'. Use 'psr_cos_ids' directly to replace domain. Also declare it to 'const'. (suggested by Jan Beulich) - change code flow to remove 'psr_get' but add 'psr_get_feat' to make codes more reasonable. (suggested by Jan Beulich) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - move cos check into common function because this check is required by all features. (suggested by Jan Beulich) - fix coding style issue. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v7: - rename '__psr_get' to 'psr_get'. (suggested by Wei Liu) v6: - modify commit message to make it clearer. (suggested by Konrad Rzeszutek Wilk) - remove one extra space in code. (suggested by Konrad Rzeszutek Wilk) - remove unnecessary comment. (suggested by Konrad Rzeszutek Wilk) - write a helper function to move get info and get val functions into it. Because most codes of 'get_info' and 'get_val' are same. (suggested by Konrad Rzeszutek Wilk) v5: - rename 'dat[]' to 'data[]' (suggested by Jan Beulich) - modify variables names to make them better, e.g. 'feat_tmp' to 'feat'. (suggested by Jan Beulich) - check if feature type match in caller of feature callback function. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/domctl.c | 30 ++++++++++++++------- xen/arch/x86/psr.c | 67 ++++++++++++++++++++++++++++++++++++++++------- xen/include/asm-x86/psr.h | 4 +-- 3 files changed, 80 insertions(+), 21 deletions(-)