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

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

On 17-08-02 06:35:46, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 08/01/17 11:04 AM >>>
> >@@ -1278,15 +1339,31 @@ static void psr_cpu_init(void)
> >cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s);
> >if ( regs.b & PSR_RESOURCE_TYPE_L3 )
> >{
> >+        bool do_l3_cat_init = true;
> >+
> >cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s);
>  >
> >feat = feat_l3;
> >feat_l3 = NULL;
>  >
> >-        if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) )
> >-            feat_props[FEAT_TYPE_L3_CAT] = &l3_cat_props;
> >-        else
> >-            feat_l3 = feat;
> >+        if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) )
> >+        {
> >+            /* If CDP init fails, try to work as L3 CAT. */
> >+            if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CDP) )
> >+            {
> >+                feat_props[FEAT_TYPE_L3_CDP] = &l3_cdp_props;
> >+                /* CDP init succeeds, no need to do L3 CAT init. */
> >+                do_l3_cat_init = false;
> >+            }
> >+        }
> 
> The comment ahead of the inner if() now really describes the (implicit)
> else case. That's somewhat misleading. How about putting feat back
> into feat_l3 in an actual "else", and using that at once instead of the
> somewhat clumsily named "do_l3_cat_init" local variable? That would
> additionally avoid the need for me to ask you to fold the two if()s. Plus
> the resulting code would become more similar ...
> 
> >+        if ( do_l3_cat_init )
> >+        {
> >+            if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) )
> >+                feat_props[FEAT_TYPE_L3_CAT] = &l3_cat_props;
> >+            else
> >+                feat_l3 = feat;
> >+        }
> 
> ... to this.
> 
Thanks for the comment! But I do not understand the intention to put feat
back into feat_l3 in else. After putting feat back into feat_l3, how to
handle L3 CAT init? The L3 CAT init should be entered under two cases:
1. No CDP capability in regs.c. In such case, the feat equals feat_l3 and
   feat_l3 has been NULL at above step.
2. CDP init fails. In such case, the feat equals feat_l3. If we put feat
   back into feat_l3, then they are equal.

So, we cannot use feat or feat_l3 to decide entering L3 CAT init or not.

I think we may check if 'feat_props[FEAT_TYPE_L3_CDP]' is NULL to decide
entering L3 CAT init or not. Because under above two cases, it is NULL.
E.g.
    feat = feat_l3;
    feat_l3 = NULL;
    if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) )
    {
        /* If CDP init fails, try to work as L3 CAT. */
        if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CDP) )
            feat_props[FEAT_TYPE_L3_CDP] = &l3_cdp_props;
    } 

    if ( !feat_props[FEAT_TYPE_L3_CDP] )
    {
        if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) )
            feat_props[FEAT_TYPE_L3_CAT] = &l3_cat_props;
        else
            feat_l3 = feat;
    }

How do you think? Thanks!

> And btw, to avoid spamming the list with another full version of this
> series, I'd be fine with you just submitting v15.1 of this one patch.
> 
> Jan

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