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

["Laszlo Ersek" <lersek@redhat.com>]

On 07/10/19 21:38, Kinney, Michael D wrote:
> 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.

Should we ask Ray & Eric to submit a suitable patch for Maintainers.txt
(MdePkg) right now, or should we wait until Leif's & Hao's work on the
"wildcard path association" feature (for Maintainers.txt) completes?

> 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!
Laszlo

> 
>> -----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