From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily
Date: Mon, 6 Mar 2017 15:55:42 +0100 [thread overview]
Message-ID: <CAKv+Gu8jq-qp-J+V-D4xh9Zr050tNoAUUNC-MvgdLBf4mzfBjg@mail.gmail.com> (raw)
In-Reply-To: <20170306141203.GT16034@bivouac.eciton.net>
On 6 March 2017 at 15:12, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 02, 2017 at 10:36:13AM +0000, Ard Biesheuvel wrote:
>> Currently, any range passed to CpuArchProtocol::SetMemoryAttributes is
>> fully broken down into page mappings if the start or the size of the
>> region happens to be misaliged relative to the section size of 1 MB.
>>
>> This is going to hurt when we enable strict memory permissions, given
>
> "Hurt" -> "cause unnecessary performance penalties" or "hurt" ->
> "break everything"?
>
The former. It will map all of RAM using page mappings, which uses
more space unnecessarily
>> that we remap the entire RAM space non-executable (modulo the code
>> bits) when the CpuArchProtocol is installed.
>>
>> So refactor the code to iterate over the range in a way that ensures
>> that all naturally aligned section sized subregions are not broken up.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Many thanks for getting rid of the magic values, and in general making
> the code more logical. One question below:
>
>> ---
>> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++++++++----
>> 1 file changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> index 89e429925ba9..ce4d529bda67 100644
>> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> @@ -679,6 +679,7 @@ SetMemoryAttributes (
>> )
>> {
>> EFI_STATUS Status;
>> + UINT64 ChunkLength;
>>
>> //
>> // Ignore invocations that only modify permission bits
>> @@ -687,14 +688,44 @@ SetMemoryAttributes (
>> return EFI_SUCCESS;
>> }
>>
>> - if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) {
>> - // Is the base and length a multiple of 1 MB?
>> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
>> - Status = UpdateSectionEntries (BaseAddress, Length, Attributes, VirtualMask);
>> - } else {
>> - // Base and/or length is not a multiple of 1 MB
>> - DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU page 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
>> - Status = UpdatePageEntries (BaseAddress, Length, Attributes, VirtualMask);
>> + while (Length > 0) {
>
> Would this not end up returning an uninitialized Status if called with
> Length == 0?
>
Yes, well spotted. I will just add an early 'return EFI_SUCCESS' for this case.
>> + if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
>> + Length >= TT_DESCRIPTOR_SECTION_SIZE) {
>> +
>> + ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
>> +
>> + DEBUG ((DEBUG_PAGE,
>> + "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
>> + BaseAddress, ChunkLength, Attributes));
>> +
>> + Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
>> + VirtualMask);
>> + } else {
>> +
>> + //
>> + // Process page by page until the next section boundary, but only if
>> + // we have more than a section's worth of area to deal with after that.
>> + //
>> + ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
>> + (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
>> + if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
>> + ChunkLength = Length;
>> + }
>> +
>> + DEBUG ((DEBUG_PAGE,
>> + "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
>> + BaseAddress, ChunkLength, Attributes));
>> +
>> + Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
>> + VirtualMask);
>> + }
>> +
>> + if (EFI_ERROR (Status)) {
>> + break;
>> + }
>> +
>> + BaseAddress += ChunkLength;
>> + Length -= ChunkLength;
>> }
>>
>> // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
>> --
>> 2.7.4
>>
next prev parent reply other threads:[~2017-03-06 14:55 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-02 10:36 [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily Ard Biesheuvel
2017-03-06 14:12 ` Leif Lindholm
2017-03-06 14:55 ` Ard Biesheuvel [this message]
2017-03-06 16:24 ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance Ard Biesheuvel
2017-03-06 14:27 ` Leif Lindholm
2017-03-06 15:06 ` Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes() Ard Biesheuvel
2017-03-06 14:48 ` Leif Lindholm
2017-03-06 15:11 ` Ard Biesheuvel
2017-03-06 16:40 ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 4/4] ArmVirtPkg: enable PE/COFF image and memory protection for ARM platforms Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu8jq-qp-J+V-D4xh9Zr050tNoAUUNC-MvgdLBf4mzfBjg@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox