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 72864C48BC4 for ; Tue, 20 Feb 2024 11:55:58 +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=C+yd/bckoeeNw3NlMTJ1H/2gTyLwBoqyqAWF2iVwKsc=; b=BEi50NBZxEtuOr UWQzW1+qOclB3THu6V0Omzpnds8ITJVgM3i8OKodbhKiWdQN8OtN1xlMOzZbL3vxZAyBFw1ZAebS/ bIQ4vd3K68vWyn4IbX0ZwYnUo1DY66ooU7DQ3BeBk+7s2//ish/SwUpxftEdgnlXofN5z9s96ZnDJ 7ZT84nHpt3+RQC0zaoeXFhAgG/e3yOIQxVf6RLxg+XD011rq4KDFg88+LeOJiWBTRpQ0gmpyuOwPH QFgzZJR44PqHqPczUuQxrz7FtJ007FS4/JM5/fCLvEH7eGl+6UsGZ/DRXqRWakX2UBXYovJ2lJALP kBgSMZFHEYgIwmkeb1YQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rcOix-0000000EYcC-2QM3; Tue, 20 Feb 2024 11:55:47 +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 1rcOiu-0000000EYan-1j46 for linux-arm-kernel@lists.infradead.org; Tue, 20 Feb 2024 11:55:45 +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 D15ABFEC; Tue, 20 Feb 2024 03:56:20 -0800 (PST) Received: from FVFF77S0Q05N.cambridge.arm.com (FVFF77S0Q05N.cambridge.arm.com [10.1.26.173]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B38453F762; Tue, 20 Feb 2024 03:55:40 -0800 (PST) Date: Tue, 20 Feb 2024 11:55:30 +0000 From: Mark Rutland To: Itaru Kitayama Cc: skseofh@gmail.com, catalin.marinas@arm.com, will@kernel.org, ryan.roberts@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] arm64: add early fixmap initialization flag Message-ID: References: <20240217140326.2367186-1-skseofh@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240220_035544_557351_0E9A58B6 X-CRM114-Status: GOOD ( 33.74 ) 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 Tue, Feb 20, 2024 at 09:29:14AM +0900, Itaru Kitayama wrote: > On Mon, Feb 19, 2024 at 10:48:26AM +0000, Mark Rutland wrote: > > On Sat, Feb 17, 2024 at 11:03:26PM +0900, skseofh@gmail.com wrote: > > > From: Daero Lee > > > > > > early_fixmap_init may be called multiple times. Since there is no > > > change in the page table after early fixmap initialization, an > > > initialization flag was added. > > > > Why is that better? > > > > We call early_fixmap_init() in two places: > > > > * early_fdt_map() > > * setup_arch() > > > > ... and to get to setup_arch() we *must* have gone through early_fdt_map(), > > since __primary_switched() calls that before going to setup_arch(). > > > > So AFAICT we can remove the second call to early_fixmap_init() in setup_arch(), > > and rely on the earlier one in early_fdt_map(). > > Removing the second call makes the code base a bit harder to understand > as the functions related to DT and ACPI setup are not separated cleanly. > I prefer calling the early_fixmap_init() in setup_arch() as well. I appreciate what you're saying here, but I don't think that it's better to keep the second call in setup_arch(). We rely on having a (stub) DT in order to find UEFI and ACPI tables, so the DT and ACPI setup can never be truly separated. We always need to map that DT in order to find the UEFI+ACPI tables, and in order to do that we must initialize the fixmap first. I don't think there's any good reason to keep a redundant call in setup_arch(); that's just misleading and potentially problematic if we ever change early_fixmap_init() so that it's not idempotent. I agree it's somewhat a layering violation for early_fdt_map() to be responsible for initialising the fixmap, so how about we just pull that out, e.g. as below? Mark. ---->8---- >From 5f07f9c1d352f760fa7aba97f1b4f42d9cb99496 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Tue, 20 Feb 2024 11:09:17 +0000 Subject: [PATCH] arm64: clean up fixmap initalization Currently we have redundant initialization of the fixmap, first in early_fdt_map(), and then again in setup_arch() before we call early_ioremap_init(). This redundant initialization isn't harmful, as early_fixmap_init() happens to be idempotent, but it's redundant and potentially confusing. We need to call early_fixmap_init() before we map the FDT and before we call early_ioremap_init(). Ideally we'd place early_fixmap_init() and early_ioremap_init() in the same caller as early_ioremap_init() is somewhat coupled with the fixmap code. Clean this up by moving the calls to early_fixmap_init() and early_ioremap_init() into a new early_mappings_init() function, and calling this once in __primary_switched() before we call early_fdt_map(). This means we initialize the fixmap once, and keep early_fixmap_init() and early_ioremap_init() together. This is a cleanup, not a fix, and does not need to be backported to stable kernels. Reported-by: Daero Lee Signed-off-by: Mark Rutland Cc: Catalin Marinas Cc: Itaru Kitayama Cc: Will Deacon --- arch/arm64/include/asm/setup.h | 1 + arch/arm64/kernel/head.S | 2 ++ arch/arm64/kernel/setup.c | 11 ++++++----- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/arch/arm64/include/asm/setup.h b/arch/arm64/include/asm/setup.h index 2e4d7da74fb87..c8ba018bcc09f 100644 --- a/arch/arm64/include/asm/setup.h +++ b/arch/arm64/include/asm/setup.h @@ -9,6 +9,7 @@ void *get_early_fdt_ptr(void); void early_fdt_map(u64 dt_phys); +void early_mappings_init(void); /* * These two variables are used in the head.S file. diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index cab7f91949d8f..c60c5454c5704 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -510,6 +510,8 @@ SYM_FUNC_START_LOCAL(__primary_switched) #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) bl kasan_early_init #endif + bl early_mappings_init + mov x0, x21 // pass FDT address in x0 bl early_fdt_map // Try mapping the FDT early mov x0, x20 // pass the full boot status diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 42c690bb2d608..7a539534ced78 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -176,8 +176,6 @@ void __init *get_early_fdt_ptr(void) asmlinkage void __init early_fdt_map(u64 dt_phys) { int fdt_size; - - early_fixmap_init(); early_fdt_ptr = fixmap_remap_fdt(dt_phys, &fdt_size, PAGE_KERNEL); } @@ -290,6 +288,12 @@ u64 cpu_logical_map(unsigned int cpu) return __cpu_logical_map[cpu]; } +asmlinkage void __init early_mappings_init(void) +{ + early_fixmap_init(); + early_ioremap_init(); +} + void __init __no_sanitize_address setup_arch(char **cmdline_p) { setup_initial_init_mm(_stext, _etext, _edata, _end); @@ -305,9 +309,6 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p) */ arm64_use_ng_mappings = kaslr_requires_kpti(); - early_fixmap_init(); - early_ioremap_init(); - setup_machine_fdt(__fdt_pointer); /* -- 2.30.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel