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

[Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>]

On Tue, Sep 30, 2014 at 02:13:18PM +0100, Jan Beulich wrote:
> >>> 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.

Good catch, thanks.

> 
> > +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"?

Yes, CMT is disabled silently for such case. But I feel comfortable to
add some msg like this:
    if ( parse_bool(val_str) == -1 )
        printk("PSR: unknown cmt value: %s - CMT disabled!\n", val_str);

> 
> > +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)?

This is actually a good catch. The line below should be:
psr_cmt->rmid_max = min(psr_cmt->rmid_max, psr_cmt->l3.rmid_max);

> 
> > +
> > +    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.

Indeed, xmalloc will not return NULL as we expected for such case.
Then the following can be picked up again:
    BUG_ON(pqosm->rmid_max < 0xffffffff);

> 
> > +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).

Will change rmid_mask to uint64_t and move it to .c file. But will leave
l3 field here as it's needed for sysctl.c.

Chao
> 
> > +    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

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