Re: [PATCH 1/2] Relocator framework

[Robert Millan]

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."