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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.11.16 at 19:52, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 29/09/16 22:42, Daniel Kiper wrote:
>> @@ -100,20 +107,49 @@ multiboot2_header_start:
>>  gdt_boot_descr:
>>          .word   6*8-1
>>          .long   sym_phys(trampoline_gdt)
>> +        .long   0 /* Needed for 64-bit lgdt */
>> +
>> +cs32_switch_addr:
>> +        .long   sym_phys(cs32_switch)
>> +        .word   BOOT_CS32
>> +
>> +        .align 4
>> +vga_text_buffer:
>> +        .long   0xb8000
> 
> Why is this turned into a variable?

This gets zapped to zero near __efi64_start.

>> +.Lefi_mb2_tsize:
>> +        /* Check Multiboot2 information total size. */
>> +        mov     %ecx,%r8d
>> +        sub     %ebx,%r8d
>> +        cmp     %r8d,MB2_fixed_total_size(%rbx)
>> +        jbe     run_bs
>> +
>> +        /* Are EFI boot services available? */
>> +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI_BS,MB2_tag_type(%rcx)
>> +        jne     .Lefi_mb2_st
>> +
>> +        /*
>> +         * Yes, store that info in skip_realmode variable. I agree that
>> +         * its name is a bit unfortunate in this context, however, by the
>> +         * way we disable real mode and other legacy stuff which should
>> +         * not be run on EFI platforms.
>> +         */
>> +        incb    skip_realmode(%rip)
> 
> Always use add/sub 1 in preference to inc and dec.  They are the same
> length to encode in 64bit, and avoids a pipeline stall from a merge of
> the eflags register.

What you say regarding length not true - add/sub need to encode
the immediate somewhere (even if the operand was a register,
inc/dec would still be smaller than add/sub, just not by as much as
in 32-bit code). And the pipeline stall, afaik, affects only rather old
processors.

>> +x86_32_switch:
>> +        cli
>> +
>> +        /* Initialize GDTR. */
>> +        lgdt    gdt_boot_descr(%rip)
>> +
>> +        /* Reload code selector. */
>> +        ljmpl   *cs32_switch_addr(%rip)
> 
> This would be cleaner and shorter as
> 
> push $BOOT_CS32
> push $cs32_switch

I'm not sure: For one this would need to be $sym_phys(cs32_switch).
And with that I wouldn't be certain the relocation gets expressed
correctly. But yes, if it works, I too would prefer this.

Jan


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