[edk2-devel] [PATCH 1/1] edk2-platforms: Deduplicate RISC-V SMBIOS

Sat Aug 15 09:40:18 UTC 2020

Thanks Leif and Daniel,
Now all of the commits were reviewed. I will rebase it to the latest edk2-platform master branch and check if any problems there. Will merge it if no problem found.

Abner

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

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 at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> on behalf of Leif Lindholm <leif at nuviainc.com<mailto:leif at nuviainc.com>>
Sent: Friday, August 14, 2020 15:40
To: devel at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>>; Schaefer, Daniel <daniel.schaefer at hpe.com<mailto:daniel.schaefer at hpe.com>>
Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang at hpe.com<mailto:abner.chang at hpe.com>>; Chen, Gilbert <gilbert.chen at hpe.com<mailto:gilbert.chen at hpe.com>>; Michael D Kinney <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>; Ard Biesheuvel <ard.biesheuvel at linaro.org<mailto:ard.biesheuvel at 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 at hpe.com<mailto:abner.chang at hpe.com>>
> Cc: Gilbert Chen <gilbert.chen at hpe.com<mailto:gilbert.chen at hpe.com>>
> Cc: Leif Lindholm <leif at nuviainc.com<mailto:leif at nuviainc.com>>
> Cc: Michael D Kinney <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org<mailto:ard.biesheuvel at 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 at nuviainc.com<mailto:leif at nuviainc.com>>
If not, I might start grumbling about some unrelated cleanup in this
patch...

/
    Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#64298): https://edk2.groups.io/g/devel/message/64298
Mute This Topic: https://groups.io/mt/76053055/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200815/8bb0807b/attachment.htm>