|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/5] x86: Support enable/disable CDP dynamically and get CDP status
On 02/09/15 09:27, He Chen wrote:
> cdp_enabled is added to CAT socket info to indicate CDP is on or off on
> the socket and struct psr_cat_cbm is extended to support CDP operation.
> IA32_L3_QOS_CFG is a new MSR to enable/disable L3 CDP, when enable or
> disable CDP, all domains will reset to COS0 with fully access all L3
> cache.
>
> Signed-off-by: He Chen <he.chen@xxxxxxxxxxxxxxx>
> ---
> xen/arch/x86/psr.c | 164
> ++++++++++++++++++++++++++++++++++++++++----
> xen/arch/x86/sysctl.c | 9 ++-
> xen/include/asm-x86/psr.h | 10 ++-
> xen/include/public/sysctl.h | 5 ++
> 4 files changed, 174 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index b357816..26596dd 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -17,13 +17,20 @@
> #include <xen/cpu.h>
> #include <xen/err.h>
> #include <xen/sched.h>
> +#include <xen/domain.h>
> #include <asm/psr.h>
>
> #define PSR_CMT (1<<0)
> #define PSR_CAT (1<<1)
>
> struct psr_cat_cbm {
> - uint64_t cbm;
> + union {
> + uint64_t cbm;
> + struct {
> + uint64_t code;
> + uint64_t data;
> + }cdp;
> + }u;
Spaces after } please.
> unsigned int ref;
> };
>
> @@ -32,6 +39,7 @@ struct psr_cat_socket_info {
> unsigned int cos_max;
> struct psr_cat_cbm *cos_to_cbm;
> spinlock_t cbm_lock;
> + bool_t cdp_enabled;
> };
>
> struct psr_assoc {
> @@ -263,7 +271,7 @@ static struct psr_cat_socket_info
> *get_cat_socket_info(unsigned int socket)
> }
>
> int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
> - uint32_t *cos_max)
> + uint32_t *cos_max, uint8_t *cdp_enabled)
bool_t *cdp_enabled which...
> {
> struct psr_cat_socket_info *info = get_cat_socket_info(socket);
>
> @@ -272,6 +280,7 @@ int psr_get_cat_l3_info(unsigned int socket, uint32_t
> *cbm_len,
>
> *cbm_len = info->cbm_len;
> *cos_max = info->cos_max;
> + *cdp_enabled = (uint8_t)info->cdp_enabled;
... avoids this typecast.
>
> return 0;
> }
> @@ -283,7 +292,7 @@ int psr_get_l3_cbm(struct domain *d, unsigned int socket,
> uint64_t *cbm)
> if ( IS_ERR(info) )
> return PTR_ERR(info);
>
> - *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].cbm;
> + *cbm = info->cos_to_cbm[d->arch.psr_cos_ids[socket]].u.cbm;
>
> return 0;
> }
> @@ -314,19 +323,33 @@ static bool_t psr_check_cbm(unsigned int cbm_len,
> uint64_t cbm)
> struct cos_cbm_info
> {
> unsigned int cos;
> - uint64_t cbm;
> + uint64_t cbm_code;
> + uint64_t cbm_data;
> + bool_t cdp_mode;
This should either be plain "cdp" which would be fine, or
"cdp_enabled". The name "_mode" here is misleading.
> };
>
> static void do_write_l3_cbm(void *data)
> {
> struct cos_cbm_info *info = data;
>
> - wrmsrl(MSR_IA32_PSR_L3_MASK(info->cos), info->cbm);
> + if ( info->cdp_mode == 0 )
> + wrmsrl(MSR_IA32_PSR_L3_MASK(info->cos), info->cbm_code);
> + else
> + {
> + wrmsrl(MSR_IA32_PSR_L3_MASK(info->cos*2+1), info->cbm_code);
> + wrmsrl(MSR_IA32_PSR_L3_MASK(info->cos*2), info->cbm_data);
Please introduce
MSR_IA32_PSR_L3_MASK_CBM_CODE()
MSR_IA32_PSR_L3_MASK_CBM_DATA()
to abstract away the x * 2 + 1 calculation.
> + }
> }
>
> -static int write_l3_cbm(unsigned int socket, unsigned int cos, uint64_t cbm)
> +static int write_l3_cbm(unsigned int socket, unsigned int cos,
> + uint64_t cbm_code, uint64_t cbm_data, bool_t
> cdp_mode)
> {
> - struct cos_cbm_info info = { .cos = cos, .cbm = cbm };
> + struct cos_cbm_info info =
> + { .cos = cos,
Newline here please, so
{
.cos
> + .cbm_code = cbm_code,
> + .cbm_data = cbm_data,
> + .cdp_mode = cdp_mode,
> + };
>
> if ( socket == cpu_to_socket(smp_processor_id()) )
> do_write_l3_cbm(&info);
> @@ -364,7 +387,7 @@ int psr_set_l3_cbm(struct domain *d, unsigned int socket,
> uint64_t cbm)
> /* If still not found, then keep unused one. */
> if ( !found && cos != 0 && map[cos].ref == 0 )
> found = map + cos;
> - else if ( map[cos].cbm == cbm )
> + else if ( map[cos].u.cbm == cbm )
> {
> if ( unlikely(cos == old_cos) )
> {
> @@ -388,16 +411,16 @@ int psr_set_l3_cbm(struct domain *d, unsigned int
> socket, uint64_t cbm)
> }
>
> cos = found - map;
> - if ( found->cbm != cbm )
> + if ( found->u.cbm != cbm )
> {
> - int ret = write_l3_cbm(socket, cos, cbm);
> + int ret = write_l3_cbm(socket, cos, cbm, 0, 0);
>
> if ( ret )
> {
> spin_unlock(&info->cbm_lock);
> return ret;
> }
> - found->cbm = cbm;
> + found->u.cbm = cbm;
> }
>
> found->ref++;
> @@ -491,7 +514,7 @@ 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);
>
> @@ -556,6 +579,123 @@ static int psr_cpu_prepare(unsigned int cpu)
> return cat_cpu_prepare(cpu);
> }
>
> +static void do_write_l3_config(void *data)
> +{
> + uint64_t val;
> + uint64_t *mask = data;
> +
> + rdmsrl(PSR_L3_QOS_CFG, val);
> + wrmsrl(PSR_L3_QOS_CFG, val | *mask);
> +}
> +
> +static int config_socket_l3_cdp(unsigned int socket, uint64_t cdp_mask)
> +{
> + unsigned int cpu;
> + uint64_t mask = cdp_mask;
> +
> + if ( socket == cpu_to_socket(smp_processor_id()) )
> + do_write_l3_config(&mask);
> + else
> + {
> + cpu = get_socket_cpu(socket);
> + if ( cpu >= nr_cpu_ids )
> + return -ENOTSOCK;
> + on_selected_cpus(cpumask_of(cpu), do_write_l3_config, &mask, 1);
> + }
> + return 0;
> +}
> +
> +int psr_cat_enable_cdp(void)
> +{
> + int i, ret;
> + struct psr_cat_socket_info *info;
> + unsigned int socket;
> + struct domain *d;
> +
> + for_each_set_bit(socket, cat_socket_enable, nr_sockets)
> + {
> + ret = config_socket_l3_cdp(socket, 1 << PSR_L3_QOS_CDP_ENABLE_BIT);
> + if ( ret )
> + return ret;
> +
> + info = cat_socket_info + socket;
> + if (info->cdp_enabled)
Style. spaces inside brakces.
> + return 0;
> +
> + /* Cut half of cos_max when CDP enabled */
> + info->cos_max = info->cos_max / 2;
> +
> + spin_lock(&info->cbm_lock);
> +
> + /* Reset COS0 code CBM & data CBM for all domain */
> + 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;
> +
> + for ( i = 0; i <= info->cos_max; i++ )
> + info->cos_to_cbm[0].ref = 0;
> +
> + /* Only write mask1 since mask0 is always all 1 by CAT. */
> + ret = write_l3_cbm(socket, 1, info->cos_to_cbm[0].u.cdp.code, 0, 0);
> + if ( ret )
> + {
> + spin_unlock(&info->cbm_lock);
> + return ret;
> + }
> +
> + /* Reset all domain to COS0 */
> + for_each_domain( d )
> + {
> + d->arch.psr_cos_ids[socket] = 0;
> + info->cos_to_cbm[0].ref++;
This is a long running operation and must not be done synchronously like
this. Unfortunately, it is not easy to make restartable.
I think it would be perfectly reasonable to have cdp as a boot time
switch only, and have no ability to change it at runtime. I don't see a
reasonable case to change it dynamically at runtime; users will either
want to use it, or not.
Making this a boot-time choice (i.e. psr=cat,cdp) removes all of this
re-juggling logic, and simplifies things greatly.
Thoughts?
> + }
> +
> + spin_unlock(&info->cbm_lock);
> + info->cdp_enabled = 1;
> + }
> +
> + return 0;
> +}
> +
> +int psr_cat_disable_cdp(void)
> +{
> + int i, ret;
> + struct psr_cat_socket_info *info;
> + unsigned int socket;
> + struct domain *d;
> +
> + for_each_set_bit(socket, cat_socket_enable, nr_sockets)
> + {
> + ret = config_socket_l3_cdp(socket, 0 << PSR_L3_QOS_CDP_ENABLE_BIT);
> + if ( ret )
> + return ret;
> +
> + info = cat_socket_info + socket;
> + if( !info->cdp_enabled )
> + return 0;
> +
> + /* Restore CAT cos_max when CDP disabled */
> + info->cos_max = info->cos_max * 2 + 1;
> + spin_lock(&info->cbm_lock);
> +
> + /* Reset all CBMs and set fully open COS0 for all domains */
> + info->cos_to_cbm[0].u.cbm = (1ull << info->cbm_len) - 1;
> + for ( i = 0; i <= info->cos_max; i++ )
> + info->cos_to_cbm[i].ref = 0;
> +
> + /* Reset all domains to COS0 */
> + for_each_domain( d )
> + {
> + d->arch.psr_cos_ids[socket] = 0;
> + info->cos_to_cbm[0].ref++;
> + }
> +
> + spin_unlock(&info->cbm_lock);
> + info->cdp_enabled = 0;
> + }
> +
> + return 0;
> +}
> +
> static void psr_cpu_init(void)
> {
> if ( cat_socket_info )
> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> index f36b52f..51b03a4 100644
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -177,12 +177,19 @@ long arch_do_sysctl(
> case XEN_SYSCTL_PSR_CAT_get_l3_info:
> ret = psr_get_cat_l3_info(sysctl->u.psr_cat_op.target,
>
> &sysctl->u.psr_cat_op.u.l3_info.cbm_len,
> -
> &sysctl->u.psr_cat_op.u.l3_info.cos_max);
> +
> &sysctl->u.psr_cat_op.u.l3_info.cos_max,
> +
> &sysctl->u.psr_cat_op.u.l3_info.cdp_enabled);
>
> if ( !ret && __copy_field_to_guest(u_sysctl, sysctl,
> u.psr_cat_op) )
> ret = -EFAULT;
>
> break;
> + case XEN_SYSCTL_PSR_CAT_enable_cdp:
> + ret = psr_cat_enable_cdp();
> + break;
> + case XEN_SYSCTL_PSR_CAT_disable_cdp:
> + ret = psr_cat_disable_cdp();
> + break;
I realise you are copying the existing (incorrect) style, but please
introduce a blank line between break statements and the subsequent
case/default label. It makes the switch statement far easier to read.
> default:
> ret = -EOPNOTSUPP;
> break;
> diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h
> index a6b83df..2b79a92 100644
> --- a/xen/include/asm-x86/psr.h
> +++ b/xen/include/asm-x86/psr.h
> @@ -30,6 +30,11 @@
> /* CDP Capability */
> #define PSR_CAT_CDP_CAPABILITY 0x4
>
> +/* L3 QoS Config MSR Address */
> +#define PSR_L3_QOS_CFG 0xc81
> +
> +/* L3 CDP Enable bit*/
> +#define PSR_L3_QOS_CDP_ENABLE_BIT 0x0
>
> struct psr_cmt_l3 {
> unsigned int features;
> @@ -56,13 +61,16 @@ void psr_free_rmid(struct domain *d);
> void psr_ctxt_switch_to(struct domain *d);
>
> int psr_get_cat_l3_info(unsigned int socket, uint32_t *cbm_len,
> - uint32_t *cos_max);
> + uint32_t *cos_max, uint8_t *cdp_enabled);
> int psr_get_l3_cbm(struct domain *d, unsigned int socket, uint64_t *cbm);
> int psr_set_l3_cbm(struct domain *d, unsigned int socket, uint64_t cbm);
>
> int psr_domain_init(struct domain *d);
> void psr_domain_free(struct domain *d);
>
> +int psr_cat_enable_cdp(void);
> +int psr_cat_disable_cdp(void);
> +
> #endif /* __ASM_PSR_H__ */
>
> /*
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 58c9be2..8b97137 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -697,6 +697,8 @@ typedef struct xen_sysctl_pcitopoinfo
> xen_sysctl_pcitopoinfo_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_pcitopoinfo_t);
>
> #define XEN_SYSCTL_PSR_CAT_get_l3_info 0
> +#define XEN_SYSCTL_PSR_CAT_enable_cdp 1
> +#define XEN_SYSCTL_PSR_CAT_disable_cdp 2
> struct xen_sysctl_psr_cat_op {
> uint32_t cmd; /* IN: XEN_SYSCTL_PSR_CAT_* */
> uint32_t target; /* IN */
> @@ -704,6 +706,9 @@ 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_CDP_DISABLED 0
> + #define XEN_SYSCTL_PSR_CDP_ENABLED 1
> + uint8_t cdp_enabled; /* OUT: CDP status */
Rather than using a whole 8 bits for a boolean, and leaving alignment
fun for the next person to edit this structure, may I recommend
#define XEN_SYSCTL_PSR_CAT_L3_CDP (1u << 0)
uint32_t flags;
Instead. (Also assumes that in the future we might get CDP at the L4 cache)
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |