[edk2-devel] [PATCH v2 0/3] New RISC-V Patches

Daniel Schaefer posted 3 patches 3 years, 10 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.inf |  28 +
Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf |  60 ++
Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h     |  72 ++
Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h           | 631 ++++++++++++++++
Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h                      |  73 ++
Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c   | 789 ++++++++++++++++++++
.gitmodules                                                             |   3 +
Readme.md                                                               |  36 +
Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/opensbi             |   1 +
9 files changed, 1693 insertions(+)
create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.inf
create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf
create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h
create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h
create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h
create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
create mode 100644 .gitmodules
create mode 160000 Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/opensbi
[edk2-devel] [PATCH v2 0/3] New RISC-V Patches
Posted by Daniel Schaefer 3 years, 10 months ago
In this updated version I addressed Leif's comments and made the following
changes:

- Refactor sbi_call* to SbiCall* (EDKII style)
- Use OpenSBI constants if possible
- Include Base.h in OpensbiTypes.h
- Only use __builtin_expect with Clang and GCC (not MSVC)

I'm sorry, I hadn't explained the new branches properly.
Previously we had all code going to EDK2 via the RISC-V-V2 branch.

Now we're only making the least amount of necessary changes in edk2 and
everything else in edk2-platforms.
Those changes to edk2 can be grouped into different categories:

- Patches for RISC-V EDK2 CI enablement
- Patches for edk2 modules other than RISC-V ones, to allow building them with the RISC-V toolchain
- Other RISC-V enablement like PE/COFF relocation

Those have all been reviewed and merged to edk2 master.

Previously we had two packages just for RISC-V on our edk2 branch:
  RiscVPkg and RiscVPlatformPkg
They are now under
  Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorPkg
in edk2-platforms.
You, Leif, have previously reviewed those. In addition to this old code, which
was moved, we need some more patches to allow running PEI in S-Mode and
building in edk2-platforms. That's what this patch series is about.

In the previous version of this patchseries I forgot to attach the biggest new
commit, which adds RiscVEdk2SbiLib. It wraps the ecall interface for calling
SBI in a C API and lets PEI and DXE call SBI interfaces. Because we need more
M-Mode capabilities in PEI and DXE than SBI gives us, we register another SBI
extension, that gives us access to the mscratch register.

I hope now it makes more sense.

- Daniel

Cc: Abner Chang <abner.chang@hpe.com>
Cc: Gilbert Chen <gilbert.chen@hpe.com>
Cc: Michael D Kinney <michael.k.kinney@intel.com>
Cc: Leif Lindholm <leif@nuviainc.com>

Abner Chang (1):
  ProcessorPkg/Library: Add RiscVOpensbiLib

Daniel Schaefer (2):
  ProcessorPkg/RiscVOpensbLib: Add opensbi submodule
  ProcessorPkg/Library: Add RiscVEdk2SbiLib

 Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.inf |  28 +
 Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf |  60 ++
 Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h     |  72 ++
 Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h           | 631 ++++++++++++++++
 Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h                      |  73 ++
 Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c   | 789 ++++++++++++++++++++
 .gitmodules                                                             |   3 +
 Readme.md                                                               |  36 +
 Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/opensbi             |   1 +
 9 files changed, 1693 insertions(+)
 create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.inf
 create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf
 create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h
 create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h
 create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h
 create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
 create mode 100644 .gitmodules
 create mode 160000 Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/opensbi

-- 
2.26.1


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

View/Reply Online (#59682): https://edk2.groups.io/g/devel/message/59682
Mute This Topic: https://groups.io/mt/74227138/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches
Posted by Leif Lindholm 3 years, 10 months ago
On Fri, May 15, 2020 at 15:39:34 +0200, Daniel Schaefer wrote:
> In this updated version I addressed Leif's comments and made the following
> changes:
> 
> - Refactor sbi_call* to SbiCall* (EDKII style)
> - Use OpenSBI constants if possible
> - Include Base.h in OpensbiTypes.h
> - Only use __builtin_expect with Clang and GCC (not MSVC)
> 
> I'm sorry, I hadn't explained the new branches properly.
> Previously we had all code going to EDK2 via the RISC-V-V2 branch.
> 
> Now we're only making the least amount of necessary changes in edk2 and
> everything else in edk2-platforms.
> Those changes to edk2 can be grouped into different categories:
> 
> - Patches for RISC-V EDK2 CI enablement
> - Patches for edk2 modules other than RISC-V ones, to allow building them with the RISC-V toolchain
> - Other RISC-V enablement like PE/COFF relocation
> 
> Those have all been reviewed and merged to edk2 master.
> 
> Previously we had two packages just for RISC-V on our edk2 branch:
>   RiscVPkg and RiscVPlatformPkg
> They are now under
>   Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorPkg
> in edk2-platforms.

Understood. I took my eye off the ball there for a while, but I'm a
bit confused as to why RiscVPkg isn't going into EDK2. That is very
counterintuitive. And clearly it will need revisiting if we are to add
first-class CI checks like those we do with OvmfPkg/ArmVirtPkg.

> You, Leif, have previously reviewed those. In addition to this old code, which
> was moved, we need some more patches to allow running PEI in S-Mode and
> building in edk2-platforms. That's what this patch series is about.

I *did* have some outstanding comments specifically with regards to
large amounts of code duplication between the SMBIOS implementation of
some closely related RISC-V platforms. That now needs to be revisited.

> In the previous version of this patchseries I forgot to attach the biggest new
> commit, which adds RiscVEdk2SbiLib. It wraps the ecall interface for calling
> SBI in a C API and lets PEI and DXE call SBI interfaces. Because we need more
> M-Mode capabilities in PEI and DXE than SBI gives us, we register another SBI
> extension, that gives us access to the mscratch register.

Without looking at it yet, it sounds like that may resolve the only
remaining major issue I had with RiscVPkg.

> I hope now it makes more sense.

It is more clear, as per above I am not sure it makes more sense :)
Thanks!

Best Regards,

Leif

> - Daniel
> 
> Cc: Abner Chang <abner.chang@hpe.com>
> Cc: Gilbert Chen <gilbert.chen@hpe.com>
> Cc: Michael D Kinney <michael.k.kinney@intel.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> 
> Abner Chang (1):
>   ProcessorPkg/Library: Add RiscVOpensbiLib
> 
> Daniel Schaefer (2):
>   ProcessorPkg/RiscVOpensbLib: Add opensbi submodule
>   ProcessorPkg/Library: Add RiscVEdk2SbiLib
> 
>  Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.inf |  28 +
>  Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf |  60 ++
>  Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h     |  72 ++
>  Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h           | 631 ++++++++++++++++
>  Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h                      |  73 ++
>  Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c   | 789 ++++++++++++++++++++
>  .gitmodules                                                             |   3 +
>  Readme.md                                                               |  36 +
>  Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/opensbi             |   1 +
>  9 files changed, 1693 insertions(+)
>  create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.inf
>  create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/RiscVOpensbiLib.inf
>  create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h
>  create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h
>  create mode 100644 Silicon/RISC-V/ProcessorPkg/Include/OpensbiTypes.h
>  create mode 100644 Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c
>  create mode 100644 .gitmodules
>  create mode 160000 Silicon/RISC-V/ProcessorPkg/Library/RiscVOpensbiLib/opensbi
> 
> -- 
> 2.26.1
> 
> 
> 
> 

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

View/Reply Online (#59960): https://edk2.groups.io/g/devel/message/59960
Mute This Topic: https://groups.io/mt/74227138/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
Posted by Daniel Schaefer 3 years, 10 months ago
On 5/20/20 1:43 PM, Leif Lindholm wrote:
> On Fri, May 15, 2020 at 15:39:34 +0200, Daniel Schaefer wrote:
>> Previously we had two packages just for RISC-V on our edk2 branch:
>>    RiscVPkg and RiscVPlatformPkg
>> They are now under
>>    Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorPkg
>> in edk2-platforms.
> 
> Understood. I took my eye off the ball there for a while, but I'm a
> bit confused as to why RiscVPkg isn't going into EDK2. That is very
> counterintuitive. And clearly it will need revisiting if we are to add
> first-class CI checks like those we do with OvmfPkg/ArmVirtPkg.

Yes, I understand your concern. Personally I'd like it also to be in 
EDK2 straight away, however Mike, Bret and Sean have raised valid concerns:

1. RISC-V is very new and potentially unstable - it's quicker to make 
changes in edk2-platforms.

2. If we define new interfaces and libraries in edk2, we can't remove 
them easily because it would be a backwards-incompatible change. 
edk2-platforms isn't quite as strict.

3. Long-term, I think many agree, we should aim to move much of the 
RISC-V code into UefiCpuPkg and OvmfPkg. Mike mentioned that would need 
coordination with ARM maintainers because it might make sense to move 
their code there as well.

Maybe Mike, Bret or Sean would like to provide some more comments?

> I *did* have some outstanding comments specifically with regards to
> large amounts of code duplication between the SMBIOS implementation of
> some closely related RISC-V platforms. That now needs to be revisited.

The SMBIOS code hasn't changed. It has moved to
   Silicon/SiFive/{E51,U54,U54MCCoreplex}/Library/PeiCoreInfoHobLib
You're referring to this library, right?

They build the SMBIOS entries for a particular processor. Yes, the 
values do have a lot of overlap but these files are like configuration 
files. They don't do much, they only set the values of the properties.

Currently it is not possible to let the UEFI firmware get this 
information from the hardware at runtime, especially now, since we're 
running in S-Mode.
To allow that, we created a RISC-V working group to be able to retrieve 
all of this information dynamically from the processor (among other 
goals). Then the vendor will not have to modify these files and hardcode 
the values anymore. Which enables us to create a single library for all 
processors.
See: https://github.com/riscv/configuration-structure

I hope I described everything properly, please correct me otherwise, Abner.

> 
>> In the previous version of this patchseries I forgot to attach the biggest new
>> commit, which adds RiscVEdk2SbiLib. It wraps the ecall interface for calling
>> SBI in a C API and lets PEI and DXE call SBI interfaces. Because we need more
>> M-Mode capabilities in PEI and DXE than SBI gives us, we register another SBI
>> extension, that gives us access to the mscratch register.
> 
> Without looking at it yet, it sounds like that may resolve the only
> remaining major issue I had with RiscVPkg.
> 
>> I hope now it makes more sense.
> 
> It is more clear, as per above I am not sure it makes more sense :)
> Thanks!

