Message ID | 20240217140326.2367186-1-skseofh@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-69895-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2685:b0:108:e6aa:91d0 with SMTP id mn5csp337233dyc; Sat, 17 Feb 2024 06:05:52 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCUICdsvielb8hLb+EoUz3DCRMOQqt1c7AcjRnJQFcz3Po0dAR/mJGNlEs/yEqsEWXbOsrLc3Fy1Rq6bwTV/r6uIPCAz4A== X-Google-Smtp-Source: AGHT+IF82y5GaZn5lGJXVM1GP4WzuI2p9Jogwo6YUdPVzI8bDe38nUFFtaVk2Q3D7Bw8aAVEv39k X-Received: by 2002:a17:903:110c:b0:1d9:a2b1:8693 with SMTP id n12-20020a170903110c00b001d9a2b18693mr9715866plh.23.1708178752127; Sat, 17 Feb 2024 06:05:52 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708178752; cv=pass; d=google.com; s=arc-20160816; b=BR7C0KAncqed7UXTSk0ZHrNL7MSFULTY28UvLSMJRyxsdsp1tL3XxWLemV/U9Jddj5 DFWb7+4kHjfv6ojdfLNrJb3bnioZj2clyQMHqgUAqYFrthAEppKH4E+L3soRgewVi/3X TapJ6YMhiGRoaXmkBTwhu/INnr4VxJBZLtoUzctSwDV623V0rfix5yb/snz6Crx47CzR FgY4Mp6q59h40Z7GJWlndd9nguMeGhJnSxkrO68/eZjCVHVI5IBLS68AsGwoM8uWOny4 xYLapMlyXVcuKeUpAK7jAs6JccHRiTI5XzuKA2sIm9YyE3YgexL+FcVmeLAKhDZjuP9C +b6A== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:cc:to :from:dkim-signature; bh=6c0Z1DTg0XtAkeLkrVtLodDmYXe6vkXZYPCGkelpqWQ=; fh=hLmERoft3feeiVMN++1jtBjr5YtNcMEzVwIqg89mR7Q=; b=LN4843vRxJ9/cqAsPHYirxR0Tu5l+xW1j5+SFVmYsHzFfUS1RieRUKxobrRYF0fVIF KCDH6+PolbZCBooNMg1BOWBPQxfMO7Y8+3YUEVGcMRjFvY92SxW8ckgiOzpJt4DDjvUM S0YX74LiJ3MNeFiXeNhuU7hAKnKBMlxYTNpPkyom8uvqKhN6obQI/B9Ds87KPFExoPSl XNJ74suecVdkfHq0TdM2N1QO7VYR9H4IiyAQ8ggD09Wd4vDkQK3eNRXa9ZlHBQ0y7DQL 20DOOkgSldCpXU7D+N9ebuvfuM8mwHAH7hXKODhxVTXmbpQvvuitnO+ZmXkG3sUxbzvm zTVA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=LCcSL3AU; arc=pass (i=1 spf=pass spfdomain=gmail.com dkim=pass dkdomain=gmail.com dmarc=pass fromdomain=gmail.com); spf=pass (google.com: domain of linux-kernel+bounces-69895-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-69895-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [2604:1380:40f1:3f00::1]) by mx.google.com with ESMTPS id e16-20020a170902ef5000b001d569371c91si1479603plx.632.2024.02.17.06.05.51 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 17 Feb 2024 06:05:52 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-69895-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=LCcSL3AU; arc=pass (i=1 spf=pass spfdomain=gmail.com dkim=pass dkdomain=gmail.com dmarc=pass fromdomain=gmail.com); spf=pass (google.com: domain of linux-kernel+bounces-69895-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-69895-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id 542A8B20CDC for <ouuuleilei@gmail.com>; Sat, 17 Feb 2024 14:04:23 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 72F3D7B3E4; Sat, 17 Feb 2024 14:03:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="LCcSL3AU" Received: from mail-pl1-f173.google.com (mail-pl1-f173.google.com [209.85.214.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8AFA67AE59 for <linux-kernel@vger.kernel.org>; Sat, 17 Feb 2024 14:03:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.173 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708178616; cv=none; b=C92IVauKgOVjMEonfiBPxlaSEGNJXy1FTefaQvQlmzbe1TJBueBExHNeDn0RytKGhe4sfACOqDwsrBeqyIxIlZsxmyDow5Mnc5PfaXtyb/Xfn3UK3mzem6rmGefa/BL1QuBktwvgQRDYyQGN6aiWaku7+HkIWvTKZePXWzty5tU= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708178616; c=relaxed/simple; bh=N3kVayaMp3wVwgj+kIB/WmRnuaDXhFcV290FNAS36B8=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version; b=iHgFm/l6nmVfbe+xo6Znst5wfyb7CqAD1YrrluZctTqvacLyiPDO6KZcad/STIU3z3U5kkJ+0fR4TjqSDRKy3+H0oHsSc8pyi63Clbf4MuXb6nsxkY+fX3byrpC1qwcNI3xK13muCJsE3x4qet9ugQKAONvvs1a8zDyi+QWn6dA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=LCcSL3AU; arc=none smtp.client-ip=209.85.214.173 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pl1-f173.google.com with SMTP id d9443c01a7336-1dbae7b8ff2so7839105ad.3 for <linux-kernel@vger.kernel.org>; Sat, 17 Feb 2024 06:03:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708178615; x=1708783415; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=6c0Z1DTg0XtAkeLkrVtLodDmYXe6vkXZYPCGkelpqWQ=; b=LCcSL3AUyGx/PiFIAPqmDqCmrPgxibJDR7ismyJTe4SA+SywS5u2qw8DScK2hFAdwx YbkF+ZWgP+s31jEMbOHP5ET7fFNoVWZJqtVgykHcoOkJLqJ0S/ORW/0NoRZ2tnwoeD3k pz/tjCRqv5b/ta9vvFrjvYGZkZKI40YhIeZMqHy+sJj7r4QvMJsGXgcg4fgBg0GijETy NUb74oHnupOG5qc3hr8xPs6G5l6xneDGcytzXE1C6B8OxWxAlSPW6EEokmEMgdVbsb5u 1KO++hPTbvv1OMhuu9OdSxG70n8e3Mr4bn2frZ+DZ2rd/G9bVG11zDEsncG6q4j0dJRl 0mlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708178615; x=1708783415; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=6c0Z1DTg0XtAkeLkrVtLodDmYXe6vkXZYPCGkelpqWQ=; b=NYZJnf5vsk2KKtt4ELUgw5TU6+ZzuI5ALLA4BnErAtADu8/xGnCdLzPrvwMc3ivSBR p1EYcI+8cNkoILZVAFLkBm1/YWwlf2rduL1CrJ0L/uio416jD8wtgdTuSXuzvUDL0tbz ktMcTtlu5FAX8OYrMJwnTfj5Veosape9tVY8p32PYm3Xnkryg4+eDHXxDDZROYI3XbiT flNjD7WafPssyjlG6O9sLQtfKCKnTGJhyaI6BsVYCI9E8znrWEZnU170eDwwPjl9ImBh neRlDiqJ9l9LkSJU30lH5ClHm5d2sIm1B1ClOYjOqN66F4EVS0LCbM3EHBcxs5348ztJ ye1g== X-Forwarded-Encrypted: i=1; AJvYcCX1yUzrWX+mA3e6ol1CaUmuGzn/e3Zd+UuqXcvEowJydlC5I6h+MYZ3v27GWlB2XAEgGHI5v2WT7aMhkPBGB3RFxBPwPWLll85AcT9l X-Gm-Message-State: AOJu0Yz05cW4BjE9hBTdUcXpHl4RTNzLV6OqkgNkZRLgjfac0QtsPcuc egQpQoD1+iCY6BzOntyTgJIIhpJ6laENDQuviVDl+ig6He21LGNE X-Received: by 2002:a17:902:7847:b0:1db:5213:222 with SMTP id e7-20020a170902784700b001db52130222mr6737543pln.5.1708178614392; Sat, 17 Feb 2024 06:03:34 -0800 (PST) Received: from localhost.localdomain ([49.142.40.215]) by smtp.gmail.com with ESMTPSA id h3-20020a170902748300b001dbcb39dd80sm1032713pll.183.2024.02.17.06.03.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 17 Feb 2024 06:03:34 -0800 (PST) From: skseofh@gmail.com To: catalin.marinas@arm.com, will@kernel.org Cc: ryan.roberts@arm.com, mark.rutland@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Daero Lee <skseofh@gmail.com> Subject: [PATCH] arm64: add early fixmap initialization flag Date: Sat, 17 Feb 2024 23:03:26 +0900 Message-Id: <20240217140326.2367186-1-skseofh@gmail.com> X-Mailer: git-send-email 2.25.1 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791155243549976773 X-GMAIL-MSGID: 1791155243549976773 |
Series |
arm64: add early fixmap initialization flag
|
|
Commit Message
skseofh@gmail.com
Feb. 17, 2024, 2:03 p.m. UTC
From: Daero Lee <skseofh@gmail.com> 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. Signed-off-by: Daero Lee <skseofh@gmail.com> --- arch/arm64/mm/fixmap.c | 7 +++++++ 1 file changed, 7 insertions(+)
Comments
On Sat, Feb 17, 2024 at 11:03:26PM +0900, skseofh@gmail.com wrote: > From: Daero Lee <skseofh@gmail.com> > > 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(). Mark. > > Signed-off-by: Daero Lee <skseofh@gmail.com> > --- > arch/arm64/mm/fixmap.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c > index c0a3301203bd..fbdd5f30f3a1 100644 > --- a/arch/arm64/mm/fixmap.c > +++ b/arch/arm64/mm/fixmap.c > @@ -32,6 +32,8 @@ static pte_t bm_pte[NR_BM_PTE_TABLES][PTRS_PER_PTE] __page_aligned_bss; > static pmd_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss __maybe_unused; > static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss __maybe_unused; > > +static int early_fixmap_initialized __initdata; > + > static inline pte_t *fixmap_pte(unsigned long addr) > { > return &bm_pte[BM_PTE_TABLE_IDX(addr)][pte_index(addr)]; > @@ -100,10 +102,15 @@ void __init early_fixmap_init(void) > unsigned long addr = FIXADDR_TOT_START; > unsigned long end = FIXADDR_TOP; > > + if (early_fixmap_initialized) > + return; > + > pgd_t *pgdp = pgd_offset_k(addr); > p4d_t *p4dp = p4d_offset(pgdp, addr); > > early_fixmap_init_pud(p4dp, addr, end); > + > + early_fixmap_initialized = 1; > } > > /* > -- > 2.25.1 >
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 <skseofh@gmail.com> > > > > 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. Itaru. > > Mark. > > > > > Signed-off-by: Daero Lee <skseofh@gmail.com> > > --- > > arch/arm64/mm/fixmap.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c > > index c0a3301203bd..fbdd5f30f3a1 100644 > > --- a/arch/arm64/mm/fixmap.c > > +++ b/arch/arm64/mm/fixmap.c > > @@ -32,6 +32,8 @@ static pte_t bm_pte[NR_BM_PTE_TABLES][PTRS_PER_PTE] __page_aligned_bss; > > static pmd_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss __maybe_unused; > > static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss __maybe_unused; > > > > +static int early_fixmap_initialized __initdata; > > + > > static inline pte_t *fixmap_pte(unsigned long addr) > > { > > return &bm_pte[BM_PTE_TABLE_IDX(addr)][pte_index(addr)]; > > @@ -100,10 +102,15 @@ void __init early_fixmap_init(void) > > unsigned long addr = FIXADDR_TOT_START; > > unsigned long end = FIXADDR_TOP; > > > > + if (early_fixmap_initialized) > > + return; > > + > > pgd_t *pgdp = pgd_offset_k(addr); > > p4d_t *p4dp = p4d_offset(pgdp, addr); > > > > early_fixmap_init_pud(p4dp, addr, end); > > + > > + early_fixmap_initialized = 1; > > } > > > > /* > > -- > > 2.25.1 > >
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 <skseofh@gmail.com> > > > > > > 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 <mark.rutland@arm.com> 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 <skseofh@gmail.com> Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Itaru Kitayama <itaru.kitayama@linux.dev> Cc: Will Deacon <will@kernel.org> --- 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); /*
On Tue, Feb 20, 2024 at 11:55:30AM +0000, Mark Rutland wrote: > 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 <skseofh@gmail.com> > > > > > > > > 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. Okay. > > 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 <mark.rutland@arm.com> > 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 <skseofh@gmail.com> > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Itaru Kitayama <itaru.kitayama@linux.dev> > Cc: Will Deacon <will@kernel.org> > --- > 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); > > /* Thanks for this. Makes sense to me; would you post a proper patch so I can build and do a boot test, just to make sure? Reviewed-by: Itaru Kitayama <itaru.kitayama@fujitsu.com> Thanks, Itaru. > -- > 2.30.2 >
On Wed, Feb 21, 2024 at 08:14:00AM +0900, Itaru Kitayama wrote: > On Tue, Feb 20, 2024 at 11:55:30AM +0000, Mark Rutland wrote: > > From 5f07f9c1d352f760fa7aba97f1b4f42d9cb99496 Mon Sep 17 00:00:00 2001 > > From: Mark Rutland <mark.rutland@arm.com> > > 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. > Thanks for this. Makes sense to me; would you post a proper patch > so I can build and do a boot test, just to make sure? I took a look, and Ard's recent changes to the boot code have removed the duplicate call to early_fixmap_init() by other means, so we don't need to do anything, and can forget about this patch. :) Mark.
diff --git a/arch/arm64/mm/fixmap.c b/arch/arm64/mm/fixmap.c index c0a3301203bd..fbdd5f30f3a1 100644 --- a/arch/arm64/mm/fixmap.c +++ b/arch/arm64/mm/fixmap.c @@ -32,6 +32,8 @@ static pte_t bm_pte[NR_BM_PTE_TABLES][PTRS_PER_PTE] __page_aligned_bss; static pmd_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss __maybe_unused; static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss __maybe_unused; +static int early_fixmap_initialized __initdata; + static inline pte_t *fixmap_pte(unsigned long addr) { return &bm_pte[BM_PTE_TABLE_IDX(addr)][pte_index(addr)]; @@ -100,10 +102,15 @@ void __init early_fixmap_init(void) unsigned long addr = FIXADDR_TOT_START; unsigned long end = FIXADDR_TOP; + if (early_fixmap_initialized) + return; + pgd_t *pgdp = pgd_offset_k(addr); p4d_t *p4dp = p4d_offset(pgdp, addr); early_fixmap_init_pud(p4dp, addr, end); + + early_fixmap_initialized = 1; } /*