| Message ID | 1481688484-5093-13-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -186,6 +186,9 @@ struct feat_ops { > unsigned int (*exceeds_cos_max)(const uint64_t val[], > const struct feat_node *feat, > unsigned int cos); > + /* write_msr is used to write out feature MSR register. */ > + int (*write_msr)(unsigned int cos, const uint64_t val[], > + struct feat_node *feat); Looks like this function again returns number-of-values, yet this time without a comment saying so. While you don't need to replicate that description multiple time, please at least has a brief reference. That said, with the type checks moved out I think this return value model won't be needed anymore - the caller, having checked the type, could then simply call the get-num-val (or however it was named) hook to know how many array entries to skip. > @@ -889,9 +909,67 @@ static int alloc_new_cos(const struct psr_socket_info *info, > return -ENOENT; > } > > +static unsigned int get_socket_cpu(unsigned int socket) > +{ > + if ( likely(socket < nr_sockets) ) > + return cpumask_any(socket_cpumask[socket]); > + > + return nr_cpu_ids; > +} > + > +struct cos_write_info > +{ > + unsigned int cos; > + struct list_head *feat_list; > + const uint64_t *val; > +}; > + > +static void do_write_psr_msr(void *data) > +{ > + struct cos_write_info *info = (struct cos_write_info *)data; > + unsigned int cos = info->cos; > + struct list_head *feat_list= info->feat_list; > + const uint64_t *val = info->val; > + struct feat_node *feat_tmp; > + int ret; > + > + if ( !feat_list ) > + return; > + > + /* We need set all features values into MSRs. */ > + list_for_each_entry(feat_tmp, feat_list, list) > + { > + ret = feat_tmp->ops.write_msr(cos, val, feat_tmp); > + if ( ret <= 0) Missing blank. > + return; > + > + val += ret; > + } > +} > + > static int write_psr_msr(unsigned int socket, unsigned int cos, > const uint64_t *val) > { > + struct psr_socket_info *info = get_socket_info(socket); > + > + struct cos_write_info data = No blank lines between declarations please (unless there are extraordinarily many). Jan
On 17-01-10 08:15:15, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -186,6 +186,9 @@ struct feat_ops { > > unsigned int (*exceeds_cos_max)(const uint64_t val[], > > const struct feat_node *feat, > > unsigned int cos); > > + /* write_msr is used to write out feature MSR register. */ > > + int (*write_msr)(unsigned int cos, const uint64_t val[], > > + struct feat_node *feat); > > Looks like this function again returns number-of-values, yet this time > without a comment saying so. While you don't need to replicate > that description multiple time, please at least has a brief reference. > That said, with the type checks moved out I think this return value > model won't be needed anymore - the caller, having checked the > type, could then simply call the get-num-val (or however it was > named) hook to know how many array entries to skip. > For write msr, we may need iterate the whole feature list to write values for every feature if the input value is not same as old on the COS ID. So, I prefer to keep current return value, the number-of-values handled. That would be clear and easy to implement. Of course, we can call get_cos_num to get the returen value or define a macro to replace the digit. How do you think? > > @@ -889,9 +909,67 @@ static int alloc_new_cos(const struct psr_socket_info *info, > > return -ENOENT; > > } > > > > +static unsigned int get_socket_cpu(unsigned int socket) > > +{ > > + if ( likely(socket < nr_sockets) ) > > + return cpumask_any(socket_cpumask[socket]); > > + > > + return nr_cpu_ids; > > +} > > + > > +struct cos_write_info > > +{ > > + unsigned int cos; > > + struct list_head *feat_list; > > + const uint64_t *val; > > +}; > > + > > +static void do_write_psr_msr(void *data) > > +{ > > + struct cos_write_info *info = (struct cos_write_info *)data; > > + unsigned int cos = info->cos; > > + struct list_head *feat_list= info->feat_list; > > + const uint64_t *val = info->val; > > + struct feat_node *feat_tmp; > > + int ret; > > + > > + if ( !feat_list ) > > + return; > > + > > + /* We need set all features values into MSRs. */ > > + list_for_each_entry(feat_tmp, feat_list, list) > > + { > > + ret = feat_tmp->ops.write_msr(cos, val, feat_tmp); > > + if ( ret <= 0) > > Missing blank. > Sorry. > > + return; > > + > > + val += ret; > > + } > > +} > > + > > static int write_psr_msr(unsigned int socket, unsigned int cos, > > const uint64_t *val) > > { > > + struct psr_socket_info *info = get_socket_info(socket); > > + > > + struct cos_write_info data = > > No blank lines between declarations please (unless there are > extraordinarily many). > Ok, thanks! > Jan
>>> On 11.01.17 at 07:22, <yi.y.sun@linux.intel.com> wrote: > On 17-01-10 08:15:15, Jan Beulich wrote: >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: >> > --- a/xen/arch/x86/psr.c >> > +++ b/xen/arch/x86/psr.c >> > @@ -186,6 +186,9 @@ struct feat_ops { >> > unsigned int (*exceeds_cos_max)(const uint64_t val[], >> > const struct feat_node *feat, >> > unsigned int cos); >> > + /* write_msr is used to write out feature MSR register. */ >> > + int (*write_msr)(unsigned int cos, const uint64_t val[], >> > + struct feat_node *feat); >> >> Looks like this function again returns number-of-values, yet this time >> without a comment saying so. While you don't need to replicate >> that description multiple time, please at least has a brief reference. >> That said, with the type checks moved out I think this return value >> model won't be needed anymore - the caller, having checked the >> type, could then simply call the get-num-val (or however it was >> named) hook to know how many array entries to skip. >> > For write msr, we may need iterate the whole feature list to write values for > every feature if the input value is not same as old on the COS ID. So, I prefer > to keep current return value, the number-of-values handled. That would be clear > and easy to implement. Of course, we can call get_cos_num to get the returen > value or define a macro to replace the digit. How do you think? Well, my general reservation here is that this way you require about half a dozen functions to all return the same value. If the value changes (or if somebody clones the set), there's the risk of one not getting properly updated. Therefore I'd much prefer for just one function to return the count. And I'm relatively certain that with the type checks moved out, this will actually end up being the more natural way. Jan
On 17-01-11 07:01:23, Jan Beulich wrote: > >>> On 11.01.17 at 07:22, <yi.y.sun@linux.intel.com> wrote: > > On 17-01-10 08:15:15, Jan Beulich wrote: > >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: > >> > --- a/xen/arch/x86/psr.c > >> > +++ b/xen/arch/x86/psr.c > >> > @@ -186,6 +186,9 @@ struct feat_ops { > >> > unsigned int (*exceeds_cos_max)(const uint64_t val[], > >> > const struct feat_node *feat, > >> > unsigned int cos); > >> > + /* write_msr is used to write out feature MSR register. */ > >> > + int (*write_msr)(unsigned int cos, const uint64_t val[], > >> > + struct feat_node *feat); > >> > >> Looks like this function again returns number-of-values, yet this time > >> without a comment saying so. While you don't need to replicate > >> that description multiple time, please at least has a brief reference. > >> That said, with the type checks moved out I think this return value > >> model won't be needed anymore - the caller, having checked the > >> type, could then simply call the get-num-val (or however it was > >> named) hook to know how many array entries to skip. > >> > > For write msr, we may need iterate the whole feature list to write values for > > every feature if the input value is not same as old on the COS ID. So, I prefer > > to keep current return value, the number-of-values handled. That would be clear > > and easy to implement. Of course, we can call get_cos_num to get the returen > > value or define a macro to replace the digit. How do you think? > > Well, my general reservation here is that this way you require about > half a dozen functions to all return the same value. If the value > changes (or if somebody clones the set), there's the risk of one not > getting properly updated. Therefore I'd much prefer for just one > function to return the count. And I'm relatively certain that with the > type checks moved out, this will actually end up being the more > natural way. > I imagine the way as your suggestion. It might be below flow for this write_msr. list_for_each_entry(feat...) { feat->write_msr(..., val_array); val_array += feat->get_cos_num(); ...... } Is that what you think? Thanks! > Jan
>>> On 12.01.17 at 02:22, <yi.y.sun@linux.intel.com> wrote: > On 17-01-11 07:01:23, Jan Beulich wrote: >> >>> On 11.01.17 at 07:22, <yi.y.sun@linux.intel.com> wrote: >> > On 17-01-10 08:15:15, Jan Beulich wrote: >> >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: >> >> > --- a/xen/arch/x86/psr.c >> >> > +++ b/xen/arch/x86/psr.c >> >> > @@ -186,6 +186,9 @@ struct feat_ops { >> >> > unsigned int (*exceeds_cos_max)(const uint64_t val[], >> >> > const struct feat_node *feat, >> >> > unsigned int cos); >> >> > + /* write_msr is used to write out feature MSR register. */ >> >> > + int (*write_msr)(unsigned int cos, const uint64_t val[], >> >> > + struct feat_node *feat); >> >> >> >> Looks like this function again returns number-of-values, yet this time >> >> without a comment saying so. While you don't need to replicate >> >> that description multiple time, please at least has a brief reference. >> >> That said, with the type checks moved out I think this return value >> >> model won't be needed anymore - the caller, having checked the >> >> type, could then simply call the get-num-val (or however it was >> >> named) hook to know how many array entries to skip. >> >> >> > For write msr, we may need iterate the whole feature list to write values for >> > every feature if the input value is not same as old on the COS ID. So, I prefer >> > to keep current return value, the number-of-values handled. That would be clear >> > and easy to implement. Of course, we can call get_cos_num to get the returen >> > value or define a macro to replace the digit. How do you think? >> >> Well, my general reservation here is that this way you require about >> half a dozen functions to all return the same value. If the value >> changes (or if somebody clones the set), there's the risk of one not >> getting properly updated. Therefore I'd much prefer for just one >> function to return the count. And I'm relatively certain that with the >> type checks moved out, this will actually end up being the more >> natural way. >> > I imagine the way as your suggestion. It might be below flow for this > write_msr. > > list_for_each_entry(feat...) { > feat->write_msr(..., val_array); > val_array += feat->get_cos_num(); > ...... > } > > Is that what you think? Thanks! Yes, something along these lines. Jan
On 17-01-12 02:40:41, Jan Beulich wrote: > >>> On 12.01.17 at 02:22, <yi.y.sun@linux.intel.com> wrote: > > On 17-01-11 07:01:23, Jan Beulich wrote: > >> >>> On 11.01.17 at 07:22, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-01-10 08:15:15, Jan Beulich wrote: > >> >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote: > >> >> > --- a/xen/arch/x86/psr.c > >> >> > +++ b/xen/arch/x86/psr.c > >> >> > @@ -186,6 +186,9 @@ struct feat_ops { > >> >> > unsigned int (*exceeds_cos_max)(const uint64_t val[], > >> >> > const struct feat_node *feat, > >> >> > unsigned int cos); > >> >> > + /* write_msr is used to write out feature MSR register. */ > >> >> > + int (*write_msr)(unsigned int cos, const uint64_t val[], > >> >> > + struct feat_node *feat); > >> >> > >> >> Looks like this function again returns number-of-values, yet this time > >> >> without a comment saying so. While you don't need to replicate > >> >> that description multiple time, please at least has a brief reference. > >> >> That said, with the type checks moved out I think this return value > >> >> model won't be needed anymore - the caller, having checked the > >> >> type, could then simply call the get-num-val (or however it was > >> >> named) hook to know how many array entries to skip. > >> >> > >> > For write msr, we may need iterate the whole feature list to write values for > >> > every feature if the input value is not same as old on the COS ID. So, I prefer > >> > to keep current return value, the number-of-values handled. That would be clear > >> > and easy to implement. Of course, we can call get_cos_num to get the returen > >> > value or define a macro to replace the digit. How do you think? > >> > >> Well, my general reservation here is that this way you require about > >> half a dozen functions to all return the same value. If the value > >> changes (or if somebody clones the set), there's the risk of one not > >> getting properly updated. Therefore I'd much prefer for just one > >> function to return the count. And I'm relatively certain that with the > >> type checks moved out, this will actually end up being the more > >> natural way. > >> > > I imagine the way as your suggestion. It might be below flow for this > > write_msr. > > > > list_for_each_entry(feat...) { > > feat->write_msr(..., val_array); > > val_array += feat->get_cos_num(); > > ...... > > } > > > > Is that what you think? Thanks! > > Yes, something along these lines. > Got it, thanks! > Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index ac98c39..ec757a2 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -186,6 +186,9 @@ struct feat_ops { unsigned int (*exceeds_cos_max)(const uint64_t val[], const struct feat_node *feat, unsigned int cos); + /* write_msr is used to write out feature MSR register. */ + int (*write_msr)(unsigned int cos, const uint64_t val[], + struct feat_node *feat); }; @@ -441,6 +444,22 @@ static unsigned int l3_cat_exceeds_cos_max(const uint64_t val[], return 1; } +static int l3_cat_write_msr(unsigned int cos, const uint64_t val[], + struct feat_node *feat) +{ + if ( cos > feat->info.l3_cat_info.cos_max ) + /* L3 CAT uses one COS. */ + return 1; + + if ( feat->cos_reg_val[cos] != val[0] ) + { + feat->cos_reg_val[cos] = val[0]; + wrmsrl(MSR_IA32_PSR_L3_MASK(cos), val[0]); + } + /* L3 CAT uses one COS. */ + return 1; +} + struct feat_ops l3_cat_ops = { .init_feature = l3_cat_init_feature, .get_max_cos_max = l3_cat_get_max_cos_max, @@ -452,6 +471,7 @@ struct feat_ops l3_cat_ops = { .get_cos_max_from_type = l3_cat_get_cos_max_from_type, .compare_val = l3_cat_compare_val, .exceeds_cos_max = l3_cat_exceeds_cos_max, + .write_msr = l3_cat_write_msr, }; static void __init parse_psr_bool(char *s, char *value, char *feature, @@ -889,9 +909,67 @@ static int alloc_new_cos(const struct psr_socket_info *info, return -ENOENT; } +static unsigned int get_socket_cpu(unsigned int socket) +{ + if ( likely(socket < nr_sockets) ) + return cpumask_any(socket_cpumask[socket]); + + return nr_cpu_ids; +} + +struct cos_write_info +{ + unsigned int cos; + struct list_head *feat_list; + const uint64_t *val; +}; + +static void do_write_psr_msr(void *data) +{ + struct cos_write_info *info = (struct cos_write_info *)data; + unsigned int cos = info->cos; + struct list_head *feat_list= info->feat_list; + const uint64_t *val = info->val; + struct feat_node *feat_tmp; + int ret; + + if ( !feat_list ) + return; + + /* We need set all features values into MSRs. */ + list_for_each_entry(feat_tmp, feat_list, list) + { + ret = feat_tmp->ops.write_msr(cos, val, feat_tmp); + if ( ret <= 0) + return; + + val += ret; + } +} + static int write_psr_msr(unsigned int socket, unsigned int cos, const uint64_t *val) { + struct psr_socket_info *info = get_socket_info(socket); + + struct cos_write_info data = + { + .cos = cos, + .feat_list = &info->feat_list, + .val = val, + }; + + if ( socket == cpu_to_socket(smp_processor_id()) ) + do_write_psr_msr(&data); + else + { + unsigned int cpu = get_socket_cpu(socket); + + if ( cpu >= nr_cpu_ids ) + return -ENOTSOCK; + on_selected_cpus(cpumask_of(cpu), do_write_psr_msr, &data, 1); + } + return 0; }
Continue with previous patches, we have got all features values and COS ID to set. Then, we write MSRs of all features except the setting value is same as original value. Till now, set value process is completed. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- xen/arch/x86/psr.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)