|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-xen-4.5 v3 04/16] x86: Introduce MultiBoot Data (MBD) type
On Thu, Oct 09, 2014 at 11:28:04AM +0100, Andrew Cooper wrote:
> On 08/10/14 18:52, 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>
> > ---
> > v3 - suggestions/fixes:
> > - rename some variables
> > (suggested by Andrew Cooper),
> > - remove unneeded initialization
> > (suggested by Andrew Cooper),
> > - improve comments
> > (suggested by Andrew Cooper),
> > - further patch split rearrangement
> > (suggested by Andrew Cooper and Jan Beulich).
> >
> > v2 - suggestions/fixes:
> > - improve inline assembly
> > (suggested by Andrew Cooper and Jan Beulich),
> > - use __used attribute
> > (suggested by Andrew Cooper),
> > - patch split rearrangement
> > (suggested by Andrew Cooper and Jan Beulich).
> > ---
> > xen/arch/x86/Makefile | 1 +
> > xen/arch/x86/boot/cmdline.S | 9 ++---
> > xen/arch/x86/boot/head.S | 2 +-
> > xen/arch/x86/boot/reloc.c | 69 +++++++++++++++++++++-----------
> > xen/arch/x86/boot/x86_64.S | 10 +++--
> > xen/arch/x86/boot_info.c | 59 +++++++++++++++++++++++++++
> > xen/arch/x86/setup.c | 41 ++++++++++++-------
> > xen/arch/x86/x86_64/asm-offsets.c | 5 +--
> > xen/include/asm-x86/mbd.h | 79
> > +++++++++++++++++++++++++++++++++++++
> > 9 files changed, 225 insertions(+), 50 deletions(-)
> > create mode 100644 xen/arch/x86/boot_info.c
> > create mode 100644 xen/include/asm-x86/mbd.h
> >
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index 86ca5f8..8425e65 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -43,6 +43,7 @@ obj-y += pci.o
> > obj-y += percpu.o
> > obj-y += physdev.o
> > obj-y += psr.o
> > +obj-y += boot_info.o
> > obj-y += setup.o
> > obj-y += shutdown.o
> > obj-y += smp.o
> > diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
> > index 00687eb..dd1a027 100644
> > --- a/xen/arch/x86/boot/cmdline.S
> > +++ b/xen/arch/x86/boot/cmdline.S
> > @@ -152,17 +152,14 @@ cmdline_parse_early:
> > pusha
> >
> > /* Bail if there is no command line to parse. */
> > - mov sym_phys(multiboot_ptr),%ebx
> > - mov MB_flags(%ebx),%eax
> > - test $4,%al
> > - jz .Lcmdline_exit
> > - mov MB_cmdline(%ebx),%eax
> > + mov sym_phys(mbd_pa),%ebx
> > + mov MBD_cmdline_pa(%ebx),%eax
>
> What sets the value of MBD_cmdline_pa? You are blindly consuming it
> here without checks the flags for its presence, but I can't spot
> anything in this patch which ever sets it.
This translates to mbd->cmdline. If mbd->cmdline == 0 then there is not
cmdline passed to xen.gz. It translates to test %eax,%eax one line below.
Please check also xen/arch/x86/boot/reloc.c:__reloc() and
xen/arch/x86/boot/reloc.c:mb_mbd() for more details.
Hmmm... Now I am thinking that MBD_cmdline_pa() should be MBD_cmdline()
as it was earlier. This will make less confusion.
> > test %eax,%eax
> > jz .Lcmdline_exit
> >
> > /* Check for 'no-real-mode' command-line option. */
> > pushl $sym_phys(.Lno_rm_opt)
> > - pushl MB_cmdline(%ebx)
> > + pushl MBD_cmdline_pa(%ebx)
> > call .Lfind_option
> > test %eax,%eax
> > setnz %al
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index cd43952..f1b872a 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -120,7 +120,7 @@ __start:
> > mov $sym_phys(cpu0_stack)+1024,%esp
> > push %ebx
> > call reloc
> > - mov %eax,sym_phys(multiboot_ptr)
> > + mov %eax,sym_phys(mbd_pa)
> >
> > /* Initialize BSS (no nasty surprises!) */
> > mov $sym_phys(__bss_start),%edi
> > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> > index b678f67..59edb4d 100644
> > --- a/xen/arch/x86/boot/reloc.c
> > +++ b/xen/arch/x86/boot/reloc.c
> > @@ -12,8 +12,11 @@
> >
> > typedef unsigned int u32;
> >
> > +#include "../../../include/xen/compiler.h"
> > #include "../../../include/xen/multiboot.h"
> >
> > +#include "../../../include/asm/mbd.h"
> > +
> > /* entered with %eax = BOOT_TRAMPOLINE */
> > asm (
> > " .text \n"
> > @@ -22,7 +25,7 @@ asm (
> > " call 1f \n"
> > "1: pop %ebx \n"
> > " mov %eax,alloc-1b(%ebx) \n"
> > - " jmp reloc \n"
> > + " jmp __reloc \n"
>
> Why do you need to change the name of the reloc() function? reloc()
> seems like a fine name.
I will leave it as is.
> > );
> >
> > /*
> > @@ -50,6 +53,13 @@ static u32 alloc_struct(u32 bytes)
> > return s;
> > }
> >
> > +static void zero_struct(u32 s, u32 bytes)
>
> "static void memclear(void *ptr, size_t bytes)" would seem to be more
> appropriate, and avoid you needing to cast the pointers you actually
> pass into it.
OK.
> > +{
> > + asm volatile(
> > + " rep stosb \n"
>
> For a single instruction instruction asm statement, just use 'asm
> volatile ("rep stosb" ...' No need for these multi-line hoops to create
> a readable .S
OK.
> > + : "+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.
> > +
> > + if ( mbi->flags & MBI_MEMMAP )
> > + {
> > + mbd->mmap_size = mbi->mmap_length;
> > + mbd->mmap = copy_struct(mbi->mmap_addr, mbi->mmap_length);
> > + }
> >
> > if ( mbi->flags & MBI_MODULES )
> > {
> > - module_t *mods = (module_t *)copy_struct(
> > - mbi->mods_addr, mbi->mods_count * sizeof(module_t));
> > + mbd->mods_nr = mbi->mods_count;
> > + mbd->mods = alloc_struct(mbi->mods_count * sizeof(boot_module_t));
> >
> > - mbi->mods_addr = (u32)mods;
> > + mbi_mods = (module_t *)mbi->mods_addr;
> > + mbd_mods = (boot_module_t *)mbd->mods;
> >
> > for ( i = 0; i < mbi->mods_count; i++ )
> > {
> > - if ( mods[i].string )
> > - mods[i].string = copy_string(mods[i].string);
> > + mbd_mods[i].start = mbi_mods[i].mod_start;
> > + mbd_mods[i].end = mbi_mods[i].mod_end;
> > + mbd_mods[i].cmdline = copy_string(mbi_mods[i].string);
> > + mbd_mods[i].relocated = 0;
> > }
> > }
> >
> > - if ( mbi->flags & MBI_MEMMAP )
> > - mbi->mmap_addr = copy_struct(mbi->mmap_addr, mbi->mmap_length);
> > + return mbd;
> > +}
> >
> > - if ( mbi->flags & MBI_LOADERNAME )
> > - mbi->boot_loader_name = copy_string(mbi->boot_loader_name);
> > +static mbd_t __used *__reloc(void *mbi)
> > +{
> > + mbd_t *mbd;
> >
> > - /* Mask features we don't understand or don't relocate. */
> > - mbi->flags &= (MBI_MEMLIMITS |
> > - MBI_BOOTDEV |
> > - MBI_CMDLINE |
> > - MBI_MODULES |
> > - MBI_MEMMAP |
> > - MBI_LOADERNAME);
> > + mbd = (mbd_t *)alloc_struct(sizeof(mbd_t));
> > + zero_struct((u32)mbd, sizeof(mbd_t));
> >
> > - return mbi;
> > + return mb_mbd(mbd, mbi);
> > }
> > diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
> > index bfbafd2..ef8c735 100644
> > --- a/xen/arch/x86/boot/x86_64.S
> > +++ b/xen/arch/x86/boot/x86_64.S
> > @@ -29,8 +29,12 @@
> > test %ebx,%ebx
> > jnz start_secondary
> >
> > - /* Pass off the Multiboot info structure to C land. */
> > - mov multiboot_ptr(%rip),%edi
> > + /* Init mbi. */
>
> This is wholly useless as a comment. "Initialise Multiboot Info" would
> be better, but still identifiable from the name of the function.
OK.
> > + mov mbd_pa(%rip),%edi
> > + call __init_mbi
> > +
> > + /* Pass off the mbi to C land. */
>
> Again, for the commit it would be useful to expand mbi.
OK.
> > + movq %rax,%rdi
> > call __start_xen
> > ud2 /* Force a panic (invalid opcode). */
> >
> > @@ -38,7 +42,7 @@
> >
> > .data
> > .align 8
> > -multiboot_ptr:
> > +mbd_pa:
> > .long 0
> >
> > .word 0
> > diff --git a/xen/arch/x86/boot_info.c b/xen/arch/x86/boot_info.c
> > new file mode 100644
> > index 0000000..a2799aa
> > --- /dev/null
> > +++ b/xen/arch/x86/boot_info.c
> > @@ -0,0 +1,59 @@
> > +/*
> > + * Copyright (c) 2013, 2014 Oracle Co., Daniel Kiper
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program. If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <xen/types.h>
> > +#include <xen/cache.h>
> > +#include <xen/init.h>
> > +#include <xen/multiboot.h>
> > +
> > +#include <asm/mbd.h>
> > +#include <asm/page.h>
> > +
> > +static multiboot_info_t __read_mostly mbi;
> > +
> > +extern void enable_exception_support(void);
>
> No externs in .c files. This should be in a header file.
I am still do not understand what is wrong with that. We need
that function only here. So, why we should put it in a header?
> (I am still not particularly happy about splitting exception support
> like this, but I can't currently suggest a better alternative)
I am too.
> > +
> > +unsigned long __init __init_mbi(u32 mbd_pa)
> > +{
> > + mbd_t *mbd = __va(mbd_pa);
> > +
> > + enable_exception_support();
> > +
> > + if ( mbd->boot_loader_name ) {
>
> Xen style
OK.
> > + mbi.flags = MBI_LOADERNAME;
> > + mbi.boot_loader_name = mbd->boot_loader_name;
> > + }
> > +
> > + if ( mbd->cmdline ) {
> > + mbi.flags |= MBI_CMDLINE;
> > + mbi.cmdline = mbd->cmdline;
> > + }
> > +
> > + mbi.flags |= MBI_MEMLIMITS;
> > + mbi.mem_lower = mbd->mem_lower;
> > + mbi.mem_upper = mbd->mem_upper;
> > +
> > + mbi.flags |= MBI_MEMMAP;
> > + mbi.mmap_length = mbd->mmap_size;
> > + mbi.mmap_addr = mbd->mmap;
> > +
> > + mbi.flags |= MBI_MODULES;
> > + mbi.mods_count = mbd->mods_nr;
> > + mbi.mods_addr = mbd->mods;
> > +
> > + return __pa(&mbi);
> > +}
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 8c8b91f..24e1be3 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -529,6 +529,25 @@ static char * __init cmdline_cook(char *p, char
> > *loader_name)
> > return p;
> > }
> >
> > +void __init enable_exception_support(void)
>
> This should have bsp somewhere in the name, as it is specifically
> different to the ap method of gaining exception support.
I think that enable_bsp_exception_support(void) will be fine.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |