| Message ID | 1493801063-38513-16-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -352,9 +352,21 @@ static bool l3_cdp_get_feat_info(const struct feat_node *feat, > return true; > } > > +static void l3_cdp_write_msr(unsigned int cos, uint32_t val, enum cbm_type type) > +{ > + /* Data */ > + if ( type == PSR_CBM_TYPE_L3_DATA ) > + wrmsrl(MSR_IA32_PSR_L3_MASK_DATA(cos), val); > + > + /* Code */ > + if ( type == PSR_CBM_TYPE_L3_CODE ) > + wrmsrl(MSR_IA32_PSR_L3_MASK_CODE(cos), val); > +} With l3_cat_write_msr() ignoring "type" altogether I think this wants to be an if/else pair (or even a conditional expression for the first wrmsrl() argument). > @@ -765,7 +777,8 @@ static int insert_val_into_array(uint32_t val[], > > /* Value setting position is same as feature array. */ > for ( i = 0; i < props->cos_num; i++ ) > - if ( type == props->type[i] ) > + if ( type == props->type[i] || > + (feat_type == PSR_SOCKET_L3_CDP && type == PSR_CBM_TYPE_L3) ) Didn't the earlier patch take care of doing this substitution? Non- feature-specific code clearly shouldn't have such special cases if at all avoidable. Jan
On 17-05-31 03:44:31, Jan Beulich wrote: > >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -352,9 +352,21 @@ static bool l3_cdp_get_feat_info(const struct feat_node *feat, > > return true; > > } > > > > +static void l3_cdp_write_msr(unsigned int cos, uint32_t val, enum cbm_type type) > > +{ > > + /* Data */ > > + if ( type == PSR_CBM_TYPE_L3_DATA ) > > + wrmsrl(MSR_IA32_PSR_L3_MASK_DATA(cos), val); > > + > > + /* Code */ > > + if ( type == PSR_CBM_TYPE_L3_CODE ) > > + wrmsrl(MSR_IA32_PSR_L3_MASK_CODE(cos), val); > > +} > > With l3_cat_write_msr() ignoring "type" altogether I think this wants > to be an if/else pair (or even a conditional expression for the first > wrmsrl() argument). > Ok, thanks! > > @@ -765,7 +777,8 @@ static int insert_val_into_array(uint32_t val[], > > > > /* Value setting position is same as feature array. */ > > for ( i = 0; i < props->cos_num; i++ ) > > - if ( type == props->type[i] ) > > + if ( type == props->type[i] || > > + (feat_type == PSR_SOCKET_L3_CDP && type == PSR_CBM_TYPE_L3) ) > > Didn't the earlier patch take care of doing this substitution? Non- > feature-specific code clearly shouldn't have such special cases if > at all avoidable. > User can set both DATA and CODE to same value at same time with below command: xl psr-cat-set dom_id 0x3ff Because no '-c' or '-d' is input, the cbm type will be 'PSR_CBM_TYPE_L3'. To handle this case, we have to add a special case here. If the cbm tyep is 'PSR_CBM_TYPE_L3' and the feature type is CDP, we set both DATA and CODE. This should be the simplest way to handle this case. > Jan
>>> On 02.06.17 at 09:59, <yi.y.sun@linux.intel.com> wrote: > On 17-05-31 03:44:31, Jan Beulich wrote: >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> > @@ -765,7 +777,8 @@ static int insert_val_into_array(uint32_t val[], >> > >> > /* Value setting position is same as feature array. */ >> > for ( i = 0; i < props->cos_num; i++ ) >> > - if ( type == props->type[i] ) >> > + if ( type == props->type[i] || >> > + (feat_type == PSR_SOCKET_L3_CDP && type == PSR_CBM_TYPE_L3) ) >> >> Didn't the earlier patch take care of doing this substitution? Non- >> feature-specific code clearly shouldn't have such special cases if >> at all avoidable. >> > User can set both DATA and CODE to same value at same time with below > command: > xl psr-cat-set dom_id 0x3ff > > Because no '-c' or '-d' is input, the cbm type will be 'PSR_CBM_TYPE_L3'. > > To handle this case, we have to add a special case here. If the cbm tyep is > 'PSR_CBM_TYPE_L3' and the feature type is CDP, we set both DATA and CODE. This > should be the simplest way to handle this case. Simplest or not, it is not really appropriate to have such special cases here. Along the lines of the earlier abstractions I've recommended (and which, at least afaic, made the overall series quite a bit more comprehensible), please re-consider how this can be done without having special case logic here (I can't immediately suggest an option, I'm sorry). Jan
On 17-06-06 01:48:13, Jan Beulich wrote: > >>> On 02.06.17 at 09:59, <yi.y.sun@linux.intel.com> wrote: > > On 17-05-31 03:44:31, Jan Beulich wrote: > >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > >> > @@ -765,7 +777,8 @@ static int insert_val_into_array(uint32_t val[], > >> > > >> > /* Value setting position is same as feature array. */ > >> > for ( i = 0; i < props->cos_num; i++ ) > >> > - if ( type == props->type[i] ) > >> > + if ( type == props->type[i] || > >> > + (feat_type == PSR_SOCKET_L3_CDP && type == PSR_CBM_TYPE_L3) ) > >> > >> Didn't the earlier patch take care of doing this substitution? Non- > >> feature-specific code clearly shouldn't have such special cases if > >> at all avoidable. > >> > > User can set both DATA and CODE to same value at same time with below > > command: > > xl psr-cat-set dom_id 0x3ff > > > > Because no '-c' or '-d' is input, the cbm type will be 'PSR_CBM_TYPE_L3'. > > > > To handle this case, we have to add a special case here. If the cbm tyep is > > 'PSR_CBM_TYPE_L3' and the feature type is CDP, we set both DATA and CODE. This > > should be the simplest way to handle this case. > > Simplest or not, it is not really appropriate to have such special cases > here. Along the lines of the earlier abstractions I've recommended > (and which, at least afaic, made the overall series quite a bit more > comprehensible), please re-consider how this can be done without > having special case logic here (I can't immediately suggest an option, > I'm sorry). > How about a callback function here to handle this insertion? For L3/L2 CAT, use a function just to assign new_val to val[]. For CDP, in its callback function, check 'type' to decide insert new_val to both DATA and CODE or just one item according to type. > Jan
>>> On 06.06.17 at 10:22, <yi.y.sun@linux.intel.com> wrote: > On 17-06-06 01:48:13, Jan Beulich wrote: >> >>> On 02.06.17 at 09:59, <yi.y.sun@linux.intel.com> wrote: >> > On 17-05-31 03:44:31, Jan Beulich wrote: >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> >> > @@ -765,7 +777,8 @@ static int insert_val_into_array(uint32_t val[], >> >> > >> >> > /* Value setting position is same as feature array. */ >> >> > for ( i = 0; i < props->cos_num; i++ ) >> >> > - if ( type == props->type[i] ) >> >> > + if ( type == props->type[i] || >> >> > + (feat_type == PSR_SOCKET_L3_CDP && type == PSR_CBM_TYPE_L3) ) >> >> >> >> Didn't the earlier patch take care of doing this substitution? Non- >> >> feature-specific code clearly shouldn't have such special cases if >> >> at all avoidable. >> >> >> > User can set both DATA and CODE to same value at same time with below >> > command: >> > xl psr-cat-set dom_id 0x3ff >> > >> > Because no '-c' or '-d' is input, the cbm type will be 'PSR_CBM_TYPE_L3'. >> > >> > To handle this case, we have to add a special case here. If the cbm tyep is >> > 'PSR_CBM_TYPE_L3' and the feature type is CDP, we set both DATA and CODE. This >> > should be the simplest way to handle this case. >> >> Simplest or not, it is not really appropriate to have such special cases >> here. Along the lines of the earlier abstractions I've recommended >> (and which, at least afaic, made the overall series quite a bit more >> comprehensible), please re-consider how this can be done without >> having special case logic here (I can't immediately suggest an option, >> I'm sorry). >> > How about a callback function here to handle this insertion? For L3/L2 CAT, > use a function just to assign new_val to val[]. For CDP, in its callback > function, check 'type' to decide insert new_val to both DATA and CODE or just > one item according to type. Well, I'm not sure what to say. The history of this series tells me that you suggesting a new callback is likely to be not better than having open coded special case logic here. IOW neither is a good (or should I say preferred) solution here, and I'm relatively certain (as I had been with all the other callbacks that are now gone) that there is a reasonably clean solution without either, by simply using suitable abstracted data structures. As expressed back then, even if I can't immediately suggest how to make this work, I'm still insisting that you at least try to come up with a clean solution here. Jan
On 17-06-06 02:43:42, Jan Beulich wrote: > >>> On 06.06.17 at 10:22, <yi.y.sun@linux.intel.com> wrote: > > On 17-06-06 01:48:13, Jan Beulich wrote: > >> >>> On 02.06.17 at 09:59, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-05-31 03:44:31, Jan Beulich wrote: > >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: > >> >> > @@ -765,7 +777,8 @@ static int insert_val_into_array(uint32_t val[], > >> >> > > >> >> > /* Value setting position is same as feature array. */ > >> >> > for ( i = 0; i < props->cos_num; i++ ) > >> >> > - if ( type == props->type[i] ) > >> >> > + if ( type == props->type[i] || > >> >> > + (feat_type == PSR_SOCKET_L3_CDP && type == PSR_CBM_TYPE_L3) ) > >> >> > >> >> Didn't the earlier patch take care of doing this substitution? Non- > >> >> feature-specific code clearly shouldn't have such special cases if > >> >> at all avoidable. > >> >> > >> > User can set both DATA and CODE to same value at same time with below > >> > command: > >> > xl psr-cat-set dom_id 0x3ff > >> > > >> > Because no '-c' or '-d' is input, the cbm type will be 'PSR_CBM_TYPE_L3'. > >> > > >> > To handle this case, we have to add a special case here. If the cbm tyep is > >> > 'PSR_CBM_TYPE_L3' and the feature type is CDP, we set both DATA and CODE. This > >> > should be the simplest way to handle this case. > >> > >> Simplest or not, it is not really appropriate to have such special cases > >> here. Along the lines of the earlier abstractions I've recommended > >> (and which, at least afaic, made the overall series quite a bit more > >> comprehensible), please re-consider how this can be done without > >> having special case logic here (I can't immediately suggest an option, > >> I'm sorry). > >> > > How about a callback function here to handle this insertion? For L3/L2 CAT, > > use a function just to assign new_val to val[]. For CDP, in its callback > > function, check 'type' to decide insert new_val to both DATA and CODE or just > > one item according to type. > > Well, I'm not sure what to say. The history of this series tells me > that you suggesting a new callback is likely to be not better than > having open coded special case logic here. IOW neither is a good > (or should I say preferred) solution here, and I'm relatively > certain (as I had been with all the other callbacks that are now > gone) that there is a reasonably clean solution without either, by > simply using suitable abstracted data structures. As expressed > back then, even if I can't immediately suggest how to make this > work, I'm still insisting that you at least try to come up with a > clean solution here. > Ok, I should think more. :) Then, please check below solution. This case only happens in CDP mode that the input cbm_type corresponds to L3 CAT but current feat_type is CDP. In all other modes, the input cbm_type corresponds to its own mode. So, maybe we can implement codes as below. //Add an input parameter 'bool strict' static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type, boot strict) { ... switch ( type ) { case PSR_CBM_TYPE_L3: feat_type = PSR_SOCKET_L3_CAT; /* * If type is L3 CAT but we cannot find it in feat_props array, * try CDP. */ if ( !feat_props[feat_type] && !strict ) feat_type = PSR_SOCKET_L3_CDP; break; case PSR_CBM_TYPE_L3_DATA: case PSR_CBM_TYPE_L3_CODE: feat_type = PSR_SOCKET_L3_CDP; break; ... } //Input feat_type is PSR_SOCKET_L3_CDP, type is PSR_CBM_TYPE_L3 static int insert_val_into_array(feat_type, type) { ... for ( i = 0; i < props->cos_num; i++ ) { if ( type == props->type[i] || feat_type != psr_cbm_type_to_feat_type(type, true) ) { val[i] = new_val; ret = 0; } } return ret; }
>>> On 06.06.17 at 12:43, <yi.y.sun@linux.intel.com> wrote: > Ok, I should think more. :) > > Then, please check below solution. > > This case only happens in CDP mode that the input cbm_type corresponds to L3 > CAT but current feat_type is CDP. In all other modes, the input cbm_type > corresponds to its own mode. So, maybe we can implement codes as below. > > //Add an input parameter 'bool strict' > static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type, boot > strict) > { > ... > switch ( type ) > { > case PSR_CBM_TYPE_L3: > feat_type = PSR_SOCKET_L3_CAT; > > /* > * If type is L3 CAT but we cannot find it in feat_props array, > * try CDP. > */ > if ( !feat_props[feat_type] && !strict ) > feat_type = PSR_SOCKET_L3_CDP; > > break; > > case PSR_CBM_TYPE_L3_DATA: > case PSR_CBM_TYPE_L3_CODE: > feat_type = PSR_SOCKET_L3_CDP; > break; > ... > } > > //Input feat_type is PSR_SOCKET_L3_CDP, type is PSR_CBM_TYPE_L3 > static int insert_val_into_array(feat_type, type) > { > ... > for ( i = 0; i < props->cos_num; i++ ) > { > if ( type == props->type[i] || > feat_type != psr_cbm_type_to_feat_type(type, true) ) > { > val[i] = new_val; > ret = 0; > } > } > > return ret; > } Thanks, this looks better, but I will want to see this in context of the next version of the patch series before making up a final opinion. Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 55ac221..bfdc20f 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -352,9 +352,21 @@ static bool l3_cdp_get_feat_info(const struct feat_node *feat, return true; } +static void l3_cdp_write_msr(unsigned int cos, uint32_t val, enum cbm_type type) +{ + /* Data */ + if ( type == PSR_CBM_TYPE_L3_DATA ) + wrmsrl(MSR_IA32_PSR_L3_MASK_DATA(cos), val); + + /* Code */ + if ( type == PSR_CBM_TYPE_L3_CODE ) + wrmsrl(MSR_IA32_PSR_L3_MASK_CODE(cos), val); +} + static struct feat_props l3_cdp_props = { .cos_num = 2, .get_feat_info = l3_cdp_get_feat_info, + .write_msr = l3_cdp_write_msr, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -765,7 +777,8 @@ static int insert_val_into_array(uint32_t val[], /* Value setting position is same as feature array. */ for ( i = 0; i < props->cos_num; i++ ) - if ( type == props->type[i] ) + if ( type == props->type[i] || + (feat_type == PSR_SOCKET_L3_CDP && type == PSR_CBM_TYPE_L3) ) val[i] = new_val; return 0;
This patch implements L3 CDP set value related callback function. With this patch, 'psr-cat-cbm-set' command can work for L3 CDP. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v11: - move 'feat->cos_reg_val' assignment and value comparison in 'write_msr' callback function out as generic codes. (suggested by Jan Beulich) - changes about setting both CDP DATA and CODE at same time. - move 'type[]' declaration into previous patch which introduced 'cos_num'. (suggested by Jan Beulich) - changes about 'type[]'. (suggested by Jan Beulich) - move 'compare_val' to previous patch. (suggested by Jan Beulich) - changes about 'get_val' which has been replace by generic codes. (suggested by Jan Beulich) - remove 'restore_default_val' which is unnecessary now. (suggested by Jan Beulich) v10: - remove 'l3_cdp_get_old_val' and use 'l3_cdp_get_val' to replace it. (suggested by Jan Beulich) - remove 'l3_cdp_set_new_val'. - modify 'insert_val_to_array' flow to handle multiple COSs case. (suggested by Jan Beulich) - remove 'l3_cdp_compare_val' and implement a generic function 'comapre_val'. (suggested by Jan Beulich) - remove 'l3_cdp_fits_cos_max'. (suggested by Jan Beulich) - introduce macro 'PSR_MAX_COS_NUM'. - introduce a new member in 'feat_props', 'type[PSR_MAX_COS_NUM]' to record all 'cbm_type' the feature has. (suggested by Jan Beulich) - modify 'gather_val_array' flow to handle multiple COSs case. (suggested by Jan Beulich) - modify 'find_cos' flow and implement 'compare_val' to handle multiple COSs case. (suggested by Jan Beulich) - modify 'fits_cos_max' flow to handle multiple COSs case. (suggested by Jan Beulich) - changes about 'props'. (suggested by Jan Beulich) - remove cast in 'l3_cdp_write_msr'. (suggested by Jan Beulich) - implement 'compare_val' function to compare if feature values are what we expect in finding flow. - implement 'restore_default_val' function to restore feature's COS values to default if the feature has multiple COSs. It is called when the COS ID is reduced to 0. v9: - add comment to explain why CDP uses 2 COSs. (suggested by Wei Liu) - use 'cat_default_val'. (suggested by Wei Liu) - remove 'l3_cdp_get_cos_num' because we can directly get cos_num from feat_node now. (suggested by Jan Beulich) - remove cos checking because it has been moved to common function. (suggested by Jan Beulich) - l3_cdp_set_new_val parameter 'm' is changed to 'new_val'. (suggested by Jan Beulich) - directly use get_cdp_data(feat, 0) and get_cdp_code(feat, 0) to get default value. (suggested by Jan Beulich) - modify 'l3_cdp_write_msr' flow to write value into register according to input type. - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) v8: - modify 'l3_cdp_write_msr' type to 'void'. v5: - remove type check in callback function. (suggested by Jan Beulich) - modify return value of callback functions because we do not need them to return number of entries the feature uses. In caller, we call 'get_cos_num' to get the number of entries the feature uses. (suggested by Jan Beulich) - remove 'l3_cdp_get_cos_max_from_type'. - rename 'l3_cdp_exceeds_cos_max' to 'l3_cdp_fits_cos_max'. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/psr.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)