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

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-07-12 13:52:35, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@linux.intel.com> 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.
> 
Oh, ok. I will move the comments under the first '---'. Furthermore, I'd like
to add a 'stub_write_msr()' here to avoid crash. Will replace the stub function
to real function when implementing the write flow.

> >@@ -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?
> 
DYM if CDP init fails, enter CAT init flow to make CAT work at least?

> 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.
> 
Ok, I will keep the helper variable if cat_init_feature() fails.

Will converge the 'feat_l3_cat' and 'feat_l3_cdp' to one.

> Jan

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