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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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