From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 499D2C52D6F for ; Wed, 7 Aug 2024 21:42:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:References:Cc:To:From:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=cUCo6uBtSEYNqzmG/qsGgQhwaxt3MATQasAmDqtxOok=; b=YTEUM+L0tU6rGhuQMoqtL/7W7y 9HHRrV7Ra3bGBWYoFhZQkS2yS/U0JYpgFMs3wjfYrPxZQMTgS6ElRGTgsXNUXLtRdbfwReg1iHUco fmQ5NqabLAmFSFIxxRmiamQB3ha3z2I9HLm6slp+dKlHYfb4dWY4TSIJx/DNiAAr/F1Yks7+N8SSk luM5IwB5emGLgbtJe0fP1V84dOlxABwFX0ZXF1AAJqVdWau6suEa41Vq6POZR+jDrSYq2bG3k/Y+7 g+ZSbZ36EdHkGy3HYfLE5wal+npM6P13xQPy3Sy3F+wCvuEfYNMFNvOruNwW53jY3Wi5mN5R1Z3fT k+VTT3LA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sboQE-00000006MOV-3WTt; Wed, 07 Aug 2024 21:42:18 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sboPE-00000006MAY-0P0K for linux-arm-kernel@lists.infradead.org; Wed, 07 Aug 2024 21:41:18 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B2215367; Wed, 7 Aug 2024 14:41:38 -0700 (PDT) Received: from [10.57.81.112] (unknown [10.57.81.112]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BA35F3F766; Wed, 7 Aug 2024 14:41:11 -0700 (PDT) Message-ID: Date: Wed, 7 Aug 2024 22:41:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 33/43] arm64: Enable LPA2 at boot if supported by the system Content-Language: en-GB From: Ryan Roberts To: Ard Biesheuvel , linux-arm-kernel@lists.infradead.org Cc: Ard Biesheuvel , Catalin Marinas , Will Deacon , Marc Zyngier , Mark Rutland , Anshuman Khandual , Kees Cook References: <20240214122845.2033971-45-ardb+git@google.com> <20240214122845.2033971-78-ardb+git@google.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240807_144116_276862_722656B4 X-CRM114-Status: GOOD ( 32.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 07/08/2024 09:46, Ryan Roberts wrote: > On 06/08/2024 17:16, Ryan Roberts wrote: >> Hi Ard, >> >> Sorry to drag up this old thread; I'm doing some work in this space and am >> having a tough time convincing myself of the safety - see below... >> >> On 14/02/2024 12:29, Ard Biesheuvel wrote: >>> From: Ard Biesheuvel >>> >>> Update the early kernel mapping code to take 52-bit virtual addressing >>> into account based on the LPA2 feature. This is a bit more involved than >>> LVA (which is supported with 64k pages only), given that some page table >>> descriptor bits change meaning in this case. >>> >>> To keep the handling in asm to a minimum, the initial ID map is still >>> created with 48-bit virtual addressing, which implies that the kernel >>> image must be loaded into 48-bit addressable physical memory. This is >>> currently required by the boot protocol, even though we happen to >>> support placement outside of that for LVA/64k based configurations. >>> >>> Enabling LPA2 involves more than setting TCR.T1SZ to a lower value, >>> there is also a DS bit in TCR that needs to be set, and which changes >>> the meaning of bits [9:8] in all page table descriptors. Since we cannot >>> enable DS and every live page table descriptor at the same time, let's >>> pivot through another temporary mapping. This avoids the need to >>> reintroduce manipulations of the page tables with the MMU and caches >>> disabled. >>> >>> To permit the LPA2 feature to be overridden on the kernel command line, >>> which may be necessary to work around silicon errata, or to deal with >>> mismatched features on heterogeneous SoC designs, test for CPU feature >>> overrides first, and only then enable LPA2. >>> >>> Signed-off-by: Ard Biesheuvel >> >> [...] >> >>> +static void __init remap_idmap_for_lpa2(void) >>> +{ >>> + /* clear the bits that change meaning once LPA2 is turned on */ >>> + pteval_t mask = PTE_SHARED; >>> + >>> + /* >>> + * We have to clear bits [9:8] in all block or page descriptors in the >>> + * initial ID map, as otherwise they will be (mis)interpreted as >>> + * physical address bits once we flick the LPA2 switch (TCR.DS). Since >>> + * we cannot manipulate live descriptors in that way without creating >>> + * potential TLB conflicts, let's create another temporary ID map in a >>> + * LPA2 compatible fashion, and update the initial ID map while running >>> + * from that. >>> + */ >>> + create_init_idmap(init_pg_dir, mask); >> >> Given the init_idmap always uses 48 bit VA, and the swapper VA size is >> determined through Kconfig and may be smaller than 48 bit, how can you be >> certain that init_pg_dir is big enough to hold the init idmap? Surely swapper >> may use fewer levels and therefore be sized for fewer pages? >> >> I wonder if its possible that we end up running into the early_init_stack then >> off the end of the BSS? > > I'm bad at maths so decided to test this impirically by compiling the macros up > into a test program and spitting out the values for all supported combinations > of page size and va bits: > > PAGE_SHIFT=12: > VA_BITS=52: > INIT_DIR_SIZE=53248 > INIT_IDMAP_DIR_SIZE=32768 > INIT_IDMAP_FDT_SIZE=24576 > VA_BITS=48: > INIT_DIR_SIZE=45056 > INIT_IDMAP_DIR_SIZE=32768 > INIT_IDMAP_FDT_SIZE=24576 > VA_BITS=39: > INIT_DIR_SIZE=36864 > INIT_IDMAP_DIR_SIZE=32768 > INIT_IDMAP_FDT_SIZE=24576 > PAGE_SHIFT=14: > VA_BITS=52: > INIT_DIR_SIZE=131072 > INIT_IDMAP_DIR_SIZE=131072 > INIT_IDMAP_FDT_SIZE=98304 > VA_BITS=48: > INIT_DIR_SIZE=131072 > INIT_IDMAP_DIR_SIZE=131072 > INIT_IDMAP_FDT_SIZE=98304 > VA_BITS=47: > INIT_DIR_SIZE=98304 <<< TOO SMALL! > INIT_IDMAP_DIR_SIZE=131072 > INIT_IDMAP_FDT_SIZE=98304 > VA_BITS=36: > INIT_DIR_SIZE=65536 <<< TOO SMALL! > INIT_IDMAP_DIR_SIZE=131072 > INIT_IDMAP_FDT_SIZE=98304 > PAGE_SHIFT=16: > VA_BITS=52: > INIT_DIR_SIZE=327680 > INIT_IDMAP_DIR_SIZE=327680 > INIT_IDMAP_FDT_SIZE=262144 > VA_BITS=48: > INIT_DIR_SIZE=327680 > INIT_IDMAP_DIR_SIZE=327680 > INIT_IDMAP_FDT_SIZE=262144 > VA_BITS=42: > INIT_DIR_SIZE=196608 <<< TOO SMALL! > INIT_IDMAP_DIR_SIZE=327680 > INIT_IDMAP_FDT_SIZE=262144 > > There are 3 configurations where the space allocated in BSS for the init_pg_dir > is smaller than the space required for the init_idmap_pg_dir. So I think there > is definitely a problem here? > > As I said, I'm doing work in this area at the moment, so propose to send some > patches to fix this by ensuring the space allocated for init_pg_dir is > MAX(INIT_DIR_SIZE, INIT_IDMAP_DIR_SIZE). I'm also going to track the limit of > the buffer that is being allocated from so we can runtime check we don't > overflow. Shout if you disagree. There's no bug here; We only take this path if LPA2 is enabled, and LPA2 is only enabled if 52-bit VA is configured. So in that case, init_pg_dir must be at least as big as init_idmap_pg_dir and can definitely hold the 48 bit VA init idmap. Sorry for the noise. > > Thanks, > Ryan > > >> >> Thanks, >> Ryan >> >>> + dsb(ishst); >>> + set_ttbr0_for_lpa2((u64)init_pg_dir); >>> + >>> + /* >>> + * Recreate the initial ID map with the same granularity as before. >>> + * Don't bother with the FDT, we no longer need it after this. >>> + */ >>> + memset(init_idmap_pg_dir, 0, >>> + (u64)init_idmap_pg_dir - (u64)init_idmap_pg_end); >>> + >>> + create_init_idmap(init_idmap_pg_dir, mask); >>> + dsb(ishst); >>> + >>> + /* switch back to the updated initial ID map */ >>> + set_ttbr0_for_lpa2((u64)init_idmap_pg_dir); >>> + >>> + /* wipe the temporary ID map from memory */ >>> + memset(init_pg_dir, 0, (u64)init_pg_end - (u64)init_pg_dir); >>> +} >> >