Re: [PATCH 1/2] Relocator framework

[Vladimir 'phcoder' Serbinenko]

>> +#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 :-)).
grub_addr_t would actually do the job.
>
>> +#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).
>
Ok
>> +     /* 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.
>
On AFAIK x86 movsl works on unaligned addresses too. I'll recheck
>> +#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 ?
>
Apple linker can't handle 64-bit differences
> Truncating it seems like it could be problematic if %eax overflows.
>
Actually it's not a problem since this code is put together with
kernel which executes in 32-bit mode so it can't be over 4GiB. If we
base 64-bit relocator it will be a problem if OS wants to go over
4GiB.
> [1] btw, APPLE_CC macro for assembler checks is really confusing
>
I'll try with __apple__
> Please use macros for such things (see GRUB_MEMORY_MACHINE_CR0_PE_ON).
Will look into it
>
> --
> 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."
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git