Re: [Xen-devel] [PATCH v6 09/15] x86: add multiboot2 protocol support for EFI platforms

2016-09-19 Thread Jan Beulich 
>>> On 19.09.16 at 17:18,  wrote:
> On Mon, Sep 19, 2016 at 06:29:55AM -0600, Jan Beulich wrote:
>> >>> On 12.09.16 at 22:18,  wrote:
>> > --- a/xen/arch/x86/efi/stub.c
>> > +++ b/xen/arch/x86/efi/stub.c
>> > @@ -3,6 +3,43 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +
>> > +/*
>> > + * Here we are in EFI stub. EFI calls are not supported due to lack
>> > + * of relevant functionality in compiler and/or linker.
>> > + *
>> > + * efi_multiboot2() is an exception. Please look below for more details.
>> > + */
>> > +
>> > +paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, 
>> > EFI_SYSTEM_TABLE *SystemTable)
>>
>> Long line.
>>
>> > +{
>> > +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.
>> > + *
>> > + * We have to open code MS x64 calling convention
>> > + * in assembly because here this convention is not
>> > + * directly supported by C compiler and/or linker.
>>
>> So how can lack of calling convention support be due to missing
>> functionality in the linker? Please avoid such misleading comments:
>> Linker capabilities get probed solely for PE32+ output format
>> support. With the linker part dropped here,
> 
> The problem here is that we are not able to tell why stub.c is build.
> There is a chance that C compiler does not support MS x64 calling
> convention or linker is not able to emit PE32+ output or both. So,
> I think that we should say so. However, I can agree that comment
> can be improved.

The first comment (higher up, but still visible in context) already
mentions both compiler and linker. That's sufficient for the purpose
that you mention, imo. The second comment should be solely about
why this needs to be an asm(), and that has nothing to do with the
linker.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6 09/15] x86: add multiboot2 protocol support for EFI platforms

2016-09-19 Thread Daniel Kiper 
On Mon, Sep 19, 2016 at 06:29:55AM -0600, Jan Beulich wrote:
> >>> On 12.09.16 at 22:18,  wrote:
> > --- a/xen/arch/x86/efi/stub.c
> > +++ b/xen/arch/x86/efi/stub.c
> > @@ -3,6 +3,43 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/*
> > + * Here we are in EFI stub. EFI calls are not supported due to lack
> > + * of relevant functionality in compiler and/or linker.
> > + *
> > + * efi_multiboot2() is an exception. Please look below for more details.
> > + */
> > +
> > +paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, 
> > EFI_SYSTEM_TABLE *SystemTable)
>
> Long line.
>
> > +{
> > +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.
> > + *
> > + * We have to open code MS x64 calling convention
> > + * in assembly because here this convention is not
> > + * directly supported by C compiler and/or linker.
>
> So how can lack of calling convention support be due to missing
> functionality in the linker? Please avoid such misleading comments:
> Linker capabilities get probed solely for PE32+ output format
> support. With the linker part dropped here,

The problem here is that we are not able to tell why stub.c is build.
There is a chance that C compiler does not support MS x64 calling
convention or linker is not able to emit PE32+ output or both. So,
I think that we should say so. However, I can agree that comment
can be improved.

Daniel

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v6 09/15] x86: add multiboot2 protocol support for EFI platforms

2016-09-19 Thread Jan Beulich 
>>> On 12.09.16 at 22:18,  wrote:
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -3,6 +3,43 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * Here we are in EFI stub. EFI calls are not supported due to lack
> + * of relevant functionality in compiler and/or linker.
> + *
> + * efi_multiboot2() is an exception. Please look below for more details.
> + */
> +
> +paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, 
> EFI_SYSTEM_TABLE *SystemTable)

Long line.

> +{
> +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.
> + *
> + * We have to open code MS x64 calling convention
> + * in assembly because here this convention is not
> + * directly supported by C compiler and/or linker.

So how can lack of calling convention support be due to missing
functionality in the linker? Please avoid such misleading comments:
Linker capabilities get probed solely for PE32+ output format
support. With the linker part dropped here,
Acked-by: Jan Beulich 

Jan




___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v6 09/15] x86: add multiboot2 protocol support for EFI platforms

