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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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:
> >> > --- 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

In 64-bit mode yes but 32-bit mode requires additional call and pop.
Is it OK for you?

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

Make sense. However, do you suggest that I should avoid numeric labels at all?
Probably they are useful in some situations.

Daniel

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