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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Sep 24, 2014 at 07:40:44PM +0100, Andrew Cooper wrote:
> On 24/09/14 18:19, Daniel Kiper wrote:

[...]

> > @@ -556,21 +575,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >          .stop_bits = 1
> >      };
> >
> > -    /* Critical region without IDT or TSS.  Any fault is deadly! */
> > -
> > -    set_processor_id(0);
> > -    set_current((struct vcpu *)0xfffff000); /* debug sanity. */
> > -    idle_vcpu[0] = current;
> > -
> > -    percpu_init_areas();
> > -
> > -    init_idt_traps();
> > -    load_system_tables();
> > -
> > -    smp_prepare_boot_cpu();
> > -    sort_exception_tables();
> > -
> > -    /* Full exception support from here on in. */
> > +    if ( !efi_enabled ) {
> > +        /* Exception support was enabled before __start_xen() call. */
> > +    }
> > +    else
> > +    {
> > +        enable_exception_support();
> > +    }
>
> For this, absolutely not.
>
> Pass mbi_pa into __start_xen() and have __start_xen() call
> __init_boot_info() after it has set up exception support.  None of the
> contents of MBD are needed beforehand.

Ehhh... I do not like this solution too. However, I want to avoid
passing mbd to __start_xen(). If we do that as you suggest then
EFI case will look bad because we would not be able to pass boot_info
directly to __start_xen(). boot_info is build directly in EFI case.
We do not need mbd. It means that __init_boot_info() is not used on
EFI platforms too. Additionally, I think that __start_xen() is firm border
between preloader and Xen main code and this as is should not see
any variables belonging to preloader (mbd is preloader stuff). Hence,
I think that __start_xen() should take boot_info as an argument.
However, I agree that we should work out better solution for
exception initialization.

> >      loader = (mbi->flags & MBI_LOADERNAME)
> >          ? (char *)__va(mbi->boot_loader_name) : "unknown";
> > diff --git a/xen/arch/x86/x86_64/asm-offsets.c 
> > b/xen/arch/x86/x86_64/asm-offsets.c
> > index 3994f4d..2123eb3 100644
> > --- a/xen/arch/x86/x86_64/asm-offsets.c
> > +++ b/xen/arch/x86/x86_64/asm-offsets.c
> > @@ -12,7 +12,7 @@
> >  #include <compat/xen.h>
> >  #include <asm/fixmap.h>
> >  #include <asm/hardirq.h>
> > -#include <xen/multiboot.h>
> > +#include <asm/mbd.h>
> >
> >  #define DEFINE(_sym, _val)                                                 
> > \
> >      asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" 
> > \
> > @@ -163,6 +163,5 @@ void __dummy__(void)
> >      OFFSET(CPUINFO_features, struct cpuinfo_x86, x86_capability);
> >      BLANK();
> >
> > -    OFFSET(MB_flags, multiboot_info_t, flags);
> > -    OFFSET(MB_cmdline, multiboot_info_t, cmdline);
> > +    OFFSET(MBD_cmdline, mbd_t, cmdline);
> >  }
> > diff --git a/xen/include/asm-x86/mbd.h b/xen/include/asm-x86/mbd.h
> > new file mode 100644
> > index 0000000..96e3044
> > --- /dev/null
> > +++ b/xen/include/asm-x86/mbd.h
> > @@ -0,0 +1,70 @@
> > +/*
> > + * 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/>.
> > + */
> > +
> > +#ifndef __MBD_H__
> > +#define __MBD_H__
>
> #include <xen/types.h>
>
> Do not rely on the translation using including this file to make u32
> available for you.

I did not include this header deliberately. Sadly mbd.h is used in
reloc.c and if we do that then build will be broken. However, if you
wish I could add ifndef __FOOBAR__ and then define __FOOBAR__ in reloc.c.
Or maybe you have better idea?

Daniel

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