Re: [PATCH]: boot2 optimizations to shrink it

On Friday, October 22, 2010 9:37:02 am Roman Divacky wrote:
> Hi,
> 
> can you guys review this patch:
> 
> 	http://lev.vlakno.cz/~rdivacky/boot2.safe.patch
> 
> It shrinks boot2 by 52 bytes by:
> 
> 	eliminating memcpy() calls
> 
> 	buffer shrinking as we are only ever called with argument
> 	< 256 for the %u modifier
> 
> 	constifying write-only variable
> 
> Rui Paulo tested this, is this patch ok? May I commit this?

_at__at_ -348,7 +348,7 _at__at_
 	    return;
 	p += hdr.ex.a_data + roundup2(hdr.ex.a_bss, PAGE_SIZE);
 	bootinfo.bi_symtab = VTOP(p);
-	memcpy(p, &hdr.ex.a_syms, sizeof(hdr.ex.a_syms));
+	*p = hdr.ex.a_syms;
 	p += sizeof(hdr.ex.a_syms);
 	if (hdr.ex.a_syms) {
 	    if (xfsread(ino, p, hdr.ex.a_syms))
_at__at_ -385,7 +385,7 _at__at_
 	    if (xfsread(ino, &es, sizeof(es)))
 		return;
 	    for (i = 0; i < 2; i++) {
-		memcpy(p, &es[i].sh_size, sizeof(es[i].sh_size));
+		*p = es[i].sh_size;
 		p += sizeof(es[i].sh_size);
 		fs_off = es[i].sh_offset;
 		if (xfsread(ino, p, es[i].sh_size))

These are ok.

_at__at_ -567,7 +567,7 _at__at_
 printf(const char *fmt,...)
 {
     va_list ap;
-    char buf[10];
+    char buf[4];                /* we are only passed values < 256 */
     char *s;
     unsigned u;
     int c;

This may not always be true in the future.  I know I've printed out LBAs 
before for debugging issues with zfsboot.  I'd really rather not make this 
change if at all possible.

_at__at_ -612,10 +612,10 _at__at_
 static int
 drvread(void *buf, unsigned lba, unsigned nblk)
 {
-    static unsigned c = 0x2d5c7c2f;
+    static const unsigned c = 0x2d5c7c2f;
 
     if (!OPT_CHECK(RBX_QUIET))
-	printf("%c\b", c = c << 8 | c >> 24);
+	printf("%c\b", c << 8 | c >> 24);
     v86.ctl = V86_ADDR | V86_CALLF | V86_FLAGS;
     v86.addr = XREADORG;		/* call to xread in boot1 */
     v86.es = VTOPSEG(buf);

This breaks the twiddle.  With this change it will only ever print the second 
character in the twiddle set rather than rotating them.  Perhaps you could use 
an rol instruction for this instead?  Something like:

	if (!OPT_CHECK(RBX_QUIET)) {
		__asm ("rol %0,$8" : "=m" (&c));
		printf("%c\b", c);
	}

-- 
John Baldwin