Re: [Xen-devel] [PATCH v7 03/14] x86: detect and initialize Intel CAT feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.15 at 10:56, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -19,14 +19,26 @@
>  #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_init_bitmap;
> +static unsigned long *__read_mostly cat_socket_enable_bitmap;

Didn't we agree to fold these two into one? Apart from that the
_bitmap name tag doesn't seem very useful...

> @@ -194,16 +210,100 @@ 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 ( socket >= nr_sockets )
> +        return;
> +
> +    /* Avoid initializing more than one times for the same socket. */
> +    if ( test_and_set_bit(socket, cat_socket_init_bitmap) )
> +        return;
> +
> +    cpuid_count(PSR_CPUID_LEVEL_CAT, 0, &eax, &ebx, &ecx, &edx);
> +    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);


Is opt_cos_max being zero (or even one) going to result in a useful /
working environment? I.e. shouldn't you rather disable CAT in that
case?

> +static void cat_cpu_fini(void)
> +{
> +    unsigned int cpu = smp_processor_id();
> +    unsigned int socket = cpu_to_socket(cpu);

This is the only use of "cpu", i.e. the variable is pretty pointless.

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

Are these the right notifiers for doing things involving memory
allocation / freeing?

> @@ -217,9 +317,12 @@ static int __init psr_presmp_init(void)
>      if ( (opt_psr & PSR_CMT) && opt_rmid_max )
>          init_psr_cmt(opt_rmid_max);
>  
> +    if ( opt_psr & PSR_CAT )
> +        init_psr_cat();
> +
>      psr_cpu_init();
> -    if ( psr_cmt_enabled() )
> -        register_cpu_notifier(&cpu_nfb);
> +    if ( psr_cmt_enabled() || cat_socket_info )
> +          register_cpu_notifier(&cpu_nfb);

Please don't corrupt indentation here.

Jan


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