Re: [PATCH 1/2] Relocator framework

[Robert Millan <rmh@aybabtu.com>]

On Mon, Aug 03, 2009 at 02:06:03PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Hello. As discussed on IRC it would be preferable if all loaders use
> flexible technics for loading kernels using relocators as currently
> multiboot and xnu does.

Well, as for what I said on IRC, I'm not sure if it'd be preferable.  I
think it *could* be, if it makes things simpler & more maintainable (which
greatly depends on how the patches for multiboot.c/linux.c look like).

Anyway, I'll comment some things on this patch:

> +#ifdef __x86_64__
> +extern grub_uint64_t grub_relocator32_backward_src;
> +#else
> +extern grub_uint32_t grub_relocator32_backward_src;
> +#endif

You could make this a pointer, or grub_uintptr_t
(the latter we don't yet have, it seems like a good excuse to
add it if a pointer is not suitable :-)).

> +#ifndef __x86_64__
> +	/* mov imm32, %eax */
> +	.byte	0xb8
> +RELOCATOR_VARIABLE(dest)
> +	.long 0
> +	mov %eax, %edi

Please use size qualifiers (e.g. movl).

Also, remember to indent the first parameter like we do elsewhere
(e.g. ".long\t0", "movl\t%eax, %edi", etc).

> +	/* Backward movsl is implicitly off-by-four.  compensate that.  */
> +	subl	$4,	%esi
> +	subl	$4,	%edi
> +
> +	/* Backward copy.  */
> +	std
> +	addl	%ecx, %esi
> +	addl	%ecx, %edi
> +
> +	rep
> +	movsl

Are you sure moving to movsl is a good idea?  Maybe the payload size is not
4-aligned.

> +#ifdef APPLE_CC
> +	add $(cont0 - base), %eax
> +#else
> +	add $(cont0 - base), %rax
> +#endif

What's the issue at hand?  Apple assembler [1] can't add an inmediate to
%rax ?

Truncating it seems like it could be problematic if %eax overflows.

[1] btw, APPLE_CC macro for assembler checks is really confusing

> +	/* Update %cs. Thanks to David Miller for pointing this mistake out. */
> +	ljmp *(jump_vector - base) (%esi,1)

Comments ought to be for useful information.  For praise/acknoledgements (and I
think David deserves it) we have the THANKS file.

> +	/* Disable paging. */
> +	mov %cr0, %eax
> +	and $0x7fffffff, %eax
> +	mov %eax, %cr0
> +
> +	/* Disable amd64. */
> +	mov $0xc0000080, %ecx
> +	rdmsr
> +	and $0xfffffeff, %eax
> +	wrmsr
> +
> +	/* Turn off PAE. */
> +	movl %cr4, %eax
> +	and $0xffffffcf, %eax
> +	mov %eax, %cr4

Please use macros for such things (see GRUB_MEMORY_MACHINE_CR0_PE_ON).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."