From: "Jan Beulich" <JBeulich@suse.com>
To: He Chen <he.chen@linux.intel.com>
Cc: wei.liu2@citrix.com, ian.campbell@citrix.com,
stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com,
ian.jackson@eu.citrix.com, xen-devel@lists.xenproject.org,
chao.p.peng@linux.intel.com, keir@xen.org
Subject: Re: [PATCH v3 1/4] x86: Support enable CDP by boot parameter and add get CDP status
Date: Mon, 14 Sep 2015 08:12:42 -0600 [thread overview]
Message-ID: <55F6F1FA02000078000A29BF@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1442201227-8610-2-git-send-email-he.chen@linux.intel.com>
>>> On 14.09.15 at 05:27, <he.chen@linux.intel.com> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -21,9 +21,16 @@
>
> #define PSR_CMT (1<<0)
> #define PSR_CAT (1<<1)
> +#define PSR_CDP (1<<2)
>
> struct psr_cat_cbm {
> - uint64_t cbm;
> + union {
> + uint64_t cbm;
> + struct {
> + uint64_t code;
> + uint64_t data;
> + } cdp;
> + } u;
While in the public interfaces use of gcc extensions is not allowed
without very special care, please make use of such in internal
headers when that helps readability (as well as in the case patch
size): No need to name the union member of the outer structure.
> @@ -490,13 +519,33 @@ static void cat_cpu_init(void)
> info->cos_to_cbm = temp_cos_to_cbm;
> temp_cos_to_cbm = NULL;
> /* cos=0 is reserved as default cbm(all ones). */
> - info->cos_to_cbm[0].cbm = (1ull << info->cbm_len) - 1;
> + info->cos_to_cbm[0].u.cbm = (1ull << info->cbm_len) - 1;
>
> spin_lock_init(&info->cbm_lock);
>
> set_bit(socket, cat_socket_enable);
> - printk(XENLOG_INFO "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
> - socket, info->cos_max, info->cbm_len);
I fail to see a replacement for this message.
> + if ( (ecx & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) )
> + {
> + if ( test_bit(socket, cdp_socket_enable) )
> + return;
This will make future changes more cumbersome, especially if one
would not require CDP as a prereq. Please fold the two if()s,
allowing execution to progress past the outer if() in all cases.
> + rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
> + wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | 1 << PSR_L3_QOS_CDP_ENABLE_BIT);
Missing parentheses around the <<.
> + info->cos_to_cbm[0].u.cdp.code = (1ull << info->cbm_len) - 1;
> + info->cos_to_cbm[0].u.cdp.data = (1ull << 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);
> +
> + /* Cut half of cos_max when CDP enabled */
> + info->cos_max = info->cos_max / 2;
Please use /= or >>=.
> @@ -535,8 +588,9 @@ static void __init init_psr_cat(void)
>
> cat_socket_enable = xzalloc_array(unsigned long, BITS_TO_LONGS(nr_sockets));
> cat_socket_info = xzalloc_array(struct psr_cat_socket_info, nr_sockets);
> + cdp_socket_enable = xzalloc_array(unsigned long, BITS_TO_LONGS(nr_sockets));
>
> - if ( !cat_socket_enable || !cat_socket_info )
> + if ( !cat_socket_enable || !cat_socket_info || !cdp_socket_enable )
> psr_cat_free();
No - just disable CDP if only that third allocation fails.
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -328,7 +328,10 @@
> #define MSR_IA32_CMT_EVTSEL 0x00000c8d
> #define MSR_IA32_CMT_CTR 0x00000c8e
> #define MSR_IA32_PSR_ASSOC 0x00000c8f
> +#define MSR_IA32_PSR_L3_QOS_CFG 0x00000c81
> #define MSR_IA32_PSR_L3_MASK(n) (0x00000c90 + (n))
> +#define MSR_IA32_PSR_L3_MASK_CODE(n) (0x00000c90 + (n * 2 + 1))
> +#define MSR_IA32_PSR_L3_MASK_DATA(n) (0x00000c90 + (n * 2))
Please always fully parenthesize uses of macro parameters (in fact
the parentheses you have could simply be moved to isolate just n).
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -704,6 +704,8 @@ struct xen_sysctl_psr_cat_op {
> struct {
> uint32_t cbm_len; /* OUT: CBM length */
> uint32_t cos_max; /* OUT: Maximum COS */
> + #define XEN_SYSCTL_PSR_CAT_L3_CDP (1u << 0)
> + uint32_t flags; /* OUT: CAT flags */
> } l3_info;
Even if only mildly incompatible, any interface change to sysctl (or
domctl) should make sure the respective interface version got
bumped after branching the previous release tree.
Jan
next prev parent reply other threads:[~2015-09-14 14:12 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-14 3:27 [PATCH v3 0/4] Intel Code and Data Prioritization (CDP) feature enabling He Chen
2015-09-14 3:27 ` [PATCH v3 1/4] x86: Support enable CDP by boot parameter and add get CDP status He Chen
2015-09-14 8:39 ` Chao Peng
2015-09-14 14:12 ` Jan Beulich [this message]
2015-09-14 3:27 ` [PATCH v3 2/4] x86: add domctl cmd to set/get CDP code/data CBM He Chen
2015-09-14 9:04 ` Chao Peng
2015-09-14 14:28 ` Jan Beulich
2015-09-14 3:27 ` [PATCH v3 3/4] tools: add tools support for Intel CDP He Chen
2015-09-14 9:11 ` Chao Peng
2015-09-14 3:27 ` [PATCH v3 4/4] docs: add document to introduce CDP command He Chen
2015-09-14 9:35 ` Chao Peng
2015-09-14 9:48 ` [PATCH v3 0/4] Intel Code and Data Prioritization (CDP) feature enabling Chao Peng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55F6F1FA02000078000A29BF@prv-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=chao.p.peng@linux.intel.com \
--cc=he.chen@linux.intel.com \
--cc=ian.campbell@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=keir@xen.org \
--cc=stefano.stabellini@eu.citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.