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

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

On 25/09/14 19:42, Daniel Kiper wrote:
> On Wed, Sep 24, 2014 at 08:14:03PM +0100, Andrew Cooper wrote:
>> On 24/09/14 18:19, 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>
>> Much clearer than before.
>>
>> Can I suggest retroactively renaming the old multiboot bits to
> Do you mean before or after this (rewritten) patch?

I would suggest a separate patch, if others concur that it is a sensible
idea.

>
>> multiboot1 to make a more marked difference between between
>> "multiboot1_proto" and "multiboot2_proto" ?
> Do you think about labels in this file only or about all
> stuff related to multiboot(1) protocol?

Frankly, even changing the constant names to be MULTIBOOT1_$FOO would
aid in clarity of the code.

>
>> It would also be better to split the changing of multiboot1 and
>> shuffling of memory calculations away from the introduction of multiboot2.
>>
>>> ---
>>> v2 - suggestions/fixes:
>>>    - use "for" instead of "while" for loops
>>>      (suggested by Jan Beulich),
>>>    - properly parenthesize macro arguments
>>>
>>>     char *boot_loader_name;
>>>      (suggested by Jan Beulich),
>>>    - change some local variables types
>>>      (suggested by Jan Beulich),
>>>    - use meaningful labels
>>>      (suggested by Andrew Cooper and Jan Beulich),
>>>    - use local labels
>>>      (suggested by Jan Beulich),
>>>    - fix coding style
>>>      (suggested by Jan Beulich),
>>>    - patch split rearrangement
>>>      (suggested by Andrew Cooper and Jan Beulich).
>>> ---
>>>  xen/arch/x86/boot/head.S     |  117 +++++++++++++-
>>>  xen/arch/x86/boot/reloc.c    |  103 +++++++++++-
>>>  xen/include/xen/multiboot2.h |  354 ++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 567 insertions(+), 7 deletions(-)
>>>  create mode 100644 xen/include/xen/multiboot2.h
>>>
>>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>>> index 7e48833..4331821 100644
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
> [...]
>
>>> @@ -81,10 +144,56 @@ __start:
>>>          mov     %ecx,%es
>>>          mov     %ecx,%ss
>>>
>>> +        /* Assume multiboot[12].mem_lower is 0 if not set by bootloader */
>>> +        xor     %edx,%edx
>>> +
>> This change does not match its comment, and seems to have appeared from
>> nowhere.
> I still do not understand what is wrong with that. We need
> to set %edx to known value here (0 in this case). If MBI_MEMLIMITS
> or MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO is not set by bootloader
> and we do not initialize %edx then we will use uninitialized
> value later.
>
>>>          /* Check for Multiboot bootloader */
>>>          cmp     $MULTIBOOT_BOOTLOADER_MAGIC,%eax
>>> -        jne     not_multiboot
>>> +        je      multiboot_proto
>>> +
>>> +        /* Check for Multiboot2 bootloader */
>>> +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
>>> +        je      multiboot2_proto
>>> +
>>> +        jmp     not_multiboot
>>> +
>>> +multiboot_proto:
>>> +        /* Get mem_lower from Multiboot information */
>>> +        testb   $MBI_MEMLIMITS,(%ebx)
>>> +        jz      trampoline_setup    /* not available? BDA value will be fine */
>>> +
>>> +        mov     4(%ebx),%edx
>>> +        shl     $10-4,%edx
>>> +        jmp     trampoline_setup
>>>
>>> +multiboot2_proto:
>>> +        /* Get Multiboot2 information address */
>>> +        mov     %ebx,%ecx
>>> +
>>> +        /* Skip Multiboot2 information fixed part */
>>> +        add     $8,%ecx
>>> +
>>> +0:
>>> +        /* Get mem_lower from Multiboot2 information */
>>> +        cmpl    $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
>>> +        jne     1f
>>> +
>>> +        mov     8(%ecx),%edx
>>> +        shl     $10-4,%edx
>>> +        jmp     trampoline_setup
>>> +
>>> +1:
>>> +        /* Is it the end of Multiboot2 information? */
>>> +        cmpl    $MULTIBOOT2_TAG_TYPE_END,(%ecx)
>>> +        je      trampoline_setup
>>> +
>>> +        /* Go to next Multiboot2 information tag */
>>> +        add     4(%ecx),%ecx
>>> +        add     $(MULTIBOOT2_TAG_ALIGN-1),%ecx
>>> +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>>> +        jmp     0b
>>> +
>>> +trampoline_setup:
>>>          /* Set up trampoline segment 64k below EBDA */
>>>          movzwl  0x40e,%ecx          /* EBDA segment */
>>>          cmp     $0xa000,%ecx        /* sanity check (high) */
>>> @@ -99,12 +208,8 @@ __start:
>>>           * Compare the value in the BDA with the information from the
>>>           * multiboot structure (if available) and use the smallest.
>>>           */
>>> -        testb   $MBI_MEMLIMITS,(%ebx)
>>> -        jz      2f                  /* not available? BDA value will be fine */
>>> -        mov     4(%ebx),%edx
>>> -        cmp     $0x100,%edx         /* is the multiboot value too small? */
>>> +        cmp     $0x4000,%edx        /* is the multiboot value too small? */
>> You are going to need some justification for changing this lower bound
>> of memory.
> It is not changed. I did "shl $10-4,%edx" above so I must change
> relevant value here too.

And I saw that, given close inspection of the patch.  It is however not
obvious at a glance, and makes review harder.

A comment or paragraph in the commit message is perfectly fine (e.g.
"Alter min memory limit handling as we now may not find it from either
multiboot1 or multiboot2").  This makes it obvious to anyone reading the
patch that there is a real need for the memory limit handling to change,
which also covers setting a sane default of 0.

>  However, it looks that it left from my
> earlier work and I am able to remove that change safely.
>
> [...]
>
>>> diff --git a/xen/include/xen/multiboot2.h b/xen/include/xen/multiboot2.h
>> This multiboot2.h is huge - can we get away with only including struct
>> definitions for tags we currently use/support?
> I did that because multiboot.h contains all multiboot(1) related stuff.
> I think that it is nice to have all defs for multiboot2 protocol too,
> however, I do not insist on that.

I suppose it doesn't matter either way for a header file like this.

~Andrew