From: Daniel Kiper <daniel.kiper@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Juergen Gross <JGross@suse.com>,
sstabellini@kernel.org, andrew.cooper3@citrix.com,
cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v5 13/16] x86: add multiboot2 protocol support for EFI platforms
Date: Tue, 30 Aug 2016 21:32:10 +0200 [thread overview]
Message-ID: <20160830193210.GA5364@olila.local.net-space.pl> (raw)
In-Reply-To: <57BF07EA020000780010901A@prv-mh.provo.novell.com>
On Thu, Aug 25, 2016 at 06:59:54AM -0600, Jan Beulich wrote:
> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> wrote:
> > @@ -100,19 +107,45 @@ multiboot2_header_start:
> > gdt_boot_descr:
> > .word 6*8-1
> > .long sym_phys(trampoline_gdt)
> > + .long 0 /* Needed for 64-bit lgdt */
> > +
> > +cs32_switch_addr:
> > + .long sym_phys(cs32_switch)
> > + .word BOOT_CS32
> > +
> > +vga_text_buffer:
> > + .long 0xb8000
>
> This now ends up being misaligned. Not a big deal, but anyway.
>
> > .section .init.text, "ax", @progbits
I hope that ".align 4" is sufficient here.
> > bad_cpu:
> > mov $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
> > - jmp print_err
> > + jmp 0f
> > not_multiboot:
> > mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
> > -print_err:
> > - mov $0xB8000,%edi # VGA framebuffer
> > + jmp 0f
> > +mb2_no_st:
> > + mov $(sym_phys(.Lbad_ldr_nst)),%esi # Error message
> > + jmp 0f
> > +mb2_no_ih:
> > + mov $(sym_phys(.Lbad_ldr_nih)),%esi # Error message
> > + jmp 0f
> > +mb2_no_bs:
> > + mov $(sym_phys(.Lbad_ldr_nbs)),%esi # Error message
> > + xor %edi,%edi # No VGA text buffer
> > + jmp 1f
> > +mb2_efi_ia_32:
> > + mov $(sym_phys(.Lbad_efi_msg)),%esi # Error message
> > + xor %edi,%edi # No VGA text buffer
> > + jmp 1f
> > +0: mov sym_phys(vga_text_buffer),%edi
> > 1: mov (%esi),%bl
>
> All the labels you add should imo be .L ones. And the two numeric
> labels, considering their use across other label boundaries, would
> perhaps better become .L ones too.
OK.
> > +__efi64_start:
> > + cld
> > +
> > + /* VGA is not available on EFI platforms. */
> > + movl $0,vga_text_buffer(%rip)
> > +
> > + /* Check for Multiboot2 bootloader. */
> > + cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> > + je .Lefi_multiboot2_proto
> > +
> > + /* Jump to not_multiboot after switching CPU to x86_32 mode. */
> > + lea not_multiboot(%rip),%edi
> > + jmp x86_32_switch
>
> Does the boot loader, btw, make any guarantees towards placing
> the image below 4G?
Yep.
[...]
> > + /*
> > + * IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable.
> > + * OUT: %rax - Highest available memory address below 1 MiB.
>
> Please be more precise, as "available" leaves open how much space
> is actually available there, and hence what this can be used for. Also
> according to ...
>
> > + * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag is not provided
> > + * on EFI platforms. Hence, it could not be used like
> > + * on legacy BIOS platforms.
> > + */
> > + call efi_multiboot2
> > +
> > + /* Convert memory address to bytes/16 and store it in safe place. */
> > + shr $4,%eax
> > + mov %eax,%ecx
>
> ... the output really is in %eax.
efi_multiboot2() is called according to System V AMD64 ABI. So, according to it
return value is stored in %rax. Hence, from spec point of view comment is correct.
Use of %eax is a bit different thing (our need/choice) and if you wish it can be
documented accordingly.
> > @@ -223,14 +425,22 @@ trampoline_setup:
> > call reloc
> > mov %eax,sym_phys(multiboot_ptr)
> >
> > - /* Initialize BSS (no nasty surprises!) */
> > + /*
> > + * Do not zero BSS on EFI platform here.
> > + * It was initialized earlier.
> > + */
> > + cmpb $0,sym_phys(skip_realmode)
> > + jnz 1f
> > +
> > + /* Initialize BSS (no nasty surprises!). */
> > mov $sym_phys(__bss_start),%edi
> > mov $sym_phys(__bss_end),%ecx
> > sub %edi,%ecx
> > - xor %eax,%eax
> > shr $2,%ecx
> > + xor %eax,%eax
> > rep stosl
>
> Please avoid pointless code movement like this.
I think that shr should be close to sub because both operations are related
to some extent. However, you are right that maybe this cleanup should be in
separate patch. Does it make sense for you?
> > +paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> > +{
> > + EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
> > + UINTN cols, gop_mode = ~0, rows;
> > +
> > + __set_bit(EFI_BOOT, &efi_flags);
> > + __set_bit(EFI_RS, &efi_flags);
> > +
> > + efi_init(ImageHandle, SystemTable);
> > +
> > + efi_console_set_mode();
> > +
> > + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
> > + &cols, &rows) == EFI_SUCCESS )
> > + efi_arch_console_init(cols, rows);
> > +
> > + gop = efi_get_gop();
> > +
> > + if ( gop )
> > + gop_mode = efi_find_gop_mode(gop, 0, 0, 0);
> > +
> > + efi_arch_edd();
> > + efi_arch_cpu();
> > +
> > + efi_tables();
> > + setup_efi_pci();
> > + efi_variables();
> > +
> > + if ( gop )
> > + efi_set_gop_mode(gop, gop_mode);
> > +
> > + efi_exit_boot(ImageHandle, SystemTable);
> > +
> > + /* Return highest available memory address below 1 MiB. */
> > + return cfg.addr;
>
> Where is it being made certain that there are 64k of space available
> right below this address, as is being assumed at trampoline_setup?
You are right. This is a bug. However, the problem is more generic
and should be fixed for all boot cases (BIOS, EFI loader and EFI with
GRUB2). Additionally, in case of BIOS and EFI with GRUB2 we should
check that it is possible to put all data from grub loader in low
memory. So, it looks that there is a place for at least two (three?)
additional patches. Do you want them at the beginning of or the end
of this patch series.
> > --- a/xen/arch/x86/efi/stub.c
> > +++ b/xen/arch/x86/efi/stub.c
> > @@ -3,6 +3,30 @@
> > #include <xen/init.h>
> > #include <xen/lib.h>
> > #include <asm/page.h>
> > +#include <asm/efibind.h>
> > +#include <efi/efidef.h>
> > +#include <efi/eficapsule.h>
> > +#include <efi/eficon.h>
> > +#include <efi/efidevp.h>
> > +#include <efi/efiapi.h>
> > +
> > +paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> > +{
> > + CHAR16 *err = L"Xen does not have EFI code build in!!!\r\nSystem halted!!!\r\n";
> > + SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr;
> > +
> > + StdErr = SystemTable->StdErr ? SystemTable->StdErr : SystemTable->ConOut;
> > +
> > + /* Print error message and halt the system. */
> > + asm volatile(
> > + " call %2 \n"
> > + "0: hlt \n"
> > + " jmp 0b \n"
> > + : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)
> > + : "rax", "r8", "r9", "r10", "r11", "cc", "memory");
>
> There are explanations missing here: First, a warning should be added
> alongside the EFI header inclusions, making clear that no services
> whatsoever can be called. And then the asm() here needs to explain
I am not convinced but if you wish...
> that it open codes an MS-ABI function call. Which then makes me
OK.
> wonder (even if it doesn't matter much) - are the clobbers actually
> correct? I think you also need to clobber rsi and rdi. Otoh there's no
Nope, MS-ABI says that %rsi and %rdi must be saved and restored by
function which uses them. So, we do not need list both registers
in clobbers here.
> need for an explicit "cc" clobber on x86.
Why?
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-08-30 19:32 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-19 22:43 [PATCH v5 00/16] x86: multiboot2 protocol support Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 01/16] x86: allow EFI reboot method neither on EFI platforms Daniel Kiper
2016-08-25 11:19 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 02/16] x86/boot: remove multiboot1_header_end from symbol table Daniel Kiper
2016-08-25 11:21 ` Jan Beulich
2016-08-30 14:27 ` Daniel Kiper
2016-08-30 15:11 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 03/16] x86/boot: create *.lnk files with linker script Daniel Kiper
2016-08-25 11:28 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 04/16] x86/boot/reloc: reduce assembly usage as much as possible Daniel Kiper
2016-08-25 11:29 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 05/16] x86/boot: call reloc() using stdcall calling convention Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 06/16] x86/boot/reloc: create generic alloc and copy functions Daniel Kiper
2016-08-25 11:34 ` Jan Beulich
2016-08-30 14:32 ` Daniel Kiper
2016-08-30 15:12 ` Jan Beulich
2016-08-31 15:13 ` Daniel Kiper
2016-08-31 15:25 ` Jan Beulich
2016-08-31 19:39 ` Daniel Kiper
2016-09-01 7:35 ` Jan Beulich
2016-09-06 15:33 ` Doug Goldstein
2016-08-19 22:43 ` [PATCH v5 07/16] x86/boot: use %ecx instead of %eax Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 08/16] x86/boot/reloc: rename some variables and rearrange code a bit Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 09/16] x86: add multiboot2 protocol support Daniel Kiper
2016-08-25 11:50 ` Jan Beulich
2016-08-30 14:41 ` Daniel Kiper
2016-08-30 15:14 ` Jan Beulich
2016-08-31 15:21 ` Daniel Kiper
2016-08-31 20:18 ` Andrew Cooper
2016-08-31 21:01 ` Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 10/16] efi: create efi_enabled() Daniel Kiper
2016-08-25 12:16 ` Jan Beulich
2016-08-30 17:19 ` Daniel Kiper
2016-08-31 12:31 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 11/16] efi: build xen.gz with EFI code Daniel Kiper
2016-08-25 12:23 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 12/16] x86/efi: create new early memory allocator Daniel Kiper
2016-09-05 12:33 ` Jan Beulich
2016-09-07 12:05 ` Daniel Kiper
2016-09-07 14:01 ` Jan Beulich
2016-09-08 8:29 ` Daniel Kiper
2016-09-08 9:59 ` Jan Beulich
2016-08-19 22:43 ` [PATCH v5 13/16] x86: add multiboot2 protocol support for EFI platforms Daniel Kiper
2016-08-25 12:59 ` Jan Beulich
2016-08-30 19:32 ` Daniel Kiper [this message]
2016-08-31 12:49 ` Jan Beulich
2016-08-31 17:07 ` Daniel Kiper
2016-09-01 7:40 ` Jan Beulich
2016-09-01 20:37 ` Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 14/16] x86/boot: implement early command line parser in C Daniel Kiper
2016-08-25 13:27 ` Jan Beulich
2016-08-30 19:58 ` Daniel Kiper
2016-08-31 13:01 ` Jan Beulich
2016-08-31 19:31 ` Daniel Kiper
2016-09-01 7:41 ` Jan Beulich
2016-09-01 20:43 ` Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 15/16] x86: make Xen early boot code relocatable Daniel Kiper
2016-08-25 14:41 ` Jan Beulich
2016-08-31 14:59 ` Daniel Kiper
2016-08-31 15:46 ` Jan Beulich
2016-08-31 20:50 ` Daniel Kiper
2016-09-01 7:46 ` Jan Beulich
2016-09-01 21:19 ` Daniel Kiper
2016-09-02 6:58 ` Jan Beulich
2016-09-02 7:28 ` Daniel Kiper
2016-08-19 22:43 ` [PATCH v5 16/16] x86: add multiboot2 protocol support for relocatable images Daniel Kiper
2016-08-22 10:10 ` [PATCH v5 00/16] x86: multiboot2 protocol support Jan Beulich
2016-08-30 14:15 ` Daniel Kiper
2016-08-30 15:09 ` Jan Beulich
2016-08-31 15:05 ` Daniel Kiper
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160830193210.GA5364@olila.local.net-space.pl \
--to=daniel.kiper@oracle.com \
--cc=JBeulich@suse.com \
--cc=JGross@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=cardoe@cardoe.com \
--cc=david.vrabel@citrix.com \
--cc=fu.wei@linaro.org \
--cc=gang.wei@intel.com \
--cc=ning.sun@intel.com \
--cc=pgnet.dev@gmail.com \
--cc=qiaowei.ren@intel.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.