Re: [PATCH v1 2/2] UefiCpuPkg/CpuCacheInfoLib: Add new CpuCacheInfoLib.

["Laszlo Ersek" <lersek@redhat.com>]

On 11/19/20 03:36, jasonlouyun wrote:
> This library uses a platform agnostic algorithm to get CPU cache
> information. It provides user with an API(GetCpuCacheInfo) to get
> detailed CPU cache information by each package, each core type
> included in this package, and each cache level & type.
> 
> Signed-off-by: Jason Lou <yun.lou@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/Library/CpuCacheInfoLib/CpuCacheInfoLib.c         | 602 ++++++++++++++++++++
>  UefiCpuPkg/Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.c      | 131 +++++
>  UefiCpuPkg/Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.c      | 130 +++++
>  UefiCpuPkg/Include/Library/CpuCacheInfoLib.h                 |  68 +++
>  UefiCpuPkg/Library/CpuCacheInfoLib/CpuCacheInfoLib.uni       |  15 +
>  UefiCpuPkg/Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.inf    |  43 ++
>  UefiCpuPkg/Library/CpuCacheInfoLib/InternalCpuCacheInfoLib.h |  64 +++
>  UefiCpuPkg/Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.inf    |  43 ++
>  UefiCpuPkg/UefiCpuPkg.dec                                    |   3 +
>  UefiCpuPkg/UefiCpuPkg.dsc                                    |   4 +
>  10 files changed, 1103 insertions(+)

I defer this review to the other UefiCpuPkg reviewers / maintainers.

Two superficial suggestions:

- please file a TianoCore feature request BZ, and reference it in the
commit messages in this series

- *at least one* of the commit message and the bugzilla ticket, but
preferably both, should describe the use case. In other words, what
platforms intend to use the new library (especially if they are open
source in edk2-platforms or otherwise), and what use cases will benefit
from having cache information.

Otherwise, this library is just dead code in edk2. I don't insist that
the consumers of this library be open source, but I do insist that
*something* be publicly explained about the use case.

Thanks
Laszlo