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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.02.17 at 14:44, <daniel.kiper@xxxxxxxxxx> wrote:
> On Mon, Feb 06, 2017 at 03:18:53AM -0700, Jan Beulich wrote:
>> >>> On 02.02.17 at 23:01, <daniel.kiper@xxxxxxxxxx> wrote:
>> >  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.

Well, it the shape things are in is required only by a later patch,
either the respective pieces should be moved there, or some
enlightening comment should be added here.

Jan


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