From: Jan Beulich <jbeulich@suse.com>
To: Oleksii <oleksii.kurochko@gmail.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
Bob Eshleman <bobbyeshleman@gmail.com>,
Alistair Francis <alistair.francis@wdc.com>,
Connor Davis <connojdavis@gmail.com>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping
Date: Wed, 14 Jun 2023 12:04:41 +0200 [thread overview]
Message-ID: <0dd8283f-ad73-8590-3800-0f9a00b4a281@suse.com> (raw)
In-Reply-To: <3fa74b20c3516dbeaceada0f8e045d697f926000.camel@gmail.com>
On 14.06.2023 11:47, Oleksii wrote:
> On Mon, 2023-06-12 at 15:48 +0200, Jan Beulich wrote:
>> On 06.06.2023 21:55, Oleksii Kurochko wrote:
>>> The way how switch to virtual address was implemented in the
>>> commit e66003e7be ("xen/riscv: introduce setup_initial_pages")
>>> wasn't safe enough so identity mapping was introduced and
>>> used.
>>
>> I don't think this is sufficient as a description. You want to make
>> clear what the "not safe enough" is, and you also want to go into
>> more detail as to the solution chosen. I'm particularly puzzled that
>> you map just two singular pages ...
> I'll update a descrption.
>
> I map two pages as it likely to be enough to switch from 1:1 mapping
I dislike your use of "likely" here: Unless this code is meant to be
redone anyway, it should just work. Everywhere.
> world. My point is that we need 1:1 mapping only for few instructions
> which are located in start() [ in .text.header section ]:
>
> li t0, XEN_VIRT_START
> la t1, start
> sub t1, t1, t0
>
> /* Calculate proper VA after jump from 1:1 mapping */
> la t0, .L_primary_switched
> sub t0, t0, t1
>
> /* Jump from 1:1 mapping world */
> jr t0
>
> And it is needed to map 1:1 cpu0_boot_stack to not to fault when
> executing epilog of enable_mmu() function as it accesses a value on the
> stack:
> ffffffffc00001b0: 6422 ld s0,8(sp)
> ffffffffc00001b2: 0141 addi sp,sp,16
> ffffffffc00001b4: 8082 ret
>
>>
>>> @@ -35,8 +40,10 @@ static unsigned long phys_offset;
>>> *
>>> * It might be needed one more page table in case when Xen load
>>> address
>>> * isn't 2 MB aligned.
>>> + *
>>> + * 3 additional page tables are needed for identity mapping.
>>> */
>>> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1)
>>> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1 + 3)
>>
>> What is this 3 coming from? It feels like the value should (again)
>> somehow depend on CONFIG_PAGING_LEVELS.
> 3 is too much in the current case. It should be 2 more.
>
> The linker address is 0xFFFFFFFC00000000 thereby mapping the linker to
> load addresses
> we need 3 pages ( with the condition that the size of Xen won't be
> larger than 2 MB ) and 1 page if the case when Xen load address isn't
> 2MV aligned.
>
> We need 2 more page tables because Xen is loaded to address 0x80200000
> by OpenSBI and the load address isn't equal to the linker address so we
> need additional 2 pages as the L2 table we already allocated when
> mapping the linker to load addresses.
>
> And it looks like 2 is enough for Sv48, Sv57 as L4, L3 and L2
> pagetables will be already allocated during mapping the linker to load
> addresses.
I agree the root table will be there. But depending on load address, I
don't think you can rely on any other tables to be re-usable from the
Xen mappings you already have. So my gut feeling would be
#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)
>>> @@ -232,22 +242,27 @@ void __init setup_initial_pagetables(void)
>>> linker_start,
>>> linker_end,
>>> load_start);
>>> +
>>> + if ( linker_start == load_start )
>>> + return;
>>> +
>>> + setup_initial_mapping(&mmu_desc,
>>> + load_start,
>>> + load_start + PAGE_SIZE,
>>> + load_start);
>>> +
>>> + setup_initial_mapping(&mmu_desc,
>>> + (unsigned long)cpu0_boot_stack,
>>> + (unsigned long)cpu0_boot_stack +
>>> PAGE_SIZE,
>>
>> Shouldn't this be STACK_SIZE (and then also be prepared for
>> STACK_SIZE > PAGE_SIZE)?
> Yes, it will be better to use STACK_SIZE but for 1:1 mapping it will be
> enough PAGE_SIZE as I mentioned above this only need for epilogue of
> enable_mmu() function.
But then it should still be the correct page of the stack that you
map (the top one aiui).
>>> -void __init noreturn noinline enable_mmu()
>>> +/*
>>> + * enable_mmu() can't be __init because __init section isn't part
>>> of identity
>>> + * mapping so it will cause an issue after MMU will be enabled.
>>> + */
>>
>> As hinted at above already - perhaps the identity mapping wants to be
>> larger, up to covering the entire Xen image? Since it's temporary
>> only anyway, you could even consider using a large page (and RWX
>> permission). You already require no overlap of link and load
>> addresses,
>> so at least small page mappings ought to be possible for the entire
>> image.
> It makes sense and started to thought about that after I applied the
> patch for Dom0 running... I think we can map 1 GB page which will cover
> the whole Xen image. Also in that case we haven't to allocate 2 more
> page tables.
But you then need to be careful about not mapping space accesses to
which may have side effects (i.e. non-RAM, which might be MMIO or
holes). Otherwise speculative execution might cause surprises,
unless such is excluded by other guarantees (of which I don't know).
>>> --- a/xen/arch/riscv/riscv64/head.S
>>> +++ b/xen/arch/riscv/riscv64/head.S
>>> @@ -31,6 +31,36 @@ ENTRY(start)
>>>
>>> jal calc_phys_offset
>>>
>>> + jal setup_initial_pagetables
>>> +
>>> + jal enable_mmu
>>> +
>>> + /*
>>> + * Calculate physical offset
>>> + *
>>> + * We can't re-use a value in phys_offset variable here as
>>> + * phys_offset is located in .bss and this section isn't
>>> + * 1:1 mapped and an access to it will cause MMU fault
>>> + */
>>> + li t0, XEN_VIRT_START
>>> + la t1, start
>>> + sub t1, t1, t0
>>
>> If I'm not mistaken this calculates start - XEN_VIRT_START, and ...
>>
>>> + /* Calculate proper VA after jump from 1:1 mapping */
>>> + la t0, .L_primary_switched
>>> + sub t0, t0, t1
>>
>> ... then this .L_primary_switched - (start - XEN_VIRT_START). Which
>> can
>> also be expressed as (.L_primary_switched - start) + XEN_VIRT_START,
>> the first part of which gas ought to be able to resolve for you. But
>> upon experimenting a little it looks like it can't. (I had thought of
>> something along the lines of
>>
>> li t0, .L_primary_switched - start
>> li t1, XEN_VIRT_START
>> add t0, t0, t1
>>
>> or even
>>
>> li t1, XEN_VIRT_START
>> add t0, t1, %pcrel_lo(.L_primary_switched - start)
>>
>> .)
> Calculation of ".L_primary_switched - start" might be an issue for gcc.
> I tried to do something similar and recieved or "Error: can't resolve
> .L_primary_switched - start" or "Error: illegal operands `li
> t0,.L_primary_switched-start'"
Matches the results of my experiments. If I can find time, I may want
to look into why exactly gas is rejecting such.
Jan
next prev parent reply other threads:[~2023-06-14 10:05 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-06 19:55 [PATCH v1 0/8] xen/riscv: introduce identity mapping Oleksii Kurochko
2023-06-06 19:55 ` [PATCH v1 1/8] xen/riscv: make sure that identity mapping isn't bigger then page size Oleksii Kurochko
2023-06-12 5:08 ` Alistair Francis
2023-06-12 7:09 ` Jan Beulich
2023-06-13 17:40 ` Oleksii
2023-06-06 19:55 ` [PATCH v1 2/8] xen/riscv: add .sbss section to .bss Oleksii Kurochko
2023-06-12 5:09 ` Alistair Francis
2023-06-12 7:04 ` Jan Beulich
2023-06-13 17:41 ` Oleksii
2023-06-06 19:55 ` [PATCH v1 3/8] xen/riscv: introduce reset_stack() function Oleksii Kurochko
2023-06-12 5:10 ` Alistair Francis
2023-06-12 7:12 ` Jan Beulich
2023-06-13 17:46 ` Oleksii
2023-06-14 12:19 ` Oleksii
2023-06-14 12:46 ` Jan Beulich
2023-06-06 19:55 ` [PATCH v1 4/8] xen/riscv: introduce function for physical offset calculation Oleksii Kurochko
2023-06-12 7:15 ` Jan Beulich
2023-06-13 17:48 ` Oleksii
2023-06-06 19:55 ` [PATCH v1 5/8] xen/riscv: introduce identity mapping Oleksii Kurochko
2023-06-12 13:48 ` Jan Beulich
2023-06-12 14:24 ` Jan Beulich
2023-06-14 9:53 ` Oleksii
2023-06-14 9:47 ` Oleksii
2023-06-14 10:04 ` Jan Beulich [this message]
2023-06-14 11:06 ` Oleksii
2023-06-14 11:38 ` Jan Beulich
2023-06-06 19:55 ` [PATCH v1 6/8] xen/riscv: add SPDX tags Oleksii Kurochko
2023-06-07 10:20 ` Julien Grall
2023-06-08 8:10 ` Oleksii
2023-06-06 19:55 ` [PATCH v1 7/8] xen/riscv: add __ASSEMBLY__ guards Oleksii Kurochko
2023-06-13 6:52 ` Alistair Francis
2023-06-06 19:55 ` [PATCH v1 8/8] xen/riscv: move extern of cpu0_boot_stack to header Oleksii Kurochko
2023-06-07 7:49 ` [PATCH v1 0/8] xen/riscv: introduce identity mapping Jan Beulich
2023-06-07 10:15 ` Oleksii
Reply instructions:
You may reply publicly 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=0dd8283f-ad73-8590-3800-0f9a00b4a281@suse.com \
--to=jbeulich@suse.com \
--cc=alistair.francis@wdc.com \
--cc=andrew.cooper3@citrix.com \
--cc=bobbyeshleman@gmail.com \
--cc=connojdavis@gmail.com \
--cc=oleksii.kurochko@gmail.com \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).