From owner-freebsd-arch Wed Aug 8 8:41:59 2001 Delivered-To: freebsd-arch@freebsd.org Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by hub.freebsd.org (Postfix) with ESMTP id D2F1637B40A; Wed, 8 Aug 2001 08:41:48 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id BAA26360; Thu, 9 Aug 2001 01:41:35 +1000 Date: Thu, 9 Aug 2001 01:39:06 +1000 (EST) From: Bruce Evans X-X-Sender: To: Mitsuru IWASAKI Cc: , Subject: Re: CFR: Some bug fixes in i386/i386/machdep.c In-Reply-To: <20010808030258E.iwasaki@jp.FreeBSD.org> Message-ID: <20010809005326.Y8258-100000@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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 On Wed, 8 Aug 2001, Mitsuru IWASAKI wrote: > Hi, I've noticed that there are some bugs in i386/i386/machdep.c. > With following patches on printing, I got strange result from dmesg. It sure is dusty in there. In FreeBSD_1.x, the segment limit computations were all written as `i386_btop(addr) - 1'. This worked in all cases because `addr' was always a multiple of PAGE_SIZE. Cloning this code for small segments gave the bugs that you observed. 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. - we mostly have addresses, not byte counts, so using i386_btop() (bytes to pages) is bogus and using atop() (address to pages) is correct. Using atop() to convert byte counts to pages is bogus, but I suspect that it is often used. Anyway, we have too many functions to convert between bytes and pages. > 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? > Following is bug fixes for above problems. Please review it. > I'll commit this weekend if no objections. > 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/07 17:41:22 > @@ -1782,6 +1782,7 @@ > struct region_descriptor r_gdt, r_idt; > #endif > int off; > + int metadata_missing = 0; 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. > @@ -1808,18 +1809,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 = i386_btop(0 - 1); > + gdt_segs[GDATA_SEL].ssd_limit = i386_btop(0 - 1); 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. > .... > - i386_btop(sizeof(struct globaldata)) - 1; > + i386_btop(sizeof(struct globaldata) + PAGE_SIZE - 1); Similarly. > @@ -1920,6 +1921,10 @@ > */ > cninit(); > > + if (metadata_missing) { > + printf("WARNING: loader(8) metadata is missing!\n"); > + } > + Style bug: too many braces. 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. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message