Re: [Xen-devel] [PATCH v18 04/10] x86: detect and initialize Cache Monitoring Technology feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.09.14 at 12:49, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/xen/arch/x86/psr.c
> @@ -0,0 +1,114 @@
> +/*
> + * pqos.c: Platform Shared Resource related service for guest.

Stale filename.

> +static void __init parse_psr_param(char *s)
> +{
> +    char *ss, *val_str;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( ss )
> +            *ss = '\0';
> +
> +        val_str = strchr(s, ':');
> +        if ( val_str )
> +            *val_str++ = '\0';
> +
> +        if ( !strcmp(s, "cmt") && ( !val_str || parse_bool(val_str) == 1 ) )
> +            opt_psr |= PSR_CMT;

Do you really mean to ignore e.g. "psr=cmt:xyz"?

> +static void __init init_psr_cmt(unsigned int rmid_max)
> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    unsigned int rmid;
> +
> +    if ( !boot_cpu_has(X86_FEATURE_CMT) )
> +        return;
> +
> +    cpuid_count(0xf, 0, &eax, &ebx, &ecx, &edx);
> +    if ( !edx )
> +        return;
> +
> +    psr_cmt = xzalloc(struct psr_cmt);
> +    if ( !psr_cmt )
> +        return;
> +
> +    psr_cmt->features = edx;
> +    psr_cmt->rmid_mask = ~(~0ull << get_count_order(ebx));
> +    psr_cmt->rmid_max = min(rmid_max, ebx);

The value written here gets replaced further down without taking
the value computed here into account - that's likely not what you
want. The two rmid_max values being recorded are kind of
confusing anyway - do you really need both at some point (other
than here)?

> +
> +    if ( psr_cmt->features & PSR_RESOURCE_TYPE_L3 )
> +    {
> +        cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
> +        psr_cmt->l3.upscaling_factor = ebx;
> +        psr_cmt->l3.rmid_max = ecx;
> +        psr_cmt->l3.features = edx;
> +    }
> +
> +    psr_cmt->rmid_max = min(rmid_max, psr_cmt->l3.rmid_max);
> +    psr_cmt->rmid_to_dom = xmalloc_array(domid_t, psr_cmt->rmid_max + 1);

This still degenerates to allocating zero bytes (and then corrupting
memory) when psr_cmt->rmid_max is 0xffffffff. At the very least
use 1UL as addend.

> +static int __init init_psr(void)
> +{
> +    if ( opt_psr & PSR_CMT && opt_rmid_max )

Please parenthesize the &.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -49,6 +49,7 @@
>  #include <xen/cpu.h>
>  #include <asm/nmi.h>
>  #include <asm/alternative.h>
> +#include <asm/psr.h>

Why?

> +struct psr_cmt {
> +    unsigned long rmid_mask;

When in patch 7 finally this field gets used for something it is to mask
a value read from an MSR. Hence its type should be uint64_t. And
of course it would have helped if the field got added there rather
than here (perhaps also for the l3 field below).

> +    unsigned int rmid_max;
> +    unsigned int features;
> +    domid_t *rmid_to_dom;
> +    struct psr_cmt_l3 l3;
> +};

Jan


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