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 4D2A780313 for ; Mon, 6 Mar 2017 06:12:08 -0800 (PST) Received: by mail-wm0-x234.google.com with SMTP id 196so18616773wmm.1 for ; Mon, 06 Mar 2017 06:12:08 -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=MjLvxKWPYjVya065MfNY5JkgkVrkf9BK0BjEt4sKTEE=; b=GF6GCXgE6mh6gleGry7OvNpdZ/pIdM+gn8K/hlI3AbOmtIh0bOkHKWycmC8o2tfxmk zIL8DGfJ6LDb2lqmJdNDBs3pu5rD0bIAawHdBzGmmyh7EHLeWzMQ/Af2zbMKco3ngsoe hxuh8KbnG9UGJTefW0uiKvky2/ff7dUwhu30A= 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=MjLvxKWPYjVya065MfNY5JkgkVrkf9BK0BjEt4sKTEE=; b=rdd9xl0m8uuPI2RcvstAve77oWake55mTvG0MQtOT5PRgkn/OV7++5LH9bb0eMi9v7 O+MVLnmGALarkC8h6XXCqvnVeRMThB2dqlFcH20puzHwBY/T58PQ+r31QcGR+jw3G9LS RD9oXWRLudOGxYRpTy7VJfB6wa2cAe/mFihzLhR0k+OnLYfw1mnsQxytRcUPhoJ66Yrb mah175eg5C+BiLaxWtkvOUelkDdSG9B2rBHJQUEUvy2/GdeXeJrkFpq8Hrf36YJW8HNL 2LFX7rtZzKb9F3GfLhtxIIpzTVoi30fLsKY9nybbZjByM/6jpLpf4xEqwI+vTeCUftEd 9Iww== X-Gm-Message-State: AMke39nLqSwz70qDA87Wbk8ZKYEG1J8BeYpF3p1XEtdDULMYizi6XGmlYujQmS49JvPJjPQU X-Received: by 10.28.61.198 with SMTP id k189mr14109507wma.71.1488809526402; Mon, 06 Mar 2017 06:12:06 -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 l41sm27046541wre.23.2017.03.06.06.12.05 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Mar 2017 06:12:05 -0800 (PST) Date: Mon, 6 Mar 2017 14:12:03 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, lersek@redhat.com Message-ID: <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> MIME-Version: 1.0 In-Reply-To: <1488450976-16257-2-git-send-email-ard.biesheuvel@linaro.org> 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 14:12:08 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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"? > 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? / Leif > + 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 >