Re: [Xen-devel] [PATCH v3 16/16] x86: add multiboot2 protocol support for relocatable images

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.06.16 at 21:16, <daniel.kiper@xxxxxxxxxx> wrote:
> On Wed, Jun 01, 2016 at 08:44:31AM -0600, Jan Beulich wrote:
>> >>> On 01.06.16 at 15:35, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Wed, May 25, 2016 at 05:03:20AM -0600, Jan Beulich wrote:
>> >> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> > @@ -178,30 +185,39 @@ efi_multiboot2_proto:
>> >> >          and     $~(MULTIBOOT2_TAG_ALIGN-1),%rcx
>> >> >
>> >> >  0:
>> >> > +        /* Get Xen image load base address from Multiboot2 
>> >> > information. */
>> >> > +        cmpl    $MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%rcx)
>> >> > +        jne     1f
>> >> > +
>> >> > +        mov     MB2_load_base_addr(%rcx),%r15d
>> >> > +        sub     $XEN_IMG_OFFSET,%r15
>> >> > +        jmp     4f
>> >>
>> >> Why do we need to read this from the table? Can't we easily calculate
>> >> this ourselves?
>> >
>> > Potentially yes but why do not use data from boot loader?
>>
>> Because it's (a) likely easier to just calculate and (b) we should
> 
> In 64-bit mode yes but 32-bit mode requires additional call and pop.
> Is it OK for you?

If you don't already calculate that offset somewhere - sure.

>> >> > +1:
>> >> >          /* Get EFI SystemTable address from Multiboot2 information. */
>> >> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
>> >> > -        jne     1f
>> >> > +        jne     2f
>> >> >
>> >> >          mov     MB2_efi64_st(%rcx),%rsi
>> >> >
>> >> >          /* Do not clear BSS twice and do not go into real mode. */
>> >> >          movb    $1,skip_realmode(%rip)
>> >> > -        jmp     3f
>> >> > +        jmp     4f
>> >> >
>> >> > -1:
>> >> > +2:
>> >> >          /* Get EFI ImageHandle address from Multiboot2 information. */
>> >> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
>> >> > -        jne     2f
>> >> > +        jne     3f
>> >> >
>> >> >          mov     MB2_efi64_ih(%rcx),%rdi
>> >> > -        jmp     3f
>> >> > +        jmp     4f
>> >> >
>> >> > -2:
>> >> > +3:
>> >> >          /* Is it the end of Multiboot2 information? */
>> >> >          cmpl    $MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
>> >> >          je      run_bs
>> >> >
>> >> > -3:
>> >> > +4:
>> >> >          /* Go to next Multiboot2 information tag. */
>> >> >          add     MB2_tag_size(%rcx),%ecx
>> >> >          add     $(MULTIBOOT2_TAG_ALIGN-1),%rcx
>> >>
>> >> See why numeric labels are bad in situations like this? The (much)
>> >> earlier patch should use .L labels here, and the patch here then
>> >> should simply follow suit.
>> >
>> > Then we should change legacy multiboot (v1) code too. Just to be in line
>> > new stuff here. Does it pays? And I am not sure that patching convenience
>> > overweight convenience of numeric labels here.
>>
>> Well, it's always this same discussion: Bad examples shouldn't be
>> used as excuse to add further bad examples. If you feel like also
>> changing the mb1 code - go for it. But if you don't, I'm fine with
>> just new code avoiding old mistakes.
> 
> Make sense. However, do you suggest that I should avoid numeric labels at 
> all?
> Probably they are useful in some situations.

Yes, they are. So I'm not asking to do away with them altogether.

Jan


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