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

Ni, Ray posted 3 patches 6 years, 7 months ago
[edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Ni, Ray 6 years, 7 months ago
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;
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43200): https://edk2.groups.io/g/devel/message/43200
Mute This Topic: https://groups.io/mt/32295048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Dong, Eric 6 years, 7 months ago
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;
> --
> 2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43412): https://edk2.groups.io/g/devel/message/43412
Mute This Topic: https://groups.io/mt/32295048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Laszlo Ersek 6 years, 7 months ago
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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43490): https://edk2.groups.io/g/devel/message/43490
Mute This Topic: https://groups.io/mt/32295048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Michael D Kinney 6 years, 7 months ago
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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43524): https://edk2.groups.io/g/devel/message/43524
Mute This Topic: https://groups.io/mt/32295048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Ni, Ray 6 years, 7 months ago
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

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Ni, Ray 6 years, 7 months ago
Laszlo, Mike,

An update to my revert proposal, I will only revert the below 2 patches:

7c5010c7f88b790f4524c4a5311819e3af5e2752
* MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging

7365eb2c8cf1d7112330d09918c0c67e8d0b827a
* UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports


Will keep the below patch in github.
7e56f8928d8461d820a81a50908adf648279f1dc
* UefiCpuPkg/PiSmmCpu: Change variable names and comments to follow SDM

Is it good to you?
Or the revert process needs to revert whole patch series?

Can you or someone else guide me where the revert process is documented?

Thanks,
Ray

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, July 11, 2019 11:25 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; devel@edk2.groups.io; Dong, Eric
> <eric.dong@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, 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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43553): https://edk2.groups.io/g/devel/message/43553
Mute This Topic: https://groups.io/mt/32295048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Laszlo Ersek 6 years, 7 months ago
On 07/11/19 06:05, Ni, Ray wrote:
> Laszlo, Mike,
> 
> An update to my revert proposal, I will only revert the below 2 patches:
> 
> 7c5010c7f88b790f4524c4a5311819e3af5e2752
> * MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
> 
> 7365eb2c8cf1d7112330d09918c0c67e8d0b827a
> * UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports
> 
> 
> Will keep the below patch in github.
> 7e56f8928d8461d820a81a50908adf648279f1dc
> * UefiCpuPkg/PiSmmCpu: Change variable names and comments to follow SDM
> 
> Is it good to you?
> Or the revert process needs to revert whole patch series?
> 
> Can you or someone else guide me where the revert process is documented?

(1) In general, reverts are always applied in the reverse order of the
original series.

(2) It's fine to revert a subset of the patches if, and only if:

- at any stage during the revert series, the tree builds and works

- at the end of the revert series, all previous commits that have *not*
been reverted contain the required maintainer approval tags.

(3) So, in the present case, you should first revert 7365eb2c8cf1
("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports",
2019-07-10). This is because the code introduced in said commit would
not build without the MdePkg/BaseLib.h changes.

Then, second, you should revert 7c5010c7f88b ("MdePkg/BaseLib.h: Update
IA32_CR4 structure for 5-level paging", 2019-07-10).

Please use the "git revert" command. When reverting 7365eb2c8cf1, state
that the reason for the revert is maintaining proper dependencies
(buildability). When reverting 7c5010c7f88b, please state in the commit
message that the reason for the revert is incorrect patch review workflow.

Finally (now with Mike's R-b available on the list), you can reapply the
MdePkg and UefiCpuPkg patches (in that order); just make sure that you
add Mike's R-b to the reapplied MdePkg patch.

(4) Summary:

$ git revert 7365eb2c8cf1
--> reason to state in the commit message: maintain buildability
--> add your Signed-off-by

$ git revert 7c5010c7f88b
--> reason to state in the commit message: lacking patch review process

$ git cherry-pick -x -e 7c5010c7f88b
--> append Mike's R-b to the commit message

$ git cherry-pick -x 7365eb2c8cf1
--> commit message will not be edited

$ git push

Thanks,
Laszlo



>> -----Original Message-----
>> From: Ni, Ray
>> Sent: Thursday, July 11, 2019 11:25 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek
>> <lersek@redhat.com>; devel@edk2.groups.io; Dong, Eric
>> <eric.dong@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, 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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43599): https://edk2.groups.io/g/devel/message/43599
Mute This Topic: https://groups.io/mt/32295048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Laszlo Ersek 6 years, 7 months ago
On 07/11/19 05:25, Ni, Ray wrote:
> 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..)

Both of these assumptions could make perfect sense *to you*, but the
rules exist to uphold a single standard for everyone. I didn't expect
Mike or Liming to find an issue in the "MdePkg/BaseLib.h" patch. I did
expect the process to be followed.

I fully support if you and Eric are added to MdePkg as co-maintainers,
for content that is closely related to UefiCpuPkg. Then, the assumptions
will be codified, and they will be clear to everyone.

To explain "single standard" a bit more: sometimes I too submit code for
MdePkg. (Right now, I happen to have a series pending review.) If I were
cynical, my thinking could go, "well, if other folks can push to MdePkg
without MdePkg maintainer approval, so can I".

Do you see my problem? Where does it end?

A maintainer A-b or R-b on the list is objective; it's a fact.

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

Yes, this can work, but I also don't mind if you use git-revert +
git-cherry-pick + git-push in this instance (as explained elsewhere), in
this particular instance. This is because no further review on the list
is necessary (in this particular case), the reverts and reapplications
are fully mechanical, you just need to make sure to pick up Mike's R-b
for the reapplied MdePkg patch (and to keep the tree buildable at every
stage). See again my other email with the detailed steps.

> Sorry again for this violation.

To emphasize -- I wasn't worried that the patch would cause breakage. My
point is that all contributors should be held to the same standard.

Thanks
Laszlo


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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43604): https://edk2.groups.io/g/devel/message/43604
Mute This Topic: https://groups.io/mt/32295048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/3] MdePkg/BaseLib.h: Update IA32_CR4 structure for 5-level paging
Posted by Laszlo Ersek 6 years, 7 months ago
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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43602): https://edk2.groups.io/g/devel/message/43602
Mute This Topic: https://groups.io/mt/32295048/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-