|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 4/7] xen/x86: Migrate to XBI structure
Hi,
Sorry for late reply but I am recovering after conferences and vacation.
On Sun, Aug 10, 2014 at 06:05:38PM +0100, Andrew Cooper wrote:
> On 09/08/14 00:04, Daniel Kiper wrote:
> > We have all constants and structures in place. So, finally break multiboot
> > (v1)
> > protocol dependency. It means that most of Xen code (excluding preloader)
> > could be bootloader agnostic and does not need almost any knowledge about
> > boot protocol. Additionally, we are able to pass all boot data to
> > __start_xen()
> > in one bucket without any side channels. I do not mention that we are also
> > able to easily identify boot data in Xen code.
> >
> > Here is boot data flow for legacy BIOS platform:
> >
> > BIOS -> GRUB -> multiboot[12]* -> __reloc() -> MBD ->-\
> > /
> > -----------------<-----------------
> > \
> > \
> > ---> __init_xbi() -> XBI_MB -> __start_xen() -> XBI
> > /
> > BIOS ->-/
> >
> > * multiboot2 is not implemented yet. Look for it in next patch.
> >
> > Here is boot data flow for EFI platform:
> >
> > EFI -> efi_start() -> XBI_EFI -> __start_xen() -> XBI
> >
> > WARNING: ARM build could be broken by this patch. We need to agree XBI
> > integration into ARM. Personally I think that it is worth storing all
> > data from any bootloader and preloader in XBI on any architecture. This
> > give a chance to share more code between architectures. However, every
> > architecture should define its own XBI (in relevant include/asm directory).
> > Despite that it looks that some parts of it could be common, e.g. modules
> > data, command line arguments, boot loader name, EFI data, etc., even if
> > types
> > would not be the same. So, as it was stated above a lot of code could be
> > shared among architectures.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
>
> This patch is massive, and really needs splitting up somewhat. There
> are several logically distinct bits to it.
Well... It is quite difficult because original mbi is touched in many places.
So, if you remove its definition then it means that you must remove all
references
to it to not break build. Hence, one option which I can see is that we can
introduce
MBD in first patch (probably we will need some transitional code which will
bind MBD
with mbi used after preloader phase) and later in second patch we can replace
mbi
with XBI (or BI, whatever, how we call it is not so important). This way we
will have
more or less half of below code in every of two patches. I hope that help.
> > ---
> > xen/arch/x86/Makefile | 1 +
> > xen/arch/x86/boot/cmdline.S | 9 +-
> > xen/arch/x86/boot/head.S | 31 ++--
> > xen/arch/x86/boot/reloc.c | 153 ++++++++++++-----
> > xen/arch/x86/boot/x86_64.S | 10 +-
> > xen/arch/x86/dmi_scan.c | 7 +-
> > xen/arch/x86/domain_build.c | 24 +--
> > xen/arch/x86/efi/boot.c | 212 +++++++++++------------
> > xen/arch/x86/efi/efi.h | 3 -
> > xen/arch/x86/efi/runtime.c | 52 ++++--
> > xen/arch/x86/init_xbi.c | 254 +++++++++++++++++++++++++++
> > xen/arch/x86/microcode.c | 39 ++---
> > xen/arch/x86/mpparse.c | 9 +-
> > xen/arch/x86/platform_hypercall.c | 19 +--
> > xen/arch/x86/setup.c | 340
> > +++++++++++--------------------------
> > xen/arch/x86/x86_64/asm-offsets.c | 5 +-
> > xen/drivers/acpi/osl.c | 9 +-
> > xen/drivers/video/vesa.c | 5 +-
> > xen/drivers/video/vga.c | 16 +-
> > xen/include/asm-x86/config.h | 2 -
> > xen/include/asm-x86/e820.h | 8 -
> > xen/include/asm-x86/edd.h | 6 -
> > xen/include/asm-x86/setup.h | 10 +-
> > xen/include/xen/efi.h | 10 --
> > xen/include/xen/vga.h | 4 -
> > xen/include/xsm/xsm.h | 14 +-
> > xen/xsm/xsm_core.c | 6 +-
> > xen/xsm/xsm_policy.c | 14 +-
> > 28 files changed, 723 insertions(+), 549 deletions(-)
> > create mode 100644 xen/arch/x86/init_xbi.c
> >
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index c1e244d..bb2264a 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -42,6 +42,7 @@ obj-y += numa.o
> > obj-y += pci.o
> > obj-y += percpu.o
> > obj-y += physdev.o
> > +obj-y += init_xbi.o
>
> init_xbi is not a fantastic filename. xbi alone would be better
> (subject to my concerned about the name xbi).
>
> > 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..7316011 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_ptr),%ebx
> > + mov MBD_cmdline(%ebx),%eax
>
> FLAGS & MBI_CMDLINE is the prerequisite for the 'cmdline' field being
> valid. You cannot drop that check (although making use of some manifest
> constants would help)
We do not check original mbi struct here. It is MBD and if Xen command line is
empty then MBD_cmdline(%ebx) == 0. Hence, everything works as expected.
> > 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(%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 10ecf51..79bce3c 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -86,14 +86,14 @@ __start:
> > jne not_multiboot
> >
> > /* Set up trampoline segment 64k below EBDA */
> > - movzwl 0x40e,%eax /* EBDA segment */
> > - cmp $0xa000,%eax /* sanity check (high) */
> > + movzwl 0x40e,%ecx /* EBDA segment */
> > + cmp $0xa000,%ecx /* sanity check (high) */
> > jae 0f
> > - cmp $0x4000,%eax /* sanity check (low) */
> > + cmp $0x4000,%ecx /* sanity check (low) */
> > jae 1f
> > 0:
> > - movzwl 0x413,%eax /* use base memory size on failure */
> > - shl $10-4,%eax
> > + movzwl 0x413,%ecx /* use base memory size on failure */
> > + shl $10-4,%ecx
> > 1:
> > /*
> > * Compare the value in the BDA with the information from the
> > @@ -105,22 +105,23 @@ __start:
> > cmp $0x100,%edx /* is the multiboot value too small? */
> > jb 2f /* if so, do not use it */
> > shl $10-4,%edx
> > - cmp %eax,%edx /* compare with BDA value */
> > - cmovb %edx,%eax /* and use the smaller */
> > + cmp %ecx,%edx /* compare with BDA value */
> > + cmovb %edx,%ecx /* and use the smaller */
> >
> > 2: /* Reserve 64kb for the trampoline */
> > - sub $0x1000,%eax
> > + sub $0x1000,%ecx
> >
> > /* From arch/x86/smpboot.c: start_eip had better be page-aligned!
> > */
> > - xor %al, %al
> > - shl $4, %eax
> > - mov %eax,sym_phys(trampoline_phys)
> > + xor %cl, %cl
> > + shl $4, %ecx
> > + mov %ecx,sym_phys(trampoline_phys)
> >
> > - /* Save the Multiboot info struct (after relocation) for later
> > use. */
> > + /* Save the Multiboot data (after relocation) for later use. */
> > mov $sym_phys(cpu0_stack)+1024,%esp
> > - push %ebx
> > - call reloc
> > - mov %eax,sym_phys(multiboot_ptr)
> > + push %eax /* Multiboot magic */
> > + push %ebx /* Multiboot information address */
> > + call reloc /* %ecx contains trampoline address */
> > + mov %eax,sym_phys(mbd_ptr)
> >
> > /* 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 fa0fb6b..29f4887 100644
> > --- a/xen/arch/x86/boot/reloc.c
> > +++ b/xen/arch/x86/boot/reloc.c
[...]
> > +static void zero_struct(u32 s, u32 bytes)
> > +{
> > + asm volatile(
> > + " cld \n"
>
> __start has already performed cld for us. There is nothing which will
> have set it between then and here.
Hmmm... Are we sure that gcc will not insert an instruction
which will change direction flag? cld does not cost a lot
and we will be on safe side here.
[...]
> > + * __THIS__ IS NOT ENTRY POINT!!!
> > + * PLEASE LOOK AT THE BEGINNING OF THIS FILE!!!
>
> I don't think this needs stating...
It is not obvious. I spent some time to discover this. I do not
mention that strange build method does not ease reading and
understanding. I think that it is worth saving time of other
guys who will read this file first time.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |