[edk2-devel] Where to put RISC-V packages

["Daniel Schaefer" <daniel.schaefer@hpe.com>]

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@intel.com>
> *Sent:* Thursday, May 28, 2020 10:18 PM
> *To:* Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>; Bret 
> Barkelew <Bret.Barkelew@microsoft.com>; Schaefer, Daniel (DualStudy) 
> <daniel.schaefer@hpe.com>; Sean Brogan <sean.brogan@microsoft.com>; Ard 
> Biesheuvel (ard.biesheuvel@linaro.org) <ard.biesheuvel@linaro.org>; 
> Zimmer, Vincent <vincent.zimmer@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>
> *Cc:* Leif Lindholm <leif@nuviainc.com>
> *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@hpe.com 
> <mailto:abner.chang@hpe.com>>
> *Sent:* Thursday, May 28, 2020 10:01 PM
> *To:* Kinney, Michael D <michael.d.kinney@intel.com 
> <mailto:michael.d.kinney@intel.com>>; Bret Barkelew 
> <Bret.Barkelew@microsoft.com <mailto:Bret.Barkelew@microsoft.com>>; 
> Schaefer, Daniel (DualStudy) <daniel.schaefer@hpe.com 
> <mailto:daniel.schaefer@hpe.com>>; Sean Brogan 
> <sean.brogan@microsoft.com <mailto:sean.brogan@microsoft.com>>; Ard 
> Biesheuvel (ard.biesheuvel@linaro.org 
> <mailto:ard.biesheuvel@linaro.org>) <ard.biesheuvel@linaro.org 
> <mailto:ard.biesheuvel@linaro.org>>; Zimmer, Vincent 
> <vincent.zimmer@intel.com <mailto:vincent.zimmer@intel.com>>
> *Cc:* Leif Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>
> *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@intel.com]
> *Sent:* Friday, May 29, 2020 12:36 PM
> *To:* Bret Barkelew <Bret.Barkelew@microsoft.com 
> <mailto:Bret.Barkelew@microsoft.com>>; Schaefer, Daniel (DualStudy) 
> <daniel.schaefer@hpe.com <mailto:daniel.schaefer@hpe.com>>; Sean Brogan 
> <sean.brogan@microsoft.com <mailto:sean.brogan@microsoft.com>>; Ard 
> Biesheuvel (ard.biesheuvel@linaro.org 
> <mailto:ard.biesheuvel@linaro.org>) <ard.biesheuvel@linaro.org 
> <mailto:ard.biesheuvel@linaro.org>>; Kinney, Michael D 
> <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; 
> Zimmer, Vincent <vincent.zimmer@intel.com <mailto:vincent.zimmer@intel.com>>
> *Cc:* Leif Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>; 
> Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com 
> <mailto:abner.chang@hpe.com>>
> *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@microsoft.com 
> <mailto:Bret.Barkelew@microsoft.com>>
> *Sent:* Thursday, May 28, 2020 8:11 PM
> *To:* Daniel Schaefer <daniel.schaefer@hpe.com 
> <mailto:daniel.schaefer@hpe.com>>; Kinney, Michael D 
> <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Sean 
> Brogan <sean.brogan@microsoft.com <mailto:sean.brogan@microsoft.com>>
> *Cc:* Leif Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>; 
> Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com 
> <mailto:abner.chang@hpe.com>>
> *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@hpe.com>
> *Sent: *Thursday, May 28, 2020 8:44 AM
> *To: *Kinney, Michael D <mailto:michael.d.kinney@intel.com>; Bret 
> Barkelew <mailto:Bret.Barkelew@microsoft.com>; Sean Brogan 
> <mailto:sean.brogan@microsoft.com>
> *Cc: *Leif Lindholm <mailto:leif@nuviainc.com>; Chang, Abner (HPS SW/FW 
> Technologist) <mailto:abner.chang@hpe.com>
> *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.
>