From owner-freebsd-arch Wed Aug 8 11:21:39 2001 Delivered-To: freebsd-arch@freebsd.org Received: from tasogare.imasy.or.jp (tasogare.imasy.or.jp [202.227.24.5]) by hub.freebsd.org (Postfix) with ESMTP id 3885837B411; Wed, 8 Aug 2001 11:21:15 -0700 (PDT) (envelope-from iwasaki@jp.FreeBSD.org) Received: from localhost (iwasaki.imasy.or.jp [202.227.24.92]) by tasogare.imasy.or.jp (8.11.3+3.4W/8.11.3/tasogare) with ESMTP/inet id f78IL0I71108; Thu, 9 Aug 2001 03:21:00 +0900 (JST) (envelope-from iwasaki@jp.FreeBSD.org) To: bde@zeta.org.au Cc: iwasaki@jp.FreeBSD.org, arch@FreeBSD.ORG, audit@FreeBSD.ORG Subject: Re: CFR: Some bug fixes in i386/i386/machdep.c In-Reply-To: <20010809005326.Y8258-100000@besplex.bde.org> References: <20010808030258E.iwasaki@jp.FreeBSD.org> <20010809005326.Y8258-100000@besplex.bde.org> X-Mailer: Mew version 1.94.1 on Emacs 19.34 / Mule 2.3 (SUETSUMUHANA) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-Id: <20010809032100T.iwasaki@jp.FreeBSD.org> Date: Thu, 09 Aug 2001 03:21:00 +0900 From: Mitsuru IWASAKI X-Dispatcher: imput version 20000228(IM140) Lines: 147 Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Hi, Thanks Mike and Bruce for your comments. > I think `atop(addr - 1)' is correct. Reasons: > - the segment limit is a maximum page number, not a number of pages, so > rounding down is correct. Yes, this is what I wanted to confirm. The segment limit for GPRIV_SEL should be 0 because sizeof(struct privatespace) (or sizeof(struct globaldata)) is less than PAGE_SIZE. And I'll change all i386_btop() to atop() within my patch. > > Also I've found too early warning printing before calling cninit(). > > This would never warn to users. > > Does it get put into the message buffer that early? I couldn't find any messages printed before cninit() from dmesg. > Some style bugs here. The declarations of ints should be: > > int gsel_tss, metadata_missing, off, x; > > (except some of these variables shouldn't be ints), and the initializer > should be elsewhere. OK. > > #ifdef SMP > > gdt_segs[GPRIV_SEL].ssd_limit = > > - i386_btop(sizeof(struct privatespace)) - 1; > > + i386_btop(sizeof(struct privatespace) + PAGE_SIZE - 1); > > I think the " + PAGE_SIZE" term gives an off by one error. Moving the > " - 1" term into the macro arg is sufficient. Understood. > > + if (metadata_missing) { > > + printf("WARNING: loader(8) metadata is missing!\n"); > > + } > > + > > Style bug: too many braces. OK. > Grepping for i386_btop in machdep.c shows some related bugs: > > /* > * The code segment limit has to cover the user area until we move > * the signal trampoline out of the user area. This is safe because > * the code segment cannot be written to directly. > */ > #define VM_END_USER_R_ADDRESS (VM_END_USER_RW_ADDRESS + UPAGES * PAGE_SIZE) > ldt_segs[LUCODE_SEL].ssd_limit = i386_btop(VM_END_USER_R_ADDRESS) - 1; > ldt_segs[LUDATA_SEL].ssd_limit = i386_btop(VM_END_USER_RW_ADDRESS) - 1; > > The " - 1" term should be moved into the macro arg here too. The comment > and the UPAGES term rotted long ago. Peter Wemm seems to be collecting > UPAGES garbage now (he just committed removal of signal trampoline garbage > iin libkern/mcount.c). Perhaps he has already fixed this. OK, I'll change this as well, except for the comments ;) Thanks again, here is my revised diffs. Index: machdep.c =================================================================== RCS file: /home/ncvs/src/sys/i386/i386/machdep.c,v retrieving revision 1.465 diff -u -r1.465 machdep.c --- machdep.c 2001/07/26 23:06:44 1.465 +++ machdep.c 2001/08/08 17:36:31 @@ -1774,24 +1774,23 @@ init386(first) int first; { - int x; struct gate_descriptor *gdp; - int gsel_tss; + int gsel_tss, metadata_missing, off, x; #ifndef SMP /* table descriptors - used to load tables by microp */ struct region_descriptor r_gdt, r_idt; #endif - int off; proc0.p_addr = proc0paddr; atdevbase = ISA_HOLE_START + KERNBASE; + metadata_missing = 0; if (bootinfo.bi_modulep) { preload_metadata = (caddr_t)bootinfo.bi_modulep + KERNBASE; preload_bootstrap_relocate(KERNBASE); } else { - printf("WARNING: loader(8) metadata is missing!\n"); + metadata_missing = 1; } if (bootinfo.bi_envp) kern_envp = (caddr_t)bootinfo.bi_envp + KERNBASE; @@ -1808,18 +1807,18 @@ * XXX text protection is temporarily (?) disabled. The limit was * i386_btop(round_page(etext)) - 1. */ - gdt_segs[GCODE_SEL].ssd_limit = i386_btop(0) - 1; - gdt_segs[GDATA_SEL].ssd_limit = i386_btop(0) - 1; + gdt_segs[GCODE_SEL].ssd_limit = atop(0 - 1); + gdt_segs[GDATA_SEL].ssd_limit = atop(0 - 1); #ifdef SMP gdt_segs[GPRIV_SEL].ssd_limit = - i386_btop(sizeof(struct privatespace)) - 1; + atop(sizeof(struct privatespace) - 1); gdt_segs[GPRIV_SEL].ssd_base = (int) &SMP_prvspace[0]; gdt_segs[GPROC0_SEL].ssd_base = (int) &SMP_prvspace[0].globaldata.gd_common_tss; SMP_prvspace[0].globaldata.gd_prvspace = &SMP_prvspace[0].globaldata; #else gdt_segs[GPRIV_SEL].ssd_limit = - i386_btop(sizeof(struct globaldata)) - 1; + atop(sizeof(struct globaldata) - 1); gdt_segs[GPRIV_SEL].ssd_base = (int) &__globaldata; gdt_segs[GPROC0_SEL].ssd_base = (int) &__globaldata.gd_common_tss; @@ -1876,8 +1875,8 @@ * the code segment cannot be written to directly. */ #define VM_END_USER_R_ADDRESS (VM_END_USER_RW_ADDRESS + UPAGES * PAGE_SIZE) - ldt_segs[LUCODE_SEL].ssd_limit = i386_btop(VM_END_USER_R_ADDRESS) - 1; - ldt_segs[LUDATA_SEL].ssd_limit = i386_btop(VM_END_USER_RW_ADDRESS) - 1; + ldt_segs[LUCODE_SEL].ssd_limit = atop(VM_END_USER_R_ADDRESS - 1); + ldt_segs[LUDATA_SEL].ssd_limit = atop(VM_END_USER_RW_ADDRESS - 1); for (x = 0; x < sizeof ldt_segs / sizeof ldt_segs[0]; x++) ssdtosd(&ldt_segs[x], &ldt[x].sd); @@ -1920,6 +1919,9 @@ */ cninit(); + if (metadata_missing) + printf("WARNING: loader(8) metadata is missing!\n"); + #ifdef DEV_ISA isa_defaultirq(); #endif To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message