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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.14 at 21:24, <daniel.kiper@xxxxxxxxxx> wrote:
> 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.

There's no hard boundary between what the pre-__start_xen()
code in the EFI case (or the equivalent code in the non-EFI one)
can access, and I don't think there needs to be.

>> > --- /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?

Add a comment explaining why the header is deliberately not
being included? Or only use standard C types (perhaps with
suitable BUILD_BUG_ON()s put elsewhere)?

Jan


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