Where to put RISC-V packages


Daniel Schaefer
 

Hi all,

(CC edk2 devel ML - shouldn't be a private discussion again)

let me try to summarize the points of each:

- edk2-platforms isn't a less stable edk2 per se
- edk2-platforms can be used for architectural decisions that are not
stable yet and therefore different platforms might implement them
differently
- Still we'd rather put unstable things in edk2-platforms for now
- In the future CPU Init shouldn't be totally separate packages but the
overlapping code should be combined in
UefiCpuPkg/MdePkg/MdeModulePkg/...
- This applies to RISC-V, as well as ARM
- UefiCpuPkg is too x86 focused
-> needs lots of work and RISC-V shouldn't wait for that
- RISC-V changes should preferably be integrated into existing packages
instead of building an entirely separate hierarchy
-> We have done this. For example DxeIpl is now in edk2 in
MdeModulePkg.

I hope everyone can agree with this summary.
I suggest that we do the following:

- Merge all of the RISC-V code to edk2-platforms
- Evaluate the packages individually and move them to edk2 one by one,
or to merge them into existing edk2 packages
- In the end in edk2-platforms should only remain code for specific
platforms, like the SiFive Unleashed board. No generic RISC-V code,
right?

I'm not experienced enough with EDK2 to evaluate whether packages might
fit into EDK2 directly. Maybe we should have a call and have a look at
each on individually? Below I looked into the libraries and drivers of
every package that we want to upstream with our branch. I describe, what
they do and if they are platform specific. Then I issue my guess for
where it would belong.
I hope that gives everyone a bit more insight into all of the code we'd
like to merge.

Again: Would it be okay to merge everything as is into edk2-platforms. (DxeIpl
was previously merged into edk2). And then slowly migrate them over into edk2?
Or should be evaluate each package individually now instead of merging
everything at once.

- Platform/RISC-V/PlatformPkg
- FirmwareContextProcessorSpecificLib
Take processors specific data from SMBIOS and put in HOB
=> edk2
- OpensbiPlatformLibNull
Null library for platform provided OpenSBI functions.
Those functions are defined in the SiFive directories
=> edk2
- PlatformBootManagerLib
I'm not sure why this is there. It doesn't look RISC-V specific.
cc: Abner
=> ???
- PlatformMemoryTestLibNull
Same as above
=> ???
- PlatformUpdateProgressLibNull
Same as above
=> ???
- RiscVPlatformTempMemoryInitLibNull
Initialize temporary RAM on stack. Same on every RISC-V CPU
Depends on platform's PCD values
=> edk2
- Sec
SEC Phase, same for every RISC-V CPU
=> edk2
- Silicon/RISC-V/ProcessorPkg
- PeiServicesTablePointerLibOpenSbi
Saving/loading PeiServicesTable in RISC-V scratch space with OpenSBI
On every RISC-V CPU
=> edk2
- RiscVCpuLib
ASM functions to read RISC-V CSRs on every RISC-V CPU
For UefiCpuPkg?
=> edk2
- RiscVEdk2SbiLib
Wrapper library to call the SBI ecalls on every RISC-V CPU
=> edk2
- RiscVExceptionLib
CpuExceptionHandlerLib implementation for RISC-V
Currently only uses CSR accesses
cc Abner: Will this be platform specific with the CLIC proposals?
=> edk2/edk2-platforms??
- RiscVOpensbiLib
OpenSBI library
Needs to be called in early init on every RISC-V CPU
=> edk2?
- RiscVPlatformTimerLibNull,RiscVTimerLib
TimerLib implementation for RISC-V
Depends on platform specific PCDs only
=> edk2?
- CpuDxe
Produces: gEfiCpuArchProtocolGuid
Would go into UefiCpuPkg
=> edk2
- SmbiosDxe
Create SMBIOS entries type 4, type 7 and type 44
Values need to be set by platform
=> edk2-platforms

All packages under edk2-platforms/{Silicon,Platform}/SiFive/ are not
included here, as they are obviously platform specific and need to go
into edk2-platforms.

Thanks,
Daniel

On 5/29/20 7:25 AM, Sean Brogan wrote:
Sorry was just getting to this.  You guys are fast.  This is mostly to Daniel’s original email but I think it lines up with Abner’s questions and Mike’s response.
Daniel,
I think both Mike and Bret said most everything needed but I'll clarify just in case.
I don't think the conversations below (Daniels) is accurate for my concerns about RiscV support or my view of edk2-platforms.
In my view Edk2 packages are best when they are functionality based. This allows for proper code base layering and makes sure that abstractions are created in a way that will facilitate cross platform core development.  For packages that are based on other characteristics, like architecture, dependencies at the package level usually become tangled and brittle which slowly leaks out into the rest of the code base and then makes the entire repository harder to work with.
For that reason I advocated that the RiscV support be integrated into the appropriate packages and modules.  Where there is unique functionality that then needs to be evaluated to determine if its core functionality or platform functionality. Depending on that it would then land in the appropriate edk2 package or something in edk2-platforms.
Thanks
Sean
*From:* Kinney, Michael D <michael.d.kinney@...>
*Sent:* Thursday, May 28, 2020 10:18 PM
*To:* Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; Bret Barkelew <Bret.Barkelew@...>; Schaefer, Daniel (DualStudy) <daniel.schaefer@...>; Sean Brogan <sean.brogan@...>; Ard Biesheuvel (ard.biesheuvel@...) <ard.biesheuvel@...>; Zimmer, Vincent <vincent.zimmer@...>; Kinney, Michael D <michael.d.kinney@...>
*Cc:* Leif Lindholm <leif@...>
*Subject:* [EXTERNAL] RE: Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
Abner,
The architectural RISCV CPU content that does not change between RISCV CPU implementations would be a candidate for MdePkg, MdeModulePkg, or UefiCpuPkg.  We would need to do another review of the content along with the ARM/AARCH64 content to see how best to organize the CPU related content for now and future.
RISCV platform content would still need to go into edk2-platforms. Non-architectural RISCV CPU content would also need to go into edk2-platforms.
Mike
*From:* Chang, Abner (HPS SW/FW Technologist) <abner.chang@... <mailto:abner.chang@...>>
*Sent:* Thursday, May 28, 2020 10:01 PM
*To:* Kinney, Michael D <michael.d.kinney@... <mailto:michael.d.kinney@...>>; Bret Barkelew <Bret.Barkelew@... <mailto:Bret.Barkelew@...>>; Schaefer, Daniel (DualStudy) <daniel.schaefer@... <mailto:daniel.schaefer@...>>; Sean Brogan <sean.brogan@... <mailto:sean.brogan@...>>; Ard Biesheuvel (ard.biesheuvel@... <mailto:ard.biesheuvel@...>) <ard.biesheuvel@... <mailto:ard.biesheuvel@...>>; Zimmer, Vincent <vincent.zimmer@... <mailto:vincent.zimmer@...>>
*Cc:* Leif Lindholm <leif@... <mailto:leif@...>>
*Subject:* RE: Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
*From:* Kinney, Michael D [mailto:michael.d.kinney@...]
*Sent:* Friday, May 29, 2020 12:36 PM
*To:* Bret Barkelew <Bret.Barkelew@... <mailto:Bret.Barkelew@...>>; Schaefer, Daniel (DualStudy) <daniel.schaefer@... <mailto:daniel.schaefer@...>>; Sean Brogan <sean.brogan@... <mailto:sean.brogan@...>>; Ard Biesheuvel (ard.biesheuvel@... <mailto:ard.biesheuvel@...>) <ard.biesheuvel@... <mailto:ard.biesheuvel@...>>; Kinney, Michael D <michael.d.kinney@... <mailto:michael.d.kinney@...>>; Zimmer, Vincent <vincent.zimmer@... <mailto:vincent.zimmer@...>>
*Cc:* Leif Lindholm <leif@... <mailto:leif@...>>; Chang, Abner (HPS SW/FW Technologist) <abner.chang@... <mailto:abner.chang@...>>
*Subject:* RE: [EXTERNAL] Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
We did a community review meeting.  I recall Ard and Vincent being present.
We discuss the CPU execution mode for PEI and DXE and there was a discussion that RISCV has 2 modes and they want flexibility to use both.  This choice is not defined in the PI spec yet.  We suggested that it could follow the ARM/Thumb model and RISCV could choose a single mode for all PEIMs and DXE drivers and choose to go into the other mode in the module implementation as needed.
*/[Abner] Yes. we done this as we discussed in the community meeting. Those code are belong to RiscV*pkg and currently lay on devel-riscvplatfoms which Leif is reviewing now./*
Given that these fundamental CPU execution mode design choices were still in flux, it did not seem like it was ready for edk2 yet.
*/[Abner] The current implementation is PEI/DXE are in the same mode./*
edk2-staging or an edk2-platforms branch seemed more appropriate until all that was worked out.
*/[Abner] Seems like that work is done. Maybe I have to review PI/UEFI spec for the necessary changes./*
edk2-platforms/master seemed ok as well if different RISCV CPU modes would be used for different platform solutions until a unified approach by the RISCV vendors could be determined  Once that was solidified, promoting to edk2 would be possible.
*/[Abner] this means RicsVPkg and RicsVPlatformPkg eventually will be located in edk2 but not edk2-platforms if above issues are addressed? We don’t have to consider UefiCpuPkg for each arch now? We are good with this though./*
Hopefully this clarifies for Leif why there was some resistance to edk2 repo right now.  Still on deck for the future.
Mike
*From:* Bret Barkelew <Bret.Barkelew@... <mailto:Bret.Barkelew@...>>
*Sent:* Thursday, May 28, 2020 8:11 PM
*To:* Daniel Schaefer <daniel.schaefer@... <mailto:daniel.schaefer@...>>; Kinney, Michael D <michael.d.kinney@... <mailto:michael.d.kinney@...>>; Sean Brogan <sean.brogan@... <mailto:sean.brogan@...>>
*Cc:* Leif Lindholm <leif@... <mailto:leif@...>>; Chang, Abner (HPS SW/FW Technologist) <abner.chang@... <mailto:abner.chang@...>>
*Subject:* RE: [EXTERNAL] Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
I think Sean has his own feedback, but my response to Abner was that that I didn’t like the idea of continuing a pattern of separate packages for significant portions of the CPU init. There have been a few times where it has created unnecessary divergence that made it very difficult to write cross-architecture code. I, personally, wasn’t fighting the code landing in edk2 (except for RiscVPlatform, because it had platform in the name), but I was inquiring to see whether it made more sense to break things up among Mde, UefiCpu, and a couple other packages to encourage adhering to similar patterns and interfaces between all the different architectures. I’ve previously wondered openly whether it made sense to do the same with the Arm packages.
I’ll be honest about not being super familiar with the RISCV code and it would be easy for me to just shrug and say put it wherever, but I know that architecture mobility/agility is important for us and wouldn’t be surprised if I had to use RISCV in the future, and so wanted to make sure that it was as close to what I was familiar with as possible.
- Bret
*From: *Daniel Schaefer <mailto:daniel.schaefer@...>
*Sent: *Thursday, May 28, 2020 8:44 AM
*To: *Kinney, Michael D <mailto:michael.d.kinney@...>; Bret Barkelew <mailto:Bret.Barkelew@...>; Sean Brogan <mailto:sean.brogan@...>
*Cc: *Leif Lindholm <mailto:leif@...>; Chang, Abner (HPS SW/FW Technologist) <mailto:abner.chang@...>
*Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms
Hi Mike, Bret and Sean,
you have previously expressed concern about merging HPE's RISC-V code
into edk2 and suggested merging it into edk2-platforms. Apparently you
discussed this with Abner in a private email thread. According to that
information I tried to summarize your points in an email on the edk2 ML.
I hope I got it right.
As you can see, we have followed that suggestion and sent the new
patches required for that to the ML.
Leif, and I, are still not convinced it's the right choice to not
include it in edk2 proper.
We would appreciate if you could address the concerns Leif has mentioned
in the below email on the public ML.
Thanks!
Daniel
On 5/28/20 1:54 PM, Leif Lindholm wrote:
> 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.

Join devel@edk2.groups.io to automatically receive all group messages.