2020-11-04 06:41:31

by Anup Patel

[permalink] [raw]
Subject: [PATCH] RISC-V: Use non-PGD mappings for early DTB access

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 <[email protected]>
---
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;

/*
--
2.25.1


2020-11-04 23:44:34

by Atish Patra

[permalink] [raw]
Subject: Re: [PATCH] RISC-V: Use non-PGD mappings for early DTB access

On Tue, Nov 3, 2020 at 10:37 PM Anup Patel <[email protected]> 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.
>

Not sure why this is an issue with Unleashed only. Microchip Polarfire
SoC seems to work fine with early DTB pa as 0x88000000.
However, unleashed fails to boot if the DTB is loaded any address
below 0xC0000000 and it is mapped via PGD.

> 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 <[email protected]>
> ---
> 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;
>
> /*
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

FWIW: Tested on Unleashed & Polarfire SoC.
Tested-by: Atish Patra <[email protected]>
Reviewed-by: Atish Patra <[email protected]>

@palmer: Can you consider this patch in next rcX PR ?

--
Regards,
Atish

2020-11-06 08:05:11

by Palmer Dabbelt

[permalink] [raw]
Subject: Re: [PATCH] RISC-V: Use non-PGD mappings for early DTB access

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 <[email protected]>
> ---
> 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?
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.

Thanks!

2020-11-06 08:56:11

by Anup Patel

[permalink] [raw]
Subject: Re: [PATCH] RISC-V: Use non-PGD mappings for early DTB access

On Fri, Nov 6, 2020 at 1:30 PM Palmer Dabbelt <[email protected]> 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 <[email protected]>
> > ---
> > 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 ??

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
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)

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.

> 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,
Anup

2020-11-06 12:10:57

by Bin Meng

[permalink] [raw]
Subject: Re: [PATCH] RISC-V: Use non-PGD mappings for early DTB access

On Fri, Nov 6, 2020 at 4:52 PM Anup Patel <[email protected]> wrote:
>
> On Fri, Nov 6, 2020 at 1:30 PM Palmer Dabbelt <[email protected]> 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 <[email protected]>
> > > ---
> > > 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