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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Aug 31, 2016 at 06:49:51AM -0600, Jan Beulich wrote:
> >>> 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:

[...]

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

I think that we should no blindly assume that a given amount of memory
is available. However, we do that in a few places including this one which
you pointed out. So, it should be fixed. Though, the question is: Is it
possible? Right now I am not sure that it is true in all cases mentioned
above. There is a chance that in case pointed out by you it is. Anyway,
I will check it.

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

About "... a warning should be added alongside the EFI header inclusions,
making clear that no services whatsoever can be called". AIUI, "warning" ==
"comment" here. However, I think that everybody who reads this file is
aware that "no services whatsoever can be called". So, I am not sure
where is the point.

[...]

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

So, when somebody uses this new flag then "cc" will not be add here.
This is not big deal but I think that extra "cc" clobbers does not
hurt too.

Daniel

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