Re: [Xen-devel] [PATCH v14 4/9] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Feb 06, 2017 at 03:18:53AM -0700, Jan Beulich wrote:
> >>> On 02.02.17 at 23:01, <daniel.kiper@xxxxxxxxxx> wrote:
> > This way Xen can be loaded on EFI platforms using GRUB2 and
> > other boot loaders which support multiboot2 protocol.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> > ---
> > v14 - suggestions/fixes:
> >     - mark .init.data section as writable; by the way we must change
> >       similar definition in xen/arch/x86/boot/x86_64.S because otherwise
> >       compiler complains about section types conflicts
>
> Did you observe a problem here, or was this just based on memory
> from having seen such in other cases? I ask because I doubt there

It happened here. Though I am not able to reproduce it now... Hmmm...

> would be any problem here, as the compiler isn't involved: This is
> assembly code, and iirc you validly talk about a compiler diagnostic.
> Hence the wrong attributes want correcting, but in a separate
> patch.

OK.

> >          .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                       # No VGA text buffer
> > +        jmp     .Lsend_chr
> > +.Lmb2_efi_ia_32:
> > +        mov     $(sym_phys(.Lbad_efi_msg)),%esi # Error message
> > +        xor     %edi,%edi                       # No VGA text buffer
> > +        jmp     .Lsend_chr
>
> I continue to have problems to understand when you use / don't use
> the VGA buffer here: I think at the very least we want a comment as
> to when which of the two models applies. The situation isn't being
> helped by .Lmb2_no_bs reachable via two paths (one neighboring a
> use of .Lmb2_efi_ia_32, and another neighboring uses of
> .Lmb2_no_st and .Lmb2_no_ih). Since you zap vga_text_buffer in
> the EFI case, couldn't you simply use that one instead of clearing
> %edi here?

Here it is much simpler to do. Otherwise we must move jumps to
.Lmb2_no_bs and .Lmb2_efi_ia_32 behind Multiboot2 scanning loop.
Earlier we could not have MB2_load_base_addr value which is needed
to calculate real vga_text_buffer address in 32-bit mode. Ahhh...
This is not obvious when you look at this patch alone because
MB2_load_base_addr is added by "x86: add multiboot2 protocol
support for relocatable images" patch.

> > +void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
> > +                                    EFI_SYSTEM_TABLE *SystemTable)
> > +{
> > +    static const CHAR16 __initconst 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 may
> > +     * not be directly supported by C compiler.
> > +     */
> > +    asm volatile(
> > +    "    call *%2                     \n"
> > +    "0:  hlt                          \n"
> > +    "    jmp  0b                      \n"
> > +       : "+c" (StdErr) : "d" (err), "rm" (StdErr->OutputString)
> > +       : "rax", "r8", "r9", "r10", "r11", "memory");
>
> Regardless of the hlt I think this would better be correct: As done
> in various other places, you simply need to indicate "=d" as a
> (dummy) output; since you clobber StdErr anyway, you could
> simply use that one:
>
>     asm volatile(
>     "    call *%2                     \n"
>     "0:  hlt                          \n"
>     "    jmp  0b                      \n"
>        : "+c" (StdErr), "=d" (StdErr) : "1" (err), "rm" (StdErr->OutputString)
>        : "rax", "r8", "r9", "r10", "r11", "memory");
>
> Otherwise I'd ask for consistency, i.e. for %rcx to be an input only,
> too.

OK, I will try.

Daniel

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