2016-09-12 Thread Daniel Kiper 
This way Xen can be loaded on EFI platforms using GRUB2 and
other boot loaders which support multiboot2 protocol.

Signed-off-by: Daniel Kiper 
---
v6 - suggestions/fixes:
   - improve label names in assembly
 error printing code
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - various minor cleanups and fixes
 (suggested by Jan Beulich).

v4 - suggestions/fixes:
   - remove redundant BSS alignment,
   - update BSS alignment check,
   - use __set_bit() instead of set_bit() if possible
 (suggested by Jan Beulich),
   - call efi_arch_cpu() from efi_multiboot2()
 even if the same work is done later in
 other place right now
 (suggested by Jan Beulich),
   - xen/arch/x86/efi/stub.c:efi_multiboot2()
 fail properly on EFI platforms,
   - do not read data beyond the end of multiboot2
 information in xen/arch/x86/boot/head.S
 (suggested by Jan Beulich),
   - use 32-bit registers in x86_64 code if possible
 (suggested by Jan Beulich),
   - multiboot2 information address is 64-bit
 in x86_64 code, so, treat it is as is
 (suggested by Jan Beulich),
   - use cmovcc if possible,
   - leave only one space between rep and stosq
 (suggested by Jan Beulich),
   - improve error handling,
   - improve early error messages,
 (suggested by Jan Beulich),
   - improve early error messages printing code,
   - improve label names
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - various minor cleanups.

v3 - suggestions/fixes:
   - take into account alignment when skipping multiboot2 fixed part
 (suggested by Konrad Rzeszutek Wilk),
   - improve segment registers initialization
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich and Konrad Rzeszutek Wilk),
   - improve commit message
 (suggested by Jan Beulich).

v2 - suggestions/fixes:
   - generate multiboot2 header using macros
 (suggested by Jan Beulich),
   - switch CPU to x86_32 mode before
 jumping to 32-bit code
 (suggested by Andrew Cooper),
   - reduce code changes to increase patch readability
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - ignore MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag on EFI platform
 and find on my own multiboot2.mem_lower value,
   - stop execution if EFI platform is detected
 in legacy BIOS path.
---
 xen/arch/x86/boot/head.S  |  255 ++---
 xen/arch/x86/efi/efi-boot.h   |   49 ++-
 xen/arch/x86/efi/stub.c   |   37 ++
 xen/arch/x86/x86_64/asm-offsets.c |2 +
 xen/arch/x86/xen.lds.S|4 +-
 xen/common/efi/boot.c |   11 ++
 6 files changed, 336 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 383ff79..00fa47d 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -89,6 +89,13 @@ multiboot2_header_start:
0, /* Number of the lines - no preference. */ \
0  /* Number of bits per pixel - no preference. */
 
+/* Inhibit bootloader from calling ExitBootServices(). */
+mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL)
+
+/* EFI64 entry point. */
+mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \
+   sym_phys(__efi64_start)
+
 /* Multiboot2 header end tag. */
 mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
 .Lmultiboot2_header_end:
@@ -100,20 +107,49 @@ 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
+
+.align 4
+vga_text_buffer:
+.long   0xb8000
 
 .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
 .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
+.Lbad_ldr_nbs: .asciz "ERR: Bootloader shutdown EFI x64 boot services!"
+.Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
+.Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
+.Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
 
 .section .init.text, "ax", @progbits
 
 bad_cpu:
 mov $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
-jmp print_err
+jmp .Lget_vtb
 not_multiboot:
 mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
-print_err:
-mov $0xB8000,%edi  # VGA framebuffer
-1:  mov (%esi),%bl
+jmp .Lget_vtb
+.Lmb2_no_st:
+mov $(sym_phys(.Lbad_ldr_nst)),%esi # Error message
+jmp .Lget_vtb
+.Lmb2_no_ih:
+mov $(sym_phys(.Lbad_ldr_nih)),%esi # Error message
+jmp .Lget_vtb
+.Lmb2_no_bs:
+mov $(sym_phys(.Lbad_ldr_nbs)),%esi # Error message
+xor %edi,%edi