Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
There is a very good discussion here on the roles and responsibility and potential suggestions for changes to the Wiki page that document those roles and responsibilities. May I suggest that someone start a new thread that discusses the proposed changes to the Wiki page and leave this thread for the review of the changes to Maintainers.txt? Thanks, Mike > -Original Message- > From: Yao, Jiewen > Sent: Sunday, October 29, 2023 7:54 PM > To: devel@edk2.groups.io; ler...@redhat.com; pedro.falc...@gmail.com; > Kinney, Michael D > Cc: Andrew Fish ; Leif Lindholm > ; Warkentin, Andrei > ; West, Catharine > ; Bi, Dandan ; Daniel > Schaefer ; David Woodhouse > ; De, Debkumar ; Dong, > Eric ; Jiang, Guomin ; > Wu, Hao A ; James Bottomley ; > Wang, Jian J ; Justen, Jordan L > ; Julien Grall ; Peter > Grehan ; Zhang, Qi1 ; Ng, Ray > Han Lim ; Stefan Berger > ; Hou, Wenxing ; Lu, > Xiaoyu1 > Subject: RE: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on > active community members > > > I'll re-raise my point about relaxing the contribution conditions > too -- > > given this state, I'd propose a "merge by default" approach, with a > > reasonable timeout. > > [Jiewen] Yes. I agree this approach. > A reasonable timeout seems enough to allow people to think and > feedback. > > > > Also, I would like to propose another the contribution condition > relax. > > Currently, our agreed condition to merge is: > 1) Reviewed-by from Maintainer. > 2) Acked-by from Maintainer + Reviewed-by from Reviewer > > I propose to change the second condition: > 2) Acked-by from Maintainer + Reviewed-by from anyone who can be > trusted by the maintainer. > > > That is based upon the current situation - anyone can be a reviewer > just because they want to be CCed and has no expectation to review the > code. > Restricting R-B from a reviewer does not make sense to me. > > Thank you > Yao, Jiewen > > > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of > Laszlo Ersek > > Sent: Sunday, October 29, 2023 9:30 PM > > To: devel@edk2.groups.io; pedro.falc...@gmail.com; Kinney, Michael D > > > > Cc: Andrew Fish ; Leif Lindholm > ; > > Warkentin, Andrei ; West, Catharine > > ; Bi, Dandan ; Daniel > > Schaefer ; David Woodhouse > ; > > De, Debkumar ; Dong, Eric > ; > > Jiang, Guomin ; Wu, Hao A > ; > > James Bottomley ; Wang, Jian J > ; > > Justen, Jordan L ; Julien Grall > ; > > Peter Grehan ; Zhang, Qi1 ; > Ng, > > Ray Han Lim ; Stefan Berger > > ; Hou, Wenxing ; Lu, > Xiaoyu1 > > > > Subject: Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based > on active > > community members > > > > On 10/29/23 03:16, Pedro Falcato wrote: > > > On Sat, Oct 28, 2023 at 8:23 PM Michael D Kinney > > > wrote: > > >> > > >> Over the past few months, all the of the Maintainers and > > >> Reviewers listed in Maintainers.txt have been contacted to make > > >> sure Maintainers.txt accurately represents the TianoCore > > >> community members that are actively participating in their > > >> roles. Based on specific feedback, bounced emails, and no > > >> responses, updates have been made. > > >> > > >> * RISCV64: Daniel Schaefer replaced with Andrei Warkentin > > >> * ArmVirtPkg Xen has no remaining reviewers and review > > >> responsibility defaults to ArmVirtPkg Maintainers/Reviewers. > > >> * ACPI modules related to S3 has no remaining reviewers and > > >> review responsibility defaults to MdeModulePkg Maintainers/ > > >> Reviewers. > > >> * OVMF CSM modules has no remaining reviewers and review > > >> responsibility defaults to OvmfPkg Maintainers/Reviewers. > > >> * Bounce: Chan Laura > > >> * Many smaller updates removing individuals that are no > > >> longer involved or have replacement coverage. > > > > > > Mike, > > > > > > Thank you so much for doing this thankless task. Some comments: > > > > > >> diff --git a/Maintainers.txt b/Maintainers.txt > > >> index 3f40cdeb5554..2b03ccbe54aa 100644 > > >> --- a/Maintainers.txt > > >> +++ b/Maintainers.txt > > >> @@ -93,7 +93,7 @@ M: Sami Mujawar > > [samimujawar] > > >> RISCV64 > > >> F: */RiscV64/ > > >> M: Sunil V L [vlsunil] > > >> -R: Daniel Schaefer [JohnAZoidberg] > > >> +R: Andrei Warkentin [andreiw] > > >> > > >> LOONGARCH64 > > >> F: */LoongArch64/ > > >> @@ -157,16 +157,6 @@ R: Leif Lindholm > > [leiflindholm] > > >> R: Sami Mujawar [samimujawar] > > >> R: Gerd Hoffmann [kraxel] > > >> > > >> -ArmVirtPkg: modules used on Xen > > >> -F: ArmVirtPkg/ArmVirtXen.* > > >> -F: ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/ > > >> -F: ArmVirtPkg/Library/XenVirtMemInfoLib/ > > >> -F: ArmVirtPkg/PrePi/ > > >> -F: ArmVirtPkg/XenAcpiPlatformDxe/ > > >> -F: ArmVirtPkg/XenPlatformHasAcpiDtDxe/ > > >> -F: ArmVirtPkg/XenioFdtDxe/ > > >> -R: Julien Grall [jgrall] > > > > > > ArmVirtPkg Xen modules seize to have a dedicated maintainer. Can > the > > > generic ArmVirtPkg maintainers handle *more code* (particularly, > > >
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
On 10/29/23 5:23 AM, Michael D Kinney wrote: Over the past few months, all the of the Maintainers and Reviewers listed in Maintainers.txt have been contacted to make sure Maintainers.txt accurately represents the TianoCore community members that are actively participating in their roles. Based on specific feedback, bounced emails, and no responses, updates have been made. * RISCV64: Daniel Schaefer replaced with Andrei Warkentin * ArmVirtPkg Xen has no remaining reviewers and review responsibility defaults to ArmVirtPkg Maintainers/Reviewers. * ACPI modules related to S3 has no remaining reviewers and review responsibility defaults to MdeModulePkg Maintainers/ Reviewers. * OVMF CSM modules has no remaining reviewers and review responsibility defaults to OvmfPkg Maintainers/Reviewers. * Bounce: Chan Laura * Many smaller updates removing individuals that are no longer involved or have replacement coverage. Cc: Andrew Fish Cc: Leif Lindholm Cc: Andrei Warkentin Cc: Catharine West Cc: Dandan Bi Cc: Daniel Schaefer Cc: David Woodhouse Cc: Debkumar De Cc: Eric Dong Cc: Guomin Jiang Cc: Hao A Wu Cc: James Bottomley Cc: Jian J Wang Cc: Jordan Justen Cc: Julien Grall Cc: Peter Grehan Cc: Qi Zhang Cc: Ray Han Lim Ng Cc: Stefan Berger Cc: Wenxing Hou Cc: Xiaoyu Lu Signed-off-by: Michael D Kinney ... Reviewed-by: Peter Grehan -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110287): https://edk2.groups.io/g/devel/message/110287 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/1] Maintainers.txt: Update maintainers list
For UefiCpuPkg part, Reviewed-by: Ray Ni Thanks, Ray From: Attar, AbdulLateef (Abdul Lateef) Sent: Thursday, October 19, 2023 2:14 PM To: Chang, Abner ; devel@edk2.groups.io Cc: Andrew Fish ; Leif Lindholm ; Kinney, Michael D ; Nickle Wang ; Wang, Jian J ; Gao, Liming ; Liu, Zhiguang ; Dong, Eric ; Ni, Ray ; Kumar, Rahul R ; Gerd Hoffmann Subject: RE: [PATCH 1/1] Maintainers.txt: Update maintainers list [AMD Official Use Only - General] Acked-by: Abdul Lateef Attar -Original Message- From: Chang, Abner Sent: Thursday, October 19, 2023 11:13 AM To: devel@edk2.groups.io Cc: Andrew Fish ; Leif Lindholm ; Michael D Kinney ; Attar, AbdulLateef (Abdul Lateef) ; Nickle Wang ; Jian J Wang ; Liming Gao ; Zhiguang Liu ; Eric Dong ; Ray Ni ; Rahul Kumar ; Gerd Hoffmann Subject: [PATCH 1/1] Maintainers.txt: Update maintainers list From: Abner Chang - Add two entries of MdePkg and MdeModulePkg for manageability modules and files. - Add one entry of UefiCpuPkg AMD related files. Signed-off-by: Abner Chang Cc: Andrew Fish Cc: Leif Lindholm Cc: Michael D Kinney Cc: Abdul Lateef Attar Cc: Nickle Wang Cc: Jian J Wang Cc: Liming Gao Cc: Zhiguang Liu Cc: Eric Dong Cc: Ray Ni Cc: Rahul Kumar Cc: Gerd Hoffmann --- Maintainers.txt | 21 + 1 file changed, 21 insertions(+) diff --git a/Maintainers.txt b/Maintainers.txt index 1a7525b1b4c..4bec19832f4 100644 --- a/Maintainers.txt +++ b/Maintainers.txt @@ -443,6 +443,13 @@ F: MdeModulePkg/Include/Protocol/UsbEthernetProtocol.h M: Richard Ho [richardho] R: Rebecca Cran [bcran] +MdeModulePkg: Manageability modules +F: MdeModulePkg/Include/*Ipmi*.* +F: MdeModulePkg/Library/*Ipmi*.* +M: Abner Chang [changab] +R: Abdul Lateef Attar [abdattar] +R: Nickle Wang [nicklela] + MdePkg F: MdePkg/ W: https://github.com/tianocore/tianocore.github.io/wiki/MdePkg @@ -468,6 +475,14 @@ R: Gua Guo [gguo11837463] R: Chasel Chiu [ChaselChiu] R: James Lu [jameslu8] +MdePkg: Manageability industryStandard standard C header files +F: MdePkg/Include/IndustryStandard/*Ipmi*.h +F: MdePkg/Include/IndustryStandard/*Mctp*.h +F: MdePkg/Include/IndustryStandard/*Pldm*.h +M: Abner Chang [changab] +R: Abdul Lateef Attar [abdattar] +R: Nickle Wang [nicklela] + NetworkPkg F: NetworkPkg/ W: https://github.com/tianocore/tianocore.github.io/wiki/NetworkPkg @@ -657,6 +672,12 @@ F: UefiCpuPkg/ResetVector/ R: Debkumar De [dde01] R: Catharine West [catharine-intl] +UefiCpuPkg: AMD related files +F: UefiCpuPkg/Library/MmSaveStateLib/*Amd*.* +F: UefiCpuPkg/Library/SmmCpuFeaturesLib/*Amd*.* +M: Abdul Lateef Attar [abdattar] +R: Abner Chang [changab] + UefiPayloadPkg F: UefiPayloadPkg/ W: https://github.com/tianocore/tianocore.github.io/wiki/UefiPayloadPkg -- 2.37.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110286): https://edk2.groups.io/g/devel/message/110286 Mute This Topic: https://groups.io/mt/102055114/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
> I'll re-raise my point about relaxing the contribution conditions too -- > given this state, I'd propose a "merge by default" approach, with a > reasonable timeout. [Jiewen] Yes. I agree this approach. A reasonable timeout seems enough to allow people to think and feedback. Also, I would like to propose another the contribution condition relax. Currently, our agreed condition to merge is: 1) Reviewed-by from Maintainer. 2) Acked-by from Maintainer + Reviewed-by from Reviewer I propose to change the second condition: 2) Acked-by from Maintainer + Reviewed-by from anyone who can be trusted by the maintainer. That is based upon the current situation - anyone can be a reviewer just because they want to be CCed and has no expectation to review the code. Restricting R-B from a reviewer does not make sense to me. Thank you Yao, Jiewen > -Original Message- > From: devel@edk2.groups.io On Behalf Of Laszlo Ersek > Sent: Sunday, October 29, 2023 9:30 PM > To: devel@edk2.groups.io; pedro.falc...@gmail.com; Kinney, Michael D > > Cc: Andrew Fish ; Leif Lindholm ; > Warkentin, Andrei ; West, Catharine > ; Bi, Dandan ; Daniel > Schaefer ; David Woodhouse ; > De, Debkumar ; Dong, Eric ; > Jiang, Guomin ; Wu, Hao A ; > James Bottomley ; Wang, Jian J ; > Justen, Jordan L ; Julien Grall ; > Peter Grehan ; Zhang, Qi1 ; Ng, > Ray Han Lim ; Stefan Berger > ; Hou, Wenxing ; Lu, Xiaoyu1 > > Subject: Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active > community members > > On 10/29/23 03:16, Pedro Falcato wrote: > > On Sat, Oct 28, 2023 at 8:23 PM Michael D Kinney > > wrote: > >> > >> Over the past few months, all the of the Maintainers and > >> Reviewers listed in Maintainers.txt have been contacted to make > >> sure Maintainers.txt accurately represents the TianoCore > >> community members that are actively participating in their > >> roles. Based on specific feedback, bounced emails, and no > >> responses, updates have been made. > >> > >> * RISCV64: Daniel Schaefer replaced with Andrei Warkentin > >> * ArmVirtPkg Xen has no remaining reviewers and review > >> responsibility defaults to ArmVirtPkg Maintainers/Reviewers. > >> * ACPI modules related to S3 has no remaining reviewers and > >> review responsibility defaults to MdeModulePkg Maintainers/ > >> Reviewers. > >> * OVMF CSM modules has no remaining reviewers and review > >> responsibility defaults to OvmfPkg Maintainers/Reviewers. > >> * Bounce: Chan Laura > >> * Many smaller updates removing individuals that are no > >> longer involved or have replacement coverage. > > > > Mike, > > > > Thank you so much for doing this thankless task. Some comments: > > > >> diff --git a/Maintainers.txt b/Maintainers.txt > >> index 3f40cdeb5554..2b03ccbe54aa 100644 > >> --- a/Maintainers.txt > >> +++ b/Maintainers.txt > >> @@ -93,7 +93,7 @@ M: Sami Mujawar > [samimujawar] > >> RISCV64 > >> F: */RiscV64/ > >> M: Sunil V L [vlsunil] > >> -R: Daniel Schaefer [JohnAZoidberg] > >> +R: Andrei Warkentin [andreiw] > >> > >> LOONGARCH64 > >> F: */LoongArch64/ > >> @@ -157,16 +157,6 @@ R: Leif Lindholm > [leiflindholm] > >> R: Sami Mujawar [samimujawar] > >> R: Gerd Hoffmann [kraxel] > >> > >> -ArmVirtPkg: modules used on Xen > >> -F: ArmVirtPkg/ArmVirtXen.* > >> -F: ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/ > >> -F: ArmVirtPkg/Library/XenVirtMemInfoLib/ > >> -F: ArmVirtPkg/PrePi/ > >> -F: ArmVirtPkg/XenAcpiPlatformDxe/ > >> -F: ArmVirtPkg/XenPlatformHasAcpiDtDxe/ > >> -F: ArmVirtPkg/XenioFdtDxe/ > >> -R: Julien Grall [jgrall] > > > > ArmVirtPkg Xen modules seize to have a dedicated maintainer. Can the > > generic ArmVirtPkg maintainers handle *more code* (particularly, > > functionality that's not trivial to test, unless you actively use > > Xen)? > > An alternative to removing this entire section is to replace Julien's > line with the following status line: > > S: Orphan > > The definition in Maintainers.txt is: > > Orphan: No current maintainer [but maybe you could take the > role as you write your new code]. > > I think this might be clearer for all three of: contributors, consumers, > and existent maintainers. > > - Contributors: An ArmVirtPkg maintainer may techincally merge your > code, but you won't get substantive feedback > > - Consumers: you can build and run this code, but if it breaks, you get > to keep both parts > > - Existent ArmVirtPkg maintainers: you can rest assured in the knowledge > that you are not saddled with deep technical reviews for this subsystem > that you can't even boot in your environment. You're only responsible > for the technical act of merging patches. > > > > >> BaseTools > >> F: BaseTools/ > >> W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools > >> @@ -187,8 +177,7 @@ F: CryptoPkg/ > >> W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg > >> M: Jiewen Yao [jyao1] > >> M: Yi Li [liyi77] > >> -R: Xiaoyu
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
Thanks Mike. I am reading the WIKI page. > and/or provides testing or regression testing for the Package (or some > modules thereof), in certain platforms and environments. [Jiewen] Are we expecting Reviewer to provide testing or regression testing for the package? Is that what the reviewer *commits* to do? For example, Maintainer may ask the reviewer to do some testing, right? > Reviewer is responsible for timely responses on emails addressed to them > (preferably less than calendar week). [Jiewen] Is that what the reviewer *commits* to do? For example, Maintainer may ask the reviewer to provide feedback, right? Those are more than just CCed. Thank you Yao, Jiewen > -Original Message- > From: Kinney, Michael D > Sent: Monday, October 30, 2023 1:23 AM > To: Yao, Jiewen ; j...@linux.ibm.com; Laszlo Ersek > ; devel@edk2.groups.io; pedro.falc...@gmail.com > Cc: Andrew Fish ; Leif Lindholm ; > Warkentin, Andrei ; West, Catharine > ; Bi, Dandan ; Daniel > Schaefer ; David Woodhouse ; > De, Debkumar ; Dong, Eric ; > Jiang, Guomin ; Wu, Hao A ; > Wang, Jian J ; Justen, Jordan L > ; Julien Grall ; Peter Grehan > ; Zhang, Qi1 ; Ng, Ray Han Lim > ; Stefan Berger ; Hou, > Wenxing ; Lu, Xiaoyu1 ; > Kinney, Michael D > Subject: RE: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active > community members > > This is the Wiki page where TianoCore documents the TianoCore community > member roles. > > https://github.com/tianocore/tianocore.github.io/wiki/TianoCore-Who-we-are > > We can update/edit as needed to accurately reflect what all the Maintainers > and Reviewers agree are their roles and responsibilities as assigned in > Maintainers.txt. > > Thanks, > > Mike > > > > -Original Message- > > From: Yao, Jiewen > > Sent: Sunday, October 29, 2023 9:26 AM > > To: j...@linux.ibm.com; Laszlo Ersek ; > > devel@edk2.groups.io; pedro.falc...@gmail.com; Kinney, Michael D > > > > Cc: Andrew Fish ; Leif Lindholm > > ; Warkentin, Andrei > > ; West, Catharine > > ; Bi, Dandan ; Daniel > > Schaefer ; David Woodhouse > > ; De, Debkumar ; Dong, > > Eric ; Jiang, Guomin ; > > Wu, Hao A ; Wang, Jian J ; > > Justen, Jordan L ; Julien Grall > > ; Peter Grehan ; Zhang, Qi1 > > ; Ng, Ray Han Lim ; > > Stefan Berger ; Hou, Wenxing > > ; Lu, Xiaoyu1 > > Subject: RE: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on > > active community members > > > > OK. Maintainer should do code review. I have no doubt on that. > > > > My confusion is about "reviewer" role. What is criteria and what is > > responsibility? > > > > Are you saying that "reviewer" just means that someone raised the hand > > and said: "I want to be notified", and there is no expectation that > > he/she would review the patch? > > > > I would like to understand more on how that works and what that means. > > Would you please give a URL for the reviewer definition in Linux > > Kernel? > > > > Thank you > > Yao, Jiewen > > > > > > > > > -Original Message- > > > From: James Bottomley > > > Sent: Monday, October 30, 2023 12:02 AM > > > To: Yao, Jiewen ; Laszlo Ersek > > ; > > > devel@edk2.groups.io; pedro.falc...@gmail.com; Kinney, Michael D > > > > > > Cc: Andrew Fish ; Leif Lindholm > > ; > > > Warkentin, Andrei ; West, Catharine > > > ; Bi, Dandan ; Daniel > > > Schaefer ; David Woodhouse > > ; > > > De, Debkumar ; Dong, Eric > > ; > > > Jiang, Guomin ; Wu, Hao A > > ; > > > Wang, Jian J ; Justen, Jordan L > > > ; Julien Grall ; Peter > > Grehan > > > ; Zhang, Qi1 ; Ng, Ray Han > > Lim > > > ; Stefan Berger ; > > Hou, > > > Wenxing ; Lu, Xiaoyu1 > > > Subject: Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based > > on active > > > community members > > > > > > On Sun, 2023-10-29 at 15:42 +, Yao, Jiewen wrote: > > > > > I'd say that's pretty close. A reviewer role is a request for > > > > > keeping > > > > > the reviewer in the loop. > > > > > > > > [Jiewen] I am disappointed on that. > > > > To me, that is NOT a real reviewer. See below description on what > > is > > > > "code review". > > > > https://google.github.io/eng-practices/review/ > > > > https://about.gitlab.com/topics/version-control/what-is-code- > > review/ > > > > > > Well, that's what someone's view of what a patch review should > > consist > > > of, not what a reviewer's role in MAINTAINERS should be. > > > > > > In general, you really don't want to force people to review patches, > > > because you'd like a reviewer to be familiar with the area and > > > comfortable with the patch. So are you saying that anyone listed as > > a > > > reviewer in a particular area should be capable of reviewing any > > patch? > > > and further that they should be expected to review every patch? > > > Because that's definitely not what the R role in the Linux Kernel > > would > > > mean. > > > > > > I know that's not what happened to me in Confidential Computing, > > > because I had a very specific area around SEV and SEV-ES secret > > > injection
Re: [edk2-devel] [PATCH V1 2/2] OvmfPkg/BaseMemEncryptTdxLib: Handle retry result of MapGPA
On Friday, October 27, 2023 7:05 PM, Gerd Hoffmann wrote:
> > + while (RetryCount < MAX_RETRIES_PER_PAGE) {
> > +TdStatus = TdVmCallMapGPA (PhysicalAddress, Length,
> );
> > +if (TdStatus != TDVMCALL_STATUS_RETRY) {
> > + break;
> > +}
> > +
> > +DEBUG ((DEBUG_VERBOSE, "%a: TdVmcall(MAPGPA) Retry
> > + PhysicalAddress is %llx, MapGpaRetryaddr is %llx\n", __func__,
> > + PhysicalAddress, MapGpaRetryaddr));
> > +
> > +EndAddress = PhysicalAddress + Length;
>
> The end address doesn't change, you can move that out of the loop.
It would be updated in next version.
>
> > +if ((MapGpaRetryaddr < PhysicalAddress) || (MapGpaRetryaddr >
> EndAddress)) {
> > + DEBUG ((DEBUG_ERROR, "%a: TdVmcall(MAPGPA) failed Retry
> PhysicalAddress is %llx, MapGpaRetryaddr is %llx\n", __func__,
> PhysicalAddress, MapGpaRetryaddr));
> > + break;
> > +}
>
> The error message is misleading. The actual problem is that the
> 'PhysicalAddress <= MapGpaRetryaddr <= EndAddress' sanity check failed.
The error message would be updated in next version.
>
Thanks
Ceping
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110283): https://edk2.groups.io/g/devel/message/110283
Mute This Topic: https://groups.io/mt/102212640/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 1/1] IpmiFeaturePkg/GenericIpmi: Support Standalone MM
[AMD Official Use Only - General]
Hi Lisa, please check the comments in line.
> -Original Message-
> From: Lixia Huang
> Sent: Monday, October 30, 2023 9:20 AM
> To: devel@edk2.groups.io
> Cc: Chang, Abner ; Nate DeSimone
>
> Subject: [PATCH v2 1/1] IpmiFeaturePkg/GenericIpmi: Support Standalone
> MM
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Add Standalone Mm Generic Impi driver. And add type 'PcdsFixedAtBuild'
> for PcdIpmiSmmIoBaseAddress to access in StandaloneMm driver
>
> Cc: Abner Chang
> Cc: Nate DeSimone
> Signed-off-by: Lixia Huang
> ---
>
> Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Standa
> loneMm/StandaloneMmGenericIpmi.c | 148
>
> Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Standa
> loneMm/StandaloneMmGenericIpmi.inf | 52 +++
>
> Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec
> | 2 +
> 3 files changed, 202 insertions(+)
>
> diff --git
> a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.c
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.c
> new file mode 100644
> index ..a8751f5b1a1d
> --- /dev/null
> +++
> b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/Stan
> daloneMm/StandaloneMmGenericIpmi.c
> @@ -0,0 +1,148 @@
> +/** @file
> + Generic StandaloneMm IPMI stack driver
> +
> + @copyright
We don't usually have @copyright tag in file header. Is this a new requirement
for the patch?
> + Copyright 2023 Intel Corporation.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +//
> +// Statements that include other files
> +//
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "IpmiHooks.h"
> +#include "IpmiBmcCommon.h"
> +#include "IpmiBmc.h"
Could you please move above three include statement below #include
?
> +#include
> +
> +IPMI_BMC_INSTANCE_DATA *mIpmiInstance;
> +EFI_HANDLE mHandle;
> +
> +EFI_STATUS
> +SmmInitializeIpmiKcsPhysicalLayer (
> + VOID
> + )
> +/*++
> +
> +Routine Description:
> + Setup and initialize the BMC for the SMM phase. In order to verify the BMC
> is functioning
> + as expected, the BMC Selftest is performed. The results are then checked
> and any errors are
> + reported to the error manager. Errors are collected throughout this
> routine
> and reported
> + just prior to installing the driver. If there are more errors than
> MAX_SOFT_COUNT, then they
> + will be ignored.
> +
> +Arguments:
> + ImageHandle - Handle of this driver image
> + SystemTable - Table containing standard EFI services
> +
> +Returns:
> + EFI_SUCCESS - Successful driver initialization
> +
> +--*/
You still follow the old Intel coding style? I think we should have function
header above the function. Could we relocate the function header?
> +{
> + EFI_STATUS Status;
> +
> + DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer entry \n"));
> +
> + Status = gMmst->MmAllocatePool (
> +EfiRuntimeServicesData,
> +sizeof (IPMI_BMC_INSTANCE_DATA),
> +(VOID **));
> +
> + if (EFI_ERROR (Status)) {
> +DEBUG ((DEBUG_ERROR, "mIpmiInstance mem alloc failed - 0x%x\n",
> Status));
> +return Status;
> + } else {
> +
> +//
> +// Initialize the KCS transaction timeout. Assume delay unit is 1000 us.
> +//
> +mIpmiInstance->KcsTimeoutPeriod = (BMC_KCS_TIMEOUT * 1000*1000) /
> KCS_DELAY_UNIT;
> +
> +//
> +// Initialize IPMI IO Base, we still use SMS IO base to get device ID and
> Seltest result since SMM IF may have different cmds supported
> +//
> +mIpmiInstance->IpmiIoBase = FixedPcdGet16
> (PcdIpmiSmmIoBaseAddress);
> +mIpmiInstance->Signature= SM_IPMI_BMC_SIGNATURE;
> +mIpmiInstance->SlaveAddress = BMC_SLAVE_ADDRESS;
> +mIpmiInstance->BmcStatus= BMC_NOTREADY;
> +mIpmiInstance->IpmiTransport.IpmiSubmitCommand =
> IpmiSendCommand;
> +mIpmiInstance->IpmiTransport.GetBmcStatus = IpmiGetBmcStatus;
> +
> +mHandle = NULL;
> +Status = gMmst->MmInstallProtocolInterface (
> + ,
> + ,
> + EFI_NATIVE_INTERFACE,
> + >IpmiTransport
> + );
> +ASSERT_EFI_ERROR (Status);
> +
> +DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer exit \n"));
> +
> +return EFI_SUCCESS;
> + }
> +}
> +
> +/**
> + The module Entry Point of driver.
> +
> + @param ImageHandleThe firmware allocated handle for the EFI image.
> + @param SystemTableA pointer to the MM
[edk2-devel] [PATCH v2 1/1] IpmiFeaturePkg/GenericIpmi: Support Standalone MM
Add Standalone Mm Generic Impi driver. And add type 'PcdsFixedAtBuild'
for PcdIpmiSmmIoBaseAddress to access in StandaloneMm driver
Cc: Abner Chang
Cc: Nate DeSimone
Signed-off-by: Lixia Huang
---
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
| 148
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.inf
| 52 +++
Features/Intel/OutOfBandManagement/IpmiFeaturePkg/IpmiFeaturePkg.dec
| 2 +
3 files changed, 202 insertions(+)
diff --git
a/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
new file mode 100644
index ..a8751f5b1a1d
--- /dev/null
+++
b/Features/Intel/OutOfBandManagement/IpmiFeaturePkg/GenericIpmi/StandaloneMm/StandaloneMmGenericIpmi.c
@@ -0,0 +1,148 @@
+/** @file
+ Generic StandaloneMm IPMI stack driver
+
+ @copyright
+ Copyright 2023 Intel Corporation.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+//
+// Statements that include other files
+//
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "IpmiHooks.h"
+#include "IpmiBmcCommon.h"
+#include "IpmiBmc.h"
+#include
+
+IPMI_BMC_INSTANCE_DATA *mIpmiInstance;
+EFI_HANDLE mHandle;
+
+EFI_STATUS
+SmmInitializeIpmiKcsPhysicalLayer (
+ VOID
+ )
+/*++
+
+Routine Description:
+ Setup and initialize the BMC for the SMM phase. In order to verify the BMC
is functioning
+ as expected, the BMC Selftest is performed. The results are then checked
and any errors are
+ reported to the error manager. Errors are collected throughout this routine
and reported
+ just prior to installing the driver. If there are more errors than
MAX_SOFT_COUNT, then they
+ will be ignored.
+
+Arguments:
+ ImageHandle - Handle of this driver image
+ SystemTable - Table containing standard EFI services
+
+Returns:
+ EFI_SUCCESS - Successful driver initialization
+
+--*/
+{
+ EFI_STATUS Status;
+
+ DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer entry \n"));
+
+ Status = gMmst->MmAllocatePool (
+EfiRuntimeServicesData,
+sizeof (IPMI_BMC_INSTANCE_DATA),
+(VOID **));
+
+ if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_ERROR, "mIpmiInstance mem alloc failed - 0x%x\n", Status));
+return Status;
+ } else {
+
+//
+// Initialize the KCS transaction timeout. Assume delay unit is 1000 us.
+//
+mIpmiInstance->KcsTimeoutPeriod = (BMC_KCS_TIMEOUT * 1000*1000) /
KCS_DELAY_UNIT;
+
+//
+// Initialize IPMI IO Base, we still use SMS IO base to get device ID and
Seltest result since SMM IF may have different cmds supported
+//
+mIpmiInstance->IpmiIoBase = FixedPcdGet16
(PcdIpmiSmmIoBaseAddress);
+mIpmiInstance->Signature= SM_IPMI_BMC_SIGNATURE;
+mIpmiInstance->SlaveAddress = BMC_SLAVE_ADDRESS;
+mIpmiInstance->BmcStatus= BMC_NOTREADY;
+mIpmiInstance->IpmiTransport.IpmiSubmitCommand = IpmiSendCommand;
+mIpmiInstance->IpmiTransport.GetBmcStatus = IpmiGetBmcStatus;
+
+mHandle = NULL;
+Status = gMmst->MmInstallProtocolInterface (
+ ,
+ ,
+ EFI_NATIVE_INTERFACE,
+ >IpmiTransport
+ );
+ASSERT_EFI_ERROR (Status);
+
+DEBUG ((DEBUG_INFO,"SmmInitializeIpmiKcsPhysicalLayer exit \n"));
+
+return EFI_SUCCESS;
+ }
+}
+
+/**
+ The module Entry Point of driver.
+
+ @param ImageHandleThe firmware allocated handle for the EFI image.
+ @param SystemTableA pointer to the MM System Table.
+
+ @retval EFI_SUCCESSThe entry point is executed successfully.
+ @retval Other Some error occurs when executing this entry point.
+
+**/
+EFI_STATUS
+InitializeGenericIpmiStandaloneMm (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_MM_SYSTEM_TABLE*SystemTable
+ )
+{
+ SmmInitializeIpmiKcsPhysicalLayer ();
+ return EFI_SUCCESS;
+}
+
+/**
+ Unloads an image.
+
+ @param[in] ImageHandleHandle that identifies the image to be
unloaded.
+
+ @retval EFI_SUCCESS The image has been unloaded.
+ @retval EFI_INVALID_PARAMETER ImageHandle is not a valid image handle.
+
+**/
+EFI_STATUS
+EFIAPI
+GenericIpmiStandaloneMmUnload (
+ IN EFI_HANDLE ImageHandle
+ )
+{
+ EFI_STATUSStatus;
+
+ Status = EFI_SUCCESS;
+ if (mIpmiInstance != NULL) {
+if (mHandle != NULL) {
+ Status = gMmst->MmUninstallProtocolInterface (
+mHandle,
+,
+
Re: [edk2-devel] [PATCH 0/2] Add Platform Hook Lib into StandaloneMmCore
Thanks Laszlo and Michael. As I synced with Ray, the platform hook is un necessary, we can implement in another way. So, let's drop this patch. Thanks again for the kind review. BR, Wei -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110280): https://edk2.groups.io/g/devel/message/110280 Mute This Topic: https://groups.io/mt/102214566/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] Event: Tools, CI, Code base construction meeting series - Monday, October 30, 2023 #cal-reminder
*Reminder: Tools, CI, Code base construction meeting series* *When:* Monday, October 30, 2023 4:30pm to 5:30pm (UTC-07:00) America/Los Angeles *Where:* https://teams.microsoft.com/l/meetup-join/19%3ameeting_ZDI2ZDg4NmMtMjI1My00MzI5LWFmYjAtMGQyNjUzNTBjZGYw%40thread.v2/0?context=%7b%22Tid%22%3a%2272f988bf-86f1-41af-91ab-2d7cd011db47%22%2c%22Oid%22%3a%2223af6561-6e1c-450d-b917-d9d674eb3cb6%22%7d View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=2072476 ) *Description:* TianoCore community, Microsoft and Intel will be hosting a series of open meetings to discuss build, CI, tools, and other related topics. If you are interested, have ideas/opinions please join us. These meetings will be Monday 4:30pm Pacific Time on Microsoft Teams. MS Teams Link in following discussion: * https://github.com/tianocore/edk2/discussions/2614 Anyone is welcome to join. * tianocore/edk2: EDK II (github.com) * tianocore/edk2-basetools: EDK II BaseTools Python tools as a PIP module (github.com) https://github.com/tianocore/edk2-basetools * tianocore/edk2-pytool-extensions: Extensions to the edk2 build system allowing for a more robust and plugin based build system and tool execution environment (github.com) https://github.com/tianocore/edk2-pytool-extensions * tianocore/edk2-pytool-library: Python library package that supports UEFI development (github.com) https://github.com/tianocore/edk2-pytool-library MS Teams Browser Clients * https://docs.microsoft.com/en-us/microsoftteams/get-clients?tabs=Windows#browser-client -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110279): https://edk2.groups.io/g/devel/message/110279 Mute This Topic: https://groups.io/mt/102265424/21656 Mute #cal-reminder:https://edk2.groups.io/g/devel/mutehashtag/cal-reminder Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
On 10/28/23 15:23, Michael D Kinney wrote: Over the past few months, all the of the Maintainers and Reviewers listed in Maintainers.txt have been contacted to make sure Maintainers.txt accurately represents the TianoCore community members that are actively participating in their roles. Based on specific feedback, bounced emails, and no responses, updates have been made. * RISCV64: Daniel Schaefer replaced with Andrei Warkentin * ArmVirtPkg Xen has no remaining reviewers and review responsibility defaults to ArmVirtPkg Maintainers/Reviewers. * ACPI modules related to S3 has no remaining reviewers and review responsibility defaults to MdeModulePkg Maintainers/ Reviewers. * OVMF CSM modules has no remaining reviewers and review responsibility defaults to OvmfPkg Maintainers/Reviewers. * Bounce: Chan Laura * Many smaller updates removing individuals that are no longer involved or have replacement coverage. Cc: Andrew Fish Cc: Leif Lindholm Cc: Andrei Warkentin Cc: Catharine West Cc: Dandan Bi Cc: Daniel Schaefer Cc: David Woodhouse Cc: Debkumar De Cc: Eric Dong Cc: Guomin Jiang Cc: Hao A Wu Cc: James Bottomley Cc: Jian J Wang Cc: Jordan Justen Cc: Julien Grall Cc: Peter Grehan Cc: Qi Zhang Cc: Ray Han Lim Ng Cc: Stefan Berger Cc: Wenxing Hou Cc: Xiaoyu Lu Signed-off-by: Michael D Kinney Reviewed-by: Stefan Berger -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110278): https://edk2.groups.io/g/devel/message/110278 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 0/5] Cache Management Operations Support For RISC-V
On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma wrote: > > Implementing code to support Cache Management Operations (CMO) defined by > RISC-V CMO instructions.https://github.com/riscv/riscv-CMOs > This is a re-write of original series v5. > The patchset contains 5 patches- created based on V5 feedback. > 1. Restructuring of existing code and move instruction declarations into > BaseLib > 2. Renaming existing functions to denote type of instruction used to maanage > cache. >This is useful for further patches where more cache management > instructions are added. > 3. Add the new cache maintenance operations to BaseLib, including the > new assembly instruction encodings. > 4. Update BaseCacheMaintenanceLib (utilizing the new BaseLib primitives) > 5. Add platform level PCD to allow overriding of RISC-V features. > With or without nits fixed: Reviewed-by: Pedro Falcato But I would *really* prefer it if you could test this on a real board with this extension. -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110277): https://edk2.groups.io/g/devel/message/110277 Mute This Topic: https://groups.io/mt/102256459/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 3/5] MdePkg: Implement RISC-V Cache Management Operations
On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma wrote: > > Implement Cache Management Operations (CMO) defined by > RISC-V spec https://github.com/riscv/riscv-CMOs. > > Notes: > 1. CMO only supports block based Operations. Meaning cache >flush/invd/clean Operations are not available for the entire >range. In that case we fallback on fence.i instructions. > 2. Operations are implemented using Opcodes to make them compiler >independent. binutils 2.39+ compilers support CMO instructions. > > Test: > 1. Ensured correct instructions are refelecting in asm nit: reflecting > 2. Not able to verify actual instruction in HW as Qemu ignores >any actual cache operations. Do you have no way to test this in hardware? Since Rivos is a RISCV vendor and all ;) I don't like inviting the idea of merging CPU architectural changes without actually testing them in something resembling real silicon (i.e QEMU KVM is _fine_, QEMU TCG really isn't). -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110276): https://edk2.groups.io/g/devel/message/110276 Mute This Topic: https://groups.io/mt/102256466/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
On Sun, Oct 29, 2023 at 2:46 PM Dhaval Sharma wrote:
>
> Use newly defined cache management operations for RISC-V where possible
> It builds up on the support added for RISC-V cache management
> instructions in BaseLib.
> Cc: Michael D Kinney
> Cc: Liming Gao
> Cc: Zhiguang Liu
> Cc: Laszlo Ersek
>
> Signed-off-by: Dhaval Sharma
> Acked-by: Laszlo Ersek
> ---
>
> Notes:
> V7:
> - Added PcdLib
> - Restructure DEBUG message based on feedback on V6
> - Make naming consistent to CMO, remove all CBO references
> - Add ASSERT for not supported functions instead of plain debug message
> - Added RB tag
> V6:
> - Utilize cache management instructions if HW supports it
> This patch is part of restructuring on top of v5
>
> MdePkg/MdePkg.dec | 8 +
> MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf | 5 +
> MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c| 168
> +---
> MdePkg/MdePkg.uni | 4 +
> 4 files changed, 165 insertions(+), 20 deletions(-)
>
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index ac54338089e8..fa92673ff633 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64,
> PcdsPatchableInModule.AARCH64]
># @Prompt CPU Rng algorithm's GUID.
>
> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
>
> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> + #
> + # Configurability to override RISC-V CPU Features
> + # BIT 0 = Cache Management Operations. This bit is relevant only if
> + # previous stage has feature enabled and user wants to disable it.
> + #
> +
> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
> +
> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>## This value is used to set the base address of PCI express hierarchy.
># @Prompt PCI Express Base Address.
> diff --git
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> index 6fd9cbe5f6c9..601a38d6c109 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> @@ -56,3 +56,8 @@ [LibraryClasses]
>BaseLib
>DebugLib
>
> +[LibraryClasses.RISCV64]
> + PcdLib
> +
> +[Pcd.RISCV64]
> + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index 4eb18edb9aa7..5b3104afb67e 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -2,6 +2,7 @@
>RISC-V specific functionality for cache.
>
>Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights
> reserved.
> + Copyright (c) 2023, Rivos Inc. All rights reserved.
>
>SPDX-License-Identifier: BSD-2-Clause-Patent
> **/
> @@ -9,10 +10,115 @@
> #include
> #include
> #include
> +#include
> +
> +//
> +// TODO: Grab cache block size and make Cache Management Operation
> +// enabling decision based on RISC-V CPU HOB in
> +// future when it is available.
> +//
> +#define RISCV_CACHE_BLOCK_SIZE 64
> +#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1
> +
> +typedef enum {
> + Clean,
> + Flush,
> + Invld,
> +} CACHE_OP;
small nit: You may want to do something like CACHE_OP_{CLEAN, FLUSH,
INVID}. but since this is a file-local enum I don't consider this
merge-blocking.
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110275): https://edk2.groups.io/g/devel/message/110275
Mute This Topic: https://groups.io/mt/102256468/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
On Sun, Oct 29, 2023 at 1:30 PM Laszlo Ersek wrote: > > On 10/29/23 03:16, Pedro Falcato wrote: > >> diff --git a/Maintainers.txt b/Maintainers.txt > >> index 3f40cdeb5554..2b03ccbe54aa 100644 > >> --- a/Maintainers.txt > >> +++ b/Maintainers.txt > >> @@ -93,7 +93,7 @@ M: Sami Mujawar [samimujawar] > >> RISCV64 > >> F: */RiscV64/ > >> M: Sunil V L [vlsunil] > >> -R: Daniel Schaefer [JohnAZoidberg] > >> +R: Andrei Warkentin [andreiw] > >> > >> LOONGARCH64 > >> F: */LoongArch64/ > >> @@ -157,16 +157,6 @@ R: Leif Lindholm > >> [leiflindholm] > >> R: Sami Mujawar [samimujawar] > >> R: Gerd Hoffmann [kraxel] > >> > >> -ArmVirtPkg: modules used on Xen > >> -F: ArmVirtPkg/ArmVirtXen.* > >> -F: ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/ > >> -F: ArmVirtPkg/Library/XenVirtMemInfoLib/ > >> -F: ArmVirtPkg/PrePi/ > >> -F: ArmVirtPkg/XenAcpiPlatformDxe/ > >> -F: ArmVirtPkg/XenPlatformHasAcpiDtDxe/ > >> -F: ArmVirtPkg/XenioFdtDxe/ > >> -R: Julien Grall [jgrall] > > > > ArmVirtPkg Xen modules seize to have a dedicated maintainer. Can the > > generic ArmVirtPkg maintainers handle *more code* (particularly, > > functionality that's not trivial to test, unless you actively use > > Xen)? > > An alternative to removing this entire section is to replace Julien's > line with the following status line: > > S: Orphan > > The definition in Maintainers.txt is: > > Orphan: No current maintainer [but maybe you could take the > role as you write your new code]. > > I think this might be clearer for all three of: contributors, consumers, > and existent maintainers. > > - Contributors: An ArmVirtPkg maintainer may techincally merge your > code, but you won't get substantive feedback > > - Consumers: you can build and run this code, but if it breaks, you get > to keep both parts > > - Existent ArmVirtPkg maintainers: you can rest assured in the knowledge > that you are not saddled with deep technical reviews for this subsystem > that you can't even boot in your environment. You're only responsible > for the technical act of merging patches. I agree with this solution, but I do think there should be a "time limit" for orphaned code. You don't want to keep orphaned code for too long, this is not a practice we should support (which may lead to corporate code dumps where corps just dump a bunch of patches on the mailing list, fire and forget, and they're still "supported"). > > > > >> BaseTools > >> F: BaseTools/ > >> W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools > >> @@ -187,8 +177,7 @@ F: CryptoPkg/ > >> W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg > >> M: Jiewen Yao [jyao1] > >> M: Yi Li [liyi77] > >> -R: Xiaoyu Lu [xiaoyuxlu] > >> -R: Guomin Jiang [guominjia] > >> +R: Wenxing Hou [Wenxing-hou] > >> > >> DynamicTablesPkg > >> F: DynamicTablesPkg/ > >> @@ -202,7 +191,6 @@ W: > >> https://github.com/tianocore/tianocore.github.io/wiki/EmbeddedPkg > >> M: Leif Lindholm [leiflindholm] > >> M: Ard Biesheuvel [ardbiesheuvel] > >> M: Abner Chang [changab] > >> -R: Daniel Schaefer [JohnAZoidberg] > >> > >> EmulatorPkg > >> F: EmulatorPkg/ > >> @@ -228,7 +216,6 @@ F: FmpDevicePkg/ > >> W: https://github.com/tianocore/tianocore.github.io/wiki/FmpDevicePkg > >> M: Liming Gao [lgao4] > >> M: Michael D Kinney [mdkinney] > >> -R: Guomin Jiang [guominjia] > >> R: Wei6 Xu [xuweiintel] > >> > >> IntelFsp2Pkg > >> @@ -237,7 +224,6 @@ W: > >> https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg > >> M: Chasel Chiu [ChaselChiu] > >> M: Nate DeSimone [nate-desimone] > >> M: Duggapu Chinni B [cbduggap] > >> -M: Ray Han Lim Ng [rayhanlimng] > >> R: Star Zeng [lzeng14] > >> R: Ted Kuo [tedkuo1] > >> R: Ashraf Ali S [AshrafAliS] > >> @@ -258,7 +244,6 @@ R: Susovan Mohapatra > >> [susovanmohapatra] > >> MdeModulePkg > >> F: MdeModulePkg/ > >> W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg > >> -M: Jian J Wang [jwang36] > >> M: Liming Gao [lgao4] > > > > MdeModulePkg now only has a single maintainer (Liming, who also > > handles a myriad of other tasks and packages) > > This leads me to my main point: it may be time for edk2 to adopt a > leaner contribution process. > > We can insist on no patch going in without maintainer approval, but that > -- i.e., maintainer authority -- only works as long as it goes hand in > hand with maintainer responsibility: timely reviews. If the community > cannot offer enough working hours for reviewing patches for a subsystem, > then the requirements to contribute to that subsystem should be relaxed. > The other alternative is that the subsystem goes into stasis, where it > becomes effectively impossible to contribute to a subsystem. > > (NB this "relaxation of contribution rules" is entirely orthogonal to > using a mailing list vs. github pull requests. I still strongly prefer > the mailing list.) > > So maybe we could say, if there is no patch
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
On Sun, Oct 29, 2023 at 4:25 PM Yao, Jiewen wrote: > > OK. Maintainer should do code review. I have no doubt on that. > > My confusion is about "reviewer" role. What is criteria and what is > responsibility? > > Are you saying that "reviewer" just means that someone raised the hand and > said: "I want to be notified", and there is no expectation that he/she would > review the patch? > > I would like to understand more on how that works and what that means. The Linux development process, as I understand it (it may be a bit imprecise, AFAIK lots of it isn't written): Maintainers are the primary caretakers of the code. They'll review and merge patches (in linux, they usually add their own Signed-off-by, they don't do Reviewed-by). Sometimes, they'll post a patch on the mailing list, and if there's no poor feedback, they just merge it to their trees, unilaterally (for Linux, Linus usually pulls from maintainer trees, and if he doesn't like something he *will* tell you). Reviewers are people that want to be CC'd and want to review patches, but they're not expected to always do so. There's of course an expectation that reviewers are relatively competent in the area they're reviewing. There's an expectation that you will help out and participate in code review from time to time. As an example: SLAB ALLOCATOR M: Christoph Lameter M: Pekka Enberg M: David Rientjes M: Joonsoo Kim M: Andrew Morton M: Vlastimil Babka R: Roman Gushchin R: Hyeonggon Yoo <42.hye...@gmail.com> L: linux...@kvack.org S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git F: include/linux/sl?b*.h F: mm/sl?b* For a patch for mm/slab.c, you CC all M's and R's. Maintainers are the one with the 'power' to merge it, and should give you feedback. Reviewers sometimes help out (but are secondary in the patch review role), but they cannot merge patches. You only merge a patch if there's an understanding that most people and stakeholders are ok with it; for example, you may want feedback from people that are not M's and R's. If everyone is ok with your patch, a maintainer will apply it (in this case, it's vbabka's tree so he usually takes care of it), and it will eventually trickle up to Linus (who manages the 'master' git tree) who gives the final seal of approval when pulling changes. For a smaller example, we can look at EFI, which has a sole maintainer (Ard) and no reviewers; this is ok (EFI is a lot less central to the kernel than SLAB, there are a lot less patches), but stakeholders in the various changes should still test and review. This is a nice rough description of the whole development process: https://docs.kernel.org/process/2.Process.html Note that EDK2 is considerably smaller than the kernel in scope and patch volume, so it probably doesn't make much sense to be as distributed as Linux. PS: It's worth noting that the Linux equivalent to GetMaintainers.py takes into account the git blame of the files you're touching, terms you mention in the commit message; that is, it will automatically pick up people that have touched the code before or are responsible for features you're interacting with. -- Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110273): https://edk2.groups.io/g/devel/message/110273 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
This is the Wiki page where TianoCore documents the TianoCore community member roles. https://github.com/tianocore/tianocore.github.io/wiki/TianoCore-Who-we-are We can update/edit as needed to accurately reflect what all the Maintainers and Reviewers agree are their roles and responsibilities as assigned in Maintainers.txt. Thanks, Mike > -Original Message- > From: Yao, Jiewen > Sent: Sunday, October 29, 2023 9:26 AM > To: j...@linux.ibm.com; Laszlo Ersek ; > devel@edk2.groups.io; pedro.falc...@gmail.com; Kinney, Michael D > > Cc: Andrew Fish ; Leif Lindholm > ; Warkentin, Andrei > ; West, Catharine > ; Bi, Dandan ; Daniel > Schaefer ; David Woodhouse > ; De, Debkumar ; Dong, > Eric ; Jiang, Guomin ; > Wu, Hao A ; Wang, Jian J ; > Justen, Jordan L ; Julien Grall > ; Peter Grehan ; Zhang, Qi1 > ; Ng, Ray Han Lim ; > Stefan Berger ; Hou, Wenxing > ; Lu, Xiaoyu1 > Subject: RE: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on > active community members > > OK. Maintainer should do code review. I have no doubt on that. > > My confusion is about "reviewer" role. What is criteria and what is > responsibility? > > Are you saying that "reviewer" just means that someone raised the hand > and said: "I want to be notified", and there is no expectation that > he/she would review the patch? > > I would like to understand more on how that works and what that means. > Would you please give a URL for the reviewer definition in Linux > Kernel? > > Thank you > Yao, Jiewen > > > > > -Original Message- > > From: James Bottomley > > Sent: Monday, October 30, 2023 12:02 AM > > To: Yao, Jiewen ; Laszlo Ersek > ; > > devel@edk2.groups.io; pedro.falc...@gmail.com; Kinney, Michael D > > > > Cc: Andrew Fish ; Leif Lindholm > ; > > Warkentin, Andrei ; West, Catharine > > ; Bi, Dandan ; Daniel > > Schaefer ; David Woodhouse > ; > > De, Debkumar ; Dong, Eric > ; > > Jiang, Guomin ; Wu, Hao A > ; > > Wang, Jian J ; Justen, Jordan L > > ; Julien Grall ; Peter > Grehan > > ; Zhang, Qi1 ; Ng, Ray Han > Lim > > ; Stefan Berger ; > Hou, > > Wenxing ; Lu, Xiaoyu1 > > Subject: Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based > on active > > community members > > > > On Sun, 2023-10-29 at 15:42 +, Yao, Jiewen wrote: > > > > I'd say that's pretty close. A reviewer role is a request for > > > > keeping > > > > the reviewer in the loop. > > > > > > [Jiewen] I am disappointed on that. > > > To me, that is NOT a real reviewer. See below description on what > is > > > "code review". > > > https://google.github.io/eng-practices/review/ > > > https://about.gitlab.com/topics/version-control/what-is-code- > review/ > > > > Well, that's what someone's view of what a patch review should > consist > > of, not what a reviewer's role in MAINTAINERS should be. > > > > In general, you really don't want to force people to review patches, > > because you'd like a reviewer to be familiar with the area and > > comfortable with the patch. So are you saying that anyone listed as > a > > reviewer in a particular area should be capable of reviewing any > patch? > > and further that they should be expected to review every patch? > > Because that's definitely not what the R role in the Linux Kernel > would > > mean. > > > > I know that's not what happened to me in Confidential Computing, > > because I had a very specific area around SEV and SEV-ES secret > > injection and really had no familiarity at all with say the memory > > acceptance patches. > > > > > Our definition seems more like *a notification receiver*, instead > of > > > a real code reviewer. I would say, it is a very misleading > > > definition. > > > > Actually, I wouldn't, but then I'm more coming from a Linux Kernel > > background. To us, the reviewer list is simply a list of people git > > blame might not find who might have the expertise to review the > patch > > but on whom there would be no expectation that they would review the > > patch. > > > > James -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110272): https://edk2.groups.io/g/devel/message/110272 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
OK. Maintainer should do code review. I have no doubt on that. My confusion is about "reviewer" role. What is criteria and what is responsibility? Are you saying that "reviewer" just means that someone raised the hand and said: "I want to be notified", and there is no expectation that he/she would review the patch? I would like to understand more on how that works and what that means. Would you please give a URL for the reviewer definition in Linux Kernel? Thank you Yao, Jiewen > -Original Message- > From: James Bottomley > Sent: Monday, October 30, 2023 12:02 AM > To: Yao, Jiewen ; Laszlo Ersek ; > devel@edk2.groups.io; pedro.falc...@gmail.com; Kinney, Michael D > > Cc: Andrew Fish ; Leif Lindholm ; > Warkentin, Andrei ; West, Catharine > ; Bi, Dandan ; Daniel > Schaefer ; David Woodhouse ; > De, Debkumar ; Dong, Eric ; > Jiang, Guomin ; Wu, Hao A ; > Wang, Jian J ; Justen, Jordan L > ; Julien Grall ; Peter Grehan > ; Zhang, Qi1 ; Ng, Ray Han Lim > ; Stefan Berger ; Hou, > Wenxing ; Lu, Xiaoyu1 > Subject: Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active > community members > > On Sun, 2023-10-29 at 15:42 +, Yao, Jiewen wrote: > > > I'd say that's pretty close. A reviewer role is a request for > > > keeping > > > the reviewer in the loop. > > > > [Jiewen] I am disappointed on that. > > To me, that is NOT a real reviewer. See below description on what is > > "code review". > > https://google.github.io/eng-practices/review/ > > https://about.gitlab.com/topics/version-control/what-is-code-review/ > > Well, that's what someone's view of what a patch review should consist > of, not what a reviewer's role in MAINTAINERS should be. > > In general, you really don't want to force people to review patches, > because you'd like a reviewer to be familiar with the area and > comfortable with the patch. So are you saying that anyone listed as a > reviewer in a particular area should be capable of reviewing any patch? > and further that they should be expected to review every patch? > Because that's definitely not what the R role in the Linux Kernel would > mean. > > I know that's not what happened to me in Confidential Computing, > because I had a very specific area around SEV and SEV-ES secret > injection and really had no familiarity at all with say the memory > acceptance patches. > > > Our definition seems more like *a notification receiver*, instead of > > a real code reviewer. I would say, it is a very misleading > > definition. > > Actually, I wouldn't, but then I'm more coming from a Linux Kernel > background. To us, the reviewer list is simply a list of people git > blame might not find who might have the expertise to review the patch > but on whom there would be no expectation that they would review the > patch. > > James -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110271): https://edk2.groups.io/g/devel/message/110271 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
On Sun, 2023-10-29 at 15:42 +, Yao, Jiewen wrote: > > I'd say that's pretty close. A reviewer role is a request for > > keeping > > the reviewer in the loop. > > [Jiewen] I am disappointed on that. > To me, that is NOT a real reviewer. See below description on what is > "code review". > https://google.github.io/eng-practices/review/ > https://about.gitlab.com/topics/version-control/what-is-code-review/ Well, that's what someone's view of what a patch review should consist of, not what a reviewer's role in MAINTAINERS should be. In general, you really don't want to force people to review patches, because you'd like a reviewer to be familiar with the area and comfortable with the patch. So are you saying that anyone listed as a reviewer in a particular area should be capable of reviewing any patch? and further that they should be expected to review every patch? Because that's definitely not what the R role in the Linux Kernel would mean. I know that's not what happened to me in Confidential Computing, because I had a very specific area around SEV and SEV-ES secret injection and really had no familiarity at all with say the memory acceptance patches. > Our definition seems more like *a notification receiver*, instead of > a real code reviewer. I would say, it is a very misleading > definition. Actually, I wouldn't, but then I'm more coming from a Linux Kernel background. To us, the reviewer list is simply a list of people git blame might not find who might have the expertise to review the patch but on whom there would be no expectation that they would review the patch. James -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110270): https://edk2.groups.io/g/devel/message/110270 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
> I'd say that's pretty close. A reviewer role is a request for keeping > the reviewer in the loop. [Jiewen] I am disappointed on that. To me, that is NOT a real reviewer. See below description on what is "code review". https://google.github.io/eng-practices/review/ https://about.gitlab.com/topics/version-control/what-is-code-review/ Our definition seems more like *a notification receiver*, instead of a real code reviewer. I would say, it is a very misleading definition. Thank you Yao, Jiewen > -Original Message- > From: Laszlo Ersek > Sent: Sunday, October 29, 2023 9:48 PM > To: devel@edk2.groups.io; Yao, Jiewen ; > pedro.falc...@gmail.com; Kinney, Michael D > Cc: Andrew Fish ; Leif Lindholm ; > Warkentin, Andrei ; West, Catharine > ; Bi, Dandan ; Daniel > Schaefer ; David Woodhouse ; > De, Debkumar ; Dong, Eric ; > Jiang, Guomin ; Wu, Hao A ; > James Bottomley ; Wang, Jian J ; > Justen, Jordan L ; Julien Grall ; > Peter Grehan ; Zhang, Qi1 ; Ng, > Ray Han Lim ; Stefan Berger > ; Hou, Wenxing ; Lu, Xiaoyu1 > > Subject: Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active > community members > > On 10/29/23 09:05, Yao, Jiewen wrote: > > Those are great questions. I also would like to understand: > > > > 1) What is definition of "actively participating in their roles"? > > Here are the definitions of Maintainer and Reviewer, from > "Maintainers.txt": > > M: Package Maintainer: Cc address for patches and questions. Responsible > for reviewing and pushing package changes to source control. > R: Package Reviewer: Cc address for patches and questions. Reviewers help > maintainers review code, but don't have push access. A designated Package > Reviewer is reasonably familiar with the Package (or some modules > thereof), and/or provides testing or regression testing for the Package > (or some modules thereof), in certain platforms and environments. > > > Is there any enforcement or just volunteer work? > > I see the Maintainer role as a service to the community, with some > benefits granted in return. The "service" part should be clear. The > benefit is that you are kept in the loop, and sometimes (when you must) > you *can* say "no". (According to some seasoned reviewers, the one real > power of a maintainer -- not to be abused! -- is "saying no".) A > maintainer that's present helps set the focus, keep regressions out, > gives advice when needed, and so on. > > Enforcement would be nice (haha), but it never works. You can't force > people to help, especially if their dayjob instructions oppose their > upstream community responsibilities. That's fine; in such cases my > request is always: if you can't help, then at least don't get in the > way, step down. Don't block people from doing their work by having them > wait for your feedback. > > So volunteer work is fine, but as soon as the position grows "fangs" (= > a capacity to make others wait), then it becomes a promise, a > responsibility. > > > > > 2) What is role and *responsibility* of Reviewer role? Is it > > documented somewhere? > > Per my observation, some assigned reviewers have never reviewed any > > patch in history or provided valuable feedback. To me, reviewer role > > seems more like a notification instead of really review something. Is > > that our purpose? > > I'd say that's pretty close. A reviewer role is a request for keeping > the reviewer in the loop. Maintainers tend to appreciate that, because a > long-term reviewer may provide good insights, test results, and so on. > Trust is super important; a maintainer may push a patch based solely on > a reviewer's positive feedback, due to the latter's experience. > > > While Laszlo contributed a lots in Tianocore community, he is really a > > good "reviewer", although he has no such title. > > Thanks for the acknowledgement, I appreciate it! > > I don't like to hoard titles. I'm sure titles are good for one's career, > but I always see the *promise* (the responsibility) to the community, > first and foremost, that a title encapsulates. It weighs heavily on me. > I loathe disappointing people. For me, not to bear a title is better > than to bear it and not to deliver on it / not to live up to it. > > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110269): https://edk2.groups.io/g/devel/message/110269 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v6 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Replied inline. Most of the cases I have addressed in the new patch I
submitted.
On Wed, Oct 25, 2023 at 1:39 AM Pedro Falcato
wrote:
> On Sat, Oct 21, 2023 at 6:33 PM Dhaval Sharma wrote:
> >
> > Use newly defined cache management operations for RISC-V where possible
> > It builds up on the support added for RISC-V cache management
> > instructions in BaseLib.
> > Cc: Michael D Kinney
> > Cc: Liming Gao
> > Cc: Zhiguang Liu
> > Cc: Laszlo Ersek
> >
> > Signed-off-by: Dhaval Sharma
> > ---
> >
> > Notes:
> > V1:
> > - Utilize cache management instructions if HW supports it
> > This patch is part of restructuring on top of v5
> >
> > MdePkg/MdePkg.dec |
> 8 +
> > MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf |
> 2 +
> > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c|
> 159 +---
> > MdePkg/MdePkg.uni |
> 4 +
> > 4 files changed, 154 insertions(+), 19 deletions(-)
> >
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > index ac54338089e8..fa92673ff633 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64,
> PcdsPatchableInModule.AARCH64]
> ># @Prompt CPU Rng algorithm's GUID.
> >
> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
> >
> > +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
> > + #
> > + # Configurability to override RISC-V CPU Features
> > + # BIT 0 = Cache Management Operations. This bit is relevant only if
> > + # previous stage has feature enabled and user wants to disable it.
> > + #
> > +
> gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
> > +
> > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> >## This value is used to set the base address of PCI express
> hierarchy.
> ># @Prompt PCI Express Base Address.
> > diff --git
> a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > index 6fd9cbe5f6c9..39a7fb963b49 100644
> > --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
> > @@ -56,3 +56,5 @@ [LibraryClasses]
> >BaseLib
> >DebugLib
> >
> > +[Pcd.RISCV64]
> > + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES
> > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > index 4eb18edb9aa7..6851970c9e16 100644
> > --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> > @@ -1,7 +1,8 @@
> > /** @file
> > - RISC-V specific functionality for cache.
> > + Implement Risc-V Cache Management Operations
>
> Why the change? You're effectively implementing cache management for
> riscv, you're not exclusively using any sort of extension (such as
> CMO).
>
Done. I believe you meant to keep it a generic description.
>
> >
> >Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All
> rights reserved.
> > + Copyright (c) 2023, Rivos Inc. All rights reserved.
> >
> >SPDX-License-Identifier: BSD-2-Clause-Patent
> > **/
> > @@ -9,10 +10,111 @@
> > #include
> > #include
> > #include
> > +#include
> > +
> > +// TODO: This will be removed once RISC-V CPU HOB is available
> > +#define RISCV_CACHE_BLOCK_SIZE 64
> > +#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1
> > +
> > +typedef enum {
> > + Clean,
> > + Flush,
> > + Invld,
> > +} CACHE_OP;
> > +
> > +/**
> > +Verify CBOs are supported by this HW
> > +TODO: Use RISC-V CPU HOB once available.
> > +
> > +**/
> > +STATIC
> > +BOOLEAN
> > +RiscVIsCMOEnabled (
> > + VOID
> > + )
> > +{
> > + // TODO: Add check for CMO from CPU HOB.
>
> Too many TODOs? One TODO at the top of the file (mentioning feature
> detection, cache line size detection) should be enough. There's no
> point in peppering these out throughout the file :)
>
> Done.
> > + // If CMO is disabled in HW, skip Override check
> > + // Otherwise this PCD can override settings
> > + return ((PcdGet64 (PcdRiscVFeatureOverride) &
> RISCV_CPU_FEATURE_CMO_BITMASK) != 0);
> > +}
> > +
> > +/**
> > + Performs required opeartion on cache lines in the cache coherency
> domain
> > + of the calling CPU. If Address is not aligned on a cache line
> boundary,
> > + then entire cache line containing Address is operated. If Address +
> Length
> > + is not aligned on a cache line boundary, then the entire cache line
> > + containing Address + Length -1 is operated.
> > + If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
> > + @param Address The base address of the cache lines to
> >
[edk2-devel] [PATCH v7 5/5] OvmfPkg/RiscVVirt: Override for RV CPU Features
This PCD provides a way for platform to override any HW features that are default enabled by previous stages of FW (like OpenSBI). For the case where previous/prev stage has disabled the feature, this override is not useful and its usage should be avoided. Cc: Ard Biesheuvel Cc: Jiewen Yao Cc: Jordan Justen Cc: Gerd Hoffmann Cc: Sunil V L Cc: Andrei Warkentin Cc: Laszlo Ersek Signed-off-by: Dhaval Sharma Acked-by: Laszlo Ersek --- Notes: V7: - Added RB tag v6: - Modify PCD name according to changes made in Baselib implementation V5: - Introduce PCD for platform OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 1 + 1 file changed, 1 insertion(+) diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc index fe320525153f..5d66f7fe6ae6 100644 --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc @@ -203,6 +203,7 @@ [PcdsFeatureFlag] gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE [PcdsFixedAtBuild.common] + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0 gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|100 gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|100 gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0 -- 2.39.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110267): https://edk2.groups.io/g/devel/message/110267 Mute This Topic: https://groups.io/mt/102256471/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v7 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V
Use newly defined cache management operations for RISC-V where possible
It builds up on the support added for RISC-V cache management
instructions in BaseLib.
Cc: Michael D Kinney
Cc: Liming Gao
Cc: Zhiguang Liu
Cc: Laszlo Ersek
Signed-off-by: Dhaval Sharma
Acked-by: Laszlo Ersek
---
Notes:
V7:
- Added PcdLib
- Restructure DEBUG message based on feedback on V6
- Make naming consistent to CMO, remove all CBO references
- Add ASSERT for not supported functions instead of plain debug message
- Added RB tag
V6:
- Utilize cache management instructions if HW supports it
This patch is part of restructuring on top of v5
MdePkg/MdePkg.dec | 8 +
MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf | 5 +
MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c| 168
+---
MdePkg/MdePkg.uni | 4 +
4 files changed, 165 insertions(+), 20 deletions(-)
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index ac54338089e8..fa92673ff633 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
# @Prompt CPU Rng algorithm's GUID.
gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0037
+[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64]
+ #
+ # Configurability to override RISC-V CPU Features
+ # BIT 0 = Cache Management Operations. This bit is relevant only if
+ # previous stage has feature enabled and user wants to disable it.
+ #
+
gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0x|UINT64|0x69
+
[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
## This value is used to set the base address of PCI express hierarchy.
# @Prompt PCI Express Base Address.
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
index 6fd9cbe5f6c9..601a38d6c109 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
@@ -56,3 +56,8 @@ [LibraryClasses]
BaseLib
DebugLib
+[LibraryClasses.RISCV64]
+ PcdLib
+
+[Pcd.RISCV64]
+ gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index 4eb18edb9aa7..5b3104afb67e 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -2,6 +2,7 @@
RISC-V specific functionality for cache.
Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights
reserved.
+ Copyright (c) 2023, Rivos Inc. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -9,10 +10,115 @@
#include
#include
#include
+#include
+
+//
+// TODO: Grab cache block size and make Cache Management Operation
+// enabling decision based on RISC-V CPU HOB in
+// future when it is available.
+//
+#define RISCV_CACHE_BLOCK_SIZE 64
+#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1
+
+typedef enum {
+ Clean,
+ Flush,
+ Invld,
+} CACHE_OP;
+
+/**
+Verify CBOs are supported by this HW
+TODO: Use RISC-V CPU HOB once available.
+
+**/
+STATIC
+BOOLEAN
+RiscVIsCMOEnabled (
+ VOID
+ )
+{
+ // If CMO is disabled in HW, skip Override check
+ // Otherwise this PCD can override settings
+ return ((PcdGet64 (PcdRiscVFeatureOverride) & RISCV_CPU_FEATURE_CMO_BITMASK)
!= 0);
+}
+
+/**
+ Performs required opeartion on cache lines in the cache coherency domain
+ of the calling CPU. If Address is not aligned on a cache line boundary,
+ then entire cache line containing Address is operated. If Address + Length
+ is not aligned on a cache line boundary, then the entire cache line
+ containing Address + Length -1 is operated.
+ If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
+ @param Address The base address of the cache lines to
+ invalidate.
+ @param Length The number of bytes to invalidate from the instruction
+ cache.
+ @param Op Type of CMO operation to be performed
+ @return Address.
+
+**/
+STATIC
+VOID
+CacheOpCacheRange (
+ IN VOID *Address,
+ IN UINTN Length,
+ IN CACHE_OP Op
+ )
+{
+ UINTN CacheLineSize;
+ UINTN Start;
+ UINTN End;
+
+ if (Length == 0) {
+return;
+ }
+
+ if ((Op != Invld) && (Op != Flush) && (Op != Clean)) {
+return;
+ }
+
+ ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address));
+
+ CacheLineSize = RISCV_CACHE_BLOCK_SIZE;
+
+ Start = (UINTN)Address;
+ //
+ // Calculate the cache line alignment
+ //
+ End= (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1);
+ Start
[edk2-devel] [PATCH v7 3/5] MdePkg: Implement RISC-V Cache Management Operations
Implement Cache Management Operations (CMO) defined by
RISC-V spec https://github.com/riscv/riscv-CMOs.
Notes:
1. CMO only supports block based Operations. Meaning cache
flush/invd/clean Operations are not available for the entire
range. In that case we fallback on fence.i instructions.
2. Operations are implemented using Opcodes to make them compiler
independent. binutils 2.39+ compilers support CMO instructions.
Test:
1. Ensured correct instructions are refelecting in asm
2. Not able to verify actual instruction in HW as Qemu ignores
any actual cache operations.
Cc: Michael D Kinney
Cc: Liming Gao
Cc: Zhiguang Liu
Cc: Sunil V L
Cc: Daniel Schaefer
Cc: Laszlo Ersek
Signed-off-by: Dhaval Sharma
Reviewed-by: Laszlo Ersek
---
Notes:
V7:
- Modify instruction names as per feedback from V6
- Added RB
V6:
- Implement Cache management instructions in Baselib
MdePkg/Library/BaseLib/BaseLib.inf| 2 +-
MdePkg/Include/Library/BaseLib.h | 33
MdePkg/Include/RiscV64/RiscVasm.inc | 19
+++
MdePkg/Library/BaseLib/RiscV64/{FlushCache.S => RiscVCacheMgmt.S} | 17
++
4 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
b/MdePkg/Library/BaseLib/BaseLib.inf
index 03c7b02e828b..53389389448c 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -400,7 +400,7 @@ [Sources.RISCV64]
RiscV64/RiscVCpuBreakpoint.S | GCC
RiscV64/RiscVCpuPause.S | GCC
RiscV64/RiscVInterrupt.S | GCC
- RiscV64/FlushCache.S | GCC
+ RiscV64/RiscVCacheMgmt.S | GCC
RiscV64/CpuScratch.S | GCC
RiscV64/ReadTimer.S | GCC
RiscV64/RiscVMmu.S| GCC
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index d4b56a9601da..c42cc165dc82 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -226,6 +226,39 @@ RiscVInvalidateDataCacheAsmFence (
VOID
);
+/**
+ RISC-V flush cache block. Atomically perform a clean operation
+ followed by an invalidate operation
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheFlushAsmCmo (
+ IN UINTN
+ );
+
+/**
+Perform a write transfer to another cache or to memory if the
+data in the copy of the cache block have been modified by a store
+operation
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheCleanAsmCmo (
+ IN UINTN
+ );
+
+/**
+Deallocate the copy of the cache block
+
+**/
+VOID
+EFIAPI
+RiscVCpuCacheInvalAsmCmo (
+ IN UINTN
+ );
+
#endif // defined (MDE_CPU_RISCV64)
#if defined (MDE_CPU_LOONGARCH64)
diff --git a/MdePkg/Include/RiscV64/RiscVasm.inc
b/MdePkg/Include/RiscV64/RiscVasm.inc
new file mode 100644
index ..29de7358855c
--- /dev/null
+++ b/MdePkg/Include/RiscV64/RiscVasm.inc
@@ -0,0 +1,19 @@
+/*
+ *
+ * RISC-V cache operation encoding.
+ * Copyright (c) 2023, Rivos Inc. All rights reserved.
+ * SPDX-License-Identifier: BSD-2-Clause-Patent
+ *
+ */
+
+.macro RISCVCMOFLUSH
+.word 0x25200f
+.endm
+
+.macro RISCVCMOINVALIDATE
+.word 0x05200f
+.endm
+
+.macro RISCVCMOCLEAN
+.word 0x15200f
+.endm
diff --git a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
similarity index 56%
rename from MdePkg/Library/BaseLib/RiscV64/FlushCache.S
rename to MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
index e0eea0b5fb25..3c7be3229e3b 100644
--- a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
+++ b/MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S
@@ -3,10 +3,12 @@
// RISC-V cache operation.
//
// Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights
reserved.
+// Copyright (c) 2023, Rivos Inc. All rights reserved.
//
// SPDX-License-Identifier: BSD-2-Clause-Patent
//
//--
+.include "RiscVasm.inc"
.align 3
ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheAsmFence)
@@ -19,3 +21,18 @@ ASM_PFX(RiscVInvalidateInstCacheAsmFence):
ASM_PFX(RiscVInvalidateDataCacheAsmFence):
fence
ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheFlushAsmCmo)
+ASM_PFX (RiscVCpuCacheFlushAsmCmo):
+RISCVCMOFLUSH
+ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheCleanAsmCmo)
+ASM_PFX (RiscVCpuCacheCleanAsmCmo):
+RISCVCMOCLEAN
+ret
+
+ASM_GLOBAL ASM_PFX (RiscVCpuCacheInvalAsmCmo)
+ASM_PFX (RiscVCpuCacheInvalAsmCmo):
+RISCVCMOINVALIDATE
+ret
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110265): https://edk2.groups.io/g/devel/message/110265
Mute This Topic: https://groups.io/mt/102256466/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v7 2/5] MdePkg: Rename Cache Management Function To Clarify Fence Based Op
There are different ways to manage cache on RISC-V Processors.
One way is to use fence instruction. Another way is to use CPU
specific cache management operation instructions ratified as
per RISC-V ISA specifications to be introduced in future
patches. Current method is fence instruction based, rename the
function accordingly to add that clarity.
Cc: Michael D Kinney
Cc: Liming Gao
Cc: Zhiguang Liu
Cc: Sunil V L
Cc: Daniel Schaefer
Cc: Laszlo Ersek
Signed-off-by: Dhaval Sharma
Reviewed-by: Laszlo Ersek
---
Notes:
V7:
- Add RB tag
V6:
- As part of restructuring, adding cache instruction differentiation
in function naming
MdePkg/Include/Library/BaseLib.h| 4 ++--
MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 4 ++--
MdePkg/Library/BaseLib/RiscV64/FlushCache.S | 8
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 7142bbfa42f2..d4b56a9601da 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -212,7 +212,7 @@ RiscVClearPendingTimerInterrupt (
**/
VOID
EFIAPI
-RiscVInvalidateInstCacheAsm (
+RiscVInvalidateInstCacheAsmFence (
VOID
);
@@ -222,7 +222,7 @@ RiscVInvalidateInstCacheAsm (
**/
VOID
EFIAPI
-RiscVInvalidateDataCacheAsm (
+RiscVInvalidateDataCacheAsmFence (
VOID
);
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
index d5efcf49a4bf..4eb18edb9aa7 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -21,7 +21,7 @@ InvalidateInstructionCache (
VOID
)
{
- RiscVInvalidateInstCacheAsm ();
+ RiscVInvalidateInstCacheAsmFence ();
}
/**
@@ -193,7 +193,7 @@ InvalidateDataCache (
VOID
)
{
- RiscVInvalidateDataCacheAsm ();
+ RiscVInvalidateDataCacheAsmFence ();
}
/**
diff --git a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
b/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
index 7c10fdd268af..e0eea0b5fb25 100644
--- a/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
+++ b/MdePkg/Library/BaseLib/RiscV64/FlushCache.S
@@ -9,13 +9,13 @@
//--
.align 3
-ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheAsm)
-ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsm)
+ASM_GLOBAL ASM_PFX(RiscVInvalidateInstCacheAsmFence)
+ASM_GLOBAL ASM_PFX(RiscVInvalidateDataCacheAsmFence)
-ASM_PFX(RiscVInvalidateInstCacheAsm):
+ASM_PFX(RiscVInvalidateInstCacheAsmFence):
fence.i
ret
-ASM_PFX(RiscVInvalidateDataCacheAsm):
+ASM_PFX(RiscVInvalidateDataCacheAsmFence):
fence
ret
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110264): https://edk2.groups.io/g/devel/message/110264
Mute This Topic: https://groups.io/mt/102256464/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v7 1/5] MdePkg: Move RISC-V Cache Management Declarations Into BaseLib
The declarations for cache Management functions belong to BaseLib instead of instance source file. This helps with further restructuring of cache management code for RISC-V. Cc: Michael D Kinney Cc: Liming Gao Cc: Zhiguang Liu Cc: Laszlo Ersek Signed-off-by: Dhaval Sharma Reviewed-by: Laszlo Ersek --- Notes: V7: - Added RB tag V6: - Move cache management function declaration in baselib where it belongs MdePkg/Include/Library/BaseLib.h| 20 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 20 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h index 5d7067ee854e..7142bbfa42f2 100644 --- a/MdePkg/Include/Library/BaseLib.h +++ b/MdePkg/Include/Library/BaseLib.h @@ -206,6 +206,26 @@ RiscVClearPendingTimerInterrupt ( VOID ); +/** + RISC-V invalidate instruction cache. + +**/ +VOID +EFIAPI +RiscVInvalidateInstCacheAsm ( + VOID + ); + +/** + RISC-V invalidate data cache. + +**/ +VOID +EFIAPI +RiscVInvalidateDataCacheAsm ( + VOID + ); + #endif // defined (MDE_CPU_RISCV64) #if defined (MDE_CPU_LOONGARCH64) diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c index d08fb9f193ca..d5efcf49a4bf 100644 --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c @@ -10,26 +10,6 @@ #include #include -/** - RISC-V invalidate instruction cache. - -**/ -VOID -EFIAPI -RiscVInvalidateInstCacheAsm ( - VOID - ); - -/** - RISC-V invalidate data cache. - -**/ -VOID -EFIAPI -RiscVInvalidateDataCacheAsm ( - VOID - ); - /** Invalidates the entire instruction cache in cache coherency domain of the calling CPU. -- 2.39.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110263): https://edk2.groups.io/g/devel/message/110263 Mute This Topic: https://groups.io/mt/102256462/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v7 0/5] Cache Management Operations Support For RISC-V
Implementing code to support Cache Management Operations (CMO) defined by RISC-V CMO instructions.https://github.com/riscv/riscv-CMOs This is a re-write of original series v5. The patchset contains 5 patches- created based on V5 feedback. 1. Restructuring of existing code and move instruction declarations into BaseLib 2. Renaming existing functions to denote type of instruction used to maanage cache. This is useful for further patches where more cache management instructions are added. 3. Add the new cache maintenance operations to BaseLib, including the new assembly instruction encodings. 4. Update BaseCacheMaintenanceLib (utilizing the new BaseLib primitives) 5. Add platform level PCD to allow overriding of RISC-V features. Cc: Ard Biesheuvel Cc: Jiewen Yao Cc: Jordan Justen Cc: Gerd Hoffmann Cc: Sunil V L Cc: Andrei Warkentin Cc: Laszlo Ersek Cc: Michael D Kinney Cc: Liming Gao Cc: Zhiguang Liu Cc: Daniel Schaefer Dhaval (5): MdePkg: Move RISC-V Cache Management Declarations Into BaseLib MdePkg: Rename Cache Management Function To Clarify Fence Based Op MdePkg: Implement RISC-V Cache Management Operations MdePkg: Utilize Cache Management Operations Implementation For RISC-V OvmfPkg/RiscVVirt: Override for RV CPU Features MdePkg/MdePkg.dec | 8 + OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc| 1 + MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf | 5 + MdePkg/Library/BaseLib/BaseLib.inf | 2 +- MdePkg/Include/Library/BaseLib.h | 53 ++ MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c| 172 MdePkg/Include/RiscV64/RiscVasm.inc| 19 +++ MdePkg/Library/BaseLib/RiscV64/FlushCache.S| 21 --- MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S| 38 + MdePkg/MdePkg.uni | 4 + 10 files changed, 269 insertions(+), 54 deletions(-) create mode 100644 MdePkg/Include/RiscV64/RiscVasm.inc delete mode 100644 MdePkg/Library/BaseLib/RiscV64/FlushCache.S create mode 100644 MdePkg/Library/BaseLib/RiscV64/RiscVCacheMgmt.S -- 2.39.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110262): https://edk2.groups.io/g/devel/message/110262 Mute This Topic: https://groups.io/mt/102256459/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
On 10/29/23 14:48, Laszlo Ersek wrote: > I loathe disappointing people. English is a tricky language. The above landed as an (unintended) double-entendre. I meant: "I hate to disappoint people". I didn't mean: "I loathe people that are disappointing". Ouch. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110261): https://edk2.groups.io/g/devel/message/110261 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] RedfishPkg/RedfishCrtLib: remove multiple definitions.
On Wed, Oct 25, 2023 at 3:40 PM Nickle Wang wrote: > > > double declaration of 'strcpy' is still there. > > Thanks for catching this, Mike. Version 2 patch file was sent. > > Regards, > Nickle Hello, Nickle v2 is good enough, but it can be improved a bit. Since the definitions in this header file have become clearer and simpler, It now appears that memcmp and memmove declarations can also be removed. The logic is that we don't need to declare function prototypes for those that are overridden at the bottom of this header file. If there were such functions in the code, the linking process would fail, but there should not be such functions, since their names are replaced by the preprocessor according to the definitions. Regards, Mike. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110260): https://edk2.groups.io/g/devel/message/110260 Mute This Topic: https://groups.io/mt/102136148/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
On 10/29/23 09:05, Yao, Jiewen wrote: > Those are great questions. I also would like to understand: > > 1) What is definition of "actively participating in their roles"? Here are the definitions of Maintainer and Reviewer, from "Maintainers.txt": M: Package Maintainer: Cc address for patches and questions. Responsible for reviewing and pushing package changes to source control. R: Package Reviewer: Cc address for patches and questions. Reviewers help maintainers review code, but don't have push access. A designated Package Reviewer is reasonably familiar with the Package (or some modules thereof), and/or provides testing or regression testing for the Package (or some modules thereof), in certain platforms and environments. > Is there any enforcement or just volunteer work? I see the Maintainer role as a service to the community, with some benefits granted in return. The "service" part should be clear. The benefit is that you are kept in the loop, and sometimes (when you must) you *can* say "no". (According to some seasoned reviewers, the one real power of a maintainer -- not to be abused! -- is "saying no".) A maintainer that's present helps set the focus, keep regressions out, gives advice when needed, and so on. Enforcement would be nice (haha), but it never works. You can't force people to help, especially if their dayjob instructions oppose their upstream community responsibilities. That's fine; in such cases my request is always: if you can't help, then at least don't get in the way, step down. Don't block people from doing their work by having them wait for your feedback. So volunteer work is fine, but as soon as the position grows "fangs" (= a capacity to make others wait), then it becomes a promise, a responsibility. > > 2) What is role and *responsibility* of Reviewer role? Is it > documented somewhere? > Per my observation, some assigned reviewers have never reviewed any > patch in history or provided valuable feedback. To me, reviewer role > seems more like a notification instead of really review something. Is > that our purpose? I'd say that's pretty close. A reviewer role is a request for keeping the reviewer in the loop. Maintainers tend to appreciate that, because a long-term reviewer may provide good insights, test results, and so on. Trust is super important; a maintainer may push a patch based solely on a reviewer's positive feedback, due to the latter's experience. > While Laszlo contributed a lots in Tianocore community, he is really a > good "reviewer", although he has no such title. Thanks for the acknowledgement, I appreciate it! I don't like to hoard titles. I'm sure titles are good for one's career, but I always see the *promise* (the responsibility) to the community, first and foremost, that a title encapsulates. It weighs heavily on me. I loathe disappointing people. For me, not to bear a title is better than to bear it and not to deliver on it / not to live up to it. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110259): https://edk2.groups.io/g/devel/message/110259 Mute This Topic: https://groups.io/mt/102245264/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
On 10/29/23 03:16, Pedro Falcato wrote: > On Sat, Oct 28, 2023 at 8:23 PM Michael D Kinney > wrote: >> >> Over the past few months, all the of the Maintainers and >> Reviewers listed in Maintainers.txt have been contacted to make >> sure Maintainers.txt accurately represents the TianoCore >> community members that are actively participating in their >> roles. Based on specific feedback, bounced emails, and no >> responses, updates have been made. >> >> * RISCV64: Daniel Schaefer replaced with Andrei Warkentin >> * ArmVirtPkg Xen has no remaining reviewers and review >> responsibility defaults to ArmVirtPkg Maintainers/Reviewers. >> * ACPI modules related to S3 has no remaining reviewers and >> review responsibility defaults to MdeModulePkg Maintainers/ >> Reviewers. >> * OVMF CSM modules has no remaining reviewers and review >> responsibility defaults to OvmfPkg Maintainers/Reviewers. >> * Bounce: Chan Laura >> * Many smaller updates removing individuals that are no >> longer involved or have replacement coverage. > > Mike, > > Thank you so much for doing this thankless task. Some comments: > >> diff --git a/Maintainers.txt b/Maintainers.txt >> index 3f40cdeb5554..2b03ccbe54aa 100644 >> --- a/Maintainers.txt >> +++ b/Maintainers.txt >> @@ -93,7 +93,7 @@ M: Sami Mujawar [samimujawar] >> RISCV64 >> F: */RiscV64/ >> M: Sunil V L [vlsunil] >> -R: Daniel Schaefer [JohnAZoidberg] >> +R: Andrei Warkentin [andreiw] >> >> LOONGARCH64 >> F: */LoongArch64/ >> @@ -157,16 +157,6 @@ R: Leif Lindholm >> [leiflindholm] >> R: Sami Mujawar [samimujawar] >> R: Gerd Hoffmann [kraxel] >> >> -ArmVirtPkg: modules used on Xen >> -F: ArmVirtPkg/ArmVirtXen.* >> -F: ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/ >> -F: ArmVirtPkg/Library/XenVirtMemInfoLib/ >> -F: ArmVirtPkg/PrePi/ >> -F: ArmVirtPkg/XenAcpiPlatformDxe/ >> -F: ArmVirtPkg/XenPlatformHasAcpiDtDxe/ >> -F: ArmVirtPkg/XenioFdtDxe/ >> -R: Julien Grall [jgrall] > > ArmVirtPkg Xen modules seize to have a dedicated maintainer. Can the > generic ArmVirtPkg maintainers handle *more code* (particularly, > functionality that's not trivial to test, unless you actively use > Xen)? An alternative to removing this entire section is to replace Julien's line with the following status line: S: Orphan The definition in Maintainers.txt is: Orphan: No current maintainer [but maybe you could take the role as you write your new code]. I think this might be clearer for all three of: contributors, consumers, and existent maintainers. - Contributors: An ArmVirtPkg maintainer may techincally merge your code, but you won't get substantive feedback - Consumers: you can build and run this code, but if it breaks, you get to keep both parts - Existent ArmVirtPkg maintainers: you can rest assured in the knowledge that you are not saddled with deep technical reviews for this subsystem that you can't even boot in your environment. You're only responsible for the technical act of merging patches. > >> BaseTools >> F: BaseTools/ >> W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools >> @@ -187,8 +177,7 @@ F: CryptoPkg/ >> W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg >> M: Jiewen Yao [jyao1] >> M: Yi Li [liyi77] >> -R: Xiaoyu Lu [xiaoyuxlu] >> -R: Guomin Jiang [guominjia] >> +R: Wenxing Hou [Wenxing-hou] >> >> DynamicTablesPkg >> F: DynamicTablesPkg/ >> @@ -202,7 +191,6 @@ W: >> https://github.com/tianocore/tianocore.github.io/wiki/EmbeddedPkg >> M: Leif Lindholm [leiflindholm] >> M: Ard Biesheuvel [ardbiesheuvel] >> M: Abner Chang [changab] >> -R: Daniel Schaefer [JohnAZoidberg] >> >> EmulatorPkg >> F: EmulatorPkg/ >> @@ -228,7 +216,6 @@ F: FmpDevicePkg/ >> W: https://github.com/tianocore/tianocore.github.io/wiki/FmpDevicePkg >> M: Liming Gao [lgao4] >> M: Michael D Kinney [mdkinney] >> -R: Guomin Jiang [guominjia] >> R: Wei6 Xu [xuweiintel] >> >> IntelFsp2Pkg >> @@ -237,7 +224,6 @@ W: >> https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg >> M: Chasel Chiu [ChaselChiu] >> M: Nate DeSimone [nate-desimone] >> M: Duggapu Chinni B [cbduggap] >> -M: Ray Han Lim Ng [rayhanlimng] >> R: Star Zeng [lzeng14] >> R: Ted Kuo [tedkuo1] >> R: Ashraf Ali S [AshrafAliS] >> @@ -258,7 +244,6 @@ R: Susovan Mohapatra >> [susovanmohapatra] >> MdeModulePkg >> F: MdeModulePkg/ >> W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg >> -M: Jian J Wang [jwang36] >> M: Liming Gao [lgao4] > > MdeModulePkg now only has a single maintainer (Liming, who also > handles a myriad of other tasks and packages) This leads me to my main point: it may be time for edk2 to adopt a leaner contribution process. We can insist on no patch going in without maintainer approval, but that -- i.e., maintainer authority -- only works as long as it goes hand in hand with maintainer responsibility: timely reviews. If the community cannot offer enough working
Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active community members
Those are great questions. I also would like to understand: 1) What is definition of "actively participating in their roles"? Is there any enforcement or just volunteer work? 2) What is role and *responsibility* of Reviewer role? Is it documented somewhere? Per my observation, some assigned reviewers have never reviewed any patch in history or provided valuable feedback. To me, reviewer role seems more like a notification instead of really review something. Is that our purpose? While Laszlo contributed a lots in Tianocore community, he is really a good "reviewer", although he has no such title. Thank you Yao, Jiewen > -Original Message- > From: devel@edk2.groups.io On Behalf Of Pedro Falcato > Sent: Sunday, October 29, 2023 10:17 AM > To: devel@edk2.groups.io; Kinney, Michael D > Cc: Andrew Fish ; Leif Lindholm ; > Warkentin, Andrei ; West, Catharine > ; Bi, Dandan ; Daniel > Schaefer ; David Woodhouse ; > De, Debkumar ; Dong, Eric ; > Jiang, Guomin ; Wu, Hao A ; > James Bottomley ; Wang, Jian J ; > Justen, Jordan L ; Julien Grall ; > Peter Grehan ; Zhang, Qi1 ; Ng, > Ray Han Lim ; Stefan Berger > ; Hou, Wenxing ; Lu, Xiaoyu1 > > Subject: Re: [edk2-devel] [Patch 1/1] Maintainers.txt: Update based on active > community members > > On Sat, Oct 28, 2023 at 8:23 PM Michael D Kinney > wrote: > > > > Over the past few months, all the of the Maintainers and > > Reviewers listed in Maintainers.txt have been contacted to make > > sure Maintainers.txt accurately represents the TianoCore > > community members that are actively participating in their > > roles. Based on specific feedback, bounced emails, and no > > responses, updates have been made. > > > > * RISCV64: Daniel Schaefer replaced with Andrei Warkentin > > * ArmVirtPkg Xen has no remaining reviewers and review > > responsibility defaults to ArmVirtPkg Maintainers/Reviewers. > > * ACPI modules related to S3 has no remaining reviewers and > > review responsibility defaults to MdeModulePkg Maintainers/ > > Reviewers. > > * OVMF CSM modules has no remaining reviewers and review > > responsibility defaults to OvmfPkg Maintainers/Reviewers. > > * Bounce: Chan Laura > > * Many smaller updates removing individuals that are no > > longer involved or have replacement coverage. > > Mike, > > Thank you so much for doing this thankless task. Some comments: > > > diff --git a/Maintainers.txt b/Maintainers.txt > > index 3f40cdeb5554..2b03ccbe54aa 100644 > > --- a/Maintainers.txt > > +++ b/Maintainers.txt > > @@ -93,7 +93,7 @@ M: Sami Mujawar > [samimujawar] > > RISCV64 > > F: */RiscV64/ > > M: Sunil V L [vlsunil] > > -R: Daniel Schaefer [JohnAZoidberg] > > +R: Andrei Warkentin [andreiw] > > > > LOONGARCH64 > > F: */LoongArch64/ > > @@ -157,16 +157,6 @@ R: Leif Lindholm > [leiflindholm] > > R: Sami Mujawar [samimujawar] > > R: Gerd Hoffmann [kraxel] > > > > -ArmVirtPkg: modules used on Xen > > -F: ArmVirtPkg/ArmVirtXen.* > > -F: ArmVirtPkg/Library/XenArmGenericTimerVirtCounterLib/ > > -F: ArmVirtPkg/Library/XenVirtMemInfoLib/ > > -F: ArmVirtPkg/PrePi/ > > -F: ArmVirtPkg/XenAcpiPlatformDxe/ > > -F: ArmVirtPkg/XenPlatformHasAcpiDtDxe/ > > -F: ArmVirtPkg/XenioFdtDxe/ > > -R: Julien Grall [jgrall] > > ArmVirtPkg Xen modules seize to have a dedicated maintainer. Can the > generic ArmVirtPkg maintainers handle *more code* (particularly, > functionality that's not trivial to test, unless you actively use > Xen)? > > > BaseTools > > F: BaseTools/ > > W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools > > @@ -187,8 +177,7 @@ F: CryptoPkg/ > > W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg > > M: Jiewen Yao [jyao1] > > M: Yi Li [liyi77] > > -R: Xiaoyu Lu [xiaoyuxlu] > > -R: Guomin Jiang [guominjia] > > +R: Wenxing Hou [Wenxing-hou] > > > > DynamicTablesPkg > > F: DynamicTablesPkg/ > > @@ -202,7 +191,6 @@ W: > https://github.com/tianocore/tianocore.github.io/wiki/EmbeddedPkg > > M: Leif Lindholm [leiflindholm] > > M: Ard Biesheuvel [ardbiesheuvel] > > M: Abner Chang [changab] > > -R: Daniel Schaefer [JohnAZoidberg] > > > > EmulatorPkg > > F: EmulatorPkg/ > > @@ -228,7 +216,6 @@ F: FmpDevicePkg/ > > W: https://github.com/tianocore/tianocore.github.io/wiki/FmpDevicePkg > > M: Liming Gao [lgao4] > > M: Michael D Kinney [mdkinney] > > -R: Guomin Jiang [guominjia] > > R: Wei6 Xu [xuweiintel] > > > > IntelFsp2Pkg > > @@ -237,7 +224,6 @@ W: > https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg > > M: Chasel Chiu [ChaselChiu] > > M: Nate DeSimone [nate-desimone] > > M: Duggapu Chinni B [cbduggap] > > -M: Ray Han Lim Ng [rayhanlimng] > > R: Star Zeng [lzeng14] > > R: Ted Kuo [tedkuo1] > > R: Ashraf Ali S [AshrafAliS] > > @@ -258,7 +244,6 @@ R: Susovan Mohapatra > [susovanmohapatra] > > MdeModulePkg > > F: MdeModulePkg/ > > W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg > > -M: Jian J Wang [jwang36] > > M: Liming Gao
