|
[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
On 09/09/14 14:22, Daniel Kiper wrote:
> 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.
I am not suggesting that you should avoid large patches per se, but I am
very definitely telling you to split the logically distinct bits into
separate patches.
Even for dependent logic, some can be split up while retaining
bisectability. Move the multiboot stuff in a separate patch to the
other bits xbi bits.
>
>>> ---
>>> 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.
Right - this really not clear that the structure pointed to by ebx has
changed, as well as MBD_cmdline having nothing to do with MB_cmdline.
Again, all of this would be clear with a smaller patch whose description
states "moving multiboot data from format FOO to format BAR"
>
>>> 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?
Absolutely sure.
> cld does not cost a lot
> and we will be on safe side here.
We clear it on all entry points into Xen as we can't trust our caller
not to mess with it, but we absolutely can trust our own compiled code
not to.
>
> [...]
>
>>> + * __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
The _start symbol is at the top of the file. It is obviously not at
this point.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |