Re: [Xen-devel] [PATCH v6 1/7] x86: detect and initialize Cache QoS Monitoring feature

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, January 20, 2014 9:01 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> Ian.Campbell@xxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: Re: [PATCH v6 1/7] x86: detect and initialize Cache QoS Monitoring
> feature
> 
> >>> On 05.12.13 at 10:38, Dongxiao Xu <dongxiao.xu@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/cpu/intel.c
> > +++ b/xen/arch/x86/cpu/intel.c
> > @@ -230,6 +230,12 @@ static void __devinit init_intel(struct cpuinfo_x86 *c)
> >          ( c->cpuid_level >= 0x00000006 ) &&
> >          ( cpuid_eax(0x00000006) & (1u<<2) ) )
> >             set_bit(X86_FEATURE_ARAT, c->x86_capability);
> > +
> > +   /* Check platform QoS monitoring capability */
> > +   if ((c->cpuid_level >= 0x00000007) &&
> > +       (cpuid_ebx(0x00000007) & (1u<<12)))
> > +           set_bit(X86_FEATURE_QOSM, c->x86_capability);
> > +
> 
> This is redundant with generic_identify() setting the respective
> c->x86_capability[] element.
> 
> > +struct pqos_cqm *cqm;
> 
> __read_mostly?
cqm->rmid_to_dom will be updated time to time.

> 
> > +
> > +static void __init init_cqm(void)
> > +{
> > +    unsigned int rmid;
> > +    unsigned int eax, edx;
> > +
> > +    if ( !opt_cqm_max_rmid )
> > +        return;
> > +
> > +    cqm = xzalloc(struct pqos_cqm);
> > +    if ( !cqm )
> > +        return;
> > +
> > +    cpuid_count(0xf, 1, &eax, &cqm->upscaling_factor, &cqm->max_rmid,
> &edx);
> > +    if ( !(edx & QOS_MONITOR_EVTID_L3) )
> > +    {
> > +        xfree(cqm);
> 
> "cqm" is a global variable and - afaict - the only way for other
> entities to tell whether the functionality is enabled. Hence shouldn't
> you clear the variable here (and similarly further down)? Otherwise
> the variable should perhaps be static.

Yes, will correct it in following patches.

> 
> Jan


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