| Message ID | 1477366863-5246-5-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 25.10.16 at 05:40, <yi.y.sun@linux.intel.com> wrote: > 'cbm_len' and 'cbm_max' are CAT/CDP specific feature HW info. > So encapsulate them into 'struct psr_cat_hw_info'. If new > feature is supported, we can define other structure to save > its HW info. Part of my problem following you here is that you talk about cbm_max, but the code changes cos_max, which so far I had understood would be a generic limit, which appears to be supported by ... > @@ -26,9 +26,14 @@ > /* Per spec, the maximum COS register number is 128. */ > #define MAX_COS_REG_NUM 128 > > -struct psr_cat_socket_info { > +/* CAT/CDP HW info data structure. */ > +struct psr_cat_hw_info { > unsigned int cbm_len; > unsigned int cos_max; > +}; > + > +struct psr_cat_socket_info { > + struct psr_cat_hw_info l3_info; > /* > * Store the values of COS registers: > * CAT uses 1 entry for one COS ID; ... you leaving this comment in place. Btw., perhaps it would also help if psr.c had (near its beginning) a glossary of the various acronyms. By having a way to quickly refresh one's memory of what COS, CAT, CBM, and who know what else stand for, it might be a little easier to reason about changes like this one. Jan
>>> On 25.11.16 at 17:27, <JBeulich@suse.com> wrote: >>>> On 25.10.16 at 05:40, <yi.y.sun@linux.intel.com> wrote: >> 'cbm_len' and 'cbm_max' are CAT/CDP specific feature HW info. >> So encapsulate them into 'struct psr_cat_hw_info'. If new >> feature is supported, we can define other structure to save >> its HW info. > > Part of my problem following you here is that you talk about > cbm_max, but the code changes cos_max, which so far I had > understood would be a generic limit, So I've gone and looked back at patch 1, where indeed you say the limits might differ. Which raises the question then what opt_cos_max is representing. Having seen v1, v2, and v3 up to patch 5 I start wondering whether the whole current code wouldn't better be ripped out and then be replaced by something written from scratch. That's because the split, while having reduced individual patch size, doesn't really make the whole thing much better reviewable. And the original implementation apparently simply didn't have in mind how future additions on the hardware side could look like. Thoughts? Jan
On 16-11-25 09:57:31, Jan Beulich wrote: > >>> On 25.11.16 at 17:27, <JBeulich@suse.com> wrote: > >>>> On 25.10.16 at 05:40, <yi.y.sun@linux.intel.com> wrote: > >> 'cbm_len' and 'cbm_max' are CAT/CDP specific feature HW info. > >> So encapsulate them into 'struct psr_cat_hw_info'. If new > >> feature is supported, we can define other structure to save > >> its HW info. > > > > Part of my problem following you here is that you talk about > > cbm_max, but the code changes cos_max, which so far I had > > understood would be a generic limit, > > So I've gone and looked back at patch 1, where indeed you say > the limits might differ. Which raises the question then what > opt_cos_max is representing. > > Having seen v1, v2, and v3 up to patch 5 I start wondering > whether the whole current code wouldn't better be ripped out > and then be replaced by something written from scratch. That's > because the split, while having reduced individual patch size, > doesn't really make the whole thing much better reviewable. > And the original implementation apparently simply didn't have > in mind how future additions on the hardware side could look > like. > > Thoughts? > > Jan Firstly, I want to say sorry that the V3 still does not make you feel good. I planned to split the big patch based on data structure changes to make you understand more easily. The original implementation of psr.c does not consider much about future features because there was only L3 CAT introduced and we do not know there will be new features and how they work. That is the reason I refactor the psr.c to make it be easy to add new features by abstracting the common things. To make codes be better reviewable, I propose below three suggestions: 1) I write a design document about refactor to make you more easily understand the ideas. 2) Replace the psr.c with a new file which discards all old codes so that you do not need to consider old implementations which may cause confusion. 3) Discard the refactor codes. Just implement L2 CAT based on current codes. Because L2 CAT does not have much difference with L3. How do you think? If you can offer your ideas to design and implement the codes, that would be a great opportunity for me to learn! Thank you! BRs, Sun Yi
>>> On 29.11.16 at 05:38, <yi.y.sun@linux.intel.com> wrote: > To make codes be better reviewable, I propose below three suggestions: > 1) I write a design document about refactor to make you more easily > understand the ideas. This is appreciated, but I don't think it's strictly necessary. Describing the new design (rather than the changes to the existing one) is what likely would be more useful (I'm sorry if I've misunderstood what you said, and you in fact had meant just this), which iirc you already have in patch 1. > 2) Replace the psr.c with a new file which discards all old codes so > that you do not need to consider old implementations which may cause > confusion. > 3) Discard the refactor codes. Just implement L2 CAT based on current > codes. Because L2 CAT does not have much difference with L3. I don't think introducing a new file is the ideal approach. I'd suggest to rip out the entire implementation in a first patch, leaving just empty functions to avoid breaking the build (i.e. perhaps mostly the ones used by domctl/sysctl, and maybe some init one). Then introduce new code, ideally of course not in one big patch, but broken up into logical pieces where possible (one such split would be that of course you don't need to re-implement domctl/sysctl handling in the same patch as everything else). Jan
On 16-11-29 02:43:24, Jan Beulich wrote: > >>> On 29.11.16 at 05:38, <yi.y.sun@linux.intel.com> wrote: > > To make codes be better reviewable, I propose below three suggestions: > > 1) I write a design document about refactor to make you more easily > > understand the ideas. > > This is appreciated, but I don't think it's strictly necessary. Describing > the new design (rather than the changes to the existing one) is what > likely would be more useful (I'm sorry if I've misunderstood what you > said, and you in fact had meant just this), which iirc you already have > in patch 1. > > > 2) Replace the psr.c with a new file which discards all old codes so > > that you do not need to consider old implementations which may cause > > confusion. > > 3) Discard the refactor codes. Just implement L2 CAT based on current > > codes. Because L2 CAT does not have much difference with L3. > > I don't think introducing a new file is the ideal approach. I'd suggest > to rip out the entire implementation in a first patch, leaving just > empty functions to avoid breaking the build (i.e. perhaps mostly the > ones used by domctl/sysctl, and maybe some init one). Then > introduce new code, ideally of course not in one big patch, but > broken up into logical pieces where possible (one such split would be > that of course you don't need to re-implement domctl/sysctl handling > in the same patch as everything else). > > Jan > Thanks for your suggestion! Just want to confirm if my understanding is right. So, you mean I can remove all old codes but keep the interfaces as empty functions to make sure the build can pass. Then, implement whole functionality step by step. Right? Thanks, Sun Yi
>>> On 30.11.16 at 10:08, <yi.y.sun@linux.intel.com> wrote: > On 16-11-29 02:43:24, Jan Beulich wrote: >> >>> On 29.11.16 at 05:38, <yi.y.sun@linux.intel.com> wrote: >> > To make codes be better reviewable, I propose below three suggestions: >> > 1) I write a design document about refactor to make you more easily >> > understand the ideas. >> >> This is appreciated, but I don't think it's strictly necessary. Describing >> the new design (rather than the changes to the existing one) is what >> likely would be more useful (I'm sorry if I've misunderstood what you >> said, and you in fact had meant just this), which iirc you already have >> in patch 1. >> >> > 2) Replace the psr.c with a new file which discards all old codes so >> > that you do not need to consider old implementations which may cause >> > confusion. >> > 3) Discard the refactor codes. Just implement L2 CAT based on current >> > codes. Because L2 CAT does not have much difference with L3. >> >> I don't think introducing a new file is the ideal approach. I'd suggest >> to rip out the entire implementation in a first patch, leaving just >> empty functions to avoid breaking the build (i.e. perhaps mostly the >> ones used by domctl/sysctl, and maybe some init one). Then >> introduce new code, ideally of course not in one big patch, but >> broken up into logical pieces where possible (one such split would be >> that of course you don't need to re-implement domctl/sysctl handling >> in the same patch as everything else). >> > Thanks for your suggestion! Just want to confirm if my understanding is > right. So, you mean I can remove all old codes but keep the interfaces as > empty functions to make sure the build can pass. Then, implement whole > functionality step by step. Right? Yes. Thanks, Jan
On 16-11-30 02:42:20, Jan Beulich wrote: > >>> On 30.11.16 at 10:08, <yi.y.sun@linux.intel.com> wrote: > > On 16-11-29 02:43:24, Jan Beulich wrote: > >> >>> On 29.11.16 at 05:38, <yi.y.sun@linux.intel.com> wrote: > >> > To make codes be better reviewable, I propose below three suggestions: > >> > 1) I write a design document about refactor to make you more easily > >> > understand the ideas. > >> > >> This is appreciated, but I don't think it's strictly necessary. Describing > >> the new design (rather than the changes to the existing one) is what > >> likely would be more useful (I'm sorry if I've misunderstood what you > >> said, and you in fact had meant just this), which iirc you already have > >> in patch 1. > >> > >> > 2) Replace the psr.c with a new file which discards all old codes so > >> > that you do not need to consider old implementations which may cause > >> > confusion. > >> > 3) Discard the refactor codes. Just implement L2 CAT based on current > >> > codes. Because L2 CAT does not have much difference with L3. > >> > >> I don't think introducing a new file is the ideal approach. I'd suggest > >> to rip out the entire implementation in a first patch, leaving just > >> empty functions to avoid breaking the build (i.e. perhaps mostly the > >> ones used by domctl/sysctl, and maybe some init one). Then > >> introduce new code, ideally of course not in one big patch, but > >> broken up into logical pieces where possible (one such split would be > >> that of course you don't need to re-implement domctl/sysctl handling > >> in the same patch as everything else). > >> > > Thanks for your suggestion! Just want to confirm if my understanding is > > right. So, you mean I can remove all old codes but keep the interfaces as > > empty functions to make sure the build can pass. Then, implement whole > > functionality step by step. Right? > > Yes. > > Thanks, Jan Got it. Thanks! Sun Yi
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index a0342ce..97f1c33 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -26,9 +26,14 @@ /* Per spec, the maximum COS register number is 128. */ #define MAX_COS_REG_NUM 128 -struct psr_cat_socket_info { +/* CAT/CDP HW info data structure. */ +struct psr_cat_hw_info { unsigned int cbm_len; unsigned int cos_max; +}; + +struct psr_cat_socket_info { + struct psr_cat_hw_info l3_info; /* * Store the values of COS registers: * CAT uses 1 entry for one COS ID; @@ -235,7 +240,7 @@ static inline void psr_assoc_init(void) if ( test_bit(socket, cat_socket_enable) ) psra->cos_mask = ((1ull << get_count_order( - cat_socket_info[socket].cos_max)) - 1) << 32; + cat_socket_info[socket].l3_info.cos_max)) - 1) << 32; } if ( psr_cmt_enabled() || psra->cos_mask ) @@ -299,8 +304,8 @@ int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len, if ( IS_ERR(info) ) return PTR_ERR(info); - *cbm_len = info->cbm_len; - *cos_max = info->cos_max; + *cbm_len = info->l3_info.cbm_len; + *cos_max = info->l3_info.cos_max; *flags = 0; if ( cdp_is_enabled(socket) ) @@ -465,14 +470,14 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket, if ( IS_ERR(info) ) return PTR_ERR(info); - if ( !psr_check_cbm(info->cbm_len, cbm) ) + if ( !psr_check_cbm(info->l3_info.cbm_len, cbm) ) return -EINVAL; if ( !cdp_enabled && (type == PSR_CBM_TYPE_L3_CODE || type == PSR_CBM_TYPE_L3_DATA) ) return -ENXIO; - cos_max = info->cos_max; + cos_max = info->l3_info.cos_max; old_cos = d->arch.psr_cos_ids[socket]; ref = info->cos_ref; @@ -617,11 +622,11 @@ static void cat_cpu_init(void) { cpuid_count(PSR_CPUID_LEVEL_CAT, 1, &eax, &ebx, &ecx, &edx); info = cat_socket_info + socket; - info->cbm_len = (eax & 0x1f) + 1; - info->cos_max = min(opt_cos_max, edx & 0xffff); + info->l3_info.cbm_len = (eax & 0x1f) + 1; + info->l3_info.cos_max = min(opt_cos_max, edx & 0xffff); /* cos=0 is reserved as default cbm(all ones). */ - info->cos_reg_val[0] = (1ull << info->cbm_len) - 1; + info->cos_reg_val[0] = (1ull << info->l3_info.cbm_len) - 1; spin_lock_init(&info->ref_lock); @@ -631,23 +636,23 @@ static void cat_cpu_init(void) cdp_socket_enable && !test_bit(socket, cdp_socket_enable) ) { /* CODE */ - get_cdp_code(info, 0) = (1ull << info->cbm_len) - 1; + get_cdp_code(info, 0) = (1ull << info->l3_info.cbm_len) - 1; /* DATA */ - get_cdp_data(info, 0) = (1ull << info->cbm_len) - 1; + get_cdp_data(info, 0) = (1ull << info->l3_info.cbm_len) - 1; /* We only write mask1 since mask0 is always all ones by default. */ - wrmsrl(MSR_IA32_PSR_L3_MASK(1), (1ull << info->cbm_len) - 1); + wrmsrl(MSR_IA32_PSR_L3_MASK(1), (1ull << info->l3_info.cbm_len) - 1); rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val); wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | (1 << PSR_L3_QOS_CDP_ENABLE_BIT)); /* Cut half of cos_max when CDP is enabled. */ - info->cos_max >>= 1; + info->l3_info.cos_max >>= 1; set_bit(socket, cdp_socket_enable); } printk(XENLOG_INFO "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u, CDP:%s\n", - socket, info->cos_max, info->cbm_len, + socket, info->l3_info.cos_max, info->l3_info.cbm_len, cdp_is_enabled(socket) ? "on" : "off"); } }
'cbm_len' and 'cbm_max' are CAT/CDP specific feature HW info. So encapsulate them into 'struct psr_cat_hw_info'. If new feature is supported, we can define other structure to save its HW info. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- xen/arch/x86/psr.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-)