[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> 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:
>> > --- a/xen/arch/x86/boot/head.S
>> > +++ b/xen/arch/x86/boot/head.S
>> > @@ -79,6 +79,13 @@ multiboot2_header_start:
>> >          /* Align modules at page boundry. */
>> >          mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED)
>> >
>> > +        /* Load address preference. */
>> > +        mb2ht_init MB2_HT(RELOCATABLE), MB2_HT(OPTIONAL), \
>> > +                   sym_offset(start), /* Min load address. */ \
>> > +                   0xffffffff, /* Max load address (4 GiB - 1). */ \
>>
>> Hardly - that would allow us to be loaded at 4G - 2M, no matter
>> how large the image. Or else the comment is misleading.
> 
> This is the highest address at which memory region allocated for image
> may end.

You saying "end" then means the comment is misleading.

>> > @@ -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
perhaps trust ourselves more than an external entity?

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

>> > @@ -313,14 +329,23 @@ multiboot2_proto:
>> >          and     $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>> >
>> >  0:
>> > +        /* Get Xen image load base address from Multiboot2 information. */
>> > +        cmpl    $MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%ecx)
>> > +        jne     1f
>> > +
>> > +        mov     MB2_load_base_addr(%ecx),%esi
>> > +        sub     $XEN_IMG_OFFSET,%esi
>> > +        jmp     3f
>>
>> The redundancy once again suggests some form of abstraction
>> (helper function, macro, ...).
> 
> Do you suggest that we should macroize multiboot2 header accesses?

The whole logic here. And if macros (rather than functions), then
I'm thinking of assembler macros more than of C ones. All I really
wish is that we don't have the same kind of code in multiple places,
even more so when we talk about assembly code.

Jan

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.