From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x234.google.com (mail-wm0-x234.google.com [IPv6:2a00:1450:400c:c09::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 4E89580324 for ; Mon, 6 Mar 2017 08:24:49 -0800 (PST) Received: by mail-wm0-x234.google.com with SMTP id v186so68363795wmd.0 for ; Mon, 06 Mar 2017 08:24:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=A4m4XZHLgwYPvH4HWLkM0Tn49OgZIPwxiq6WGeiVqdk=; b=MXgJxXt9vWJqA/MfHo1CUliDV76N7e/Rw8fyOtTvnQrNFIqzt/Dm/84aXB1WRYfoVU pRRmxTxUXl/+WDTMaB/CwYuPgRxn8o0Acf9f1d2lwSJSwefRAGEMcfEiJGERF8mXw/wK 2KOzh8fKzzesq0RzbCri/K1oZw7dBuh8is7hg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=A4m4XZHLgwYPvH4HWLkM0Tn49OgZIPwxiq6WGeiVqdk=; b=SvB56hRUoc+AfN1lgMThYIprZ9CwLZOlZdupkS5bplmbWeObaNiOSJVFtxAZmYNdsv tE45B+SvwtD+66Ehu7RyZG2a3nYtnseT5DWBB9Q0H386O479XC2tNh26AP0uwxu/7SRV LyyYpiiw5YYw3UIUYRC0kwPOntdR4boqg91U9n2Z3Wn7JSzJHETxqX9C+KBxONvlvPHu o4quPZSrp6cbOvsHwHMcLTKoZh+omlNcq7hPqhCu5Xyex5YQkNgB4jp/ag8d8Vv/71GJ SKhfeny1qYyitud8jJJsPpCWpxsDoiYWXLAtoG7ZeaUkwAJNXPVaywXfWWSOJzoT5e+M AbAg== X-Gm-Message-State: AMke39mmhSw1TAKxuKS2RZhlUZclsVUaFDo83uiMUNUEb3UUr8lmakZm/3GE/2cnWikLUOoe X-Received: by 10.28.62.204 with SMTP id l195mr13549077wma.88.1488817487790; Mon, 06 Mar 2017 08:24:47 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id c2sm11087798wre.55.2017.03.06.08.24.46 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Mar 2017 08:24:47 -0800 (PST) Date: Mon, 6 Mar 2017 16:24:45 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Laszlo Ersek Message-ID: <20170306162445.GB16034@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> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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 16:24:49 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Mar 06, 2017 at 03:55:42PM +0100, Ard Biesheuvel wrote: > 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 OK, can you expand the statement to say "hurt performance" then? > >> 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. Works for me. If you also tweak the commit message: Reviewed-by: Leif Lindholm > >> + 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 > >>