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

["Abner Chang" <abner.chang@hpe.com>]

> -----Original Message-----
> From: Schaefer, Daniel (DualStudy)
> Sent: Wednesday, June 3, 2020 7:57 PM
> To: Sean Brogan <sean.brogan@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Chang, Abner (HPS SW/FW Technologist)
> <abner.chang@hpe.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
> Ard Biesheuvel (ard.biesheuvel@linaro.org) <ard.biesheuvel@linaro.org>;
> Zimmer, Vincent <vincent.zimmer@intel.com>; Leif Lindholm
> <leif@nuviainc.com>
> Cc: devel@edk2.groups.io
> Subject: [edk2-devel] Where to put RISC-V packages
> 
> 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
Agree. This is the first step after those new changes get "Reviewed-by".
> - 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 agree with above. But those architecture-specific drivers may still stay in their own package.
> 
> 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
Yes, we will have to review above modules and move those to UefiCpuPkg/MdePkg/MdeModulePkg as many as we can. But I guess some maybe still have to stay in RiscVPkg under edk2, unless we can move those RISC-V specific definitions to MdePkg/Include. We can discuss this later and create another branch as POC code based on edk2/edk2-platform RISC-V port.
> 
> 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.
> >