Your concerns are very valid, however due to the mentioned trade-offs it 
might not make sense to address them.

- Daniel

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

View/Reply Online (#59973): https://edk2.groups.io/g/devel/message/59973
Mute This Topic: https://groups.io/mt/74353494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
Posted by Daniel Schaefer 3 years, 10 months ago
please reply here, fixed Mike's email address, sorry...

On 5/20/20 6:03 PM, Daniel Schaefer wrote:
> On 5/20/20 1:43 PM, Leif Lindholm wrote:
>> On Fri, May 15, 2020 at 15:39:34 +0200, Daniel Schaefer wrote:
>>> Previously we had two packages just for RISC-V on our edk2 branch:
>>>    RiscVPkg and RiscVPlatformPkg
>>> They are now under
>>>    Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorPkg
>>> in edk2-platforms.
>>
>> Understood. I took my eye off the ball there for a while, but I'm a
>> bit confused as to why RiscVPkg isn't going into EDK2. That is very
>> counterintuitive. And clearly it will need revisiting if we are to add
>> first-class CI checks like those we do with OvmfPkg/ArmVirtPkg.
> 
> Yes, I understand your concern. Personally I'd like it also to be in 
> EDK2 straight away, however Mike, Bret and Sean have raised valid concerns:
> 
> 1. RISC-V is very new and potentially unstable - it's quicker to make 
> changes in edk2-platforms.
> 
> 2. If we define new interfaces and libraries in edk2, we can't remove 
> them easily because it would be a backwards-incompatible change. 
> edk2-platforms isn't quite as strict.
> 
> 3. Long-term, I think many agree, we should aim to move much of the 
> RISC-V code into UefiCpuPkg and OvmfPkg. Mike mentioned that would need 
> coordination with ARM maintainers because it might make sense to move 
> their code there as well.
> 
> Maybe Mike, Bret or Sean would like to provide some more comments?
> 
>> I *did* have some outstanding comments specifically with regards to
>> large amounts of code duplication between the SMBIOS implementation of
>> some closely related RISC-V platforms. That now needs to be revisited.
> 
> The SMBIOS code hasn't changed. It has moved to
>    Silicon/SiFive/{E51,U54,U54MCCoreplex}/Library/PeiCoreInfoHobLib
> You're referring to this library, right?
> 
> They build the SMBIOS entries for a particular processor. Yes, the 
> values do have a lot of overlap but these files are like configuration 
> files. They don't do much, they only set the values of the properties.
> 
> Currently it is not possible to let the UEFI firmware get this 
> information from the hardware at runtime, especially now, since we're 
> running in S-Mode.
> To allow that, we created a RISC-V working group to be able to retrieve 
> all of this information dynamically from the processor (among other 
> goals). Then the vendor will not have to modify these files and hardcode 
> the values anymore. Which enables us to create a single library for all 
> processors.
> See: https://github.com/riscv/configuration-structure
> 
> I hope I described everything properly, please correct me otherwise, Abner.
> 
>>
>>> In the previous version of this patchseries I forgot to attach the 
>>> biggest new
>>> commit, which adds RiscVEdk2SbiLib. It wraps the ecall interface for 
>>> calling
>>> SBI in a C API and lets PEI and DXE call SBI interfaces. Because we 
>>> need more
>>> M-Mode capabilities in PEI and DXE than SBI gives us, we register 
>>> another SBI
>>> extension, that gives us access to the mscratch register.
>>
>> Without looking at it yet, it sounds like that may resolve the only
>> remaining major issue I had with RiscVPkg.
>>
>>> I hope now it makes more sense.
>>
>> It is more clear, as per above I am not sure it makes more sense :)
>> Thanks!
> 
> Your concerns are very valid, however due to the mentioned trade-offs it 
> might not make sense to address them.
> 
> - Daniel

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

View/Reply Online (#59974): https://edk2.groups.io/g/devel/message/59974
Mute This Topic: https://groups.io/mt/74353494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
Posted by Daniel Schaefer 3 years, 10 months ago

On 5/20/20 6:07 PM, Daniel Schaefer wrote:
> please reply here, fixed Mike's email address, sorry...
> 
> On 5/20/20 6:03 PM, Daniel Schaefer wrote:
>> On 5/20/20 1:43 PM, Leif Lindholm wrote:
>>> On Fri, May 15, 2020 at 15:39:34 +0200, Daniel Schaefer wrote:
>>>> Previously we had two packages just for RISC-V on our edk2 branch:
>>>>    RiscVPkg and RiscVPlatformPkg
>>>> They are now under
>>>>    Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorPkg
>>>> in edk2-platforms.
>>>
>>> Understood. I took my eye off the ball there for a while, but I'm a
>>> bit confused as to why RiscVPkg isn't going into EDK2. That is very
>>> counterintuitive. And clearly it will need revisiting if we are to add
>>> first-class CI checks like those we do with OvmfPkg/ArmVirtPkg.
>>
>> Yes, I understand your concern. Personally I'd like it also to be in 
>> EDK2 straight away, however Mike, Bret and Sean have raised valid 
>> concerns:
>>
>> 1. RISC-V is very new and potentially unstable - it's quicker to make 
>> changes in edk2-platforms.
>>
>> 2. If we define new interfaces and libraries in edk2, we can't remove 
>> them easily because it would be a backwards-incompatible change. 
>> edk2-platforms isn't quite as strict.
>>
>> 3. Long-term, I think many agree, we should aim to move much of the 
>> RISC-V code into UefiCpuPkg and OvmfPkg. Mike mentioned that would 
>> need coordination with ARM maintainers because it might make sense to 
>> move their code there as well.
>>
>> Maybe Mike, Bret or Sean would like to provide some more comments?
>>
>>> I *did* have some outstanding comments specifically with regards to
>>> large amounts of code duplication between the SMBIOS implementation of
>>> some closely related RISC-V platforms. That now needs to be revisited.
>>
>> The SMBIOS code hasn't changed. It has moved to
>>    Silicon/SiFive/{E51,U54,U54MCCoreplex}/Library/PeiCoreInfoHobLib
>> You're referring to this library, right?
>>
>> They build the SMBIOS entries for a particular processor. Yes, the 
>> values do have a lot of overlap but these files are like configuration 
>> files. They don't do much, they only set the values of the properties.
>>
>> Currently it is not possible to let the UEFI firmware get this 
>> information from the hardware at runtime, especially now, since we're 
>> running in S-Mode.
>> To allow that, we created a RISC-V working group to be able to 
>> retrieve all of this information dynamically from the processor (among 
>> other goals). Then the vendor will not have to modify these files and 
>> hardcode the values anymore. Which enables us to create a single 
>> library for all processors.
>> See: https://github.com/riscv/configuration-structure
>>
>> I hope I described everything properly, please correct me otherwise, 
>> Abner.
>>
>>>
>>>> In the previous version of this patchseries I forgot to attach the 
>>>> biggest new
>>>> commit, which adds RiscVEdk2SbiLib. It wraps the ecall interface for 
>>>> calling
>>>> SBI in a C API and lets PEI and DXE call SBI interfaces. Because we 
>>>> need more
>>>> M-Mode capabilities in PEI and DXE than SBI gives us, we register 
>>>> another SBI
>>>> extension, that gives us access to the mscratch register.
>>>
>>> Without looking at it yet, it sounds like that may resolve the only
>>> remaining major issue I had with RiscVPkg.
>>>
>>>> I hope now it makes more sense.
>>>
>>> It is more clear, as per above I am not sure it makes more sense :)
>>> Thanks!
>>
>> Your concerns are very valid, however due to the mentioned trade-offs 
>> it might not make sense to address them.
>>
>> - Daniel

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

