Re: [Xen-devel] [PATCH v7 12/14] x86: make Xen early boot code relocatable

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Sep 28, 2016 at 03:06:31AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 21:55, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Mon, Sep 26, 2016 at 09:03:30AM -0600, Jan Beulich wrote:
> >> >>> On 23.09.16 at 23:47, <daniel.kiper@xxxxxxxxxx> wrote:
> >> > @@ -426,32 +453,65 @@ trampoline_bios_setup:
> >> >          xor     %cl, %cl
> >> >
> >> >  trampoline_setup:
> >> > +        /*
> >> > +         * Called on legacy BIOS and EFI platforms.
> >> > +         *
> >> > +         * Initialize 0-15 bits of BOOT_FS segment descriptor base 
> >> > address.
> >> > +         */
> >> > +        mov     %si,BOOT_FS+2+sym_esi(trampoline_gdt)
> >> > +
> >> > +        /* Initialize 16-23 bits of BOOT_FS segment descriptor base 
> >> > address. */
> >> > +        mov     %esi,%edx
> >> > +        shr     $16,%edx
> >>
> >> I'd have liked it even better if you had done this with a single
> >> instruction, but anyway.
> >
> > Do you think about "shld $16,%esi,%edx"?
>
> Yes.

By the way, my 12 years old daughter told me how to do this...
Daddy, just mix all arguments all together. And later when
I showed her final solution she said: I told you... :-)))

> >> > +1:      mov     %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
> >> > +        mov     %eax,sym_fs(l2_identmap)-8(%ebx,%ecx,8)
> >> > +        sub     $(1<<L2_PAGETABLE_SHIFT),%eax
> >> > +        loop    1b
> >> > +
> >> > +        /* Initialize L3 boot-map page directory entry. */
> >> > +        lea     
> >> > __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax
> >> > +        mov     $4,%ecx
> >> > +1:      mov     %eax,sym_fs(l3_bootmap)-8(,%ecx,8)
> >> > +        sub     $(L2_PAGETABLE_ENTRIES*8),%eax
> >> > +        loop    1b
> >> >
> >> >          /*
> >> >           * During boot, hook 4kB mappings of first 2MB of memory into 
> >> > L2.
> >> > -         * This avoids mixing cachability for the legacy VGA region, 
> >> > and is
> >> > -         * corrected when Xen relocates itself.
> >> > +         * This avoids mixing cachability for the legacy VGA region.
> >> >           */
> >> > -        mov     $sym_phys(l1_identmap)+__PAGE_HYPERVISOR,%edi
> >> > -        mov     %edi,sym_phys(l2_xenmap)
> >> > +        lea     __PAGE_HYPERVISOR+sym_esi(l1_identmap),%edi
> >> > +        mov     %edi,sym_fs(l2_bootmap)
> >>
> >> Switching from l2_xenmap to l2_bootmap here?
> >
> > Do we need first 2 MiB mapped in Xen image mapping? It looks that we do not.
> > Am I missing something?
>
> My point here isn't that the change is wrong, but that it's not
> immediately obvious and hence should be explained in the commit
> message. After all the need for the mapping of these 2Mb does not
> - aiui - go away with this patch, but (presumably) with the earlier one
> moving the load address up to 2Mb. I.e. it can generally be viewed as
> an independent adjustment.

Probably. Should I move this change to patch #10 (x86: change default load
address from 1 MiB to 2 MiB) or leave it here with relevant comment?
Or do you wish separate patch for it?

> >> > @@ -674,6 +671,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >> >
> >> >      printk("Command line: %s\n", cmdline);
> >> >
> >> > +    printk("Xen image load base address: 0x%08lx\n", xen_phys_start);
> >>
> >> Please prefer %#lx in cases like this.
> >
> > If I do that then 0 is displayed as 0 instead of 0x00000000. I prefer
> > latter. If you do not care I can use "%#lx" as you wish.
>
> Please do - the base address won't ever be zero anyway, and even if

Here it can be. It happens if Xen image is loaded by boot loader without
relocation, e.g. via multiboot (v1) protocol.

> it was "0" instead of "0x00000000" is quite fine when there are no
> other lines to align with..
>
> >> > @@ -1018,6 +1018,8 @@ void __init noreturn __start_xen(unsigned long 
> >> > mbi_p)
> >> >                  : "memory" );
> >> >
> >> >              bootstrap_map(NULL);
> >> > +
> >> > +            printk("New Xen image base address: %#08lx\n", 
> >> > xen_phys_start);
> >>
> >> # and a minimum width generally don't fit together well.
> >
> > Why?
>
> Because this will result in e.g. 0x000123 instead of the apparently
> expected by you 0x00000123, as the 0x contributes to the field
> width.

Yep, this is a bit confusing.

Daniel

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