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

["Jan Beulich" <jbeulich@xxxxxxxx>]

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

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