Re: [Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.01.2020 20:31, Andrew Cooper wrote:
> On 14/01/2020 16:45, Jan Beulich wrote:
>> On 13.01.2020 18:50, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -668,6 +668,20 @@ trampoline_setup:
>>>          add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
>>>  2:      loop    1b
>>>  
>>> +        /* Map Xen into the higher mappings using 2M superpages. */
>>> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
>>> +        mov     $sym_offs(_start),   %ecx   /* %eax = PTE to write        
>>> */
>> The comment is on the wrong line, isn't it? Perhaps
>>
>>         lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), \
>>                 %eax                /* %eax = PTE to write        */
>>
>> ?
> 
> That is why the comment had the register name, rather than trying to
> claim that $sym_offs(_start) was the PTE to write.
> 
> I didn't really think splitting the lea like that across 2 lines was
> better than this.
> 
> How about /* %eax = PTE to write ^      */ which will point properly at
> %eax?

Fine with me; I assume you mean this to go on a separate line?

>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -585,6 +585,20 @@ static void __init efi_arch_memory_setup(void)
>>>      if ( !efi_enabled(EFI_LOADER) )
>>>          return;
>>>  
>>> +    /*
>>> +     * Map Xen into the higher mappings, using 2M superpages.
>>> +     *
>>> +     * NB: We are currently in physical mode, so a RIP-relative relocation
>>> +     * against _start/_end gets their position as placed by the bootloader,
>>> +     * not as expected in the final build.  This has arbitrary 2M 
>>> alignment,
>>> +     * so subtract xen_phys_start to get the appropriate slots in 
>>> l2_xenmap[].
>>> +     */
>> It may just be a language issue, but I'm struggling with the
>> "arbitrary" here. Is this in any way related to the
>> --section-alignment=0x200000 option we pass to the linker (where
>> the value isn't arbitrary at all)?
> 
> So this is the bug I spent ages trying to figure out console logging for.
> 
> The naive version of this loop (pre subtraction) ended up initialising
> slots 173...177 which, when highlighted like that, is obviously why Xen
> triple faulted when switching to the high mappings.
> 
> The point I'm trying to make is that l2_table_offset(_start) ends up
> being junk because it is a rip-relative address and we're not running at
> our linked address.  (It is in fact our physical position in memory's 2M
> slot, modulo 512).
> 
> Subtracting xen_phys_start gets the number back into the same alias
> which all the 32bit head.S code relies on, and gives us a sensible
> sequence of slots starting from 1.

Thanks for the explanation. What I'm still unclear about is this use
of "arbitrary", though. Looking at it again I guess I'm also
struggling to understand what "This" at the beginning of the sentence
refers to.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel