Date:   Fri, 6 Nov 2020 20:08:26 +0800
From:   Bin Meng <bmeng.cn@...il.com>
To:     Anup Patel <anup@...infault.org>
Cc:     Palmer Dabbelt <palmerdabbelt@...gle.com>,
        Anup Patel <Anup.Patel@....com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Atish Patra <Atish.Patra@....com>,
        Alistair Francis <Alistair.Francis@....com>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        Bin Meng <bin.meng@...driver.com>
Subject: Re: [PATCH] RISC-V: Use non-PGD mappings for early DTB access

On Fri, Nov 6, 2020 at 4:52 PM Anup Patel <anup@...infault.org> wrote:
>
> On Fri, Nov 6, 2020 at 1:30 PM Palmer Dabbelt <palmerdabbelt@...gle.com> wrote:
> >
> > On Tue, 03 Nov 2020 22:37:13 PST (-0800), Anup Patel wrote:
> > > Currently, we use PGD mappings for early DTB mapping in early_pgd
> > > but this breaks Linux kernel on SiFive Unleashed because on SiFive
> > > Unleashed PMP checks don't work correctly for PGD mappings.
> > >
> > > To fix early DTB mappings on SiFive Unleashed, we use non-PGD
> > > mappings (i.e. PMD) for early DTB access.
> > >
> > > Fixes: 8f3a2b4a96dc ("RISC-V: Move DT mapping outof fixmap")
> > > Signed-off-by: Anup Patel <anup.patel@....com>
> > > ---
> > >  arch/riscv/mm/init.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index ea933b789a88..0d13d0c36a7d 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -297,6 +297,7 @@ pmd_t fixmap_pmd[PTRS_PER_PMD] __page_aligned_bss;
> > >  #define NUM_EARLY_PMDS               (1UL + MAX_EARLY_MAPPING_SIZE / PGDIR_SIZE)
> > >  #endif
> > >  pmd_t early_pmd[PTRS_PER_PMD * NUM_EARLY_PMDS] __initdata __aligned(PAGE_SIZE);
> > > +pmd_t early_dtb_pmd[PTRS_PER_PMD] __initdata __aligned(PAGE_SIZE);
> > >
> > >  static pmd_t *__init get_pmd_virt_early(phys_addr_t pa)
> > >  {
> > > @@ -494,6 +495,18 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > >                                  load_pa + (va - PAGE_OFFSET),
> > >                                  map_size, PAGE_KERNEL_EXEC);
> > >
> > > +#ifndef __PAGETABLE_PMD_FOLDED
> > > +     /* Setup early PMD for DTB */
> > > +     create_pgd_mapping(early_pg_dir, DTB_EARLY_BASE_VA,
> > > +                        (uintptr_t)early_dtb_pmd, PGDIR_SIZE, PAGE_TABLE);
> > > +     /* Create two consecutive PMD mappings for FDT early scan */
> > > +     pa = dtb_pa & ~(PMD_SIZE - 1);
> > > +     create_pmd_mapping(early_dtb_pmd, DTB_EARLY_BASE_VA,
> > > +                        pa, PMD_SIZE, PAGE_KERNEL);
> > > +     create_pmd_mapping(early_dtb_pmd, DTB_EARLY_BASE_VA + PMD_SIZE,
> > > +                        pa + PMD_SIZE, PMD_SIZE, PAGE_KERNEL);
> > > +     dtb_early_va = (void *)DTB_EARLY_BASE_VA + (dtb_pa & (PMD_SIZE - 1));
> > > +#else
> > >       /* Create two consecutive PGD mappings for FDT early scan */
> > >       pa = dtb_pa & ~(PGDIR_SIZE - 1);
> > >       create_pgd_mapping(early_pg_dir, DTB_EARLY_BASE_VA,
> > > @@ -501,6 +514,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> > >       create_pgd_mapping(early_pg_dir, DTB_EARLY_BASE_VA + PGDIR_SIZE,
> > >                          pa + PGDIR_SIZE, PGDIR_SIZE, PAGE_KERNEL);
> > >       dtb_early_va = (void *)DTB_EARLY_BASE_VA + (dtb_pa & (PGDIR_SIZE - 1));
> > > +#endif
> > >       dtb_early_pa = dtb_pa;
> > >
> > >       /*
> >
> > We're starting to build up a handful of workarounds for these sorts of things.
> > The PMP trap vs WARL one was the last I could remember, but that's a bit
> > different as both of those behaviors were allowed by specifications at some
> > point.  IIRC there were also some TLB shootdown issues floating
> >
> > The best I can come up with is to add both some sort of "minimum support
> > specification version" Kconfig entry and an "quirks" set of Kconfig entries.
> > That would allow us to gradually jettison old ISAs as well as more cleanly add
> > support for broken hardware like this.
> >
> > Do you have a pointer to some datasheet type document that describes the issue?
>
> Unfortunately, we still don't know where all SiFive erratums are documented.
> Maybe you know the right person who can publish this ??

Last time Andrew said this erratum would be published but did not give
a name of whom from SiFive in charge ..

>
> The PMP checks not working correctly for PGD-mappings was discovered
> independently by Bin Meng on some other OS (almost 1.5 years back). Few

Yep, this undocumented erratum was initially discovered during our
VxWorks RISC-V port.

> discussions about this erratum have happened on OpenSBI mailing list as well
> although in a very different context. In fact, Andrew had confirmed about this
> erratum in sw-dev google groups.
> (Refer, https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/CfyT5Los5uA/m/yY0ES0dZBAAJ?pli=1)
> (Refer, https://github.com/riscv/opensbi/issues/103)

The issue is not just about gigapages. Using megapages also could
trigger this issue.

To me this erratum seems like: if any PTEs are set up to overlap with
PMP protected memory, FU540 will trigger an instruction access
exception as soon as SATP is written.

>
> Till now we have encountered two erratums for SiFive U54 cores:
> 1. SFENCE.VMA not working with "virtual address" parameter
> 2. PMP checks not working for PGD mappings
>
> The Microchip ICICLE kit has U54 cores but the PMP checks issue
> is only seen on SiFive Unleashed so I guess this issue is fixed
> in-between but we can't be sure without detailed documentation.

I believe Microchip PolarFire SoC suffers this same erratum but I did
not verify that. Currently PMP is configured to protect the eNVM range
on PolarFire, and this range is not mapped by any PTE yet in S-mode.

>
> > That'd probably be the line I'd like to draw for adding workarounds like this,
> > as otherwise we can't really be sure something is a hardware issue.
> >
> > That said, it's better to have the fix so this is on fixes.  I'll try to
> > remember it for the list of workarounds.
>
> I agree. At the moment, 5.10-rc2 does not boot on SiFive Unleashed
> but it boots on Microchip icicle kit and everywhere else because of
> the PMP checks issue.

Regards,
Bin

Powered by blists - more mailing lists