View/Reply Online (#59975): https://edk2.groups.io/g/devel/message/59975
Mute This Topic: https://groups.io/mt/74353494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
Posted by Abner Chang 3 years, 10 months ago

> -----Original Message-----
> From: Schaefer, Daniel (DualStudy)
> Sent: Thursday, May 21, 2020 12:18 AM
> To: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io
> Cc: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> Chen, Gilbert <gilbert.chen@hpe.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Bret.Barkelew@microsoft.com;
> sean.brogan@microsoft.com
> Subject: Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-
> platforms
> 
> 
> 
> On 5/20/20 6:07 PM, Daniel Schaefer wrote:
> > please reply here, fixed Mike's email address, sorry...
> >
> > On 5/20/20 6:03 PM, Daniel Schaefer wrote:
> >> On 5/20/20 1:43 PM, Leif Lindholm wrote:
> >>> On Fri, May 15, 2020 at 15:39:34 +0200, Daniel Schaefer wrote:
> >>>> Previously we had two packages just for RISC-V on our edk2 branch:
> >>>>    RiscVPkg and RiscVPlatformPkg
> >>>> They are now under
> >>>>    Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorPkg in
> >>>> edk2-platforms.
> >>>
> >>> Understood. I took my eye off the ball there for a while, but I'm a
> >>> bit confused as to why RiscVPkg isn't going into EDK2. That is very
> >>> counterintuitive. And clearly it will need revisiting if we are to
> >>> add first-class CI checks like those we do with OvmfPkg/ArmVirtPkg.
> >>
> >> Yes, I understand your concern. Personally I'd like it also to be in
> >> EDK2 straight away, however Mike, Bret and Sean have raised valid
> >> concerns:
> >>
> >> 1. RISC-V is very new and potentially unstable - it's quicker to make
> >> changes in edk2-platforms.
> >>
> >> 2. If we define new interfaces and libraries in edk2, we can't remove
> >> them easily because it would be a backwards-incompatible change.
> >> edk2-platforms isn't quite as strict.
> >>
> >> 3. Long-term, I think many agree, we should aim to move much of the
> >> RISC-V code into UefiCpuPkg and OvmfPkg. Mike mentioned that would
> >> need coordination with ARM maintainers because it might make sense to
> >> move their code there as well.
> >>
> >> Maybe Mike, Bret or Sean would like to provide some more comments?
> >>
> >>> I *did* have some outstanding comments specifically with regards to
> >>> large amounts of code duplication between the SMBIOS implementation
> >>> of some closely related RISC-V platforms. That now needs to be revisited.
> >>
> >> The SMBIOS code hasn't changed. It has moved to
> >>    Silicon/SiFive/{E51,U54,U54MCCoreplex}/Library/PeiCoreInfoHobLib
> >> You're referring to this library, right?
> >>
> >> They build the SMBIOS entries for a particular processor. Yes, the
> >> values do have a lot of overlap but these files are like
> >> configuration files. They don't do much, they only set the values of the
> properties.
> >>
> >> Currently it is not possible to let the UEFI firmware get this
> >> information from the hardware at runtime, especially now, since we're
> >> running in S-Mode.
> >> To allow that, we created a RISC-V working group to be able to
> >> retrieve all of this information dynamically from the processor
> >> (among other goals). Then the vendor will not have to modify these
> >> files and hardcode the values anymore. Which enables us to create a
> >> single library for all processors.
> >> See: https://github.com/riscv/configuration-structure
> >>
> >> I hope I described everything properly, please correct me otherwise,
> >> Abner.
[Abner]  Yes almost. Thanks Daniel.
One thing I would like to add,
If you take a look on SiFive Core IP https://www.sifive.com/risc-v-core-ip you can see there are different SKUs of RISC-V core. Just take some as example,
S51 - Single core
U54 - Single core
S76 - Single core
U74- single core
U54-MC - Multicore which is 4*U54 cores +1*S51 core
U74-MC - Multicore which is 4*U74 core + 1*S7 core

Those are the combinations of core IP. Silicon vendor can get those core IPs and combine them to the RISC-V processor. To have CoreInfoHobLib libraries for each different core (not multicore) to build up the core capability is reasonable and makes sense. For the multicore, it just pulling the single core CoreInfoHobLib to build up the SMBIOS table for the multicore processor. Those libraries look duplicate in logically, however only one instance of CoreInfoHobLib is built in for multiple identical cores in physically view. Maybe we still can move some identical core into the core-specific library but it is not worthwhile.

Abner

> >>
> >>>
> >>>> In the previous version of this patchseries I forgot to attach the
> >>>> biggest new commit, which adds RiscVEdk2SbiLib. It wraps the ecall
> >>>> interface for calling SBI in a C API and lets PEI and DXE call SBI
> >>>> interfaces. Because we need more M-Mode capabilities in PEI and DXE
> >>>> than SBI gives us, we register another SBI extension, that gives us
> >>>> access to the mscratch register.
> >>>
> >>> Without looking at it yet, it sounds like that may resolve the only
> >>> remaining major issue I had with RiscVPkg.
> >>>
> >>>> I hope now it makes more sense.
> >>>
> >>> It is more clear, as per above I am not sure it makes more sense :)
> >>> Thanks!
> >>
> >> Your concerns are very valid, however due to the mentioned trade-offs
> >> it might not make sense to address them.
> >>
> >> - Daniel

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

