Message-ID: <36739fc4-21ea-14f4-f2a6-52614b602dea@ghiti.fr>
Date:   Fri, 12 Jun 2020 08:59:54 -0400
From:   Alex Ghiti <alex@...ti.fr>
To:     Atish Patra <atishp@...shpatra.org>
Cc:     Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Anup Patel <anup@...infault.org>,
        Atish Patra <Atish.Patra@....com>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/2] PUD/PGDIR entries for linear mapping

Hi Atish,

Le 6/11/20 à 1:29 PM, Atish Patra a écrit :
> On Wed, Jun 10, 2020 at 11:51 PM Alex Ghiti<alex@...ti.fr>  wrote:
>> Hi Atish,
>>
>> Le 6/10/20 à 2:32 PM, Atish Patra a écrit :
>>> On Wed, Jun 3, 2020 at 8:36 AM Alexandre Ghiti<alex@...ti.fr>  wrote:
>>>> This small patchset intends to use PUD/PGDIR entries for linear mapping
>>>> in order to better utilize TLB.
>>>>
>>>> At the moment, only PMD entries can be used since on common platforms
>>>> (qemu/unleashed), the kernel is loaded at DRAM + 2MB which dealigns virtual
>>>> and physical addresses and then prevents the use of PUD/PGDIR entries.
>>>> So the kernel must be able to get those 2MB for PAGE_OFFSET to map the
>>>> beginning of the DRAM: this is achieved in patch 1.
>>>>
>>> I don't have in depth knowledge of how mm code works so this question
>>> may be a completely
>>> stupid one :). Just for my understanding,
>>> As per my understanding, kernel will map those 2MB of memory but never use it.
>>> How does the kernel ensure that it doesn't allocate any memory from those 2MB
>>> memory if it is not marked as reserved?
>> Yes, a 1GB hugepage will cover those 2MB: I rely on the previous boot
>> stage to mark this region
>> as reserved if there is something there (like opensbi). Otherwise, the
>> kernel will indeed try to
>> allocate memory from there :)
>>
> In that case, this patch mandates that the firmware region has to be
> mark "reserved"
> the device tree so that the Linux kernel doesn't try to allocate
> memory from there.
> OpenSBI is already doing it from v0.7. Thus, any user using latest
> OpenSBI can leverage
> this patch for a better TLB utilization.


Note that *currently* OpenSBI v0.7 still adds the "no-map" property 
which prevents such optimization.

> However, legacy previous boot stages(BBL) do not reserve this area via
> DT which may
> result in an unexpected crash. I am not sure how many developers still
> use BBL though.
>
> Few general suggestions to tackle this problem:
> 1. This mandatory requirement should be added to the booting document
> so that any other
> SBI implementation is also aware of it.
> 2. You may have to move the patch1 to a separate config so that any
> users of legacy boot stages
> can disable this feature.


IMHO, the region occupied by runtime services should be marked as 
reserved in the device-tree. So it seems redundant to add this as a 
requirement, I would rather consider its absence as a bug.

Even if I understand that this might break some system, I don't like the 
idea of a new config to support old "buggy" bootloaders: when will we be 
able to remove it ? We'll never know when people will stop using those 
bootloaders, so it will stay here forever...Where can I find the boot 
document you are talking about ? Can we simply state here that this 
kernel version will not be compatible with those bootloaders (we'll draw 
an exhaustive list here) ?

Alex


>> Alex
>>
>>
>>>> But furthermore, at the moment, the firmware (opensbi) explicitly asks the
>>>> kernel not to map the region it occupies, which is on those common
>>>> platforms at the very beginning of the DRAM and then it also dealigns
>>>> virtual and physical addresses. I proposed a patch here:
>>>>
>>>> https://github.com/riscv/opensbi/pull/167
>>>>
>>>> that removes this 'constraint' but *not* all the time as it offers some
>>>> kind of protection in case PMP is not available. So sometimes, we may
>>>> have a part of the memory below the kernel that is removed creating a
>>>> misalignment between virtual and physical addresses. So for performance
>>>> reasons, we must at least make sure that PMD entries can be used: that
>>>> is guaranteed by patch 1 too.
>>>>
>>>> Finally the second patch simply improves best_map_size so that whenever
>>>> possible, PUD/PGDIR entries are used.
>>>>
>>>> Below is the kernel page table without this patch on a 6G platform:
>>>>
>>>> ---[ Linear mapping ]---
>>>> 0xffffc00000000000-0xffffc00176e00000    0x0000000080200000 5998M PMD     D A . . . W R V
>>>>
>>>> And with this patchset + opensbi patch:
>>>>
>>>> ---[ Linear mapping ]---
>>>> 0xffffc00000000000-0xffffc00140000000 0x0000000080000000         5G PUD     D A . . . W R V
>>>> 0xffffc00140000000-0xffffc00177000000    0x00000001c0000000 880M PMD     D A . . . W R V
>>>>
>>>> Alexandre Ghiti (2):
>>>>     riscv: Get memory below load_pa while ensuring linear mapping is PMD
>>>>       aligned
>>>>     riscv: Use PUD/PGDIR entries for linear mapping when possible
>>>>
>>>>    arch/riscv/include/asm/page.h |  8 ++++
>>>>    arch/riscv/mm/init.c          | 69 +++++++++++++++++++++++++++++------
>>>>    2 files changed, 65 insertions(+), 12 deletions(-)
>>>>
>>>> --
>>>> 2.20.1
>>>>
>>>>
>

Powered by blists - more mailing lists