From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-183.mta1.migadu.com (out-183.mta1.migadu.com [95.215.58.183]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6FC8D292918 for ; Wed, 24 Jun 2026 12:36:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.183 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782304612; cv=none; b=ERk+c8PGRDDDy6ctJs+BPvExUzyXsg1M/tFA8dxOs8j3Rr9c3gfinjbx70XDL7bILew3AYz5HvnSBSE6sW8vhWC/Bp0zXeNHe73CZ06iweF+DlLbbV8KyfkSw3Lk/83dy6y5Znic9saCE5JYnuLNeh6MHaLJe7ICpy9Yf0NH3a0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782304612; c=relaxed/simple; bh=3Cu7F61eQhRGmZ3GVI1lBR0ywo212SXPrKgJKji0Ooc=; h=Mime-Version:Content-Type:Date:Message-Id:Cc:Subject:From:To: References:In-Reply-To; b=MYPa97TfeIL0593VFm0cxrBVjcAI/B3S2TMSD5CVyIp1yUJLCbvLPKEX9poEddBne6Qu/4YS7M7xzWbDv3HisGS3E+6Oq4VygIvM1lzXnHCYQth3qmWej7oIaINhDTXAVbjXkxzox+ZHwPdpY+VR+POV6c3IgDrfo9ON22pW6IU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=UtHvTbTg; arc=none smtp.client-ip=95.215.58.183 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="UtHvTbTg" Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1782304607; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=AeHguvZW8+mWDabmfI/lNteZ0xl2tWaUx6KJdOn24/c=; b=UtHvTbTgv2ReT3uC6HecwjcVC5rJdDekkNYYwjfiyLyiTeao0r8kBKbUO90ww27sYQawWI 4y1pjK8H5DckdE7uBMd63rWJvO+/X9uf8V1RoU/aRwpCXv3OJVBHkoq1IrUhsmFrTV1Lh+ qyf+ePwV16fF7XcgnASa40qExVWOi7U= Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 24 Jun 2026 12:36:35 +0000 Message-Id: Cc: Subject: Re: [PATCH v2 2/3] x86/mm: simplify calculation of max_pfn_mapped X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: "Brendan Jackman" To: "Brendan Jackman" , "Dave Hansen" , "Brendan Jackman" , "Ingo Molnar" , "Borislav Petkov" , "Dave Hansen" , , "H. Peter Anvin" , "Andy Lutomirski" , "Peter Zijlstra" , "Thomas Gleixner" References: <20260503-x86-init-cleanup-v2-0-bb690bd2477c@google.com> <20260503-x86-init-cleanup-v2-2-bb690bd2477c@google.com> In-Reply-To: X-Migadu-Flow: FLOW_OUT On Wed Jun 3, 2026 at 10:20 AM UTC, Brendan Jackman wrote: > On Tue Jun 2, 2026 at 9:39 PM UTC, Dave Hansen wrote: >> On 5/3/26 06:04, Brendan Jackman wrote: >> ... >>> Luckily, init_memory_mapping() avoids all these conditions. In that >>> case, the return value is just paddr_end. And that value is already >>> present, no need to depend on the confusing return value. >> >> It feels like we should say something about split_mem_range() here. All >> of the guaranteed non-fiddly behavior originates in there, right? > > [pasting back the conditions from the commit message for context] >>> but only in these conditions: >>>=20 >>> 1. There is a mismatch between the alignment of the requested range = and >>> the page sizes allowed by page_size_mask >>> >>> 2. The range ends in a region that is not mapped according to >>> e820. >>> >>> 3. The range ends in a region that was already mapped (note this cas= e is >>> particularly fiddly because the return value depends on what leve= l >>> the existing mapping is at. This is probably a bug, see [0] for >>> discussion). > > split_mem_range() is responsible for excluding point 1, since it returns > the correct page_size_mask. The other two are actually down to the > callers, right?=20 > > So how about for point 1 I mention that in the commit message, then for > points 2 and 3 maybe they should actually be code comments, i.e. > documented as preconditions for calling init_memory_mapping()? For posterity: I realised that treating point 2 as a separate case is bogus here. I was looking at the e820__mapped_any() blocks in phys_*_init() and noting that they don't update the local paddr_last. But actualy, those blocks only run for paddr>=3Dpaddr_end, which can already only happen in case 1 i.e. when the range is misaligned wrt page_size_mask. So I'm just gonna drop that bit. This realisation is really reinforcing that removing this return value is the right thing to do. >>> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c >>> index ae3e9e0820153..1a6a6fc700bb5 100644 >>> --- a/arch/x86/mm/init.c >>> +++ b/arch/x86/mm/init.c >>> @@ -544,10 +544,11 @@ void __ref init_memory_mapping(unsigned long star= t, >>> memset(mr, 0, sizeof(mr)); >>> nr_range =3D split_mem_range(mr, 0, start, end); >>> =20 >>> - for (i =3D 0; i < nr_range; i++) >>> - paddr_last =3D kernel_physical_mapping_init(mr[i].start, mr[i].end, >>> - mr[i].page_size_mask, >>> - prot); >>> + for (i =3D 0; i < nr_range; i++) { >>> + kernel_physical_mapping_init(mr[i].start, mr[i].end, >>> + mr[i].page_size_mask, prot); >>> + paddr_last =3D mr[i].end; >>> + } >> >> I guess this is actually: >> >> for (i =3D 0; i < nr_range; i++) >> kernel_physical_mapping_init(...); >> >> paddr_last =3D mr[nr_range-1].end; >> >> Right? But what you have is probably just as compact. > > Oh, weird. My code might be just as compact but it's confusing, it > should be written your way for sure.