From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f73.google.com (mail-wm1-f73.google.com [209.85.128.73]) (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 7157621D011 for ; Fri, 28 Nov 2025 14:03:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.73 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764338594; cv=none; b=czbBlIb/u4/smG3Z8IPFdAAf2vMv2VZqsaI8UmCkFaQmD8pLTr/UJXnJUaN7AF/dxZzxQ+3HZsnyqTkC8y5FpYW/5y78AS7yiAXGfg/l5oSgHPQdxhAwfOPGXmIn1jTrCQqVLYGwr7yonpOsxlqbwPvqVs8dfd1PuPdsZU+4AL0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764338594; c=relaxed/simple; bh=S+Z5YWz2PM6SaBjkZIVC3sXxn0ukesggnGgkrow8BkA=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=iRfU3zq3dKIHDrjc0dP4hv9kJHFxnXzyEgS9VrkKiUMon6PrTFyWcXGDWlde1bD9v+DpyK0YoPk2h4AgsVv7NWoPFlHXmdGlh2ckbz+D4S/I8hF6MWT3x5g6V2gNUUl0Ix330yNxMyPD4Mg7gjDCVsd90ilb2PXrcSNYfTQ1d1k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--jackmanb.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=azP1YuI6; arc=none smtp.client-ip=209.85.128.73 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--jackmanb.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="azP1YuI6" Received: by mail-wm1-f73.google.com with SMTP id 5b1f17b1804b1-47775585257so14040815e9.1 for ; Fri, 28 Nov 2025 06:03:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1764338591; x=1764943391; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=Gal2Vwmmzqjb3RMXbU5ToMCsZaUPd/m33j3gzGXLEsw=; b=azP1YuI6JE5EglNOvGuduAkOCUWsR+Luf/v3q/gJohpLlQMO2KXj6AP/S8pFiRT/RB RKlQXNPrwgLukvIOwyEJzGaWdGtguXQf5vnBuKSZdnx7Ww0ZAE6XngUJ05bLmmMz32Yp 4ofbhH02H91wLWHK/0PHENWk1qd+YHK22GWlK8MxZenNTeHIrcj27VnVaCR/Jm3Mzj33 AoRRNRMnQ/A1ZD05PrY9usFk16wcpvCQ6peyxvVRidUi54SslYLzsSQH6nHblS+DZvgs NQ9mNTYlDbFW46wsso4xpFrRsm/wycOtBxyDwW5eckB5mjVh5zFDCoBoWBxjWc7Qsigf Cmmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764338591; x=1764943391; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Gal2Vwmmzqjb3RMXbU5ToMCsZaUPd/m33j3gzGXLEsw=; b=rKMK6sGHo6bZPGO5QRd85gAbTEB+YSA8G9qt51+viQhsQ3X8DBuzDirQvCAn4cwe02 2OYlCuGAqSCFt0jz8RYncSU5u41rZ+AyCzbz/8kLxWvHQyjU/JHWo811sTkQuVBUrzXe /2KJkuZkXhU8t8GVdq9TkpWM3H9lNpKFKaYRGivCy1xS6g8AJd5Xqf6bSgLfGbssjAVB 9MkLg/nGOVUFL0QeM1rjVFjzM5Dt1BRJzZQeAGCpjX8SRQiKSIx2VEPvZbenqWtkkL+u bovP521H+Hcuan6t1dftOj5uNPrfnZMCk0c8PuGYBxIWj8iwXWhOA3mO16+67S6a5w7U 9ZFw== X-Forwarded-Encrypted: i=1; AJvYcCVyleZOIde4TiE70ctilnDK2DmDIj9m0qTJghz9Uyd+xxf+93ch8tutJdE6qK3htHG0++oHUu1fI/klRyA=@vger.kernel.org X-Gm-Message-State: AOJu0YyO/Td6jQk9Shzcg+CdhISh4o7XYE/GvnVKIeu0hGrT8JGpt6sT 03wRWmSIvCQXD6EZZnuYwh+PBw9Y42qgSk0Y3kKkzNhI3i7w2msUoS08/zKYaMKrltmpACkcvmN X1sr7UFqkSo930w== X-Google-Smtp-Source: AGHT+IGamWyWCb3wzowLUczsgETda/vV4fGof8Pe7BvnORZlHRXHsj0GcGMv0UxmLUAjhIYdmqDDzVxWWiv8Hg== X-Received: from wmwz1-n2.prod.google.com ([2002:a05:600d:6401:20b0:477:9ddb:5f06]) (user=jackmanb job=prod-delivery.src-stubby-dispatcher) by 2002:a05:600c:8b35:b0:477:3543:3a3b with SMTP id 5b1f17b1804b1-477c10c85d5mr265265725e9.6.1764338590804; Fri, 28 Nov 2025 06:03:10 -0800 (PST) Date: Fri, 28 Nov 2025 14:03:09 +0000 In-Reply-To: <20251127143501.GAaShhlVTH5iQpCdPM@fat_crate.local> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20251003-x86-init-cleanup-v1-0-f2b7994c2ad6@google.com> <20251003-x86-init-cleanup-v1-2-f2b7994c2ad6@google.com> <20251127143501.GAaShhlVTH5iQpCdPM@fat_crate.local> X-Mailer: aerc 0.21.0 Message-ID: Subject: Re: [PATCH 2/4] x86/mm: harmonize return value of phys_pte_init() From: Brendan Jackman To: Borislav Petkov , Brendan Jackman Cc: Thomas Gleixner , Ingo Molnar , Dave Hansen , , "H. Peter Anvin" , Andy Lutomirski , Peter Zijlstra , Content-Type: text/plain; charset="UTF-8" On Thu Nov 27, 2025 at 2:35 PM UTC, Borislav Petkov wrote: > On Fri, Oct 03, 2025 at 04:56:42PM +0000, Brendan Jackman wrote: >> In the case that they encounter pre-existing mappings, all the other >> phys_*_init()s include those pre-mapped PFNs in the returned value. >> Excluding those PFNs only when they are mapped at 4K seems like an >> error. So make it consistent. >> >> The other functions only include the existing mappings if the >> page_size_mask would have allowed creating those mappings. >> 4K pages can't be disabled by page_size_mask so that condition is not >> needed here; paddr_last can be assigned unconditionally before checking >> for existing mappings. >> >> Signed-off-by: Brendan Jackman >> --- >> arch/x86/mm/init_64.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c >> index 9e45b371a6234b41bd7177b81b5d432341ae7214..968a5092dbd7ee3e7007fa0c769eff7d7ecb0ba3 100644 >> --- a/arch/x86/mm/init_64.c >> +++ b/arch/x86/mm/init_64.c >> @@ -492,6 +492,8 @@ phys_pte_init(pte_t *pte_page, unsigned long paddr, unsigned long paddr_end, >> continue; >> } >> >> + paddr_last = paddr_next; >> + >> /* >> * We will re-use the existing mapping. >> * Xen for example has some special requirements, like mapping > > I don't understand: the other phys_*_init() things do: > > if (!XXX_none()) > > ... > > paddr_last = paddr_next; > > while you've raised the assignment above that test. Well they actually do this: if (!p*_none()) { if (!p*_leaf()) { paddr_last = ... continue; } if (page_size_mask & *) { paddr_last = ... continue; } } if (page_size_mask & *) { paddr_last = * continue; } paddr_last = * That is, they update paddr_last unconditionally. While before this patch, phys_pte_init() skips the update in the !pte_non() case. > Also "seems like an error" needs a lot more poking at because if it is an > error, then its incarnation must be really nasty and subtle or it is not, and > then we don't care. And it has been that way for a while now... Before the patchset, the return value of kernel_physical_mapping_init() means something like: 1. The last physical address that was mapped. 2. ... This includes addresses that were already mapped before the call 3. ... UNLESS that pre-existing mapping was 4K. In patch 4/4 I'm claiming: > The exact definition of this is pretty fiddly, but only when there is a mismatch > between the alignment of the requested range and the page sizes allowed > by page_size_mask, or when the range ends in a region that is not mapped > according to e820. Which would not be true given point 3 above. Without this phys_pte_init() change, the return value of init_memory_mapping() is fiddly even if you are allow arbitary page sizes and all the paddrs you're trying to map definitely exist, because of the 4K special-case in point 4. Instead of trying to justify why init_memory_mapping() doesn't care even about that special-case, I just removed that special-case because I think it was probably a bug anyway. HOWEVER... with the wisdom of hindsight... this was a VERY obscure and confusing way to go about writing the patchset. I apologise! I think the right way to do this is to drop this patch (2/4) and evaluate the remainder against the claim that init_memory_mapping() doesn't care about the return value at all. So that would have to mean: a. It only calls kernel_physical_mapping_init() for physical ranges that exist. b. It always uses a page_size_mask that matches the alignment of the ranges it's passing. c. It doesn't operate on ranges that already have mappings. Am I making a bit more sense now...?