Re: [Xen-devel] [PATCH v12 13/23] x86: refactor psr: CDP: implement CPU init flow.

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-06-30 00:40:35, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/14/17 3:26 AM >>>
> > @@ -253,6 +271,26 @@ static void cat_init_feature(const struct cpuid_leaf 
> > *regs,
> >  
> >          break;
> >  
> > +    case PSR_SOCKET_L3_CDP:
> > +    {
> > +        uint64_t val;
> > +
> > +        /* Cut half of cos_max when CDP is enabled. */
> > +        feat->cos_max >>= 1;
> 
> I'm afraid this is off by one in the unusual but possible case of cos_max
> being an even number.
> 
This accords to spec:
"For CDP operations, COS_MAX_CDP is equal to (CPUID.(EAX=10H, 
ECX=1):EDX.COS_MAX_CAT >>1)."

HW should make sure it is even number.

> > +        wrmsrl(MSR_IA32_PSR_L3_MASK(0), cat_default_val(feat->cbm_len));
> > +        wrmsrl(MSR_IA32_PSR_L3_MASK(1), cat_default_val(feat->cbm_len));
> > +        rdmsrl(MSR_IA32_PSR_L3_QOS_CFG, val);
> > +        wrmsrl(MSR_IA32_PSR_L3_QOS_CFG,
> > +               val | (1ull << PSR_L3_QOS_CDP_ENABLE_BIT));
> > +
> > +        /* cos=0 is reserved as default cbm(all bits within cbm_len are 
> > 1). */
> 
> Along the lines of a comment to an earlier patch, please add a blank ahead of
> the opeing paren.
> 
Got it.

> > +        get_cdp_code(feat, 0) = cat_default_val(feat->cbm_len);
> > +        get_cdp_data(feat, 0) = cat_default_val(feat->cbm_len);
> 
> Wouldn't you better do this prior to enabling CDP?
> 
Sure.

> > @@ -1294,11 +1344,21 @@ static void psr_cpu_init(void)
> >      {
> >          cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s);
> >  
> > -        feat = feat_l3_cat;
> > -        feat_l3_cat = NULL;
> > -        feat_props[PSR_SOCKET_L3_CAT] = &l3_cat_props;
> > -
> > -        cat_init_feature(®s, feat, info, PSR_SOCKET_L3_CAT);
> > +        if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) &&
> > +             !info->features[PSR_SOCKET_L3_CDP] )
> 
> Doesn't this last check mean you'd set up CAT in case would come here for
> the 2nd CPU on a socket? In the end the check is simplky pointless afaict,
> due to psr_cpu_init() calling cat_init_feature() only if ->feat_init is still
> false. But please remove it as being potentially confusing (and inconsistent
> with the else branch).
> 
Ok, will remove the last check here.

> Jan

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