v4:
- major cleanup of the 32-bit ARM code
- add support for EFI_MEMORY_RP using the access flag
- enable stack guard in ArmVirtPkg (which uses EFI_MEMORY_RP)
- incorporate optimization from other series [0] to avoid splitting
  block entries unnecessarily

v3:
- fix ARM32 bug in attribute conversion
- add Liming's ack to patch #1
- include draft patch (NOT FOR MERGE) used to test the changes

v2:
- drop patch to bump exposed UEFI revision to v2.10
- add missing permitted return values to protocol definition

[0] https://edk2.groups.io/g/devel/message/99801

Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Rebecca Cran <quic_rcran@quicinc.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Taylor Beebe <t@taylorbeebe.com>

Ard Biesheuvel (11):
  ArmPkg/ArmMmuLib ARM: Remove half baked large page support
  ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field
  ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion
  ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask
  ArmPkg/ArmMmuLib ARM: Clear individual permission bits
  ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag
  ArmVirtPkg: Enable stack guard
  ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
  ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion
  MdePkg: Add Memory Attribute Protocol definition
  ArmPkg/CpuDxe: Implement EFI memory attributes protocol

 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  25 +-
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  |  96 +++++--
 ArmPkg/Drivers/CpuDxe/CpuDxe.c                   |   2 +
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  17 ++
 ArmPkg/Drivers/CpuDxe/CpuDxe.inf                 |   2 +
 ArmPkg/Drivers/CpuDxe/MemoryAttribute.c          | 271 ++++++++++++++++++++
 ArmPkg/Include/Chipset/ArmV7Mmu.h                |  88 +++----
 ArmPkg/Include/Library/ArmMmuLib.h               |  34 +++
 ArmPkg/Library/ArmLib/Arm/ArmV7Support.S         |   2 +
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  68 ++++-
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c  |   8 +-
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     |   2 +-
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c   | 173 +++++++++++--
 ArmVirtPkg/ArmVirt.dsc.inc                       |   2 +
 MdePkg/Include/Protocol/MemoryAttribute.h        | 142 ++++++++++
 MdePkg/MdePkg.dec                                |   3 +
 16 files changed, 833 insertions(+), 102 deletions(-)
 create mode 100644 ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
 create mode 100644 MdePkg/Include/Protocol/MemoryAttribute.h

-- 
2.39.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99885): https://edk2.groups.io/g/devel/message/99885
Mute This Topic: https://groups.io/mt/96853149/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hey Ard,

Once the Memory Attribute Protocol is made available, Windows will have 
some expectations about its functionality. Can you run this test app 
created by me and Jiewen to ensure it meets the Windows requirements? 
Part of the test needed an AARCH64 implementation which I just added - 
let me know if it doesn't work.

Thanks :)

https://github.com/TaylorBeebe/mu_basecore/tree/update_map_test_app/MdePkg/Test/ShellTest/MemoryAttributeProtocolFuncTestApp

On 2/9/2023 5:59 AM, Ard Biesheuvel wrote:
> v4:
> 
> - major cleanup of the 32-bit ARM code
> 
> - add support for EFI_MEMORY_RP using the access flag
> 
> - enable stack guard in ArmVirtPkg (which uses EFI_MEMORY_RP)
> 
> - incorporate optimization from other series [0] to avoid splitting
> 
>    block entries unnecessarily
> 
> 
> 
> v3:
> 
> - fix ARM32 bug in attribute conversion
> 
> - add Liming's ack to patch #1
> 
> - include draft patch (NOT FOR MERGE) used to test the changes
> 
> 
> 
> v2:
> 
> - drop patch to bump exposed UEFI revision to v2.10
> 
> - add missing permitted return values to protocol definition
> 
> 
> 
> [0] https://edk2.groups.io/g/devel/message/99801
> 
> 
> 
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> 
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> 
> Cc: Rebecca Cran <quic_rcran@quicinc.com>
> 
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> 
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> 
> Cc: Taylor Beebe <t@taylorbeebe.com>
> 
> 
> 
> Ard Biesheuvel (11):
> 
>    ArmPkg/ArmMmuLib ARM: Remove half baked large page support
> 
>    ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field
> 
>    ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion
> 
>    ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask
> 
>    ArmPkg/ArmMmuLib ARM: Clear individual permission bits
> 
>    ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag
> 
>    ArmVirtPkg: Enable stack guard
> 
>    ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
> 
>    ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion
> 
>    MdePkg: Add Memory Attribute Protocol definition
> 
>    ArmPkg/CpuDxe: Implement EFI memory attributes protocol
> 
> 
> 
>   ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  25 +-
> 
>   ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  |  96 +++++--
> 
>   ArmPkg/Drivers/CpuDxe/CpuDxe.c                   |   2 +
> 
>   ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  17 ++
> 
>   ArmPkg/Drivers/CpuDxe/CpuDxe.inf                 |   2 +
> 
>   ArmPkg/Drivers/CpuDxe/MemoryAttribute.c          | 271 ++++++++++++++++++++
> 
>   ArmPkg/Include/Chipset/ArmV7Mmu.h                |  88 +++----
> 
>   ArmPkg/Include/Library/ArmMmuLib.h               |  34 +++
> 
>   ArmPkg/Library/ArmLib/Arm/ArmV7Support.S         |   2 +
> 
>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  68 ++++-
> 
>   ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c  |   8 +-
> 
>   ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     |   2 +-
> 
>   ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c   | 173 +++++++++++--
> 
>   ArmVirtPkg/ArmVirt.dsc.inc                       |   2 +
> 
>   MdePkg/Include/Protocol/MemoryAttribute.h        | 142 ++++++++++
> 
>   MdePkg/MdePkg.dec                                |   3 +
> 
>   16 files changed, 833 insertions(+), 102 deletions(-)
> 
>   create mode 100644 ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
> 
>   create mode 100644 MdePkg/Include/Protocol/MemoryAttribute.h
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100039): https://edk2.groups.io/g/devel/message/100039
Mute This Topic: https://groups.io/mt/96853149/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> Hey Ard,
>
> Once the Memory Attribute Protocol is made available, Windows will have
> some expectations about its functionality. Can you run this test app
> created by me and Jiewen to ensure it meets the Windows requirements?
> Part of the test needed an AARCH64 implementation which I just added -
> let me know if it doesn't work.
>

