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

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, Daniel Kiper <daniel.kiper@oracle.com>,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	julien.grall@arm.com, david.vrabel@citrix.com,
	xen-devel@lists.xenproject.org, qiaowei.ren@intel.com,
	gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v9 07/13] x86: add multiboot2 protocol support for EFI platforms
Date: Thu, 24 Nov 2016 04:08:12 -0700	[thread overview]
Message-ID: <5836D82C02000078001219EF@prv-mh.provo.novell.com> (raw)
In-Reply-To: <79d93447-f387-ddcd-1dfa-bba4ae5bbe85@citrix.com>

>>> On 23.11.16 at 19:52, <andrew.cooper3@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel