Re: [PATCH RFC 5/7] xen/x86: Add multiboot2 protocol support

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/09/14 14:34, Daniel Kiper wrote:
> On Sun, Aug 10, 2014 at 06:23:55PM +0100, Andrew Cooper wrote:
>> On 09/08/14 00:04, Daniel Kiper wrote:
>>> Add multiboot2 protocol support. This way we are laying the foundation
>>> for EFI + GRUB2 + Xen development.
>>>
>>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>>> ---
>>>  xen/arch/x86/boot/head.S  |  112 ++++++++++++++++++++++++++++++++++++++++++++-
>>>  xen/arch/x86/boot/reloc.c |  103 ++++++++++++++++++++++++++++++++++++++++-
>>>  2 files changed, 212 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> [...]
>
>>> @@ -81,10 +144,55 @@ __start:
>>>          mov     %ecx,%es
>>>          mov     %ecx,%ss
>>>
>>> +        /* Set mem_lower to 0 */
>>> +        xor     %edx,%edx
>>> +
>> How does this affect mem_lower? it doesn't appear relevant in context.
> %edx is used to store mem_lower value from multiboot[12] protocol. It must
> have sensible value even if multiboot loader do not pass mem_lower value.
> That is why %edx is zeroed here.
>
>>>          /* Check for Multiboot bootloader */
>>> -        cmp     $0x2BADB002,%eax
>>> -        jne     not_multiboot
>>> +        cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
>>> +        je      1f
>> This $MULTIBOOT_BOOTLOADER_MAGIC should probably be separate, as there
>> are other bits of code which should use multiboot manifest constants.
> You mean?

A cleanup patch changing $0x2BADB002 -> $MULTIBOOT_BOOTLOADER_MAGIC
should be separate from a functional change of adding multiboot 2 support.

There are other places where you have mixed cleanup with adding new
functionality.

Cleanup is fine, but it *must* be separate from functional changes. 
Reviewing asm code is hard enough without having to work out whether the
change is functional or cleanup, or a mix of the two.

~Andrew