Date:   Tue, 16 Jun 2020 14:52:01 -0700
From:   Atish Patra <atishp@...shpatra.org>
To:     Alex Ghiti <alex@...ti.fr>
Cc:     Palmer Dabbelt <palmer@...belt.com>,
        Anup Patel <anup@...infault.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        Atish Patra <Atish.Patra@....com>
Subject: Re: [PATCH 0/2] PUD/PGDIR entries for linear mapping

On Sun, Jun 14, 2020 at 10:35 PM Alex Ghiti <alex@...ti.fr> wrote:
>
> Hi Atish,
>
> Le 6/12/20 à 1:43 PM, Atish Patra a écrit :
> > On Fri, Jun 12, 2020 at 6:17 AM Alex Ghiti <alex@...ti.fr> wrote:
> >> Le 6/12/20 à 8:59 AM, Alex Ghiti a écrit :
> >>> 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.
> >>>
> > Thanks for the clarification. When I said latest, I meant including
> > your patch in the mailing list.
> >
> >>>> 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.
> >>>
> > I agree. I was just suggesting to document this bug :).
>
> Oh ok then, we meant the same thing :)
> >
> >>> 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
> > Personally, I am fine with that. However, there were few concerns in the past.
> > I am leaving it to Palmer to decide.
> >
> > @Palmer Dabbelt : Any thoughts ?
> >
> >>> 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) ?
> > Yes.
> >
> >> Ok, I have just found Documentation/riscv/boot-image-header.rst: could
> >> we imagine doing something like incrementing the version and use that as
> >> a hint in the kernel not to map the 2MB offset ? That's still legacy,
> >> but at least it does not require to recompile a kernel as the check
> >> would be done at runtime.
> >>
> > I was suggesting to add a risc-v specific booting document and
> > document this "bug".
> > Documentation/riscv/boot-image-header.rst can be linked from that document or
> > the boot hader content can be included in that. No changes in code is necessary.
> >
> > Eventually, this booting document will also include other additional
> > booting constraints for RISC-V
> > such as minimum extension required to boot Linux, csr state upon
> > entering S-mode, mmu state.
>
>
> Ok I will prepare a boot document that links to the existing documents and
> add all of that, I will need you for the last constraints that I don't
> know about.
>

Thanks. I will send patches on top of your boot document patch.
This was long due. Thanks for coming up with the initial version :).

> Thanks Atish,
>
> Alex
>
> >>> 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
> >>>>>>>
> >>>>>>>
> >
> >



-- 
Regards,
Atish

Powered by blists - more mailing lists