View/Reply Online (#60012): https://edk2.groups.io/g/devel/message/60012
Mute This Topic: https://groups.io/mt/74353494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
Posted by Leif Lindholm 3 years, 10 months ago
Hi Abner,

Sorry, I should have followed up on this sooner.

On Thu, May 21, 2020 at 06:59:20 +0000, Chang, Abner (HPS SW/FW Technologist) wrote:
> > On 5/20/20 6:07 PM, Daniel Schaefer wrote:
> > > please reply here, fixed Mike's email address, sorry...
> > >
> > > On 5/20/20 6:03 PM, Daniel Schaefer wrote:
> > >> On 5/20/20 1:43 PM, Leif Lindholm wrote:
> > >>> On Fri, May 15, 2020 at 15:39:34 +0200, Daniel Schaefer wrote:
> > >>>> Previously we had two packages just for RISC-V on our edk2 branch:
> > >>>>    RiscVPkg and RiscVPlatformPkg
> > >>>> They are now under
> > >>>>    Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorPkg in
> > >>>> edk2-platforms.
> > >>>
> > >>> Understood. I took my eye off the ball there for a while, but I'm a
> > >>> bit confused as to why RiscVPkg isn't going into EDK2. That is very
> > >>> counterintuitive. And clearly it will need revisiting if we are to
> > >>> add first-class CI checks like those we do with OvmfPkg/ArmVirtPkg.
> > >>
> > >> Yes, I understand your concern. Personally I'd like it also to be in
> > >> EDK2 straight away, however Mike, Bret and Sean have raised valid
> > >> concerns:

Can you point me to the conversation I have missed?

> > >> 1. RISC-V is very new and potentially unstable - it's quicker to make
> > >> changes in edk2-platforms.

I don't see this as a valid argument.
It's not edk2-unstable, it is edk2-platforms.

edk2-platforms exists because there used to be strong feelings against
holding *real* platforms in edk2, with edk2 being originally intended
only as a code library for IBV/ISV to cherry-pick from.

But fundamentally, if code is too immature to go into the master
branch of edk2, it is too immature to go into the master branch of
edk2-platforms. If we want edk2-might-be-a-bit-shaky-but-who-cares,
then someone will have to create it.

> > >> 2. If we define new interfaces and libraries in edk2, we can't remove
> > >> them easily because it would be a backwards-incompatible change.
> > >> edk2-platforms isn't quite as strict.

Yes it is.
The only thing making it less strict is its contents - platform ports
and device drivers. The changes tend to be self-contained. Where they
are not, they need to be carefully managed.

> > >> 3. Long-term, I think many agree, we should aim to move much of the
> > >> RISC-V code into UefiCpuPkg and OvmfPkg. Mike mentioned that would
> > >> need coordination with ARM maintainers because it might make sense to
> > >> move their code there as well.

I don't think there is any need to tie the two together.
Yes, UefiCpuPkg should be a generic place where not only x86 support
can be contained, but the paths for ARM* and RISC-V into there do not
have any interdependencies.

> > >>> I *did* have some outstanding comments specifically with regards to
> > >>> large amounts of code duplication between the SMBIOS implementation
> > >>> of some closely related RISC-V platforms. That now needs to be revisited.
> > >>
> > >> The SMBIOS code hasn't changed. It has moved to
> > >>    Silicon/SiFive/{E51,U54,U54MCCoreplex}/Library/PeiCoreInfoHobLib
> > >> You're referring to this library, right?
> > >>
> > >> They build the SMBIOS entries for a particular processor. Yes, the
> > >> values do have a lot of overlap but these files are like
> > >> configuration files. They don't do much, they only set the values of the
> > properties.
> > >>
> > >> Currently it is not possible to let the UEFI firmware get this
> > >> information from the hardware at runtime, especially now, since we're
> > >> running in S-Mode.
> > >> To allow that, we created a RISC-V working group to be able to
> > >> retrieve all of this information dynamically from the processor
> > >> (among other goals). Then the vendor will not have to modify these
> > >> files and hardcode the values anymore. Which enables us to create a
> > >> single library for all processors.
> > >> See: https://github.com/riscv/configuration-structure
> > >>
> > >> I hope I described everything properly, please correct me otherwise,
> > >> Abner.
> [Abner]  Yes almost. Thanks Daniel.
> One thing I would like to add,
> If you take a look on SiFive Core IP
> > >> https://www.sifive.com/risc-v-core-ip you can see there are
> > >> different SKUs of RISC-V core. Just take some as exampl,e
> S51 - Single core
> U54 - Single core
> S76 - Single core
> U74- single core
> U54-MC - Multicore which is 4*U54 cores +1*S51 core
> U74-MC - Multicore which is 4*U74 core + 1*S7 core
> 
> Those are the combinations of core IP. Silicon vendor can get those
> core IPs and combine them to the RISC-V processor. To have
> CoreInfoHobLib libraries for each different core (not multicore) to
> build up the core capability is reasonable and makes sense. For the
> multicore, it just pulling the single core CoreInfoHobLib to build
> up the SMBIOS table for the multicore processor. Those libraries
> look duplicate in logically, however only one instance of
> CoreInfoHobLib is built in for multiple identical cores in
> physically view. Maybe we still can move some identical core into
> the core-specific library but it is not worthwhile.

OK, lets start with the *full* diff of E51 and U54 from the
(admittedly slightly dated) devel-riscvplatforms branch:

<<<
--- ./E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c	2020-05-28 12:12:11.211028141 +0100
+++ ./U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c	2020-05-28 12:12:11.211028141 +0100
@@ -7,6 +7,8 @@
 
 **/
 
+#include <IndustryStandard/RiscVOpensbi.h>
+

[LL]
U54 inserts this file in a different location, this is not actual divergence.

//
 // The package level header files this module uses
 //
@@ -19,21 +21,15 @@
 #include <Library/DebugLib.h>
 #include <Library/FirmwareContextProcessorSpecificLib.h>
 #include <Library/HobLib.h>
-#include <Library/PcdLib.h>

[LL]
Interestingly, only E51 actually includes this, but both files *use*
it - this is a bug in U54 caused by the separation.

-#include <IndustryStandard/RiscVOpensbi.h>

[LL]
This is the other end of the files placing this inlude in a different location.

-#include <Library/ResourcePublicationLib.h>
-

[LL]
This header is irrelevant and unused. Present only in E51.

 #include <Library/RiscVEdk2SbiLib.h>
-#include <ProcessorSpecificHobData.h>
-#include <RiscVImpl.h>

[LL]
Included in different location for E51/U54.

 #include <sbi/sbi_hart.h>
-#include <sbi/sbi_scratch.h>
 #include <sbi/sbi_platform.h>
+#include <sbi/sbi_scratch.h>

[LL]
sbi_scratch.h included in different order between platforms.

+#include <RiscVImpl.h>

[LL]
Included in different location for E51/U54 (other end of).

#include <SmbiosProcessorSpecificData.h>
 
 /**
-  Function to build core specific information HOB. RISC-V SMBIOS DXE driver collect
-  this information and build SMBIOS Type44.
+  Function to build core specific information HOB.

[LL]
Different documentation description for the otherwise identical functions.


   @param  ParentProcessorGuid    Parent processor od this core. ParentProcessorGuid
                                  could be the same as CoreGuid if one processor has
@@ -41,19 +37,19 @@
   @param  ParentProcessorUid     Unique ID of pysical processor which owns this core.
   @param  HartId                 Hart ID of this core.
   @param  IsBootHart             TRUE means this is the boot HART.
-  @param  GuidHobData            Pointer to receive   EFI_HOB_GUID_TYPE.
+  @param  GuidHobdata            Pointer to RISC_V_PROCESSOR_SPECIFIC_HOB_DATA.

[LL]
Different capitalisation of input variable and different documentation
for the same parameter in the identical functions. E51 gets the former
correct, U54 the latter.

 
   @return EFI_SUCCESS     The PEIM initialized successfully.
 
 **/
 EFI_STATUS
 EFIAPI
-CreateE51CoreProcessorSpecificDataHob (
+CreateU54CoreProcessorSpecificDataHob (

[LL]
We reach the first *real* difference between the two - the name of the function.
This could have been addressed with different .inf files with
different -D cflags.

   IN EFI_GUID  *ParentProcessorGuid,
   IN UINTN     ParentProcessorUid,
   IN UINTN     HartId,
   IN BOOLEAN   IsBootHart,
-  OUT RISC_V_PROCESSOR_SPECIFIC_HOB_DATA **GuidHobData
+  OUT RISC_V_PROCESSOR_SPECIFIC_HOB_DATA **GuidHobdata

[LL]
Again, difference only in capitalisation.

   )
 {
   RISC_V_PROCESSOR_SPECIFIC_HOB_DATA *CoreGuidHob;
@@ -64,7 +60,7 @@
 
   DEBUG ((DEBUG_INFO, "%a: Entry.\n", __FUNCTION__));
 
-  if (GuidHobData == NULL) {
+  if (GuidHobdata == NULL) {

[LL]
Again, difference only in capitalisation.

     return EFI_INVALID_PARAMETER;
   }
 
@@ -80,7 +76,7 @@
       FirmwareContextHartSpecific,
       ParentProcessorGuid,
       ParentProcessorUid,
-      (EFI_GUID *)PcdGetPtr (PcdSiFiveE51CoreGuid),
+      (EFI_GUID *)PcdGetPtr (PcdSiFiveU54CoreGuid),

[LL]
Different Pcd names.

       HartId,
       IsBootHart,
       &ProcessorSpecDataHob
@@ -109,7 +105,7 @@
   DEBUG ((DEBUG_INFO, "        *MachineImplId = 0x%x\n", ProcessorSpecDataHob.ProcessorSpecificData.MachineImplId.Value64_L));
 
   //
-  // Build GUID HOB for E51 core, this is for SMBIOS type 44
+  // Build GUID HOB for U54 core.

[LL]
Different comments for identical code.

   //
   ProcessorSpecDataHobGuid = PcdGetPtr (PcdProcessorSpecificDataGuidHobGuid);
   CoreGuidHob = (RISC_V_PROCESSOR_SPECIFIC_HOB_DATA *)BuildGuidDataHob (ProcessorSpecDataHobGuid, (VOID *)&ProcessorSpecDataHob, sizeof (RISC_V_PROCESSOR_SPECIFIC_HOB_DATA));
@@ -117,7 +113,7 @@
     DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core.\n"));
     ASSERT (FALSE);
   }
-  *GuidHobData = CoreGuidHob;
+  *GuidHobdata = CoreGuidHob;

[LL]
Again, difference only in capitalisation.

 return EFI_SUCCESS;
 }
 
@@ -135,17 +131,21 @@
 **/
 EFI_STATUS
 EFIAPI
-CreateE51ProcessorSmbiosDataHob (
+CreateU54ProcessorSmbiosDataHob (

[LL]
Difference in name only.

   IN UINTN     ProcessorUid,
-  OUT RISC_V_PROCESSOR_SMBIOS_HOB_DATA **SmbiosHobPtr
+  IN RISC_V_PROCESSOR_SMBIOS_HOB_DATA **SmbiosHobPtr

[LL]
I am going to go out on a limb here and suggest one of the above is
incorrect, and once that is corrected, these two lines would be identical.

   )
 {
   EFI_GUID *GuidPtr;
   RISC_V_PROCESSOR_TYPE4_HOB_DATA ProcessorDataHob;
   RISC_V_PROCESSOR_TYPE7_HOB_DATA L1InstCacheDataHob;
+  RISC_V_PROCESSOR_TYPE7_HOB_DATA L1DataCacheDataHob;
+  RISC_V_PROCESSOR_TYPE7_HOB_DATA L2CacheDataHob;

[LL]
Here is the first functional difference.

   RISC_V_PROCESSOR_SMBIOS_HOB_DATA SmbiosDataHob;
   RISC_V_PROCESSOR_TYPE4_HOB_DATA *ProcessorDataHobPtr;
   RISC_V_PROCESSOR_TYPE7_HOB_DATA *L1InstCacheDataHobPtr;
+  RISC_V_PROCESSOR_TYPE7_HOB_DATA *L1DataCacheDataHobPtr;
+  RISC_V_PROCESSOR_TYPE7_HOB_DATA *L2CacheDataHobPtr;

[LL]
Which could be merged with this inside an ifdef.

   RISC_V_PROCESSOR_SMBIOS_HOB_DATA *SmbiosDataHobPtr;
 
   if (SmbiosHobPtr == NULL) {
@@ -155,7 +155,7 @@
   // Build up SMBIOS type 7 L1 instruction cache record.
   //
   ZeroMem((VOID *)&L1InstCacheDataHob, sizeof (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
-  CopyGuid (&L1InstCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr (PcdSiFiveE51CoreGuid));
+  CopyGuid (&L1InstCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr (PcdSiFiveU54CoreGuid));

[LL]
Difference in name only.

   L1InstCacheDataHob.ProcessorUid = ProcessorUid;
   L1InstCacheDataHob.SmbiosType7Cache.SocketDesignation = TO_BE_FILLED_BY_VENDOR;
   L1InstCacheDataHob.SmbiosType7Cache.CacheConfiguration = RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_1 | \
@@ -173,7 +173,59 @@
   GuidPtr = (EFI_GUID *)PcdGetPtr (PcdProcessorSmbiosType7GuidHobGuid);
   L1InstCacheDataHobPtr = (RISC_V_PROCESSOR_TYPE7_HOB_DATA *)BuildGuidDataHob (GuidPtr, (VOID *)&L1InstCacheDataHob, sizeof (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
   if (L1InstCacheDataHobPtr == NULL) {
-    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core L1 instruction cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n"));
+    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L1 instruction cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n"));

[LL]
Difference in name only.

+    ASSERT (FALSE);
+  }
+

[LL]
Below starts the fundamental difference between the two:

+  //
+  // Build up SMBIOS type 7 L1 data cache record.
+  //
+  ZeroMem((VOID *)&L1DataCacheDataHob, sizeof (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
+  CopyGuid (&L1DataCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr (PcdSiFiveU54CoreGuid));
+  L1DataCacheDataHob.ProcessorUid = ProcessorUid;
+  L1DataCacheDataHob.SmbiosType7Cache.SocketDesignation = TO_BE_FILLED_BY_VENDOR;
+  L1DataCacheDataHob.SmbiosType7Cache.CacheConfiguration = RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_1 | \
+      RISC_V_CACHE_CONFIGURATION_LOCATION_INTERNAL | \
+      RISC_V_CACHE_CONFIGURATION_ENABLED | \
+      RISC_V_CACHE_CONFIGURATION_MODE_UNKNOWN;
+  L1DataCacheDataHob.SmbiosType7Cache.MaximumCacheSize = TO_BE_FILLED_BY_VENDOR;
+  L1DataCacheDataHob.SmbiosType7Cache.InstalledSize = TO_BE_FILLED_BY_VENDOR;
+  L1DataCacheDataHob.SmbiosType7Cache.SupportedSRAMType.Unknown = 1;
+  L1DataCacheDataHob.SmbiosType7Cache.CurrentSRAMType.Unknown = 1;
+  L1DataCacheDataHob.SmbiosType7Cache.CacheSpeed = TO_BE_FILLED_BY_VENDOR;
+  L1DataCacheDataHob.SmbiosType7Cache.ErrorCorrectionType = TO_BE_FILLED_BY_VENDOR;
+  L1DataCacheDataHob.SmbiosType7Cache.SystemCacheType = CacheTypeData;
+  L1DataCacheDataHob.SmbiosType7Cache.Associativity = TO_BE_FILLED_BY_VENDOR;
+  GuidPtr = (EFI_GUID *)PcdGetPtr (PcdProcessorSmbiosType7GuidHobGuid);
+  L1DataCacheDataHobPtr = (RISC_V_PROCESSOR_TYPE7_HOB_DATA *)BuildGuidDataHob (GuidPtr, (VOID *)&L1DataCacheDataHob, sizeof (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
+  if (L1DataCacheDataHobPtr == NULL) {
+    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L1 data cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n"));
+    ASSERT (FALSE);
+  }
+
+  //
+  // Build up SMBIOS type 7 L2 cache record.
+  //
+  ZeroMem((VOID *)&L2CacheDataHob, sizeof (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
+  CopyGuid (&L2CacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr (PcdSiFiveU54CoreGuid));
+  L2CacheDataHob.ProcessorUid = ProcessorUid;
+  L2CacheDataHob.SmbiosType7Cache.SocketDesignation = TO_BE_FILLED_BY_VENDOR;
+  L2CacheDataHob.SmbiosType7Cache.CacheConfiguration = RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_2 | \
+      RISC_V_CACHE_CONFIGURATION_LOCATION_EXTERNAL | \
+      RISC_V_CACHE_CONFIGURATION_ENABLED | \
+      RISC_V_CACHE_CONFIGURATION_MODE_UNKNOWN;
+  L2CacheDataHob.SmbiosType7Cache.MaximumCacheSize = TO_BE_FILLED_BY_VENDOR;
+  L2CacheDataHob.SmbiosType7Cache.InstalledSize = TO_BE_FILLED_BY_VENDOR;
+  L2CacheDataHob.SmbiosType7Cache.SupportedSRAMType.Unknown = 1;
+  L2CacheDataHob.SmbiosType7Cache.CurrentSRAMType.Unknown = 1;
+  L2CacheDataHob.SmbiosType7Cache.CacheSpeed = TO_BE_FILLED_BY_VENDOR;
+  L2CacheDataHob.SmbiosType7Cache.ErrorCorrectionType = TO_BE_FILLED_BY_VENDOR;
+  L2CacheDataHob.SmbiosType7Cache.SystemCacheType = CacheTypeUnified;
+  L2CacheDataHob.SmbiosType7Cache.Associativity = TO_BE_FILLED_BY_VENDOR;
+  GuidPtr = (EFI_GUID *)PcdGetPtr (PcdProcessorSmbiosType7GuidHobGuid);
+  L2CacheDataHobPtr = (RISC_V_PROCESSOR_TYPE7_HOB_DATA *)BuildGuidDataHob (GuidPtr, (VOID *)&L2CacheDataHob, sizeof (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
+  if (L2CacheDataHobPtr == NULL) {
+    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L2 cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n"));
     ASSERT (FALSE);
   }
 

[LL]
And the funamental difference ends here.

@@ -181,7 +233,7 @@
   // Build up SMBIOS type 4 record.
   //
   ZeroMem((VOID *)&ProcessorDataHob, sizeof (RISC_V_PROCESSOR_TYPE4_HOB_DATA));
-  CopyGuid (&ProcessorDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr (PcdSiFiveE51CoreGuid));
+  CopyGuid (&ProcessorDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr (PcdSiFiveU54CoreGuid));

[LL]
Differ in name only.

   ProcessorDataHob.ProcessorUid = ProcessorUid;
   ProcessorDataHob.SmbiosType4Processor.Socket = TO_BE_FILLED_BY_VENDOR;
   ProcessorDataHob.SmbiosType4Processor.ProcessorType = CentralProcessor;
@@ -196,7 +248,7 @@
   ProcessorDataHob.SmbiosType4Processor.Status = TO_BE_FILLED_BY_CODE;
   ProcessorDataHob.SmbiosType4Processor.ProcessorUpgrade = TO_BE_FILLED_BY_VENDOR;
   ProcessorDataHob.SmbiosType4Processor.L1CacheHandle = TO_BE_FILLED_BY_RISC_V_SMBIOS_DXE_DRIVER;
-  ProcessorDataHob.SmbiosType4Processor.L2CacheHandle = 0xffff;
+  ProcessorDataHob.SmbiosType4Processor.L2CacheHandle = TO_BE_FILLED_BY_RISC_V_SMBIOS_DXE_DRIVER;

[LL]
Real diff.

   ProcessorDataHob.SmbiosType4Processor.L3CacheHandle = 0xffff;
   ProcessorDataHob.SmbiosType4Processor.SerialNumber = TO_BE_FILLED_BY_CODE;
   ProcessorDataHob.SmbiosType4Processor.AssetTag = TO_BE_FILLED_BY_VENDOR;
@@ -212,24 +264,23 @@
   GuidPtr = (EFI_GUID *)PcdGetPtr (PcdProcessorSmbiosType4GuidHobGuid);
   ProcessorDataHobPtr = (RISC_V_PROCESSOR_TYPE4_HOB_DATA *)BuildGuidDataHob (GuidPtr, (VOID *)&ProcessorDataHob, sizeof (RISC_V_PROCESSOR_TYPE4_HOB_DATA));
   if (ProcessorDataHobPtr == NULL) {
-    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core RISC_V_PROCESSOR_TYPE4_HOB_DATA.\n"));
+    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core RISC_V_PROCESSOR_TYPE4_HOB_DATA.\n"));

[LL]
Difference in name only.

     ASSERT (FALSE);
   }
 
   ZeroMem((VOID *)&SmbiosDataHob, sizeof (RISC_V_PROCESSOR_SMBIOS_HOB_DATA));
   SmbiosDataHob.Processor = ProcessorDataHobPtr;
   SmbiosDataHob.L1InstCache = L1InstCacheDataHobPtr;
-  SmbiosDataHob.L1DataCache = NULL;
-  SmbiosDataHob.L2Cache = NULL;
+  SmbiosDataHob.L1DataCache = L1DataCacheDataHobPtr;
+  SmbiosDataHob.L2Cache = L2CacheDataHobPtr;

[LL]
Real diff.

   SmbiosDataHob.L3Cache = NULL;
   GuidPtr = (EFI_GUID *)PcdGetPtr (PcdProcessorSmbiosGuidHobGuid);
   SmbiosDataHobPtr = (RISC_V_PROCESSOR_SMBIOS_HOB_DATA *)BuildGuidDataHob (GuidPtr, (VOID *)&SmbiosDataHob, sizeof (RISC_V_PROCESSOR_SMBIOS_HOB_DATA));
   if (SmbiosDataHobPtr == NULL) {
-    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core RISC_V_PROCESSOR_SMBIOS_HOB_DATA.\n"));
+    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core RISC_V_PROCESSOR_SMBIOS_HOB_DATA.\n"));

[LL]
Difference in name only.

     ASSERT (FALSE);
   }
   *SmbiosHobPtr = SmbiosDataHobPtr;
   return EFI_SUCCESS;
 }
 
-

>>>

The meat of the difference between these two is less than 20% of the
lines of code in each file - and it is mutually exclusive, not some
horrific tangle of interdependencies.

The story with the difference between U54 and U54MCCoreplex isn't much
better, only works along a different axis.

"It isn't worthwhile" in an open source project isn't a question of
"how quickly can I create *one* new platform by copying instead of
refactoring/reusing", but a judgement call between:
- How many mistakes do I risk inserting while editing a new file as
  opposed to being directly able to see the differences I have caused
  while editing an existing file.
- How much do I increase reviewing effort by doing this?
- How much do I increase ongoing maintainership (or affect quality) by
  requiring bugs to be fixed in multiple places instead of one.

Not to mention:
- How many common pattern that could be broken out into common helper
  libraries do we miss when we need to compare every SoC/platform
  combination ever created, as opposed to being able to look at least
  at implementations covering families.

If it isn't important enough to take that into consideration, it
isn't important enough to upstream the SMBIOS support.

/
    Leif

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

View/Reply Online (#60400): https://edk2.groups.io/g/devel/message/60400
Mute This Topic: https://groups.io/mt/74353494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
Posted by Abner Chang 3 years, 10 months ago

> -----Original Message-----
> From: Leif Lindholm [mailto:leif@nuviainc.com]
> Sent: Thursday, May 28, 2020 7:55 PM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>
> Cc: Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com>;
> devel@edk2.groups.io; Chen, Gilbert <gilbert.chen@hpe.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Bret.Barkelew@microsoft.com;
> sean.brogan@microsoft.com
> Subject: Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-
> platforms
> 
> Hi Abner,
> 
> Sorry, I should have followed up on this sooner.
> 
> On Thu, May 21, 2020 at 06:59:20 +0000, Chang, Abner (HPS SW/FW
> Technologist) wrote:
> > > On 5/20/20 6:07 PM, Daniel Schaefer wrote:
> > > > please reply here, fixed Mike's email address, sorry...
> > > >
> > > > On 5/20/20 6:03 PM, Daniel Schaefer wrote:
> > > >> On 5/20/20 1:43 PM, Leif Lindholm wrote:
> > > >>> On Fri, May 15, 2020 at 15:39:34 +0200, Daniel Schaefer wrote:
> > > >>>> Previously we had two packages just for RISC-V on our edk2 branch:
> > > >>>>    RiscVPkg and RiscVPlatformPkg They are now under
> > > >>>>    Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorPkg
> > > >>>> in edk2-platforms.
> > > >>>
> > > >>> Understood. I took my eye off the ball there for a while, but
> > > >>> I'm a bit confused as to why RiscVPkg isn't going into EDK2.
> > > >>> That is very counterintuitive. And clearly it will need
> > > >>> revisiting if we are to add first-class CI checks like those we do with
> OvmfPkg/ArmVirtPkg.
> > > >>
> > > >> Yes, I understand your concern. Personally I'd like it also to be
> > > >> in
> > > >> EDK2 straight away, however Mike, Bret and Sean have raised valid
> > > >> concerns:
> 
> Can you point me to the conversation I have missed?
> 
> > > >> 1. RISC-V is very new and potentially unstable - it's quicker to
> > > >> make changes in edk2-platforms.
> 
> I don't see this as a valid argument.
> It's not edk2-unstable, it is edk2-platforms.
> 
> edk2-platforms exists because there used to be strong feelings against
> holding *real* platforms in edk2, with edk2 being originally intended only as
> a code library for IBV/ISV to cherry-pick from.
> 
> But fundamentally, if code is too immature to go into the master branch of
> edk2, it is too immature to go into the master branch of edk2-platforms. If
> we want edk2-might-be-a-bit-shaky-but-who-cares,
> then someone will have to create it.
> 
> > > >> 2. If we define new interfaces and libraries in edk2, we can't
> > > >> remove them easily because it would be a backwards-incompatible
> change.
> > > >> edk2-platforms isn't quite as strict.
> 
> Yes it is.
> The only thing making it less strict is its contents - platform ports and device
> drivers. The changes tend to be self-contained. Where they are not, they
> need to be carefully managed.
> 
> > > >> 3. Long-term, I think many agree, we should aim to move much of
> > > >> the RISC-V code into UefiCpuPkg and OvmfPkg. Mike mentioned that
> > > >> would need coordination with ARM maintainers because it might
> > > >> make sense to move their code there as well.
> 
> I don't think there is any need to tie the two together.
> Yes, UefiCpuPkg should be a generic place where not only x86 support can be
> contained, but the paths for ARM* and RISC-V into there do not have any
> interdependencies.
> 
> > > >>> I *did* have some outstanding comments specifically with regards
> > > >>> to large amounts of code duplication between the SMBIOS
> > > >>> implementation of some closely related RISC-V platforms. That now
> needs to be revisited.
> > > >>
> > > >> The SMBIOS code hasn't changed. It has moved to
> > > >>
> > > >> Silicon/SiFive/{E51,U54,U54MCCoreplex}/Library/PeiCoreInfoHobLib
> > > >> You're referring to this library, right?
> > > >>
> > > >> They build the SMBIOS entries for a particular processor. Yes,
> > > >> the values do have a lot of overlap but these files are like
> > > >> configuration files. They don't do much, they only set the values
> > > >> of the
> > > properties.
> > > >>
> > > >> Currently it is not possible to let the UEFI firmware get this
> > > >> information from the hardware at runtime, especially now, since
> > > >> we're running in S-Mode.
> > > >> To allow that, we created a RISC-V working group to be able to
> > > >> retrieve all of this information dynamically from the processor
> > > >> (among other goals). Then the vendor will not have to modify
> > > >> these files and hardcode the values anymore. Which enables us to
> > > >> create a single library for all processors.
> > > >> See: https://github.com/riscv/configuration-structure
> > > >>
> > > >> I hope I described everything properly, please correct me
> > > >> otherwise, Abner.
> > [Abner]  Yes almost. Thanks Daniel.
> > One thing I would like to add,
> > If you take a look on SiFive Core IP
> > > >> INVALID URI REMOVED
> > > >> om_risc-2Dv-2Dcore-
> 2Dip&d=DwIDAw&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6F
> > > >>
> ZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=CYgLjPyxDUeKYpXV_283
> 3iYF
> > > >>
> 0_kR2PGdGoEVD7Qare0&s=OQvDbn7xhejlUpQrBMNReCLl1WGUoPKdIp5Y3
> 6-6e8E
> > > >> &e=  you can see there are different SKUs of RISC-V core. Just
> > > >> take some as exampl,e
> > S51 - Single core
> > U54 - Single core
> > S76 - Single core
> > U74- single core
> > U54-MC - Multicore which is 4*U54 cores +1*S51 core U74-MC - Multicore
> > which is 4*U74 core + 1*S7 core
> >
> > Those are the combinations of core IP. Silicon vendor can get those
> > core IPs and combine them to the RISC-V processor. To have
> > CoreInfoHobLib libraries for each different core (not multicore) to
> > build up the core capability is reasonable and makes sense. For the
> > multicore, it just pulling the single core CoreInfoHobLib to build up
> > the SMBIOS table for the multicore processor. Those libraries look
> > duplicate in logically, however only one instance of CoreInfoHobLib is
> > built in for multiple identical cores in physically view. Maybe we
> > still can move some identical core into the core-specific library but
> > it is not worthwhile.
> 
> OK, lets start with the *full* diff of E51 and U54 from the (admittedly slightly
> dated) devel-riscvplatforms branch:
> 
> <<<
> --- ./E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c	2020-05-28
> 12:12:11.211028141 +0100
> +++ ./U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c	2020-05-28
> 12:12:11.211028141 +0100
> @@ -7,6 +7,8 @@
> 
>  **/
> 
> +#include <IndustryStandard/RiscVOpensbi.h>
> +
> 
> [LL]
> U54 inserts this file in a different location, this is not actual divergence.
> 
> //
>  // The package level header files this module uses  // @@ -19,21 +21,15 @@
> #include <Library/DebugLib.h>  #include
> <Library/FirmwareContextProcessorSpecificLib.h>
>  #include <Library/HobLib.h>
> -#include <Library/PcdLib.h>
> 
> [LL]
> Interestingly, only E51 actually includes this, but both files *use* it - this is a
> bug in U54 caused by the separation.
> 
> -#include <IndustryStandard/RiscVOpensbi.h>
> 
> [LL]
> This is the other end of the files placing this inlude in a different location.
> 
> -#include <Library/ResourcePublicationLib.h>
> -
> 
> [LL]
> This header is irrelevant and unused. Present only in E51.
> 
>  #include <Library/RiscVEdk2SbiLib.h>
> -#include <ProcessorSpecificHobData.h>
> -#include <RiscVImpl.h>
> 
> [LL]
> Included in different location for E51/U54.
> 
>  #include <sbi/sbi_hart.h>
> -#include <sbi/sbi_scratch.h>
>  #include <sbi/sbi_platform.h>
> +#include <sbi/sbi_scratch.h>
> 
> [LL]
> sbi_scratch.h included in different order between platforms.
> 
> +#include <RiscVImpl.h>
> 
> [LL]
> Included in different location for E51/U54 (other end of).
> 
> #include <SmbiosProcessorSpecificData.h>
> 
>  /**
> -  Function to build core specific information HOB. RISC-V SMBIOS DXE driver
> collect
> -  this information and build SMBIOS Type44.
> +  Function to build core specific information HOB.
> 
> [LL]
> Different documentation description for the otherwise identical functions.
> 
> 
>    @param  ParentProcessorGuid    Parent processor od this core.
> ParentProcessorGuid
>                                   could be the same as CoreGuid if one processor has @@ -
> 41,19 +37,19 @@
>    @param  ParentProcessorUid     Unique ID of pysical processor which owns
> this core.
>    @param  HartId                 Hart ID of this core.
>    @param  IsBootHart             TRUE means this is the boot HART.
> -  @param  GuidHobData            Pointer to receive   EFI_HOB_GUID_TYPE.
> +  @param  GuidHobdata            Pointer to
> RISC_V_PROCESSOR_SPECIFIC_HOB_DATA.
> 
> [LL]
> Different capitalisation of input variable and different documentation for the
> same parameter in the identical functions. E51 gets the former correct, U54
> the latter.
> 
> 
>    @return EFI_SUCCESS     The PEIM initialized successfully.
> 
>  **/
>  EFI_STATUS
>  EFIAPI
> -CreateE51CoreProcessorSpecificDataHob (
> +CreateU54CoreProcessorSpecificDataHob (
> 
> [LL]
> We reach the first *real* difference between the two - the name of the
> function.
> This could have been addressed with different .inf files with different -D
> cflags.
> 
>    IN EFI_GUID  *ParentProcessorGuid,
>    IN UINTN     ParentProcessorUid,
>    IN UINTN     HartId,
>    IN BOOLEAN   IsBootHart,
> -  OUT RISC_V_PROCESSOR_SPECIFIC_HOB_DATA **GuidHobData
> +  OUT RISC_V_PROCESSOR_SPECIFIC_HOB_DATA **GuidHobdata
> 
> [LL]
> Again, difference only in capitalisation.
> 
>    )
>  {
>    RISC_V_PROCESSOR_SPECIFIC_HOB_DATA *CoreGuidHob; @@ -64,7 +60,7
> @@
> 
>    DEBUG ((DEBUG_INFO, "%a: Entry.\n", __FUNCTION__));
> 
> -  if (GuidHobData == NULL) {
> +  if (GuidHobdata == NULL) {
> 
> [LL]
> Again, difference only in capitalisation.
> 
>      return EFI_INVALID_PARAMETER;
>    }
> 
> @@ -80,7 +76,7 @@
>        FirmwareContextHartSpecific,
>        ParentProcessorGuid,
>        ParentProcessorUid,
> -      (EFI_GUID *)PcdGetPtr (PcdSiFiveE51CoreGuid),
> +      (EFI_GUID *)PcdGetPtr (PcdSiFiveU54CoreGuid),
> 
> [LL]
> Different Pcd names.
> 
>        HartId,
>        IsBootHart,
>        &ProcessorSpecDataHob
> @@ -109,7 +105,7 @@
>    DEBUG ((DEBUG_INFO, "        *MachineImplId = 0x%x\n",
> ProcessorSpecDataHob.ProcessorSpecificData.MachineImplId.Value64_L));
> 
>    //
> -  // Build GUID HOB for E51 core, this is for SMBIOS type 44
> +  // Build GUID HOB for U54 core.
> 
> [LL]
> Different comments for identical code.
> 
>    //
>    ProcessorSpecDataHobGuid = PcdGetPtr
> (PcdProcessorSpecificDataGuidHobGuid);
>    CoreGuidHob = (RISC_V_PROCESSOR_SPECIFIC_HOB_DATA
> *)BuildGuidDataHob (ProcessorSpecDataHobGuid, (VOID
> *)&ProcessorSpecDataHob, sizeof
> (RISC_V_PROCESSOR_SPECIFIC_HOB_DATA));
> @@ -117,7 +113,7 @@
>      DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core.\n"));
>      ASSERT (FALSE);
>    }
> -  *GuidHobData = CoreGuidHob;
> +  *GuidHobdata = CoreGuidHob;
> 
> [LL]
> Again, difference only in capitalisation.
> 
>  return EFI_SUCCESS;
>  }
> 
> @@ -135,17 +131,21 @@
>  **/
>  EFI_STATUS
>  EFIAPI
> -CreateE51ProcessorSmbiosDataHob (
> +CreateU54ProcessorSmbiosDataHob (
> 
> [LL]
> Difference in name only.
> 
>    IN UINTN     ProcessorUid,
> -  OUT RISC_V_PROCESSOR_SMBIOS_HOB_DATA **SmbiosHobPtr
> +  IN RISC_V_PROCESSOR_SMBIOS_HOB_DATA **SmbiosHobPtr
> 
> [LL]
> I am going to go out on a limb here and suggest one of the above is incorrect,
> and once that is corrected, these two lines would be identical.
> 
>    )
>  {
>    EFI_GUID *GuidPtr;
>    RISC_V_PROCESSOR_TYPE4_HOB_DATA ProcessorDataHob;
>    RISC_V_PROCESSOR_TYPE7_HOB_DATA L1InstCacheDataHob;
> +  RISC_V_PROCESSOR_TYPE7_HOB_DATA L1DataCacheDataHob;
> + RISC_V_PROCESSOR_TYPE7_HOB_DATA L2CacheDataHob;
> 
> [LL]
> Here is the first functional difference.
> 
>    RISC_V_PROCESSOR_SMBIOS_HOB_DATA SmbiosDataHob;
>    RISC_V_PROCESSOR_TYPE4_HOB_DATA *ProcessorDataHobPtr;
>    RISC_V_PROCESSOR_TYPE7_HOB_DATA *L1InstCacheDataHobPtr;
> +  RISC_V_PROCESSOR_TYPE7_HOB_DATA *L1DataCacheDataHobPtr;
> + RISC_V_PROCESSOR_TYPE7_HOB_DATA *L2CacheDataHobPtr;
> 
> [LL]
> Which could be merged with this inside an ifdef.
> 
>    RISC_V_PROCESSOR_SMBIOS_HOB_DATA *SmbiosDataHobPtr;
> 
>    if (SmbiosHobPtr == NULL) {
> @@ -155,7 +155,7 @@
>    // Build up SMBIOS type 7 L1 instruction cache record.
>    //
>    ZeroMem((VOID *)&L1InstCacheDataHob, sizeof
> (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
> -  CopyGuid (&L1InstCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr
> (PcdSiFiveE51CoreGuid));
> +  CopyGuid (&L1InstCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr
> + (PcdSiFiveU54CoreGuid));
> 
> [LL]
> Difference in name only.
> 
>    L1InstCacheDataHob.ProcessorUid = ProcessorUid;
>    L1InstCacheDataHob.SmbiosType7Cache.SocketDesignation =
> TO_BE_FILLED_BY_VENDOR;
>    L1InstCacheDataHob.SmbiosType7Cache.CacheConfiguration =
> RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_1 | \ @@ -173,7 +173,59
> @@
>    GuidPtr = (EFI_GUID *)PcdGetPtr
> (PcdProcessorSmbiosType7GuidHobGuid);
>    L1InstCacheDataHobPtr = (RISC_V_PROCESSOR_TYPE7_HOB_DATA
> *)BuildGuidDataHob (GuidPtr, (VOID *)&L1InstCacheDataHob, sizeof
> (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
>    if (L1InstCacheDataHobPtr == NULL) {
> -    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core L1
> instruction cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n"));
> +    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L1
> + instruction cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n"));
> 
> [LL]
> Difference in name only.
> 
> +    ASSERT (FALSE);
> +  }
> +
> 
> [LL]
> Below starts the fundamental difference between the two:
> 
> +  //
> +  // Build up SMBIOS type 7 L1 data cache record.
> +  //
> +  ZeroMem((VOID *)&L1DataCacheDataHob, sizeof
> + (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
> +  CopyGuid (&L1DataCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr
> + (PcdSiFiveU54CoreGuid));  L1DataCacheDataHob.ProcessorUid =
> + ProcessorUid;
> L1DataCacheDataHob.SmbiosType7Cache.SocketDesignation =
> + TO_BE_FILLED_BY_VENDOR;
> L1DataCacheDataHob.SmbiosType7Cache.CacheConfiguration =
> RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_1 | \
> +      RISC_V_CACHE_CONFIGURATION_LOCATION_INTERNAL | \
> +      RISC_V_CACHE_CONFIGURATION_ENABLED | \
> +      RISC_V_CACHE_CONFIGURATION_MODE_UNKNOWN;
> +  L1DataCacheDataHob.SmbiosType7Cache.MaximumCacheSize =
> + TO_BE_FILLED_BY_VENDOR;
> + L1DataCacheDataHob.SmbiosType7Cache.InstalledSize =
> + TO_BE_FILLED_BY_VENDOR;
> + L1DataCacheDataHob.SmbiosType7Cache.SupportedSRAMType.Unknown
> = 1;
> + L1DataCacheDataHob.SmbiosType7Cache.CurrentSRAMType.Unknown = 1;
> + L1DataCacheDataHob.SmbiosType7Cache.CacheSpeed =
> + TO_BE_FILLED_BY_VENDOR;
> + L1DataCacheDataHob.SmbiosType7Cache.ErrorCorrectionType =
> + TO_BE_FILLED_BY_VENDOR;
> + L1DataCacheDataHob.SmbiosType7Cache.SystemCacheType =
> CacheTypeData;
> + L1DataCacheDataHob.SmbiosType7Cache.Associativity =
> + TO_BE_FILLED_BY_VENDOR;  GuidPtr = (EFI_GUID *)PcdGetPtr
> + (PcdProcessorSmbiosType7GuidHobGuid);
> +  L1DataCacheDataHobPtr = (RISC_V_PROCESSOR_TYPE7_HOB_DATA
> + *)BuildGuidDataHob (GuidPtr, (VOID *)&L1DataCacheDataHob, sizeof
> + (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
> +  if (L1DataCacheDataHobPtr == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L1
> data cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n"));
> +    ASSERT (FALSE);
> +  }
> +
> +  //
> +  // Build up SMBIOS type 7 L2 cache record.
> +  //
> +  ZeroMem((VOID *)&L2CacheDataHob, sizeof
> + (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
> +  CopyGuid (&L2CacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr
> + (PcdSiFiveU54CoreGuid));  L2CacheDataHob.ProcessorUid = ProcessorUid;
> + L2CacheDataHob.SmbiosType7Cache.SocketDesignation =
> + TO_BE_FILLED_BY_VENDOR;
> L2CacheDataHob.SmbiosType7Cache.CacheConfiguration =
> RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_2 | \
> +      RISC_V_CACHE_CONFIGURATION_LOCATION_EXTERNAL | \
> +      RISC_V_CACHE_CONFIGURATION_ENABLED | \
> +      RISC_V_CACHE_CONFIGURATION_MODE_UNKNOWN;
> +  L2CacheDataHob.SmbiosType7Cache.MaximumCacheSize =
> + TO_BE_FILLED_BY_VENDOR;
> L2CacheDataHob.SmbiosType7Cache.InstalledSize
> + = TO_BE_FILLED_BY_VENDOR;
> + L2CacheDataHob.SmbiosType7Cache.SupportedSRAMType.Unknown = 1;
> + L2CacheDataHob.SmbiosType7Cache.CurrentSRAMType.Unknown = 1;
> + L2CacheDataHob.SmbiosType7Cache.CacheSpeed =
> TO_BE_FILLED_BY_VENDOR;
> + L2CacheDataHob.SmbiosType7Cache.ErrorCorrectionType =
> + TO_BE_FILLED_BY_VENDOR;
> + L2CacheDataHob.SmbiosType7Cache.SystemCacheType =
> CacheTypeUnified;
> + L2CacheDataHob.SmbiosType7Cache.Associativity =
> + TO_BE_FILLED_BY_VENDOR;  GuidPtr = (EFI_GUID *)PcdGetPtr
> + (PcdProcessorSmbiosType7GuidHobGuid);
> +  L2CacheDataHobPtr = (RISC_V_PROCESSOR_TYPE7_HOB_DATA
> + *)BuildGuidDataHob (GuidPtr, (VOID *)&L2CacheDataHob, sizeof
> + (RISC_V_PROCESSOR_TYPE7_HOB_DATA));
> +  if (L2CacheDataHobPtr == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L2
> + cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n"));
>      ASSERT (FALSE);
>    }
> 
> 
> [LL]
> And the funamental difference ends here.
> 
> @@ -181,7 +233,7 @@
>    // Build up SMBIOS type 4 record.
>    //
>    ZeroMem((VOID *)&ProcessorDataHob, sizeof
> (RISC_V_PROCESSOR_TYPE4_HOB_DATA));
> -  CopyGuid (&ProcessorDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr
> (PcdSiFiveE51CoreGuid));
> +  CopyGuid (&ProcessorDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr
> + (PcdSiFiveU54CoreGuid));
> 
> [LL]
> Differ in name only.
> 
>    ProcessorDataHob.ProcessorUid = ProcessorUid;
>    ProcessorDataHob.SmbiosType4Processor.Socket =
> TO_BE_FILLED_BY_VENDOR;
>    ProcessorDataHob.SmbiosType4Processor.ProcessorType =
> CentralProcessor; @@ -196,7 +248,7 @@
>    ProcessorDataHob.SmbiosType4Processor.Status =
> TO_BE_FILLED_BY_CODE;
>    ProcessorDataHob.SmbiosType4Processor.ProcessorUpgrade =
> TO_BE_FILLED_BY_VENDOR;
>    ProcessorDataHob.SmbiosType4Processor.L1CacheHandle =
> TO_BE_FILLED_BY_RISC_V_SMBIOS_DXE_DRIVER;
> -  ProcessorDataHob.SmbiosType4Processor.L2CacheHandle = 0xffff;
> +  ProcessorDataHob.SmbiosType4Processor.L2CacheHandle =
> + TO_BE_FILLED_BY_RISC_V_SMBIOS_DXE_DRIVER;
> 
> [LL]
> Real diff.
> 
>    ProcessorDataHob.SmbiosType4Processor.L3CacheHandle = 0xffff;
>    ProcessorDataHob.SmbiosType4Processor.SerialNumber =
> TO_BE_FILLED_BY_CODE;
>    ProcessorDataHob.SmbiosType4Processor.AssetTag =
> TO_BE_FILLED_BY_VENDOR; @@ -212,24 +264,23 @@
>    GuidPtr = (EFI_GUID *)PcdGetPtr
> (PcdProcessorSmbiosType4GuidHobGuid);
>    ProcessorDataHobPtr = (RISC_V_PROCESSOR_TYPE4_HOB_DATA
> *)BuildGuidDataHob (GuidPtr, (VOID *)&ProcessorDataHob, sizeof
> (RISC_V_PROCESSOR_TYPE4_HOB_DATA));
>    if (ProcessorDataHobPtr == NULL) {
> -    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core
> RISC_V_PROCESSOR_TYPE4_HOB_DATA.\n"));
> +    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core
> + RISC_V_PROCESSOR_TYPE4_HOB_DATA.\n"));
> 
> [LL]
> Difference in name only.
> 
>      ASSERT (FALSE);
>    }
> 
>    ZeroMem((VOID *)&SmbiosDataHob, sizeof
> (RISC_V_PROCESSOR_SMBIOS_HOB_DATA));
>    SmbiosDataHob.Processor = ProcessorDataHobPtr;
>    SmbiosDataHob.L1InstCache = L1InstCacheDataHobPtr;
> -  SmbiosDataHob.L1DataCache = NULL;
> -  SmbiosDataHob.L2Cache = NULL;
> +  SmbiosDataHob.L1DataCache = L1DataCacheDataHobPtr;
> + SmbiosDataHob.L2Cache = L2CacheDataHobPtr;
> 
> [LL]
> Real diff.
> 
>    SmbiosDataHob.L3Cache = NULL;
>    GuidPtr = (EFI_GUID *)PcdGetPtr (PcdProcessorSmbiosGuidHobGuid);
>    SmbiosDataHobPtr = (RISC_V_PROCESSOR_SMBIOS_HOB_DATA
> *)BuildGuidDataHob (GuidPtr, (VOID *)&SmbiosDataHob, sizeof
> (RISC_V_PROCESSOR_SMBIOS_HOB_DATA));
>    if (SmbiosDataHobPtr == NULL) {
> -    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core
> RISC_V_PROCESSOR_SMBIOS_HOB_DATA.\n"));
> +    DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core
> + RISC_V_PROCESSOR_SMBIOS_HOB_DATA.\n"));
> 
> [LL]
> Difference in name only.
> 
>      ASSERT (FALSE);
>    }
>    *SmbiosHobPtr = SmbiosDataHobPtr;
>    return EFI_SUCCESS;
>  }
> 
> -
> 
> >>>
> 
> The meat of the difference between these two is less than 20% of the lines
> of code in each file - and it is mutually exclusive, not some horrific tangle of
> interdependencies.
> 
> The story with the difference between U54 and U54MCCoreplex isn't much
> better, only works along a different axis.
> 
> "It isn't worthwhile" in an open source project isn't a question of "how
> quickly can I create *one* new platform by copying instead of
> refactoring/reusing", but a judgement call between:
> - How many mistakes do I risk inserting while editing a new file as
>   opposed to being directly able to see the differences I have caused
>   while editing an existing file.
> - How much do I increase reviewing effort by doing this?
> - How much do I increase ongoing maintainership (or affect quality) by
>   requiring bugs to be fixed in multiple places instead of one.
> 
> Not to mention:
> - How many common pattern that could be broken out into common helper
>   libraries do we miss when we need to compare every SoC/platform
>   combination ever created, as opposed to being able to look at least
>   at implementations covering families.
> 
> If it isn't important enough to take that into consideration, it isn't important
> enough to upstream the SMBIOS support.

Ok Leif, Daniel will evaluate which code could be leveraged and put it in the library under either Silicon vendor's  folder or under RiscVPlatformPkg base on the functionality.
Many thanks for this.
Abner
> 
> /
>     Leif

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

View/Reply Online (#60458): https://edk2.groups.io/g/devel/message/60458
Mute This Topic: https://groups.io/mt/74353494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-