Hi Leif,

Great, thanks!
Yes, we'll fold it into the commits that introduced the mess in the first place. I just didn't want to make this effort before you sign off on the refactoring. And hope it was easier to review this way.

Yes, we dropped some tables because we rethought how to apply smbios to this flexible RISC-V SoC, which doesn't really fit into the rigid view smbios has of hardware.

Cheers,
Daniel
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Leif Lindholm <leif@nuviainc.com>
Sent: Friday, August 14, 2020 15:40
To: devel@edk2.groups.io <devel@edk2.groups.io>; Schaefer, Daniel <daniel.schaefer@hpe.com>
Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>; Chen, Gilbert <gilbert.chen@hpe.com>; Michael D Kinney <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2-devel] [PATCH 1/1] edk2-platforms: Deduplicate RISC-V SMBIOS
 
Hi Daniel,

Thanks for this rework. It looks a massive improvement.

On Fri, Aug 07, 2020 at 18:44:44 +0200, Daniel Schaefer wrote:
> There was too much code, which wasn't called but it could have generated those SMBIOS table entries:
>
> - Type 4 for each core (4xU51, 1xE51)
> - Type 7 L1 instruction/data for each core
> - Type 7 L2 for U54
> - Type 44 for each core
> - Type 4 for the coreplex
> - Type 7 L2 for the coreplex
>
> Now it only has code for those entries:
>
> - Type 4 for SOC [1x]
> - Type 7 L1 for SOC [1x] (even though every hart has own L1, but my Laptop's Intel i5 does that also)
> - Type 7 L2 for SOC [1x]
> - Type 44 for each hart, associated with CPU [5x]
>
> In addition to simplifying the SMBIOS tables, the code for U54 and E51 is
> combined, like Leif suggested in his review.
>
> Here's what happened to the files:
>
> Expanded:
>
> - Platform/RISC-V/PlatformPkg/Library/FirmwareContextProcessorSpecificLib/FirmwareContextProcessorSpecificLib.c
>
> Deleted file:
>
> - Silicon/SiFive/E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c
> - Silicon/SiFive/U54MCCoreplex/Library/PeiCoreInfoHobLib/CoreInfoHob.c
>
> Merged with E51 code into single file:
>
> - Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
>
> Added SMBIOS Type 7 for L1 Cache, removed duplicated SMBIOS (Type 4 and 7 code):
>
> - Platform/SiFive/U5SeriesPkg/Library/PeiCoreInfoHobLib/CoreInfoHob.c
>
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/SiFive/SiFive.dec                     |   2 -
>  .../FreedomU500VC707Board/U500.dsc            |   1 -
>  .../FreedomU540HiFiveUnleashedBoard/U540.dsc  |   1 -
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf   |   1 -
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf   |  47 ----
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf   |   4 +
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf   |  46 ----
>  .../FirmwareContextProcessorSpecificLib.h     |  11 +
>  .../Include/ProcessorSpecificHobData.h        |   3 +-
>  Silicon/SiFive/Include/Library/SiFiveE51.h    |  60 -----
>  Silicon/SiFive/Include/Library/SiFiveU54.h    |  50 ++--
>  .../Include/Library/SiFiveU54MCCoreplex.h     |  55 ----
>  .../FirmwareContextProcessorSpecificLib.c     |  26 ++
>  .../Universal/Pei/PlatformPei/Platform.c      |   2 +-
>  .../Universal/Pei/PlatformPei/Platform.c      |   2 +-
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c   |  58 +----
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c   | 235 -----------------
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c   | 244 +++++++-----------
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c   | 184 -------------
>  19 files changed, 178 insertions(+), 854 deletions(-)

I know you dropped some tables, but that's a *nice* diffstat.

I guess this is effectively meant to be folded into existing commits?
If so:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
If not, I might start grumbling about some unrelated cleanup in this
patch...

/
    Leif