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 91DC1C74A5B for ; Tue, 21 Mar 2023 14:20:01 +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:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ZcHNUQ7whPF4+m255udS+vZGgY1y1cYtemJTtgJiFfA=; b=28PkmN866nLOPt ovC6YmYAHwwXT8MJ38dy7TtjWEmOcdYj1hiHYEMnsCEveF4QfDK8sPuPPyRlDj0nNNWs3mzhQuE/i 3peswckRs+/012MdCI0DJtEV++Dg7g6YUBIFvFBn43W61OE5YYCMXRsviYOH7yEj+S8aUQ5RbatkU /CeSN8fvWtWonyA5W/1Yws7yBpWQD92xMWggjmbN75ll13BAO61BXgfHllHNzpBgN0LRgX5xwSyRF /zT/zEo4pqkN/qwgM5c60nf8VsfmvQIBsVnYewkZleWnRK10lyYYACd8ojh1NO79LyM1Mzv0T6Cby WK3hF75cs8wr79nQvSQA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pecpG-00CgW9-1X; Tue, 21 Mar 2023 14:18:58 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pecp9-00CgUf-1u for linux-arm-kernel@lists.infradead.org; Tue, 21 Mar 2023 14:18:56 +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 78DA7AD7; Tue, 21 Mar 2023 07:19:25 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.54.220]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 671623F6C4; Tue, 21 Mar 2023 07:18:40 -0700 (PDT) Date: Tue, 21 Mar 2023 14:18:28 +0000 From: Mark Rutland To: Ryan Roberts Cc: linux-arm-kernel@lists.infradead.org, anshuman.khandual@arm.com, ardb@kernel.org, catalin.marinas@arm.com, will@kernel.org Subject: Re: [PATCH 2/2] arm64: mm: always map fixmap at page granularity Message-ID: References: <20230314142125.502043-1-mark.rutland@arm.com> <20230314142125.502043-3-mark.rutland@arm.com> <57745ea5-a03c-cb56-54fa-c2d35001d8f8@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <57745ea5-a03c-cb56-54fa-c2d35001d8f8@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230321_071854_425149_91F00DEA X-CRM114-Status: GOOD ( 33.79 ) 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 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). 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