Re: [PATCH v8 02/13] x86: detect and initialize Intel CAT feature

["Jan Beulich" <JBeulich@suse.com>]

>>> On 21.05.15 at 10:41, <chao.p.peng@linux.intel.com> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -19,14 +19,25 @@
>  #include <asm/psr.h>
>  
>  #define PSR_CMT        (1<<0)
> +#define PSR_CAT        (1<<1)
> +
> +struct psr_cat_socket_info {
> +    unsigned int cbm_len;
> +    unsigned int cos_max;
> +};
>  
>  struct psr_assoc {
>      uint64_t val;
>  };
>  
>  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 int __initdata opt_psr;
>  static unsigned int __initdata opt_rmid_max = 255;
> +static unsigned int opt_cos_max = 255;

opt_* variables should generally be either __initdata or
__read_mostly (the latter in this case).

> @@ -194,16 +209,86 @@ void psr_ctxt_switch_to(struct domain *d)
>      }
>  }
>  
> +static void cat_cpu_init(void)
> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    struct psr_cat_socket_info *info;
> +    unsigned int socket;
> +    unsigned int cpu = smp_processor_id();
> +    const struct cpuinfo_x86 *c = cpu_data + cpu;
> +
> +    if ( !cpu_has(c, X86_FEATURE_CAT) )
> +        return;
> +
> +    socket = cpu_to_socket(cpu);
> +    if ( test_bit(socket, cat_socket_enable) )
> +        return;
> +
> +    cpuid_count(PSR_CPUID_LEVEL_CAT, 0, &eax, &ebx, &ecx, &edx);

While one would hope that X86_FEATURE_CAT implies the respective
CPUID leaf being available, I think explicitly checking this should still
be done just like is the case elsewhere.

> +    if ( ebx & PSR_RESOURCE_TYPE_L3 )
> +    {
> +        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);
> +
> +        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);
> +    }
> +}
> +
> +static void cat_cpu_fini(unsigned int cpu)
> +{
> +    unsigned int socket = cpu_to_socket(cpu);
> +
> +    if ( !socket_cpumask[socket] || cpumask_empty(socket_cpumask[socket]) )
> +        clear_bit(socket, cat_socket_enable);
> +}

This being called from the CPU_DEAD notification, you now depend
on cpu_smpboot_free) to run ahead of you. Which isn't the case
afaict, and even if it happened to be that way you shouldn't rely
on it without explicitly enforcing ordering between the two by
setting the priority of on of them to a non-default value.

> +static void __init init_psr_cat(void)
> +{
> +    if ( opt_cos_max < 1 )
> +    {
> +        printk(XENLOG_INFO "CAT: disabled, cos_max is too small\n");
> +        return;
> +    }

Is opt_cos_max == 1 really useful for anything?

>  static int cpu_callback(
>      struct notifier_block *nfb, unsigned long action, void *hcpu)
>  {
> +    unsigned int cpu = (unsigned long)hcpu;
> +
>      if ( action == CPU_STARTING )
>          psr_cpu_init();
> +    else if ( action == CPU_DEAD )
> +        psr_cpu_fini(cpu);

switch()

Jan