Re: [PATCH v2 1/4] x86: Support enable CDP by boot parameter and add get CDP status

[Chao Peng <chao.p.peng@linux.intel.com>]

On Wed, Sep 09, 2015 at 01:16:45PM +0800, He Chen wrote:
> Intel Code/Data Prioritization(CDP) feature is based on CAT. cdp_enabled
> is added to CAT socket info to indicate CDP is on or off on the socket,
> note that cos_max would be half when CDP is on. struct psr_cat_cbm is
> extended to support CDP operation. Extend psr_get_cat_l3_info sysctl to
> get CDP status.
> 
> Signed-off-by: He Chen <he.chen@linux.intel.com>
> ---
>  xen/arch/x86/psr.c              | 84 ++++++++++++++++++++++++++++++++++-------
>  xen/arch/x86/sysctl.c           |  5 ++-
>  xen/include/asm-x86/msr-index.h |  3 ++
>  xen/include/asm-x86/psr.h       | 11 +++++-
>  xen/include/public/sysctl.h     |  2 +
>  5 files changed, 89 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index c0daa2e..ba0f726 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -17,13 +17,21 @@
>  #include <xen/cpu.h>
>  #include <xen/err.h>
>  #include <xen/sched.h>
> +#include <xen/domain.h>

Is this still needed?

>  #include <asm/psr.h>
>  
>  #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;
>      unsigned int ref;
>  };
>  
> @@ -32,6 +40,7 @@ struct psr_cat_socket_info {
>      unsigned int cos_max;
>      struct psr_cat_cbm *cos_to_cbm;
>      spinlock_t cbm_lock;

As you defined the cdp stauts for each socket here ...
> +    bool_t cdp_enabled;
>  };
>  
>  struct psr_assoc {
> @@ -43,6 +52,7 @@ struct psr_cmt *__read_mostly psr_cmt;
>  
>  static unsigned long *__read_mostly cat_socket_enable;
>  static struct psr_cat_socket_info *__read_mostly cat_socket_info;
> +static unsigned long *__read_mostly cdp_socket_enable;

... this one then perhaps is redundency. If only one is need then I really
like to keep the latter one.

> @@ -470,6 +500,7 @@ static void cat_cpu_init(void)
>      struct psr_cat_socket_info *info;
>      unsigned int socket;
>      unsigned int cpu = smp_processor_id();
> +    uint64_t val;
>      const struct cpuinfo_x86 *c = cpu_data + cpu;
>  
>      if ( !cpu_has(c, X86_FEATURE_CAT) || c->cpuid_level < PSR_CPUID_LEVEL_CAT )
> @@ -490,13 +521,34 @@ 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);

If CDP is enable below, then this information is quite misleading.

> +
> +        if ( (ecx & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) )
> +        {
> +            if ( test_bit(socket, cdp_socket_enable) )
> +                return;
> +
> +            rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
> +            wrmsrl(MSR_IA32_PSR_L3_QOS_CFG, val | 1 << PSR_L3_QOS_CDP_ENABLE_BIT);
> +
> +            /* No need to write register since CBMs are fully open as default by HW */
> +            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;

If I remember correctly, HW is supposed to boot as CAT mode by default and
only IA32_L3_QOS_Mask_0 is all ones, other mask msrs however are not mentioned
in the spec which then may contain arbitrary value. So I guesss as
least you need write all ones to IA32_L3_QOS_Mask_1 explicitly and that
should be done before you enabled CDP.

> +
> +            /* Cut half of cos_max when CDP enabled */
> +            info->cos_max = info->cos_max / 2;
> +
> +            info->cdp_enabled = 1;
> +            set_bit(socket, cdp_socket_enable);
> +            printk(XENLOG_INFO "CDP: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
> +                   socket, info->cos_max, info->cbm_len);
> +        }
>      }
>  }
>  
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index e9c4723..402e7a7 100644
> --- 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_CBM_CODE(n)	(0x00000c90 + (n * 2 + 1))
> +#define MSR_IA32_PSR_L3_MASK_CBM_DATA(n)	(0x00000c90 + (n * 2))

I guess 'CBM' can be removed from these two macros, e.g.
MSR_IA32_PSR_L3_MASK_CODE & MSR_IA32_PSR_L3_MASK_DATA

Chao