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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/01/2020 09:23, Jan Beulich wrote:
> 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?

Yes.  i.e. exactly as the patch presented, but with an extra " ^" in the
comment.

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

I'll attempt to rewrite the explanation from scratch and see if that
comes out clearer.

~Andrew

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