[PATCH -fixes] platform/lib: Set no-map attribute on all PMP regions
Anup Patel
anup at brainfault.org
Thu Jun 15 06:02:45 PDT 2023
On Wed, Jun 14, 2023 at 1:50 PM Alexandre Ghiti <alexghiti at rivosinc.com> wrote:
>
> This reverts commit 6966ad0abe70 ("platform/lib: Allow the OS to map the
> regions that are protected by PMP").
>
> It was thought at the time of this commit that allowing the kernel to map
> PMP protected regions was safe but it is actually not: for example, the
> hibernation process will try to access any linear mapping page and then
> will fault on such mapped PMP regions [1]. Another issue is that the
> device tree specification [2] states that a !no-map region must be
> declared as EfiBootServicesData/Code in the EFI memory map which would make
> the PMP protected regions reclaimable by the kernel. And to circumvent
> this, RISC-V edk2 diverges from the DT specification to declare those
> regions as EfiReserved.
>
> The no-map attribute was removed to allow the kernel to use hugepages
> larger than 2MB to map the linear mapping to improve the performance but
> actually a recent talk from Mike Rapoport [3] stated that the
> performance benefit was marginal.
>
> For all those reasons, let's mark all the PMP protected regions as "no-map".
>
> [1] https://lore.kernel.org/linux-riscv/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/
> [2] "3.5.4 /reserved-memory and UEFI" https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4-rc1/devicetree-specification-v0.4-rc1.pdf
> [3] https://lwn.net/Articles/931406/
>
> Signed-off-by: Alexandre Ghiti <alexghiti at rivosinc.com>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
> ---
> include/sbi_utils/fdt/fdt_fixup.h | 14 ---------
> lib/utils/fdt/fdt_fixup.c | 49 +++++++------------------------
> platform/generic/sifive/fu540.c | 13 --------
> 3 files changed, 10 insertions(+), 66 deletions(-)
>
> diff --git a/include/sbi_utils/fdt/fdt_fixup.h b/include/sbi_utils/fdt/fdt_fixup.h
> index cab3f0f..ecd55a7 100644
> --- a/include/sbi_utils/fdt/fdt_fixup.h
> +++ b/include/sbi_utils/fdt/fdt_fixup.h
> @@ -93,20 +93,6 @@ void fdt_plic_fixup(void *fdt);
> */
> int fdt_reserved_memory_fixup(void *fdt);
>
> -/**
> - * Fix up the reserved memory subnodes in the device tree
> - *
> - * This routine adds the no-map property to the reserved memory subnodes so
> - * that the OS does not map those PMP protected memory regions.
> - *
> - * Platform codes must call this helper in their final_init() after fdt_fixups()
> - * if the OS should not map the PMP protected reserved regions.
> - *
> - * @param fdt: device tree blob
> - * @return zero on success and -ve on failure
> - */
> -int fdt_reserved_memory_nomap_fixup(void *fdt);
> -
> /**
> * General device tree fix-up
> *
> diff --git a/lib/utils/fdt/fdt_fixup.c b/lib/utils/fdt/fdt_fixup.c
> index ae6be00..e213ded 100644
> --- a/lib/utils/fdt/fdt_fixup.c
> +++ b/lib/utils/fdt/fdt_fixup.c
> @@ -210,7 +210,7 @@ void fdt_plic_fixup(void *fdt)
>
> static int fdt_resv_memory_update_node(void *fdt, unsigned long addr,
> unsigned long size, int index,
> - int parent, bool no_map)
> + int parent)
> {
> int na = fdt_address_cells(fdt, 0);
> int ns = fdt_size_cells(fdt, 0);
> @@ -239,16 +239,14 @@ static int fdt_resv_memory_update_node(void *fdt, unsigned long addr,
> if (subnode < 0)
> return subnode;
>
> - if (no_map) {
> - /*
> - * Tell operating system not to create a virtual
> - * mapping of the region as part of its standard
> - * mapping of system memory.
> - */
> - err = fdt_setprop_empty(fdt, subnode, "no-map");
> - if (err < 0)
> - return err;
> - }
> + /*
> + * Tell operating system not to create a virtual
> + * mapping of the region as part of its standard
> + * mapping of system memory.
> + */
> + err = fdt_setprop_empty(fdt, subnode, "no-map");
> + if (err < 0)
> + return err;
>
> /* encode the <reg> property value */
> val = reg;
> @@ -286,7 +284,6 @@ int fdt_reserved_memory_fixup(void *fdt)
> {
> struct sbi_domain_memregion *reg;
> struct sbi_domain *dom = sbi_domain_thishart_ptr();
> - struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
> unsigned long filtered_base[PMP_COUNT] = { 0 };
> unsigned char filtered_order[PMP_COUNT] = { 0 };
> unsigned long addr, size;
> @@ -382,33 +379,7 @@ int fdt_reserved_memory_fixup(void *fdt)
> for (j = 0; j < i; j++) {
> addr = filtered_base[j];
> size = 1UL << filtered_order[j];
> - fdt_resv_memory_update_node(fdt, addr, size, j, parent,
> - (sbi_hart_pmp_count(scratch))
> - ? false : true);
> - }
> -
> - return 0;
> -}
> -
> -int fdt_reserved_memory_nomap_fixup(void *fdt)
> -{
> - int parent, subnode;
> - int err;
> -
> - /* Locate the reserved memory node */
> - parent = fdt_path_offset(fdt, "/reserved-memory");
> - if (parent < 0)
> - return parent;
> -
> - fdt_for_each_subnode(subnode, fdt, parent) {
> - /*
> - * Tell operating system not to create a virtual
> - * mapping of the region as part of its standard
> - * mapping of system memory.
> - */
> - err = fdt_setprop_empty(fdt, subnode, "no-map");
> - if (err < 0)
> - return err;
> + fdt_resv_memory_update_node(fdt, addr, size, j, parent);
> }
>
> return 0;
> diff --git a/platform/generic/sifive/fu540.c b/platform/generic/sifive/fu540.c
> index 08f7bfc..b980f44 100644
> --- a/platform/generic/sifive/fu540.c
> +++ b/platform/generic/sifive/fu540.c
> @@ -20,18 +20,6 @@ static u64 sifive_fu540_tlbr_flush_limit(const struct fdt_match *match)
> return 0;
> }
>
> -static int sifive_fu540_fdt_fixup(void *fdt, const struct fdt_match *match)
> -{
> - /*
> - * SiFive Freedom U540 has an erratum that prevents S-mode software
> - * to access a PMP protected region using 1GB page table mapping, so
> - * always add the no-map attribute on this platform.
> - */
> - fdt_reserved_memory_nomap_fixup(fdt);
> -
> - return 0;
> -}
> -
> static const struct fdt_match sifive_fu540_match[] = {
> { .compatible = "sifive,fu540" },
> { .compatible = "sifive,fu540g" },
> @@ -43,5 +31,4 @@ static const struct fdt_match sifive_fu540_match[] = {
> const struct platform_override sifive_fu540 = {
> .match_table = sifive_fu540_match,
> .tlbr_flush_limit = sifive_fu540_tlbr_flush_limit,
> - .fdt_fixup = sifive_fu540_fdt_fixup,
> };
> --
> 2.39.2
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
More information about the opensbi
mailing list