Laszlo, Mike, Sorry I did violate the process. I had two assumptions which led me violate the process: 1. Reviewed-by from UefiCpuPkg maintainers on this IA32_CR4 change is more important than that from MdePkg maintainers. In another word, I thought if UefiCpuPkg maintainers agree with this change, MdePkg maintainers should have no concerns. (It's a wrong assumption. MdePkg maintainers may have some general suggestions, e.g.: name, location, comments and etc..) 2. This change is directly from the published white paper and there is no other option regarding this IA32_CR4 change. (It's a wrong assumption. MdePkg maintainers may have some general suggestions, e.g.: name, location, comments and etc..)
I agree I should get Reviewed-by tag from MdePkg maintainers. My assumptions are wrong. To strictly follow the process, I will: 1. Post a patch series to revert the 3 patches. Since this change doesn't break any functionality (does break the process), I will wait for Reviewed-by from each package maintainer and make sure push the patches after 24h. 2. After step #1, post a new patch series to add the 3 patches back with Reviewed-by tag from Eric and Mike, Regression-Tested-by from you. Do you think it follows the existing process? Sorry again for this violation. Thanks, Ray > -----Original Message----- > From: Kinney, Michael D > Sent: Thursday, July 11, 2019 3:39 AM > To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io; Dong, Eric > <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; > Kinney, Michael D <michael.d.kin...@intel.com> > Cc: Leif Lindholm <leif.lindh...@linaro.org>; Gao, Liming > <liming....@intel.com> > Subject: RE: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 > structure for 5-level paging > > Laszlo, > > I agree with your feedback. Process must be followed. > > I also agree that it may make sense to add some more maintainers > to the MdePkg, especially for some of the content in MdePkg that > is closely related to the UefiCpuPkg content. > > I have reviewed this patch to the BaseLib.h. The new LA57 bit > added to IA32_CR4 matches the documentation in the white paper > referenced in the series. > > Reviewed-by: Michael D Kinney <michael.d.kin...@intel.com> > > Thanks, > > Mike > > > > -----Original Message----- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Wednesday, July 10, 2019 10:17 AM > > To: devel@edk2.groups.io; Dong, Eric > > <eric.d...@intel.com>; Ni, Ray <ray...@intel.com> > > Cc: Leif Lindholm <leif.lindh...@linaro.org>; Gao, Liming > > <liming....@intel.com>; Kinney, Michael D > > <michael.d.kin...@intel.com> > > Subject: Re: [edk2-devel] [PATCH v2 2/3] > > MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level > > paging > > > > Ray, Eric, > > > > (+Liming, +Mike, +Leif) > > > > On 07/09/19 03:04, Dong, Eric wrote: > > > Reviewed-by: Eric Dong <eric.d...@intel.com> > > > > > >> -----Original Message----- > > >> From: Ni, Ray > > >> Sent: Wednesday, July 3, 2019 2:54 PM > > >> To: devel@edk2.groups.io > > >> Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek > > >> <ler...@redhat.com> > > >> Subject: [PATCH v2 2/3] MdePkg/BaseLib.h: Update > > IA32_CR4 structure > > >> for 5-level paging > > >> > > >> 5-level paging is documented in white paper: > > >> > > https://software.intel.com/sites/default/files/managed/2b > > /80/5- > > >> level_paging_white_paper.pdf > > >> > > >> Commit f8113e25001e715390127f23e2197252cbd6d1a2 > > >> changed Cpuid.h already. > > >> > > >> This patch updates IA32_CR4 structure to include LA57 > > field. > > >> > > >> Signed-off-by: Ray Ni <ray...@intel.com> > > >> Cc: Eric Dong <eric.d...@intel.com> > > >> Regression-tested-by: Laszlo Ersek <ler...@redhat.com> > > >> --- > > >> MdePkg/Include/Library/BaseLib.h | 3 ++- > > >> 1 file changed, 2 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/MdePkg/Include/Library/BaseLib.h > > >> b/MdePkg/Include/Library/BaseLib.h > > >> index ebd7dd274c..a22bfc9fad 100644 > > >> --- a/MdePkg/Include/Library/BaseLib.h > > >> +++ b/MdePkg/Include/Library/BaseLib.h > > >> @@ -5324,7 +5324,8 @@ typedef union { > > >> UINT32 OSXMMEXCPT:1; ///< Operating System > > Support for > > >> ///< Unmasked SIMD > > Floating Point > > >> ///< Exceptions. > > >> - UINT32 Reserved_0:2; ///< Reserved. > > >> + UINT32 Reserved_2:1; ///< Reserved. > > >> + UINT32 LA57:1; ///< Linear Address > > 57bit. > > >> UINT32 VMXE:1; ///< VMX Enable > > >> UINT32 Reserved_1:18; ///< Reserved. > > >> } Bits; > > > > I'm sorry but you will have to revert this patch series > > immediately. > > None of the MdePkg maintainers have approved this patch - > > - commit 7c5010c7f88b. > > > > In the first place, Mike and Liming were never CC'd on > > the patch, so they may not have noticed it, even. > > > > The situation is very similar to the recent SM3 crypto > > series that I had to revert myself. An MdePkg patch was > > pushed without package owner review. > > > > Can you guys please revert this series immediately, > > without me having to do it? > > > > > > If we think that MdePkg should have more "M" folks, in > > order to distribute the review load better, then we > > should address that problem first. Ignoring rules just > > because that's more convenient is not acceptable. > > > > Thanks, > > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43551): https://edk2.groups.io/g/devel/message/43551 Mute This Topic: https://groups.io/mt/32295048/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-