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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.04.15 at 11:55, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
>  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;
> +static struct psr_cat_socket_info *__read_mostly cat_socket_info;

So the last line looks right, and the line of context left in place is
well formed too. Why did you not follow these good examples for
the two newly added lines?

And what is cat_socket_enable_bitmap needed for anyway?

> @@ -194,8 +210,67 @@ 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);
> +    ASSERT(socket < nr_sockets);
> +
> +    info = cat_socket_info + socket;
> +
> +    /* Avoid initializing more than one times for the same socket. */
> +    if ( test_and_set_bit(socket, cat_socket_init_bitmap) )
> +        return;

So what if a package got offlined and then is being brought back
online (perhaps after having got replaced)?

> +static void __init init_psr_cat(void)
> +{
> +    size_t nlongs = BITS_TO_LONGS(nr_sockets);
> +
> +    cat_socket_init_bitmap = xzalloc_array(unsigned long, nlongs);
> +    if ( !cat_socket_init_bitmap )
> +        return;
> +
> +    cat_socket_enable_bitmap = xzalloc_array(unsigned long, nlongs);
> +    if ( !cat_socket_enable_bitmap )
> +    {
> +        xfree(cat_socket_init_bitmap);
> +        return;
> +    }
> +
> +    cat_socket_info = xzalloc_array(struct psr_cat_socket_info, nr_sockets);
> +    if ( !cat_socket_info )
> +    {
> +        xfree(cat_socket_init_bitmap);
> +        xfree(cat_socket_enable_bitmap);
> +    }
> +}

Please do all three allocs in a row, then check if any of them failed,
and on the error path free and clear all three pointers. This will
especially pay off if later further allocations get added here.

Jan


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