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 57EB6C74A5B for ; Tue, 21 Mar 2023 14:35:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: 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=EWN/asNEEW3RcwyzRK89hU6/iDVnxpu9w3iGhkeZWOk=; b=ENQ4B/eDvLylc2 vQAIax7Jvn01clORfdkTap0HQ0MyZJtHOqbnmmqiN75la0/NAea+Y+GvHqftZwS6kTc2p5pk8rb3e gEP9DPkFgUV/rkT/2PzfT6jAJDKhKcMzaINPSBtqmUnDeWUb65PfeNYJiUZoRW7ioEP7St1WrIOZs m8qu4b6NnT6QQgdKselb2q0ELUln85ziGkeyk8I9/snlQc6+aofftV3pR7bCOQr4YheAjiQzIH50P BWhQazwIsFYZyAAjGkRo44tN7VvcPfcPUXgnwpQBQFeZ86m+rDfYIAwUoAmMUw1+yY4VGDLqy3OCY u6IDs8fUIz37dYWd4O6Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1ped4C-00CkoJ-1A; Tue, 21 Mar 2023 14:34:24 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1ped48-00CkmI-2x for linux-arm-kernel@lists.infradead.org; Tue, 21 Mar 2023 14:34:22 +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 54E812F4; Tue, 21 Mar 2023 07:35:01 -0700 (PDT) Received: from [10.1.33.164] (C02CF1NRLVDN.cambridge.arm.com [10.1.33.164]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8ECF43F6C4; Tue, 21 Mar 2023 07:34:16 -0700 (PDT) Message-ID: Date: Tue, 21 Mar 2023 14:34:15 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH 2/2] arm64: mm: always map fixmap at page granularity Content-Language: en-US To: Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, anshuman.khandual@arm.com, ardb@kernel.org, catalin.marinas@arm.com, will@kernel.org References: <20230314142125.502043-1-mark.rutland@arm.com> <20230314142125.502043-3-mark.rutland@arm.com> <57745ea5-a03c-cb56-54fa-c2d35001d8f8@arm.com> From: Ryan Roberts In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230321_073421_070996_22BF3126 X-CRM114-Status: GOOD ( 31.88 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 21/03/2023 14:18, Mark Rutland wrote: > On Wed, Mar 15, 2023 at 10:23:31AM +0000, Ryan Roberts wrote: >> On 14/03/2023 14:21, Mark Rutland wrote: > >>> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h >>> index c153f069e9c9..c59d433c1801 100644 >>> --- a/arch/arm64/include/asm/fixmap.h >>> +++ b/arch/arm64/include/asm/fixmap.h >>> @@ -36,17 +36,13 @@ enum fixed_addresses { >>> FIX_HOLE, >>> >>> /* >>> - * Reserve a virtual window for the FDT that is 2 MB larger than the >>> - * maximum supported size, and put it at the top of the fixmap region. >>> - * The additional space ensures that any FDT that does not exceed >>> - * MAX_FDT_SIZE can be mapped regardless of whether it crosses any >>> - * 2 MB alignment boundaries. >>> - * >>> - * Keep this at the top so it remains 2 MB aligned. >>> + * Reserve a virtual window for the FDT that is a page bigger than the >>> + * maximum supported size. The additional space ensures that any FDT >>> + * that does not exceed MAX_FDT_SIZE can be mapped regardless of >>> + * whether it crosses any page boundary. >>> */ >>> -#define FIX_FDT_SIZE (MAX_FDT_SIZE + SZ_2M) >>> FIX_FDT_END, >>> - FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1, >>> + FIX_FDT = FIX_FDT_END + MAX_FDT_SIZE / PAGE_SIZE - 1, >> >> I don't think this is consistent with your comment - needs a +1 for the extra >> page? On a 4K system, FIX_FDT will be calculated as 512, but we need it to be >> 513 to deal with the case where the FDT is 2MB and not aligned to a page boundary. > > Indeed; the intent was to have 513 in that case, but I clearly either forgot to > fix the calculation or messed that up when rebasing. I've fixed that to: > > FIX_FDT_END + DIV_ROUND_UP(MAX_FDT_SIZE, PAGE_SIZE) + 1, > > ... which'll work even if MAX_FDT_SIZE weren't a multiple of PAGE_SIZE. > > [...] > >>> diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c >>> index 54e50552bfe3..d7a6a485f361 100644 >>> --- a/arch/arm64/mm/fixmap.c >>> +++ b/arch/arm64/mm/fixmap.c >>> @@ -16,34 +16,77 @@ >>> #include >>> #include >>> >>> -static pte_t bm_pte[PTRS_PER_PTE] __page_aligned_bss; >>> +#define NR_BM_PTE_TABLES \ >>> + SPAN_NR_ENTRIES(FIXADDR_START, FIXADDR_TOP, PMD_SHIFT) >>> +#define NR_BM_PMD_TABLES \ >>> + SPAN_NR_ENTRIES(FIXADDR_START, FIXADDR_TOP, PUD_SHIFT) >> >> One of the bear traps I stepped in last week when I was wrestling with this is >> that FIXADDR_START isn't actually the address of the first slot! It is >> __end_of_permanent_fixed_addresses. But you then have all the FIX_BTMAP slots, >> FIX_PTE/PMD/PUD/PGD before FIXADDR_START. So I think some refactoring is in >> order to properly cover all the slots. > > Ugh, yes. This is a total mess. > >> Honestly, the fact that the fixed_addresses enum is backwards is mind-bending >> too. > > That has been a long standing bugbear of mine, too. > >> Any chance we can just define FIXADDR_START and FIXADDR_TOP to cover the >> whole lot, then calculate the slot address as (FIXADDR_START + ( << >> PAGE_SHIFT))? > > Unfortunately, some code is (implicitly) relying on FIXADDR_START..FIXADDR_TOP > *not* including the FIX_BTMAP slots (e.g. virt_to_fix() and fix_to_virt() use > that to implicitly catch dodgy usage). Other code really wants FIXADDR_START to > be the base of the whole fixmap region (e.g. ptdump). So perhaps one approach would be to have 2 sets of macros: #define ARCH_FIXADDR_START #define ARCH_FIXADDR_TOP #define FIXADDR_START #define FIXADDR_TOP static_assert(ARCH_FIXADDR_START <= FIXADDR_START); static_assert(ARCH_FIXADDR_TOP >= FIXADDR_TOP); ARCH_FIXADDR_* covers the entire region exactly. And FIXADDR_* describes the sub-region that the generic code cares about. Then all the fixmap internals can be done against ARCH_FIXADDR_* and the generic code can do its checks against FIXADDR_* ? > > Cleaning that up is going to be a bit churny/painful. I'll take a look. > >> (plus reorder of start/end enum labels for FDT and BTMAP). Or if >> we really care about reclaiming the virtual address space between >> __end_of_permanent_fixed_addresses and __end_of_fixed_addresses, create a new >> FIXADDR_BOOT_START or whatever. > > I don't think we care about reclaiming the address space; it's a few pages at > best. I'll go take another look at how churny it'd be clean this up... > >>> + >>> +static_assert(NR_BM_PMD_TABLES == 1); >>> + >>> +#define __BM_TABLE_IDX(addr, shift) \ >>> + (((addr) >> (shift)) - (FIXADDR_START >> (shift))) >> >> Doesn't this run the risk of being negative for one of the slots below >> FIXADDR_START? > > Yes; I had erroneously thought FIXADDR_START was below all of those, and the > intent was to use the base of the entire fixmap. So as above we'll need to do > some more preparatory cleanup... > >>> +void __init early_fixmap_init_pte(pmd_t *pmdp, unsigned long addr) >> >> static? - same comment for pmd/pud below. > > Yup; done. > >>> +{ >>> + pmd_t pmd = READ_ONCE(*pmdp); >>> + pte_t *ptep; >>> >>> - return pud_offset_kimg(p4dp, addr); >>> + if (pmd_none(pmd)) { >>> + ptep = bm_pte[BM_PTE_TABLE_IDX(addr)];; >> >> nit: remove one of the semi-colons. > > Whoops; rebase error; done. > > Thanks, > Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel