From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x234.google.com (mail-io0-x234.google.com [IPv6:2607:f8b0:4001:c06::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 97D4180313 for ; Mon, 6 Mar 2017 06:55:43 -0800 (PST) Received: by mail-io0-x234.google.com with SMTP id f84so113430133ioj.0 for ; Mon, 06 Mar 2017 06:55:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=nBqmz3laDBOBqenRUciFsVCJZw4IZUyMnUA6pYregpM=; b=ki2gR8udWEmwX1BQAMN/fWyYJps5M05YSh0w53Hkhi9nyg04A4YTLvoHDPddIXZxgq Txd3IToCBpsXXQE6rhAOLmyi1lRVmr+hQGb0WpjXSVYURI+kIuOOlfQxYmg2gXupN+IM U31RhiYbYs9xqU3LOmWk6yGKNXq37HinOUB7M= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=nBqmz3laDBOBqenRUciFsVCJZw4IZUyMnUA6pYregpM=; b=AYGXlrGpxVrKEv9dk5Q5W1Q3bReNnXWM1UbcMomoXLZrOh6Q7zts3aEWM2PJYKnQ70 4PwT7xVo2xsK7jp2tXA2xWwBL3Y64abclupXL5q2U3UzDvYJ/LUgaSAQXfNNH9JotUUm 4vQSLgSKv70ZHdxa1Jl0N98D4jY4bwqCBHG1HWqRl505yCWTVNirxqL3kxXtQdNozmoE 6r4qDAJ+yfnm/RmumMX8sWET97+MR2D1UtcKfFrROHHJ50aELzSYWFLj/+GPqPxW1kAP qJbj/uHusrdbKEsEthwxS1lo4ynoG9NEYNSRPr9A6/mPIX7z6wIx5ui6VvWAWztnqRh8 Xqpw== X-Gm-Message-State: AMke39l22EKlfObRabuFEh/L/M4bLudVvFN04lLn2XaNZom9mA7ihjxjV4b0NhXfA23oDhclvF5PiNJ0pmtjBjWa X-Received: by 10.107.13.130 with SMTP id 124mr13306934ion.83.1488812142843; Mon, 06 Mar 2017 06:55:42 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Mon, 6 Mar 2017 06:55:42 -0800 (PST) In-Reply-To: <20170306141203.GT16034@bivouac.eciton.net> References: <1488450976-16257-1-git-send-email-ard.biesheuvel@linaro.org> <1488450976-16257-2-git-send-email-ard.biesheuvel@linaro.org> <20170306141203.GT16034@bivouac.eciton.net> From: Ard Biesheuvel Date: Mon, 6 Mar 2017 15:55:42 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Laszlo Ersek Subject: Re: [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Mar 2017 14:55:43 -0000 Content-Type: text/plain; charset=UTF-8 On 6 March 2017 at 15:12, Leif Lindholm 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 > > 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 >>