Re: [Xen-devel] [PATCH 1/6] x86: detect and initialize Intel CAT feature

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Mar 13, 2015 at 06:13:20PM +0800, Chao Peng wrote:
> Detect Intel Cache Allocation Technology(CAT) feature and store the
> cpuid information for later use. Currently only L3 cache allocation is
> supported. The L3 CAT features may vary among sockets so per-socket
> feature information is stored. The initialization can happen either at
> boot time or when CPU(s) is hot plugged after booting.
> 
> Signed-off-by: Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>
> ---
>  docs/misc/xen-command-line.markdown |  15 +++-
>  xen/arch/x86/psr.c                  | 151 
> +++++++++++++++++++++++++++++++++---
>  xen/include/asm-x86/cpufeature.h    |   1 +
>  3 files changed, 155 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index 63871cb..a4b36e6 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1090,9 +1090,9 @@ This option can be specified more than once (up to 8 
> times at present).
>  > `= <integer>`
>  
>  ### psr (Intel)
> -> `= List of ( cmt:<boolean> | rmid_max:<integer> )`
> +> `= List of ( cmt:<boolean> | rmid_max:<integer> | socket_num:<interger> )`

That should also list the 'cat:<boolean>'
>  
> -> Default: `psr=cmt:0,rmid_max:255`
> +> Default: `psr=cmt:0,rmid_max:255,cat:0,socket_num:0 (Detect at boot time)`
>  
>  Platform Shared Resource(PSR) Services.  Intel Haswell and later server
>  platforms offer information about the sharing of resources.
> @@ -1102,6 +1102,11 @@ Monitoring ID(RMID) is used to bind the domain to 
> corresponding shared
>  resource.  RMID is a hardware-provided layer of abstraction between software
>  and logical processors.
>  
> +To use the PSR cache allocation service for a certain domain, a capacity
> +bitmasks(CBM) is used to bind the domain to corresponding shared resource.
> +CBM represents cache capacity and indicates the degree of overlap and 
> isolation
> +between domains.
> +
>  The following resources are available:
>  
>  * Cache Monitoring Technology (Haswell and later).  Information regarding the
> @@ -1112,6 +1117,12 @@ The following resources are available:
>    total/local memory bandwidth. Follow the same options with Cache Monitoring
>    Technology.
>  
> +* Cache Alllocation Technology (Broadwell and later).  Information regarding
> +  the cache allocation.
> +  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
> +  * `socket_num` indicates socket number. Detecte automatically at boot time

s/Detecte/Detect/

Well, it is not exactly socket number. It is up-to socket number. Meaning
if you give it '3' it will be socket 0, 1, 2 and 3.

> +    if not specified(0). Useful for CPU hot-plug case.
> +
>  ### reboot
>  > `= t[riple] | k[bd] | a[cpi] | p[ci] | n[o] [, [w]arm | [c]old]`
>  
> diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> index 2ef83df..df557e8 100644
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -19,6 +19,14 @@
>  #include <asm/psr.h>
>  
>  #define PSR_CMT        (1<<0)
> +#define PSR_CAT        (1<<1)
> +
> +struct psr_cat_socket_info {
> +    bool_t initialized;
> +    bool_t enabled;
> +    unsigned int cbm_len;
> +    unsigned int cos_max;
> +};
>  
>  struct psr_assoc {
>      uint64_t val;
> @@ -26,8 +34,11 @@ struct psr_assoc {
>  };
>  
>  struct psr_cmt *__read_mostly psr_cmt;
> -static bool_t __initdata opt_psr;
> +static struct psr_cat_socket_info *__read_mostly cat_socket_info;
> +
> +static unsigned int __initdata opt_psr;
>  static unsigned int __initdata opt_rmid_max = 255;
> +static unsigned int __read_mostly opt_socket_num;
>  static uint64_t rmid_mask;
>  static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
>  
> @@ -58,15 +69,31 @@ static void __init parse_psr_param(char *s)
>                                      val_str);
>              }
>          }
> +        else if ( !strcmp(s, "cat") )
> +        {
> +            if ( !val_str )
> +                opt_psr |= PSR_CAT;
> +            else
> +            {
> +                int val_int = parse_bool(val_str);
> +                if ( val_int == 1 )
> +                    opt_psr |= PSR_CAT;
> +                else if ( val_int != 0 )
> +                    printk("PSR: unknown cat value: %s - CAT disabled!\n",
> +                                    val_str);
> +            }
> +        }

This looks exactly like the PSR_CMT parsing. Why not make it a function
and just pass in the s, val_str, "cat", PSR_CAT to it so it can
do this determination?

>          else if ( val_str && !strcmp(s, "rmid_max") )
>              opt_rmid_max = simple_strtoul(val_str, NULL, 0);
> +        else if ( val_str && !strcmp(s, "socket_num") )
> +            opt_socket_num = simple_strtoul(val_str, NULL, 0);
>  
>          s = ss + 1;
>      } while ( ss );
>  }
>  custom_param("psr", parse_psr_param);
>  
> -static void __init init_psr_cmt(unsigned int rmid_max)
> +static void __init psr_cmt_init(unsigned int rmid_max)
>  {
>      unsigned int eax, ebx, ecx, edx;
>      unsigned int rmid;
> @@ -115,14 +142,6 @@ static void __init init_psr_cmt(unsigned int rmid_max)
>      printk(XENLOG_INFO "Cache Monitoring Technology enabled\n");
>  }
>  
> -static int __init init_psr(void)
> -{
> -    if ( (opt_psr & PSR_CMT) && opt_rmid_max )
> -        init_psr_cmt(opt_rmid_max);
> -    return 0;
> -}
> -__initcall(init_psr);
> -
>  /* Called with domain lock held, no psr specific lock needed */
>  int psr_alloc_rmid(struct domain *d)
>  {
> @@ -189,6 +208,118 @@ void psr_assoc_rmid(unsigned int rmid)
>      }
>  }
>  
> +static void do_cat_cpu_init(void* data)
> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    struct psr_cat_socket_info *info;
> +
> +    cpuid_count(0x10, 0, &eax, &ebx, &ecx, &edx);
> +    if ( ebx & PSR_RESOURCE_TYPE_L3 )
> +    {
> +        info = data;
> +
> +        cpuid_count(0x10, 1, &eax, &ebx, &ecx, &edx);
> +        info->cbm_len = (eax & 0x1f) + 1;
> +        info->cos_max = (edx & 0xffff);
> +
> +        info->enabled = 1;
> +        printk(XENLOG_INFO "CAT: enabled on socket %d, cos_max:%d, 
> cbm_len:%d\n",

Why not make those DEBUG?

> +               (int)(info - cat_socket_info), info->cos_max, info->cbm_len);
> +    }
> +}
> +
> +static void cat_cpu_init(unsigned int cpu)
> +{
> +    struct psr_cat_socket_info *info;
> +    unsigned int socket;
> +    const struct cpuinfo_x86 *c;
> +
> +    socket = cpu_to_socket(cpu);
> +    if ( socket >= opt_socket_num )
> +    {
> +        printk(XENLOG_WARNING "CAT: disabled on socket %d because socket_num 
> is %d\n",
> +               socket, opt_socket_num);
> +        return;
> +    }
> +
> +    info = cat_socket_info + socket;
> +
> +    /* Avoid initializing more than one times for the same socket. */
> +    if ( test_and_set_bool(info->initialized) )
> +        return;
> +
> +    c = cpu_data + cpu;
> +    if ( !cpu_has(c, X86_FEATURE_CAT) )
> +        return;
> +
> +    if ( cpu == smp_processor_id() )
> +        do_cat_cpu_init(info);
> +    else
> +        on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, info, 0);
> +}
> +
> +static int cpu_callback(
> +    struct notifier_block *nfb, unsigned long action, void *hcpu)
> +{
> +    unsigned int cpu = (unsigned long)hcpu;
> +
> +    switch ( action )
> +    {
> +    case CPU_ONLINE:
> +        (void)cat_cpu_init(cpu);
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block cpu_nfb = {
> +    .notifier_call = cpu_callback
> +};
> +
> +static unsigned int get_max_socket(void)
> +{
> +    unsigned int cpu, max_apicid = boot_cpu_physical_apicid;
> +
> +    for_each_present_cpu(cpu)
> +        if (max_apicid < x86_cpu_to_apicid[cpu])
> +            max_apicid = x86_cpu_to_apicid[cpu];
> +
> +    return apicid_to_socket(max_apicid);
> +}
> +
> +static void __init psr_cat_init(void)
> +{
> +    if ( opt_socket_num == 0 )
> +        opt_socket_num = get_max_socket() + 1;
> +    else if ( opt_socket_num > NR_CPUS )
> +         printk(XENLOG_WARNING "socket_num > NR_CPUS(%d), set to NR_CPUS.\n",
> +                NR_CPUS);

Well, not really. You did not set it to NR_CPUS.

> +
> +    BUG_ON(opt_socket_num == 0);
> +
> +    cat_socket_info = xzalloc_array(struct psr_cat_socket_info, 
> opt_socket_num);
> +    if ( !cat_socket_info )
> +        return;
> +
> +    cat_cpu_init(smp_processor_id());

Do 'if (!cat_cpu_init(..)).`'

as the CPU might not support this.

At which point you should also free the cat_socket_info and
not register the cpu notifier.

> +    register_cpu_notifier(&cpu_nfb);
> +}
> +
> +static int __init psr_presmp_init(void)
> +{
> +    if ( (opt_psr & PSR_CMT) && opt_rmid_max )
> +        psr_cmt_init(opt_rmid_max);
> +
> +    if ( opt_psr & PSR_CAT )
> +        psr_cat_init();
> +
> +    return 0;
> +}
> +presmp_initcall(psr_presmp_init);
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-x86/cpufeature.h 
> b/xen/include/asm-x86/cpufeature.h
> index 7963a3a..8c0f0a6 100644
> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -149,6 +149,7 @@
>  #define X86_FEATURE_CMT      (7*32+12) /* Cache Monitoring Technology */
>  #define X86_FEATURE_NO_FPU_SEL       (7*32+13) /* FPU CS/DS stored as zero */
>  #define X86_FEATURE_MPX              (7*32+14) /* Memory Protection 
> Extensions */
> +#define X86_FEATURE_CAT      (7*32+15) /* Cache Allocation Technology */
>  #define X86_FEATURE_RDSEED   (7*32+18) /* RDSEED instruction */
>  #define X86_FEATURE_ADX              (7*32+19) /* ADCX, ADOX instructions */
>  #define X86_FEATURE_SMAP     (7*32+20) /* Supervisor Mode Access Prevention 
> */
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel