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/02/17 5:12 PM >>>
>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.

You could pull the copying of feat_l3 into feat inside the if() checking
the capability. But your alternative doesn't look too bad as well.

Jan


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