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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 07/06/17 4:07 AM >>>
>This patch implements the CPU init flow for CDP. The flow is almost
>same as L3 CAT.
>
>Note: CDP does NOT work until you apply the later patches of CDP.
>"x86: refactor psr: CDP: implement get hw info flow."
>"x86: refactor psr: CDP: implement set value callback function."

This is _not_ what I did ask for in what I assume ...

>v13:
    >- add commit message.

... this refers to: The problem isn't that it won't work, but that it'll crash 
the
hypervisor. So what I had expected you to add is a note _after_ the first
--- separator that this patch should not be applied without the other two
ones. Or alternatively for this one to add stubs which the subsequent
patches would then fill.

>@@ -262,6 +280,29 @@ static int cat_init_feature(const struct cpuid_leaf *regs,
 >
         >break;
 >
>+    case FEAT_TYPE_L3_CDP:
>+    {
>+        uint64_t val;
>+
>+        if ( feat->cos_max < 3 )
>+            return -ENOENT;

In the admittedly unlikely event that this return path gets taken (or any other
current or future one), ...

>@@ -1277,11 +1331,20 @@ static void psr_cpu_init(void)
     >{
         >cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s);
 >
>-        feat = feat_l3_cat;
>-        feat_l3_cat = NULL;
>-
>-        if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) )
>-            feat_props[FEAT_TYPE_L3_CAT] = &l3_cat_props;
>+        if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) )
>+        {
>+            feat = feat_l3_cdp;
>+            feat_l3_cdp = NULL;
>+            if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CDP) )
>+                feat_props[FEAT_TYPE_L3_CDP] = &l3_cdp_props;
>+        }
>+        else
>+        {
>+            feat = feat_l3_cat;
>+            feat_l3_cat = NULL;
>+            if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) )
>+                feat_props[FEAT_TYPE_L3_CAT] = &l3_cat_props;
>+        }

... wouldn't it be a good idea to then try to setup plain L3 CAT here?

I also notice that (already in patch 4) you leak feat here in case
cat_init_feature() fails. You should put it back into the static helper variable
you've taken it from. Furthermore, since you use at most one of feat_l3_cat
and feat_l3_cdp, so I don't see why you need to allocate two of them.

Jan


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