From owner-freebsd-current@FreeBSD.ORG Fri Sep 2 12:40:02 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8783D106564A for ; Fri, 2 Sep 2011 12:40:02 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 578A88FC17 for ; Fri, 2 Sep 2011 12:40:02 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 0E58546B35; Fri, 2 Sep 2011 08:40:02 -0400 (EDT) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 91F398A02E; Fri, 2 Sep 2011 08:40:01 -0400 (EDT) From: John Baldwin To: Robert Millan Date: Fri, 2 Sep 2011 08:40:01 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110617; KDE/4.5.5; amd64; ; ) References: <201109011728.13979.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201109020840.01128.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Fri, 02 Sep 2011 08:40:01 -0400 (EDT) Cc: freebsd-current@freebsd.org Subject: Re: [PATCH] Fix NKPT kernel config option X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Sep 2011 12:40:02 -0000 On Friday, September 02, 2011 1:23:16 am Robert Millan wrote: > 2011/9/1 John Baldwin : > > In general we force the relevant C files to use opt_*.h includes and avoid > > nested includes of those in headers. > > With this approach I can't trust that this feature will do the right > thing. I would rather modify pmap.h by hand than run the unnecessary > risk. > > > Do you know of any C files that do > > are using NKPT (or values derived from it) without including opt_pmap.h? > > Many. To obtain a full list, I suggest you put something like: > > #ifdef NKPT > #warning nkpt:good > #else > #warning nkpt:bad > #endif > > in pmap.h and build with "WERROR=" This is rather pointless as it dosen't catch the places that actually _use_ NKPT. The header is used in lots of places for other things it defines such as 'struct pmap' itself. However, NKPT is a purely MD item that is only used in the amd64 code itself. Specifically, it is used here: amd64/minidump_machdep.c: for (va = VM_MIN_KERNEL_ADDRESS; va < MAX(KERNBASE + NKPT * NBPDR, amd64/minidump_machdep.c: for (va = VM_MIN_KERNEL_ADDRESS; va < MAX(KERNBASE + NKPT * NBPDR, amd64/pmap.c: KPTphys = allocpages(firstaddr, NKPT); amd64/pmap.c: for (i = 0; i < NKPT; i++) { amd64/pmap.c: for (i = 0; i < NKPT; i++) { amd64/pmap.c: if (KERNBASE < addr && addr <= KERNBASE + NKPT * NBPDR) conf/NOTES:options NKPT=31 include/pmap.h:#ifndef NKPT include/pmap.h:#define NKPT 32 include/pmap.h:#define NKPDPE howmany(NKPT, NPDEPG)/* number of kernel PDP slots */ This gives uses in amd64/minidump_machdep.c and amd64/pmap.c and to define the macro NKPDPE. That macro is also MD and is only used here: amd64/pmap.c: KPDphys = allocpages(firstaddr, NKPDPE); amd64/pmap.c: for (i = 0; i < NKPDPE; i++) { include/pmap.h:#define NKPDPE howmany(NKPT, NPDEPG)/* number of kernel PDP slots */ Thus, the only two files that use NKPT are amd64/minidump_machdep.c and amd64/pmap.c. Both of these files include "opt_pmap.h": amd64/minidump_machdep.c:#include "opt_pmap.h" amd64/pmap.c:#include "opt_pmap.h" A more useful test would be to alter pmap.h so it says: #ifndef NKPT #define NKPT doesnt_compile #endif and build a kernel config that contains 'options NKPT=31'. It will build just fine as the sabotaged default value will never get used. Were NKPT to be widely used, the correct fix would still not be to add an include of opt_pmap.h to . Instead, the corect fix would be to move the option to opt_global.h as is done for things like SMP, INVARIANTS, etc. -- John Baldwin