Thanks, this is rather helpful.

There appears to be an issue related to
DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
run these tests, as otherwise, the DXE core tries to clear freed pages
before restoring the memory attributes.

With that out of the way, the only test that fails is 'New
EfiLoaderCode buffer attributes expected' because this firmware build
maps loader code RWX, as existing boot stages for Linux are relying on
this (including the kernel itself at this point)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100043): https://edk2.groups.io/g/devel/message/100043
Mute This Topic: https://groups.io/mt/96853149/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2/11/2023 2:05 AM, Ard Biesheuvel wrote:
> On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
>>
>> Hey Ard,
>>
>> Once the Memory Attribute Protocol is made available, Windows will have
>> some expectations about its functionality. Can you run this test app
>> created by me and Jiewen to ensure it meets the Windows requirements?
>> Part of the test needed an AARCH64 implementation which I just added -
>> let me know if it doesn't work.
>>
> 
> Thanks, this is rather helpful.
> 
> There appears to be an issue related to
> DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
> run these tests, as otherwise, the DXE core tries to clear freed pages
> before restoring the memory attributes.
> 
> With that out of the way, the only test that fails is 'New
> EfiLoaderCode buffer attributes expected' because this firmware build
> maps loader code RWX, as existing boot stages for Linux are relying on
> this (including the kernel itself at this point)

It makes sense that the NewEfiLoaderCode test fails, but I am surprised 
the FreePagesWithProtectionAttributesTestCase passes. The test ensures 
that a page with EFI_MEMORY_RP and/or EFI_MEMORY_RO has those attributes 
cleared before attempting to free the page within the FreePage routine 
and is related to the concern Marvin had.

Did you make a change to the core or is there an execution path I'm not 
seeing which allows that test to pass?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100610): https://edk2.groups.io/g/devel/message/100610
Mute This Topic: https://groups.io/mt/96853149/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 1 Mar 2023 at 21:43, Taylor Beebe <t@taylorbeebe.com> wrote:
>
>
>
> On 2/11/2023 2:05 AM, Ard Biesheuvel wrote:
> > On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
> >>
> >> Hey Ard,
> >>
> >> Once the Memory Attribute Protocol is made available, Windows will have
> >> some expectations about its functionality. Can you run this test app
> >> created by me and Jiewen to ensure it meets the Windows requirements?
> >> Part of the test needed an AARCH64 implementation which I just added -
> >> let me know if it doesn't work.
> >>
> >
> > Thanks, this is rather helpful.
> >
> > There appears to be an issue related to
> > DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
> > run these tests, as otherwise, the DXE core tries to clear freed pages
> > before restoring the memory attributes.
> >
> > With that out of the way, the only test that fails is 'New
> > EfiLoaderCode buffer attributes expected' because this firmware build
> > maps loader code RWX, as existing boot stages for Linux are relying on
> > this (including the kernel itself at this point)
>
> It makes sense that the NewEfiLoaderCode test fails, but I am surprised
> the FreePagesWithProtectionAttributesTestCase passes. The test ensures
> that a page with EFI_MEMORY_RP and/or EFI_MEMORY_RO has those attributes
> cleared before attempting to free the page within the FreePage routine
> and is related to the concern Marvin had.
>
> Did you make a change to the core or is there an execution path I'm not
> seeing which allows that test to pass?

