Re: [Xen-devel] [PATCH v2 1/5] xen/x86: Introduce MultiBoot Data (MBD) type

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.09.14 at 20:40, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 24/09/14 18:19, Daniel Kiper wrote:
>> Introduce MultiBoot Data (MBD) type. It is used to define variable
>> which carry over data from multiboot protocol (any version) through
>> Xen preloader stage. Later all or parts of this data is used
>> to initialize boot_info structure. boot_info is introduced
>> in later patches.
>>
>> MBD helps to break multiboot (v1) protocol dependency. Using it
>> we are able to save space on trampoline (we do not allocate space
>> for unused data what happens in current preloader implementation).
>> Additionally, we are able to easily add new members to MBD if we
>> want support for new features or protocols.
>>
>> There is not plan to share MBD among architectures. It will be
>> nice if boot_info will be shared among architectures. Please
>> check later patches for more details.
>>
>> Code found in xen/arch/x86/boot_info.c moves MBD data to mbi struct
>> which is referenced from main Xen code. This is temporary solution
>> which helps to split patches into logical parts. Later it will be
>> replaced by final version of boot_info support.
>>
>> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> 
> Reviewing changes to ASM code is *very* hard, and this patch is still
> very complicated.
> 
> Can I suggest patches split along the following lines.
> 
> * Shuffling of registers in head.S to end up with %eax and %ecx as
> indended for reloc.c, and %ebx currently unclobbered, and adjusting
> reloc.c for %ecx.  Review for this is a simple case of verifying that
> the changes are just a strict shuffling of registers.
> 
> * Whitespace and misc non-MBD cleanup to reloc.c
> 
> * Altering existing helper functions in preparation for MBD support
> 
> * Actually introduce mbd_t and use it.

+1

>> --- a/xen/arch/x86/boot/reloc.c
>> +++ b/xen/arch/x86/boot/reloc.c
>> @@ -1,40 +1,58 @@
>> -/******************************************************************************
>> +/*
>>   * reloc.c
>> - * 
>> + *
>>   * 32-bit flat memory-map routines for relocating Multiboot structures
>>   * and modules. This is most easily done early with paging disabled.
>> - * 
>> + *
>>   * Copyright (c) 2009, Citrix Systems, Inc.
>> - * 
>> + * Copyright (c) 2013, 2014 Oracle Co., Daniel Kiper
>> + *
>>   * Authors:
>>   *    Keir Fraser <keir@xxxxxxx>
>> + *    Daniel Kiper
>>   */
>>  
>> -/* entered with %eax = BOOT_TRAMPOLINE */
>> +typedef unsigned char u8;
>> +typedef unsigned short u16;
>> +typedef unsigned long u32;
>> +typedef unsigned long long u64;
>> +
>> +#include "../../../include/xen/compiler.h"
>> +#include "../../../include/xen/multiboot.h"
>> +
>> +#include "../../../include/asm/mbd.h"
> 
> How about playing with -I for this file in the Makefile to allow
> #include <xen/compiler.h> and so ?

Including these here is bogus anyway, even if for the ones above it's
perhaps acceptable. But expressing it to be bogus via the ../../../
prefix is quite desirable imo.

>> +
>> +/*
>> + * __HERE__ IS ENTRY POINT!!!
> 
> I am still of the firm opinion that anyone capable of editing this file
> ought to know understand the _start symbol, making this comment useless.

Indeed.

>> +/*
>> + * MultiBoot Data (MBD) type. It is used to define variable which
>> + * carry over data from multiboot protocol (any version) through
>> + * Xen preloader stage. Later all or parts of this data is used
>> + * to initialize boot_info structure.
>> + */
>> +typedef struct {
>> +    /* Pointer to boot loader name. */
>> +    u32 boot_loader_name;
>> +
>> +    /* Pointer to Xen command line. */
>> +    u32 cmdline;
> 
> These need to be very clear about being physical addresses rather than
> pointers.

Isn't their type being u32 and their placement in this structure
sufficient indication thereof?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel