Date: Thu, 09 Aug 2001 03:21:00 +0900 From: Mitsuru IWASAKI <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 Message-ID: <20010809032100T.iwasaki@jp.FreeBSD.org> In-Reply-To: <20010809005326.Y8258-100000@besplex.bde.org> References: <20010808030258E.iwasaki@jp.FreeBSD.org> <20010809005326.Y8258-100000@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010809032100T.iwasaki>