Re: [Xen-devel] [PATCH v5 13/16] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.08.16 at 21:32, <daniel.kiper@xxxxxxxxxx> wrote:
> On Thu, Aug 25, 2016 at 06:59:54AM -0600, Jan Beulich wrote:
>> >>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> wrote:
>> > @@ -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?

Separating dependent instructions by other unrelated ones can
actually improve performance. I agree that performance isn't of
much relevance here, but I think I still wouldn't agree to a change
like this even if submitted separately.

>> > +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.

If you think there are bugs in pre-existing code, then for having the
option of backporting putting such patches at the beginning would
be desirable. That said, I'm not convinced there are problems really
in need of fixing here (yet the grub environment is different and
hence will need taking care of even if we leave the other code paths
as they are now).

>> > --- 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...

Not convinced of what?

>> 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.

Oh, right, I manage to mix up calling conventions.

>> need for an explicit "cc" clobber on x86.
> 
> Why?

Because such a clobber gets added to every asm() by the compiler,
unless it uses the (new in gcc 6) flag output. I've actually suggested
to upstream a patch making it possible to avoid that automatic
addition, but there hadn't been a whole lot of useful feedback.

Jan

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