Date: Sat, 5 May 2012 20:49:47 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andriy Gapon <avg@freebsd.org> Cc: freebsd-fs@freebsd.org, freebsd-hackers@freebsd.org Subject: Re: [review request] zfsboot/zfsloader: support accessing filesystems within a pool Message-ID: <20120505194459.D1295@besplex.bde.org> In-Reply-To: <4FA4F36A.6030903@FreeBSD.org> References: <4F8999D2.1080902@FreeBSD.org> <4FA29E1B.7040005@FreeBSD.org> <4FA2A307.2090108@FreeBSD.org> <201205041125.15155.jhb@freebsd.org> <4FA4F36A.6030903@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 5 May 2012, Andriy Gapon wrote: > on 04/05/2012 18:25 John Baldwin said the following: >> On Thursday, May 03, 2012 11:23:51 am Andriy Gapon wrote: >>> on 03/05/2012 18:02 Andriy Gapon said the following: >>>> >>>> Here's the latest version of the patches: >>>> http://people.freebsd.org/~avg/zfsboot.patches.4.diff >>> >>> I've found a couple of problems in the previous version, so here's another one: >>> http://people.freebsd.org/~avg/zfsboot.patches.5.diff >>> The important change is in the first patch (__exec args). >> >> A few comments/suggestions on the args bits: > > John, > > these are excellent suggestions! Thank you! > Some comments: >> - Add #ifndef LOCORE guards to the new header around the structure so >> it can be used in assembly as well as C. > > Done. I have had to go into a few btx makefiles and add a necessary include > path and -DLOCORE to make the header usable from asm. Ugh, why not use genassym, as is done for all old uses of this header in locore.s, at least on i386 (5% of the i386 genassym.c is for this). >> - Move BI_SIZE and ARGOFF into the header as constants. > > Done. > >> - Add a CTASSERT() in loader/main.c that BI_SIZE == sizeof(struct bootinfo) Ugh, BI_SIZE was already used in locore.s. It wasn't the size of the struct, but was the offset of the field that gives the size. No CTASSERT() was needed -- the size is whatever it is, as given by sizeof() on the struct at the time of compilation of the utility that initializes the struct. It was a feature that sizeof() and offsetof() can't be used in asm so they must be translated in genassym and no macros are needed in the header (the size was fully dynamic, so the asm code only needs the offsetof() values). Of course, you could use CTASSERT()s to check that the struct layout didn't get broken. The old code just assumes that the struct is packed by the programmer and that the arch's struct packing conventions don't change, so that for example BI_SIZE = offsetof(struct bootinfo, bi_size) never changes. genassym is hard to use in boot programs, but the old design was that boot programs shouldn't use bootinfo in asm and should just use the target bootinfo.h at compile time (whatever time the target is compiled). Anyway, LOCORE means "for use in locore.[sS]", so other uses of it, e.g. in boot programs, are bogus. > > I have added a definition of CTASSERT to boostrap.h as it was not available for > sys/boot and there were two local definitions of the macro in individual files. > > However the assertion would fail right now. > The backward-compatible value of BI_SIZE (72 == 0x48) covers only part of the This isn't backwards compatible. BI_SIZE was decimal 48 (covers everything up to the bi_size field). > fields in struct bootinfo, those up to the following comment: > /* Items below only from advanced bootloader */ > I am a little bit hesitant: should I increase BI_SIZE to cover the whole struct > bootinfo or should I compare BI_SIZE to offsetof bi_kernend? Neither. BI_SIZE shouldn't exist. It defeats the bi_size field. >> Maybe >> create a 'struct kargs_ext' that looks like this: >> >> struct kargs_ext { >> uint32_t size; >> char data[0]; >> }; > > I've decided to skip on this. Use KNF indentation and KNF field prefixes (ka_) if you add it :-). Generic field names like `size' and `data' need prefixes more than mos. The old struct was: % #define N_BIOS_GEOM 8 % ... % /* % * A zero bootinfo field often means that there is no info available. % * Flags are used to indicate the validity of fields where zero is a % * normal value. % */ % struct bootinfo { % u_int32_t bi_version; % u_int32_t bi_kernelname; /* represents a char * */ % u_int32_t bi_nfs_diskless; /* struct nfs_diskless * */ % /* End of fields that are always present. */ The original size was apparently 12. % #define bi_endcommon bi_n_bios_used Another style difference. The magic 12 is essentially given by this macro. This macro is a pseudo-field, like the ones for the copyable and zeroable regions in struct proc. Its name is in lower case. % u_int32_t bi_n_bios_used; % u_int32_t bi_bios_geom[N_BIOS_GEOM]; The struct was broken in 1994 by adding the above 2 fields without providing any way to distinguish it from the old struct. % u_int32_t bi_size; % u_int8_t bi_memsizes_valid; % u_int8_t bi_bios_dev; /* bootdev BIOS unit number */ % u_int8_t bi_pad[2]; % u_int32_t bi_basemem; % u_int32_t bi_extmem; % u_int32_t bi_symtab; /* struct symtab * */ % u_int32_t bi_esymtab; /* struct symtab * */ The above 8 fields were added in 1995 (together with fixing style bugs like no prefixes for the old field names). Now the struct is determined by its size according to the bi_size field, and the bi_version field is not really used (it's much easier to add stuff to the end than to support multiple versions). This gives a range of old sizes/versions: 12: ~1993 (FreeBSD-1) 48: ~1994 (FreeBSD-1 and/or 2) 0x48: FreeBSD-2 post-1995 But these old sizes are uninteresting since only boot loaders from before 1993-1995 support only the above fields, and these loaders can't boot current kernels. % /* Items below only from advanced bootloader */ % u_int32_t bi_kernend; /* end of kernel space */ % u_int32_t bi_envp; /* environment */ % u_int32_t bi_modulep; /* preloaded modules */ Added in 1998. Still uninteresting, since boot loaders newer than that are needed to boot current kernels (mainly for elf). % uint64_t bi_hcdp; /* DIG64 HCDP table */ % uint64_t bi_fpswa; /* FPSWA interface */ % uint64_t bi_systab; /* pa of EFI system table */ % uint64_t bi_memmap; /* pa of EFI memory map */ % uint64_t bi_memmap_size; /* size of EFI memory map */ % uint64_t bi_memdesc_size; /* sizeof EFI memory desc */ % uint32_t bi_memdesc_version; /* EFI memory desc version */ Added in 2010. Are all of these uint64_t types correct? The padding seems to be broken, so that these fields would not work for amd64: we're at offset 0x48 for bi_kernend. The 3 uint32_t's added in 1998 reach 0x54. Then all the uint64_t fields are misaligned on i386, and on amd64 there is unnamed padding before the first of them to align them. But and64 doesn't use bootinfo.h in the kernel, so I think only the i386 version is used on amd64 (in the boot loader), so the misaligned case isn't used. The struct declaration is also broken at the end. The last field is 32 bits, so there is unnamed padding after it on amd64 only. This padding should be explicit, like the padding before the uint64_t fields, or just put the 32-bit field before the 64-bit fields. % }; So apart you could hard-code the size to the 1998 value of 0x54 without losing anything except the buggy 2010 fields. But it shouldn't be hard-coded. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120505194459.D1295>