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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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