Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically
Date: Thu, 16 Jan 2020 19:41:44 +0000
Message-ID: <d4cf3ef3-2000-8581-652b-fdca398352d6@citrix.com> (raw)
In-Reply-To: <ce1d41a1-6865-2a0d-9aa0-30fc82cad557@suse.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

  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-13 17:50 [Xen-devel] [PATCH 0/4] x86: Remove 16M total-size restriction Andrew Cooper
2020-01-13 17:50 ` [Xen-devel] [PATCH 1/4] x86/boot: Rename l?_identmap to l?_directmap Andrew Cooper
2020-01-14 16:16   ` Jan Beulich
2020-01-13 17:50 ` [Xen-devel] [PATCH 2/4] x86/page: Remove bifrucated PAGE_HYPERVISOR constant Andrew Cooper
2020-01-13 17:50 ` [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated " Andrew Cooper
2020-01-14 16:25   ` Jan Beulich
2020-01-15 12:53     ` Andrew Cooper
2020-01-15 13:07       ` Jan Beulich
2020-01-15 14:08   ` [Xen-devel] [PATCH v2 " Andrew Cooper
2020-01-16  9:46     ` Jan Beulich
2020-01-13 17:50 ` [Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically Andrew Cooper
2020-01-14 16:45   ` Jan Beulich
2020-01-14 19:31     ` Andrew Cooper
2020-01-15  9:23       ` Jan Beulich
2020-01-16 19:41         ` Andrew Cooper [this message]
2020-01-13 17:50 ` [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap " Andrew Cooper
2020-01-14 17:02   ` Jan Beulich
2020-01-14 17:27     ` Andrew Cooper
2020-01-15  9:40       ` Jan Beulich
2020-01-15 10:21         ` Jan Beulich
2020-01-14 13:03 ` [Xen-devel] [PATCH 0/4] x86: Remove 16M total-size restriction Andrew Cooper

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d4cf3ef3-2000-8581-652b-fdca398352d6@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=jbeulich@suse.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git