[v7] Fix multiple entries of RT_CODE in memory map

[edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Posted by Jian J Wang 7 years, 1 month ago

> v7:
>   Merge memory map after filtering paging attributes

More than one entry of RT_CODE memory might cause boot problem for some
old OSs. This patch will fix this issue to keep OS compatibility as much
as possible.

Jian J Wang (2):
  MdeModulePkg/DxeCore: Filter out all paging capabilities
  UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

 MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
 MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
 UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++-------
 4 files changed, 112 insertions(+), 22 deletions(-)

-- 
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Posted by Laszlo Ersek 7 years, 1 month ago

Jian,

On 11/21/17 07:17, Jian J Wang wrote:
>> v7:
>>   Merge memory map after filtering paging attributes
> 
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much
> as possible.
> 
> Jian J Wang (2):
>   MdeModulePkg/DxeCore: Filter out all paging capabilities
>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> 
>  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
>  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
>  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++-------
>  4 files changed, 112 insertions(+), 22 deletions(-)
> 

I don't have capacity to retest and re-review the series.

Considering the following two options, I like none of them:

(1) Version 7 is merged with my feedback tags from v6. I don't like this
because I didn't review or test version 7.

(2) Version 7 is merged without my feedback tags. I don't like this
because I've put a lot of BZ writeup, and patch review and testing
effort for this series, and I'd like the commit log to reflect that.

Instead, I would like to request the following, for v8:

Please submit a series that consists of three patches:

- patch v8 1/3: identical to v6 1/2, except for the code comment update,
- patch v8 2/3: identical to v6 2/2,
- patch v8 3/3: please implement the merging of the memory map as a
separate patch.

Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
Reviewed-by tags, from v6.

Patch v8 3/3 should be reviewed / tested separately by others. I don't
think I can find the capacity for that at the moment.

This approach will correctly reflect all the work done thus far, and it
will provide the desired result for the code as well.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Posted by Zeng, Star 7 years, 1 month ago

How about we have the v6 patch series in first with the feedback from Jiewen (about comments) and you (about MemoryMapStart) addressed?

Then we can have a separated patch for the merging.

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Tuesday, November 21, 2017 9:38 PM
To: Wang, Jian J <jian.j.wang@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Jian,

On 11/21/17 07:17, Jian J Wang wrote:
>> v7:
>>   Merge memory map after filtering paging attributes
> 
> More than one entry of RT_CODE memory might cause boot problem for 
> some old OSs. This patch will fix this issue to keep OS compatibility 
> as much as possible.
> 
> Jian J Wang (2):
>   MdeModulePkg/DxeCore: Filter out all paging capabilities
>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> 
>  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
>  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
>  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++-------
>  4 files changed, 112 insertions(+), 22 deletions(-)
> 

I don't have capacity to retest and re-review the series.

Considering the following two options, I like none of them:

(1) Version 7 is merged with my feedback tags from v6. I don't like this because I didn't review or test version 7.

(2) Version 7 is merged without my feedback tags. I don't like this because I've put a lot of BZ writeup, and patch review and testing effort for this series, and I'd like the commit log to reflect that.

Instead, I would like to request the following, for v8:

Please submit a series that consists of three patches:

- patch v8 1/3: identical to v6 1/2, except for the code comment update,
- patch v8 2/3: identical to v6 2/2,
- patch v8 3/3: please implement the merging of the memory map as a separate patch.

Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my Reviewed-by tags, from v6.

Patch v8 3/3 should be reviewed / tested separately by others. I don't think I can find the capacity for that at the moment.

This approach will correctly reflect all the work done thus far, and it will provide the desired result for the code as well.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Posted by Yao, Jiewen 7 years, 1 month ago

I am OK on that.

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, November 22, 2017 3:57 PM
> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> How about we have the v6 patch series in first with the feedback from Jiewen
> (about comments) and you (about MemoryMapStart) addressed?
> 
> Then we can have a separated patch for the merging.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 21, 2017 9:38 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> Jian,
> 
> On 11/21/17 07:17, Jian J Wang wrote:
> >> v7:
> >>   Merge memory map after filtering paging attributes
> >
> > More than one entry of RT_CODE memory might cause boot problem for
> > some old OSs. This patch will fix this issue to keep OS compatibility
> > as much as possible.
> >
> > Jian J Wang (2):
> >   MdeModulePkg/DxeCore: Filter out all paging capabilities
> >   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
> >  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
> >  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94
> +++++++++++++++++++++-------
> >  4 files changed, 112 insertions(+), 22 deletions(-)
> >
> 
> I don't have capacity to retest and re-review the series.
> 
> Considering the following two options, I like none of them:
> 
> (1) Version 7 is merged with my feedback tags from v6. I don't like this because I
> didn't review or test version 7.
> 
> (2) Version 7 is merged without my feedback tags. I don't like this because I've
> put a lot of BZ writeup, and patch review and testing effort for this series, and I'd
> like the commit log to reflect that.
> 
> 
> Instead, I would like to request the following, for v8:
> 
> Please submit a series that consists of three patches:
> 
> - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> - patch v8 2/3: identical to v6 2/2,
> - patch v8 3/3: please implement the merging of the memory map as a separate
> patch.
> 
> Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
> Reviewed-by tags, from v6.
> 
> Patch v8 3/3 should be reviewed / tested separately by others. I don't think I can
> find the capacity for that at the moment.
> 
> This approach will correctly reflect all the work done thus far, and it will provide
> the desired result for the code as well.
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Posted by Laszlo Ersek 7 years, 1 month ago

On 11/22/17 08:56, Zeng, Star wrote:
> How about we have the v6 patch series in first with the feedback from Jiewen (about comments) and you (about MemoryMapStart) addressed?
> 
> Then we can have a separated patch for the merging.

Good idea!

Thanks!
Laszlo


> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Tuesday, November 21, 2017 9:38 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map
> 
> Jian,
> 
> On 11/21/17 07:17, Jian J Wang wrote:
>>> v7:
>>>   Merge memory map after filtering paging attributes
>>
>> More than one entry of RT_CODE memory might cause boot problem for 
>> some old OSs. This patch will fix this issue to keep OS compatibility 
>> as much as possible.
>>
>> Jian J Wang (2):
>>   MdeModulePkg/DxeCore: Filter out all paging capabilities
>>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
>>
>>  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
>>  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
>>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
>>  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++-------
>>  4 files changed, 112 insertions(+), 22 deletions(-)
>>
> 
> I don't have capacity to retest and re-review the series.
> 
> Considering the following two options, I like none of them:
> 
> (1) Version 7 is merged with my feedback tags from v6. I don't like this because I didn't review or test version 7.
> 
> (2) Version 7 is merged without my feedback tags. I don't like this because I've put a lot of BZ writeup, and patch review and testing effort for this series, and I'd like the commit log to reflect that.
> 
> 
> Instead, I would like to request the following, for v8:
> 
> Please submit a series that consists of three patches:
> 
> - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> - patch v8 2/3: identical to v6 2/2,
> - patch v8 3/3: please implement the merging of the memory map as a separate patch.
> 
> Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my Reviewed-by tags, from v6.
> 
> Patch v8 3/3 should be reviewed / tested separately by others. I don't think I can find the capacity for that at the moment.
> 
> This approach will correctly reflect all the work done thus far, and it will provide the desired result for the code as well.
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Posted by Wang, Jian J 7 years, 1 month ago

Sorry just see this email. I just replied another one. Great to know it works for both of us.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 22, 2017 5:05 PM
> To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> On 11/22/17 08:56, Zeng, Star wrote:
> > How about we have the v6 patch series in first with the feedback from Jiewen
> (about comments) and you (about MemoryMapStart) addressed?
> >
> > Then we can have a separated patch for the merging.
> 
> Good idea!
> 
> Thanks!
> Laszlo
> 
> 
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> > Sent: Tuesday, November 21, 2017 9:38 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> >
> > Jian,
> >
> > On 11/21/17 07:17, Jian J Wang wrote:
> >>> v7:
> >>>   Merge memory map after filtering paging attributes
> >>
> >> More than one entry of RT_CODE memory might cause boot problem for
> >> some old OSs. This patch will fix this issue to keep OS compatibility
> >> as much as possible.
> >>
> >> Jian J Wang (2):
> >>   MdeModulePkg/DxeCore: Filter out all paging capabilities
> >>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> >>
> >>  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
> >>  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
> >>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
> >>  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++--
> -----
> >>  4 files changed, 112 insertions(+), 22 deletions(-)
> >>
> >
> > I don't have capacity to retest and re-review the series.
> >
> > Considering the following two options, I like none of them:
> >
> > (1) Version 7 is merged with my feedback tags from v6. I don't like this because
> I didn't review or test version 7.
> >
> > (2) Version 7 is merged without my feedback tags. I don't like this because I've
> put a lot of BZ writeup, and patch review and testing effort for this series, and
> I'd like the commit log to reflect that.
> >
> >
> > Instead, I would like to request the following, for v8:
> >
> > Please submit a series that consists of three patches:
> >
> > - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> > - patch v8 2/3: identical to v6 2/2,
> > - patch v8 3/3: please implement the merging of the memory map as a
> separate patch.
> >
> > Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
> Reviewed-by tags, from v6.
> >
> > Patch v8 3/3 should be reviewed / tested separately by others. I don't think I
> can find the capacity for that at the moment.
> >
> > This approach will correctly reflect all the work done thus far, and it will
> provide the desired result for the code as well.
> >
> > Thanks
> > Laszlo
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Posted by Wang, Jian J 7 years, 1 month ago

Laszlo,

Thanks for the comments. Sorry for the commit log which doesn't meet the requirement.
I appreciate everything you did to this patch series. It's not intended to ignore them in log.
I think I just need more time to get used to the commit conventions.

I've explained the reason why v7 doesn't need a re-test in another email. But I understand
it if you insist re-test is necessary. Star and Jiewen have given a proposal, similar to yours
but putting the "merge" into a new patch instead of in this series. I think it's feasible. Let me
know your opinion.

Again, thanks for all your valuable comments and test efforts on this series and all others.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 21, 2017 9:38 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> Jian,
> 
> On 11/21/17 07:17, Jian J Wang wrote:
> >> v7:
> >>   Merge memory map after filtering paging attributes
> >
> > More than one entry of RT_CODE memory might cause boot problem for
> some
> > old OSs. This patch will fix this issue to keep OS compatibility as much
> > as possible.
> >
> > Jian J Wang (2):
> >   MdeModulePkg/DxeCore: Filter out all paging capabilities
> >   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
> >  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
> >  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++----
> ---
> >  4 files changed, 112 insertions(+), 22 deletions(-)
> >
> 
> I don't have capacity to retest and re-review the series.
> 
> Considering the following two options, I like none of them:
> 
> (1) Version 7 is merged with my feedback tags from v6. I don't like this
> because I didn't review or test version 7.
> 
> (2) Version 7 is merged without my feedback tags. I don't like this
> because I've put a lot of BZ writeup, and patch review and testing
> effort for this series, and I'd like the commit log to reflect that.
> 
> 
> Instead, I would like to request the following, for v8:
> 
> Please submit a series that consists of three patches:
> 
> - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> - patch v8 2/3: identical to v6 2/2,
> - patch v8 3/3: please implement the merging of the memory map as a
> separate patch.
> 
> Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
> Reviewed-by tags, from v6.
> 
> Patch v8 3/3 should be reviewed / tested separately by others. I don't
> think I can find the capacity for that at the moment.
> 
> This approach will correctly reflect all the work done thus far, and it
> will provide the desired result for the code as well.
> 
> Thanks
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel