Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging

["Ni, Ray" <ray.ni@intel.com>]

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 <lersek@redhat.com>; devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming <liming.gao@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.kinney@intel.com>
> 
> Thanks,
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Wednesday, July 10, 2019 10:17 AM
> > To: devel@edk2.groups.io; Dong, Eric
> > <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming
> > <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@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.dong@intel.com>
> > >
> > >> -----Original Message-----
> > >> From: Ni, Ray
> > >> Sent: Wednesday, July 3, 2019 2:54 PM
> > >> To: devel@edk2.groups.io
> > >> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> > >> <lersek@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.ni@intel.com>
> > >> Cc: Eric Dong <eric.dong@intel.com>
> > >> Regression-tested-by: Laszlo Ersek <lersek@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