No, I didn't make any additional changes to the core afair.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100611): https://edk2.groups.io/g/devel/message/100611
Mute This Topic: https://groups.io/mt/96853149/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

My mistake - the DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED feature is 
why FreePagesWithProtectionAttributesTestCase might fail.

To make the clear memory feature more compatible with the memory 
attribute protocol, can you add a check to DebugClearMemoryEnabled() on 
free and clear EFI_MEMORY_RP and EFI_MEMORY_RO if they're set?

-Taylor

On 3/1/2023 1:57 PM, Ard Biesheuvel wrote:
> On Wed, 1 Mar 2023 at 21:43, Taylor Beebe <t@taylorbeebe.com> wrote:
>>
>>
>>
>> On 2/11/2023 2:05 AM, Ard Biesheuvel wrote:
>>> On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
>>>>
>>>> Hey Ard,
>>>>
>>>> Once the Memory Attribute Protocol is made available, Windows will have
>>>> some expectations about its functionality. Can you run this test app
>>>> created by me and Jiewen to ensure it meets the Windows requirements?
>>>> Part of the test needed an AARCH64 implementation which I just added -
>>>> let me know if it doesn't work.
>>>>
>>>
>>> Thanks, this is rather helpful.
>>>
>>> There appears to be an issue related to
>>> DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
>>> run these tests, as otherwise, the DXE core tries to clear freed pages
>>> before restoring the memory attributes.
>>>
>>> With that out of the way, the only test that fails is 'New
>>> EfiLoaderCode buffer attributes expected' because this firmware build
>>> maps loader code RWX, as existing boot stages for Linux are relying on
>>> this (including the kernel itself at this point)
>>
>> It makes sense that the NewEfiLoaderCode test fails, but I am surprised
>> the FreePagesWithProtectionAttributesTestCase passes. The test ensures
>> that a page with EFI_MEMORY_RP and/or EFI_MEMORY_RO has those attributes
>> cleared before attempting to free the page within the FreePage routine
>> and is related to the concern Marvin had.
>>
>> Did you make a change to the core or is there an execution path I'm not
>> seeing which allows that test to pass?
> 
> No, I didn't make any additional changes to the core afair.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100876): https://edk2.groups.io/g/devel/message/100876
Mute This Topic: https://groups.io/mt/96853149/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 8 Mar 2023 at 18:24, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> My mistake - the DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED feature is
> why FreePagesWithProtectionAttributesTestCase might fail.
>
> To make the clear memory feature more compatible with the memory
> attribute protocol, can you add a check to DebugClearMemoryEnabled() on
> free and clear EFI_MEMORY_RP and EFI_MEMORY_RO if they're set?
>

I think the best strategy here is to simply apply the default
permissions first, and only then actually free the pages, clear them
etc etc That also fixes the existing issue where we may remap fewer
pages than what we actually freed. I'll add a patch to my series for
this.


>
> On 3/1/2023 1:57 PM, Ard Biesheuvel wrote:
> > On Wed, 1 Mar 2023 at 21:43, Taylor Beebe <t@taylorbeebe.com> wrote:
> >>
> >>
> >>
> >> On 2/11/2023 2:05 AM, Ard Biesheuvel wrote:
> >>> On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
> >>>>
> >>>> Hey Ard,
> >>>>
> >>>> Once the Memory Attribute Protocol is made available, Windows will have
> >>>> some expectations about its functionality. Can you run this test app
> >>>> created by me and Jiewen to ensure it meets the Windows requirements?
> >>>> Part of the test needed an AARCH64 implementation which I just added -
> >>>> let me know if it doesn't work.
> >>>>
> >>>
> >>> Thanks, this is rather helpful.
> >>>
> >>> There appears to be an issue related to
> >>> DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
> >>> run these tests, as otherwise, the DXE core tries to clear freed pages
> >>> before restoring the memory attributes.
> >>>
> >>> With that out of the way, the only test that fails is 'New
> >>> EfiLoaderCode buffer attributes expected' because this firmware build
> >>> maps loader code RWX, as existing boot stages for Linux are relying on
> >>> this (including the kernel itself at this point)
> >>
> >> It makes sense that the NewEfiLoaderCode test fails, but I am surprised
> >> the FreePagesWithProtectionAttributesTestCase passes. The test ensures
> >> that a page with EFI_MEMORY_RP and/or EFI_MEMORY_RO has those attributes
> >> cleared before attempting to free the page within the FreePage routine
> >> and is related to the concern Marvin had.
> >>
> >> Did you make a change to the core or is there an execution path I'm not
> >> seeing which allows that test to pass?
> >
> > No, I didn't make any additional changes to the core afair.
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101097): https://edk2.groups.io/g/devel/message/101097
Mute This Topic: https://groups.io/mt/96853149/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-