Re: [Xen-devel] [PATCH for-xen-4.5 v3 04/16] x86: Introduce MultiBoot Data (MBD) type

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Oct 13, 2014 at 11:21:46AM +0100, Andrew Cooper wrote:
> On 10/10/14 12:47, Daniel Kiper wrote:
> >
> >>> +       : "+D" (s), "+c" (bytes) : "a" (0));
> >>> +}
> >>> +
> >>>  static u32 copy_struct(u32 src, u32 bytes)
> >>>  {
> >>>      u32 dst, dst_asm;
> >>> @@ -77,41 +87,56 @@ static u32 copy_string(u32 src)
> >>>      return copy_struct(src, p - (char *)src + 1);
> >>>  }
> >>>
> >>> -multiboot_info_t *reloc(multiboot_info_t *mbi_old)
> >>> +static mbd_t *mb_mbd(mbd_t *mbd, multiboot_info_t *mbi)
> >>>  {
> >>> -    multiboot_info_t *mbi = (multiboot_info_t 
> >>> *)copy_struct((u32)mbi_old, sizeof(*mbi));
> >>> -    int i;
> >>> +    boot_module_t *mbd_mods;
> >>> +    module_t *mbi_mods;
> >>> +    u32 i;
> >>> +
> >>> +    if ( mbi->flags & MBI_LOADERNAME )
> >>> +        mbd->boot_loader_name = copy_string(mbi->boot_loader_name);
> >>>
> >>>      if ( mbi->flags & MBI_CMDLINE )
> >>> -        mbi->cmdline = copy_string(mbi->cmdline);
> >>> +        mbd->cmdline = copy_string(mbi->cmdline);
> >>> +
> >>> +    if ( mbi->flags & MBI_MEMLIMITS )
> >>> +    {
> >>> +        mbd->mem_lower = mbi->mem_lower;
> >>> +        mbd->mem_upper = mbi->mem_upper;
> >>> +    }
> >> Now I am completely confused.  here you set mbd from mbi, but lower in
> >> init_mbi, you set mbi from mbd.  What is the intended dataflow?
> > We need mbi together with boot_info to make smooth transition from mbi
> > to boot_info possible. Patch #10 completely removes mbi from Xen main code.
> > I mentioned about that in commit message.
>
> That is perhaps all well and fine, but my initial point still stands.
>
> In this patch alone, you set mbd here from mbi, but later in
> __init_mbi(), initialise mbi from mbd.
>
> I can't spot anywhere else which initialises either of the structures
> with values, so I can only assume they are both 0's (or even
> uninitialised) all the way through the boot process.

__reloc() takes mbi from bootloader and initializes mbd to zero.
Later mbd is filled with needed data from mbi passed by bootloader.
xen/arch/x86/boot_info.c:static multiboot_info_t __read_mostly mbi
is initialized by GCC to 0 (I removed explicit initialization as you
requested). Later xen/arch/x86/boot_info.c:mbi is filled with data
from mbd and then passed to __start_xen().

Daniel

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