[RFT,1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions

Commit Message

Conor Dooley Aug. 2, 2023, 11:12 a.m. UTC

Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
the "no-map" property to the reserved memory nodes for the regions it
has protected using PMPs.

Our existing fix sweeping hibernation under the carpet by marking it
NONPORTABLE is insufficient as there are other ways to generate
accesses to these reserved memory regions, as Petr discovered [1]
while testing crash kernels & kdump.

Intercede during the boot process when the afflicted versions of OpenSBI
are present & set the "no-map" property in all "mmode_resv" nodes before
the kernel does its reserved memory region initialisation.

Reported-by: Song Shuai <[e-mail]>
Link: https://lore.kernel.org/all/CAAYs2=[e-mail]/
Reported-by: JeeHeng Sia <[e-mail]>
Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKfA6z8
Reported-by: Petr Tesarik <[e-mail]>
Closes: https://lore.kernel.org/linux-riscv/[e-mail]/ [1]
CC: [e-mail]
Signed-off-by: Conor Dooley <[e-mail]>
---
 arch/riscv/include/asm/sbi.h |  5 +++++
 arch/riscv/kernel/sbi.c      | 42 +++++++++++++++++++++++++++++++++++-
 arch/riscv/mm/init.c         |  3 +++
 3 files changed, 49 insertions(+), 1 deletion(-)

Comments

Alexandre Ghiti Aug. 6, 2023, 9:03 a.m. UTC | #1

Hi Conor,

On 02/08/2023 13:12, Conor Dooley wrote:
> Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> the "no-map" property to the reserved memory nodes for the regions it
> has protected using PMPs.
>
> Our existing fix sweeping hibernation under the carpet by marking it
> NONPORTABLE is insufficient as there are other ways to generate
> accesses to these reserved memory regions, as Petr discovered [1]
> while testing crash kernels & kdump.
>
> Intercede during the boot process when the afflicted versions of OpenSBI
> are present & set the "no-map" property in all "mmode_resv" nodes before
> the kernel does its reserved memory region initialisation.
>
> Reported-by: Song Shuai <[e-mail]>
> Link: https://lore.kernel.org/all/CAAYs2=[e-mail]/
> Reported-by: JeeHeng Sia <[e-mail]>
> Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKfA6z8
> Reported-by: Petr Tesarik <[e-mail]>
> Closes: https://lore.kernel.org/linux-riscv/[e-mail]/ [1]
> CC: [e-mail]
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
>   arch/riscv/include/asm/sbi.h |  5 +++++
>   arch/riscv/kernel/sbi.c      | 42 +++++++++++++++++++++++++++++++++++-
>   arch/riscv/mm/init.c         |  3 +++
>   3 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 5b4a1bf5f439..5360f3476278 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -252,6 +252,9 @@ enum sbi_pmu_ctr_type {
>   #define SBI_ERR_ALREADY_STARTED -7
>   #define SBI_ERR_ALREADY_STOPPED -8
>   
> +/* SBI implementation IDs */
> +#define SBI_IMP_OPENSBI	1
> +
>   extern unsigned long sbi_spec_version;
>   struct sbiret {
>   	long error;
> @@ -259,6 +262,8 @@ struct sbiret {
>   };
>   
>   void sbi_init(void);
> +void sbi_apply_reserved_mem_erratum(void *dtb_va);
> +
>   struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>   			unsigned long arg1, unsigned long arg2,
>   			unsigned long arg3, unsigned long arg4,
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index c672c8ba9a2a..aeb27263fa53 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -5,8 +5,10 @@
>    * Copyright (c) 2020 Western Digital Corporation or its affiliates.
>    */
>   
> +#include <linux/acpi.h>
>   #include <linux/bits.h>
>   #include <linux/init.h>
> +#include <linux/libfdt.h>
>   #include <linux/pm.h>
>   #include <linux/reboot.h>
>   #include <asm/sbi.h>
> @@ -583,6 +585,40 @@ long sbi_get_mimpid(void)
>   }
>   EXPORT_SYMBOL_GPL(sbi_get_mimpid);
>   
> +static long sbi_firmware_id;
> +static long sbi_firmware_version;
> +
> +/*
> + * For devicetrees patched by OpenSBI a "mmode_resv" node is added to cover
> + * the region OpenSBI has protected by means of a PMP. Some versions of OpenSBI,
> + * [v0.8 to v1.3), omitted the "no-map" property, but this trips up hibernation
> + * among other things.
> + */
> +void __init sbi_apply_reserved_mem_erratum(void *dtb_pa)
> +{
> +	int child, reserved_mem;
> +
> +	if (sbi_firmware_id != SBI_IMP_OPENSBI)
> +		return;
> +
> +	if (!acpi_disabled)
> +		return;
> +
> +	if (sbi_firmware_version >= 0x10003 || sbi_firmware_version < 0x8)
> +		return;
> +
> +	reserved_mem = fdt_path_offset((void *)dtb_pa, "/reserved-memory");
> +	if (reserved_mem < 0)
> +		return;
> +
> +	fdt_for_each_subnode(child, (void *)dtb_pa, reserved_mem) {
> +		const char *name = fdt_get_name((void *)dtb_pa, child, NULL);
> +
> +		if (!strncmp(name, "mmode_resv", 10))


I would check that name != NULL before strncmp.


> +			fdt_setprop((void *)dtb_pa, child, "no-map", NULL, 0);
> +	}
> +};
> +
>   void __init sbi_init(void)
>   {
>   	int ret;
> @@ -596,8 +632,12 @@ void __init sbi_init(void)
>   		sbi_major_version(), sbi_minor_version());
>   
>   	if (!sbi_spec_is_0_1()) {
> +		sbi_firmware_id = sbi_get_firmware_id();
> +		sbi_firmware_version = sbi_get_firmware_version();
> +
>   		pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
> -			sbi_get_firmware_id(), sbi_get_firmware_version());
> +			sbi_firmware_id, sbi_firmware_version);
> +
>   		if (sbi_probe_extension(SBI_EXT_TIME)) {
>   			__sbi_set_timer = __sbi_set_timer_v02;
>   			pr_info("SBI TIME extension detected\n");
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 70fb31960b63..cb16bfdeacdb 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -29,6 +29,7 @@
>   #include <asm/tlbflush.h>
>   #include <asm/sections.h>
>   #include <asm/soc.h>
> +#include <asm/sbi.h>
>   #include <asm/io.h>
>   #include <asm/ptdump.h>
>   #include <asm/numa.h>
> @@ -253,6 +254,8 @@ static void __init setup_bootmem(void)
>   	 * in the device tree, otherwise the allocation could end up in a
>   	 * reserved region.
>   	 */
> +
> +	sbi_apply_reserved_mem_erratum(dtb_early_va);
>   	early_init_fdt_scan_reserved_mem();
>   
>   	/*


Otherwise the patch looks good to me:

You can add:

Reviewed-by: Alexandre Ghiti <[e-mail]>

I just have one question though (that we discussed privately already): 
should we fix openSBI in the kernel? If yes, what makes a bug worth 
being fixed in the kernel?

Thanks,

JeeHeng Sia Aug. 7, 2023, 12:44 a.m. UTC | #2

> -----Original Message-----
> From: Conor Dooley <[e-mail]>
> Sent: Wednesday, August 2, 2023 7:13 PM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; Paul Walmsley <[e-mail]>; Atish Patra
> <[e-mail]>; Anup Patel <[e-mail]>; Alexandre Ghiti <[e-mail]>; Björn Töpel
> <[e-mail]>; Song Shuai <[e-mail]>; JeeHeng Sia <[e-mail]>; Petr Tesarik
> <[e-mail]>; [e-mail]; [e-mail]; [e-mail]
> Subject: [RFT 1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions
> 
> Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> the "no-map" property to the reserved memory nodes for the regions it
> has protected using PMPs.
> 
> Our existing fix sweeping hibernation under the carpet by marking it
> NONPORTABLE is insufficient as there are other ways to generate
> accesses to these reserved memory regions, as Petr discovered [1]
> while testing crash kernels & kdump.
> 
> Intercede during the boot process when the afflicted versions of OpenSBI
> are present & set the "no-map" property in all "mmode_resv" nodes before
> the kernel does its reserved memory region initialisation.
> 
> Reported-by: Song Shuai <[e-mail]>
> Link: https://lore.kernel.org/all/CAAYs2=[e-mail]/
> Reported-by: JeeHeng Sia <[e-mail]>
> Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKfA6z8
> Reported-by: Petr Tesarik <[e-mail]>
> Closes: https://lore.kernel.org/linux-riscv/[e-mail]/ [1]
> CC: [e-mail]
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
>  arch/riscv/include/asm/sbi.h |  5 +++++
>  arch/riscv/kernel/sbi.c      | 42 +++++++++++++++++++++++++++++++++++-
>  arch/riscv/mm/init.c         |  3 +++
>  3 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 5b4a1bf5f439..5360f3476278 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -252,6 +252,9 @@ enum sbi_pmu_ctr_type {
>  #define SBI_ERR_ALREADY_STARTED -7
>  #define SBI_ERR_ALREADY_STOPPED -8
> 
> +/* SBI implementation IDs */
> +#define SBI_IMP_OPENSBI	1
I would suggest to create an enum struct for the SBI Imp ID in the sbi.h file. What do you think?
> +
>  extern unsigned long sbi_spec_version;
>  struct sbiret {
>  	long error;
> @@ -259,6 +262,8 @@ struct sbiret {
>  };
> 
>  void sbi_init(void);
> +void sbi_apply_reserved_mem_erratum(void *dtb_va);
> +
>  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  			unsigned long arg1, unsigned long arg2,
>  			unsigned long arg3, unsigned long arg4,
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index c672c8ba9a2a..aeb27263fa53 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -5,8 +5,10 @@
>   * Copyright (c) 2020 Western Digital Corporation or its affiliates.
>   */
> 
> +#include <linux/acpi.h>
>  #include <linux/bits.h>
>  #include <linux/init.h>
> +#include <linux/libfdt.h>
>  #include <linux/pm.h>
>  #include <linux/reboot.h>
>  #include <asm/sbi.h>
> @@ -583,6 +585,40 @@ long sbi_get_mimpid(void)
>  }
>  EXPORT_SYMBOL_GPL(sbi_get_mimpid);
> 
> +static long sbi_firmware_id;
> +static long sbi_firmware_version;
> +
> +/*
> + * For devicetrees patched by OpenSBI a "mmode_resv" node is added to cover
> + * the region OpenSBI has protected by means of a PMP. Some versions of OpenSBI,
> + * [v0.8 to v1.3), omitted the "no-map" property, but this trips up hibernation
> + * among other things.
> + */
> +void __init sbi_apply_reserved_mem_erratum(void *dtb_pa)
> +{
> +	int child, reserved_mem;
> +
> +	if (sbi_firmware_id != SBI_IMP_OPENSBI)
> +		return;
> +
> +	if (!acpi_disabled)
> +		return;
> +
> +	if (sbi_firmware_version >= 0x10003 || sbi_firmware_version < 0x8)
> +		return;
> +
> +	reserved_mem = fdt_path_offset((void *)dtb_pa, "/reserved-memory");
> +	if (reserved_mem < 0)
> +		return;
> +
> +	fdt_for_each_subnode(child, (void *)dtb_pa, reserved_mem) {
> +		const char *name = fdt_get_name((void *)dtb_pa, child, NULL);
> +
> +		if (!strncmp(name, "mmode_resv", 10))
> +			fdt_setprop((void *)dtb_pa, child, "no-map", NULL, 0);
> +	}
> +};
> +
>  void __init sbi_init(void)
>  {
>  	int ret;
> @@ -596,8 +632,12 @@ void __init sbi_init(void)
>  		sbi_major_version(), sbi_minor_version());
> 
>  	if (!sbi_spec_is_0_1()) {
> +		sbi_firmware_id = sbi_get_firmware_id();
> +		sbi_firmware_version = sbi_get_firmware_version();
> +
>  		pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
> -			sbi_get_firmware_id(), sbi_get_firmware_version());
> +			sbi_firmware_id, sbi_firmware_version);
> +
>  		if (sbi_probe_extension(SBI_EXT_TIME)) {
>  			__sbi_set_timer = __sbi_set_timer_v02;
>  			pr_info("SBI TIME extension detected\n");
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 70fb31960b63..cb16bfdeacdb 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -29,6 +29,7 @@
>  #include <asm/tlbflush.h>
>  #include <asm/sections.h>
>  #include <asm/soc.h>
> +#include <asm/sbi.h>
>  #include <asm/io.h>
>  #include <asm/ptdump.h>
>  #include <asm/numa.h>
> @@ -253,6 +254,8 @@ static void __init setup_bootmem(void)
>  	 * in the device tree, otherwise the allocation could end up in a
>  	 * reserved region.
>  	 */
> +
> +	sbi_apply_reserved_mem_erratum(dtb_early_va);
>  	early_init_fdt_scan_reserved_mem();
> 
>  	/*
> --
> 2.40.1

Atish Patra Aug. 8, 2023, 7:54 a.m. UTC | #3

On Wed, Aug 2, 2023 at 4:14 AM Conor Dooley <[e-mail]> wrote:
>
> Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> the "no-map" property to the reserved memory nodes for the regions it
> has protected using PMPs.
>
> Our existing fix sweeping hibernation under the carpet by marking it
> NONPORTABLE is insufficient as there are other ways to generate
> accesses to these reserved memory regions, as Petr discovered [1]
> while testing crash kernels & kdump.
>
> Intercede during the boot process when the afflicted versions of OpenSBI
> are present & set the "no-map" property in all "mmode_resv" nodes before
> the kernel does its reserved memory region initialisation.
>

We have different mechanisms of DT being passed to the kernel.

1. A prior stage(e.g U-Boot SPL) to M-mode runtime firmware (e.g.
OpenSBI, rustSBI) passes the DT to M-mode runtime firmware and it
passes to the next stage.
In this case, M-mode runtime firmware gets a chance to update the
no-map property in DT that the kernel can parse.

2. User loads the DT from the boot loader (e.g EDK2, U-Boot proper).
Any DT patching done by the M-mode firmware is useless. If these DTBs
don't have the no-map
property, hibernation or EFI booting will have issues as well.

We are trying to solve only one part of problem #1 in this patch. I
don't think any other M-mode runtime firmware patches DT with no-map
property as well.
Please let me know if I am wrong about that. The problem is not
restricted to [v0.8 to v1.3) of OpenSBI.

The booting doc now says that no-map property must be set and any
usage of DTBs without that (via #1 or #2) will have unintended
consequences.

> Reported-by: Song Shuai <[e-mail]>
> Link: https://lore.kernel.org/all/CAAYs2=[e-mail]/
> Reported-by: JeeHeng Sia <[e-mail]>
> Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKfA6z8
> Reported-by: Petr Tesarik <[e-mail]>
> Closes: https://lore.kernel.org/linux-riscv/[e-mail]/ [1]
> CC: [e-mail]
> Signed-off-by: Conor Dooley <[e-mail]>
> ---
>  arch/riscv/include/asm/sbi.h |  5 +++++
>  arch/riscv/kernel/sbi.c      | 42 +++++++++++++++++++++++++++++++++++-
>  arch/riscv/mm/init.c         |  3 +++
>  3 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 5b4a1bf5f439..5360f3476278 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -252,6 +252,9 @@ enum sbi_pmu_ctr_type {
>  #define SBI_ERR_ALREADY_STARTED -7
>  #define SBI_ERR_ALREADY_STOPPED -8
>
> +/* SBI implementation IDs */
> +#define SBI_IMP_OPENSBI        1
> +
>  extern unsigned long sbi_spec_version;
>  struct sbiret {
>         long error;
> @@ -259,6 +262,8 @@ struct sbiret {
>  };
>
>  void sbi_init(void);
> +void sbi_apply_reserved_mem_erratum(void *dtb_va);
> +
>  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>                         unsigned long arg1, unsigned long arg2,
>                         unsigned long arg3, unsigned long arg4,
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index c672c8ba9a2a..aeb27263fa53 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -5,8 +5,10 @@
>   * Copyright (c) 2020 Western Digital Corporation or its affiliates.
>   */
>
> +#include <linux/acpi.h>
>  #include <linux/bits.h>
>  #include <linux/init.h>
> +#include <linux/libfdt.h>
>  #include <linux/pm.h>
>  #include <linux/reboot.h>
>  #include <asm/sbi.h>
> @@ -583,6 +585,40 @@ long sbi_get_mimpid(void)
>  }
>  EXPORT_SYMBOL_GPL(sbi_get_mimpid);
>
> +static long sbi_firmware_id;
> +static long sbi_firmware_version;
> +
> +/*
> + * For devicetrees patched by OpenSBI a "mmode_resv" node is added to cover
> + * the region OpenSBI has protected by means of a PMP. Some versions of OpenSBI,
> + * [v0.8 to v1.3), omitted the "no-map" property, but this trips up hibernation
> + * among other things.
> + */
> +void __init sbi_apply_reserved_mem_erratum(void *dtb_pa)
> +{
> +       int child, reserved_mem;
> +
> +       if (sbi_firmware_id != SBI_IMP_OPENSBI)
> +               return;
> +
> +       if (!acpi_disabled)
> +               return;
> +
> +       if (sbi_firmware_version >= 0x10003 || sbi_firmware_version < 0x8)
> +               return;
> +
> +       reserved_mem = fdt_path_offset((void *)dtb_pa, "/reserved-memory");
> +       if (reserved_mem < 0)
> +               return;
> +
> +       fdt_for_each_subnode(child, (void *)dtb_pa, reserved_mem) {
> +               const char *name = fdt_get_name((void *)dtb_pa, child, NULL);
> +
> +               if (!strncmp(name, "mmode_resv", 10))
> +                       fdt_setprop((void *)dtb_pa, child, "no-map", NULL, 0);
> +       }
> +};
> +
>  void __init sbi_init(void)
>  {
>         int ret;
> @@ -596,8 +632,12 @@ void __init sbi_init(void)
>                 sbi_major_version(), sbi_minor_version());
>
>         if (!sbi_spec_is_0_1()) {
> +               sbi_firmware_id = sbi_get_firmware_id();
> +               sbi_firmware_version = sbi_get_firmware_version();
> +
>                 pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
> -                       sbi_get_firmware_id(), sbi_get_firmware_version());
> +                       sbi_firmware_id, sbi_firmware_version);
> +
>                 if (sbi_probe_extension(SBI_EXT_TIME)) {
>                         __sbi_set_timer = __sbi_set_timer_v02;
>                         pr_info("SBI TIME extension detected\n");
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 70fb31960b63..cb16bfdeacdb 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -29,6 +29,7 @@
>  #include <asm/tlbflush.h>
>  #include <asm/sections.h>
>  #include <asm/soc.h>
> +#include <asm/sbi.h>
>  #include <asm/io.h>
>  #include <asm/ptdump.h>
>  #include <asm/numa.h>
> @@ -253,6 +254,8 @@ static void __init setup_bootmem(void)
>          * in the device tree, otherwise the allocation could end up in a
>          * reserved region.
>          */
> +
> +       sbi_apply_reserved_mem_erratum(dtb_early_va);
>         early_init_fdt_scan_reserved_mem();
>
>         /*
> --
> 2.40.1
>

Conor Dooley Aug. 8, 2023, 1:13 p.m. UTC | #4

On Mon, Aug 07, 2023 at 12:44:07AM +0000, JeeHeng Sia wrote:

> > +/* SBI implementation IDs */
> > +#define SBI_IMP_OPENSBI	1
> I would suggest to create an enum struct for the SBI Imp ID in
> the sbi.h file. What do you think?

I'm not really sure what the advantage of doing so is.

Conor Dooley Aug. 8, 2023, 1:39 p.m. UTC | #5

On Tue, Aug 08, 2023 at 12:54:11AM -0700, Atish Kumar Patra wrote:
> On Wed, Aug 2, 2023 at 4:14 AM Conor Dooley <[e-mail]> wrote:
> >
> > Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> > the "no-map" property to the reserved memory nodes for the regions it
> > has protected using PMPs.
> >
> > Our existing fix sweeping hibernation under the carpet by marking it
> > NONPORTABLE is insufficient as there are other ways to generate
> > accesses to these reserved memory regions, as Petr discovered [1]
> > while testing crash kernels & kdump.
> >
> > Intercede during the boot process when the afflicted versions of OpenSBI
> > are present & set the "no-map" property in all "mmode_resv" nodes before
> > the kernel does its reserved memory region initialisation.
> >
> 
> We have different mechanisms of DT being passed to the kernel.
> 
> 1. A prior stage(e.g U-Boot SPL) to M-mode runtime firmware (e.g.
> OpenSBI, rustSBI) passes the DT to M-mode runtime firmware and it
> passes to the next stage.
> In this case, M-mode runtime firmware gets a chance to update the
> no-map property in DT that the kernel can parse.
> 
> 2. User loads the DT from the boot loader (e.g EDK2, U-Boot proper).
> Any DT patching done by the M-mode firmware is useless. If these DTBs
> don't have the no-map
> property, hibernation or EFI booting will have issues as well.
> 

> We are trying to solve only one part of problem #1 in this patch.

Correct.

If someone's second stage is also providing an incorrect devicetree
then, yeah, this approach would fall apart - but it's the firmware
provided devicetree being incorrect that I am trying to account for
here. If a person incorrectly constructed one, I am not really sure what
we can do for them, they incorrect described their hardware /shrug
My patch should of course help in some of the scenarios you mention above
if the name of the reserved memory region from OpenSBI is propagated by
the second-stage bootloader, but that is just an extension of case 1,
not case 2.

> I
> don't think any other M-mode runtime firmware patches DT with no-map
> property as well.
> Please let me know if I am wrong about that. The problem is not
> restricted to [v0.8 to v1.3) of OpenSBI.

It comes down to Alex's question - do we want to fix this kind of
firmware issue in the kernel? Ultimately this is a policy decision that
"somebody" has to make. Maybe the list of firmwares that need this
grows, but hopefully this bug will be fixed everywhere going forward,
since, as you say, we now have a document.
The reason I've only targeted OpenSBI here is that it's what is being
shipped on people's SBCs etc by default & is also what is in the
versions of QEMU that people seem to be using. Unless there's some
example that I am missing, other firmwares are either not affected or
not actually being shipped on to consumers?

> The booting doc now says that no-map property must be set and any
> usage of DTBs without that (via #1 or #2) will have unintended
> consequences.

Which is great. It unfortunately does not help one iota with stuff
that's already in the wild.

Thanks,
Conor.

> > Reported-by: Song Shuai <[e-mail]>
> > Link: https://lore.kernel.org/all/CAAYs2=[e-mail]/
> > Reported-by: JeeHeng Sia <[e-mail]>
> > Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKfA6z8
> > Reported-by: Petr Tesarik <[e-mail]>
> > Closes: https://lore.kernel.org/linux-riscv/[e-mail]/ [1]
> > CC: [e-mail]
> > Signed-off-by: Conor Dooley <[e-mail]>
> > ---
> >  arch/riscv/include/asm/sbi.h |  5 +++++
> >  arch/riscv/kernel/sbi.c      | 42 +++++++++++++++++++++++++++++++++++-
> >  arch/riscv/mm/init.c         |  3 +++
> >  3 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 5b4a1bf5f439..5360f3476278 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -252,6 +252,9 @@ enum sbi_pmu_ctr_type {
> >  #define SBI_ERR_ALREADY_STARTED -7
> >  #define SBI_ERR_ALREADY_STOPPED -8
> >
> > +/* SBI implementation IDs */
> > +#define SBI_IMP_OPENSBI        1
> > +
> >  extern unsigned long sbi_spec_version;
> >  struct sbiret {
> >         long error;
> > @@ -259,6 +262,8 @@ struct sbiret {
> >  };
> >
> >  void sbi_init(void);
> > +void sbi_apply_reserved_mem_erratum(void *dtb_va);
> > +
> >  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> >                         unsigned long arg1, unsigned long arg2,
> >                         unsigned long arg3, unsigned long arg4,
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index c672c8ba9a2a..aeb27263fa53 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -5,8 +5,10 @@
> >   * Copyright (c) 2020 Western Digital Corporation or its affiliates.
> >   */
> >
> > +#include <linux/acpi.h>
> >  #include <linux/bits.h>
> >  #include <linux/init.h>
> > +#include <linux/libfdt.h>
> >  #include <linux/pm.h>
> >  #include <linux/reboot.h>
> >  #include <asm/sbi.h>
> > @@ -583,6 +585,40 @@ long sbi_get_mimpid(void)
> >  }
> >  EXPORT_SYMBOL_GPL(sbi_get_mimpid);
> >
> > +static long sbi_firmware_id;
> > +static long sbi_firmware_version;
> > +
> > +/*
> > + * For devicetrees patched by OpenSBI a "mmode_resv" node is added to cover
> > + * the region OpenSBI has protected by means of a PMP. Some versions of OpenSBI,
> > + * [v0.8 to v1.3), omitted the "no-map" property, but this trips up hibernation
> > + * among other things.
> > + */
> > +void __init sbi_apply_reserved_mem_erratum(void *dtb_pa)
> > +{
> > +       int child, reserved_mem;
> > +
> > +       if (sbi_firmware_id != SBI_IMP_OPENSBI)
> > +               return;
> > +
> > +       if (!acpi_disabled)
> > +               return;
> > +
> > +       if (sbi_firmware_version >= 0x10003 || sbi_firmware_version < 0x8)
> > +               return;
> > +
> > +       reserved_mem = fdt_path_offset((void *)dtb_pa, "/reserved-memory");
> > +       if (reserved_mem < 0)
> > +               return;
> > +
> > +       fdt_for_each_subnode(child, (void *)dtb_pa, reserved_mem) {
> > +               const char *name = fdt_get_name((void *)dtb_pa, child, NULL);
> > +
> > +               if (!strncmp(name, "mmode_resv", 10))
> > +                       fdt_setprop((void *)dtb_pa, child, "no-map", NULL, 0);
> > +       }
> > +};
> > +
> >  void __init sbi_init(void)
> >  {
> >         int ret;
> > @@ -596,8 +632,12 @@ void __init sbi_init(void)
> >                 sbi_major_version(), sbi_minor_version());
> >
> >         if (!sbi_spec_is_0_1()) {
> > +               sbi_firmware_id = sbi_get_firmware_id();
> > +               sbi_firmware_version = sbi_get_firmware_version();
> > +
> >                 pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
> > -                       sbi_get_firmware_id(), sbi_get_firmware_version());
> > +                       sbi_firmware_id, sbi_firmware_version);
> > +
> >                 if (sbi_probe_extension(SBI_EXT_TIME)) {
> >                         __sbi_set_timer = __sbi_set_timer_v02;
> >                         pr_info("SBI TIME extension detected\n");
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 70fb31960b63..cb16bfdeacdb 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -29,6 +29,7 @@
> >  #include <asm/tlbflush.h>
> >  #include <asm/sections.h>
> >  #include <asm/soc.h>
> > +#include <asm/sbi.h>
> >  #include <asm/io.h>
> >  #include <asm/ptdump.h>
> >  #include <asm/numa.h>
> > @@ -253,6 +254,8 @@ static void __init setup_bootmem(void)
> >          * in the device tree, otherwise the allocation could end up in a
> >          * reserved region.
> >          */
> > +
> > +       sbi_apply_reserved_mem_erratum(dtb_early_va);
> >         early_init_fdt_scan_reserved_mem();
> >
> >         /*
> > --
> > 2.40.1
> >

Atish Patra Aug. 9, 2023, 9:01 a.m. UTC | #6

On Tue, Aug 8, 2023 at 6:39 AM Conor Dooley <[e-mail]> wrote:
>
> On Tue, Aug 08, 2023 at 12:54:11AM -0700, Atish Kumar Patra wrote:
> > On Wed, Aug 2, 2023 at 4:14 AM Conor Dooley <[e-mail]> wrote:
> > >
> > > Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> > > the "no-map" property to the reserved memory nodes for the regions it
> > > has protected using PMPs.
> > >
> > > Our existing fix sweeping hibernation under the carpet by marking it
> > > NONPORTABLE is insufficient as there are other ways to generate
> > > accesses to these reserved memory regions, as Petr discovered [1]
> > > while testing crash kernels & kdump.
> > >
> > > Intercede during the boot process when the afflicted versions of OpenSBI
> > > are present & set the "no-map" property in all "mmode_resv" nodes before
> > > the kernel does its reserved memory region initialisation.
> > >
> >
> > We have different mechanisms of DT being passed to the kernel.
> >
> > 1. A prior stage(e.g U-Boot SPL) to M-mode runtime firmware (e.g.
> > OpenSBI, rustSBI) passes the DT to M-mode runtime firmware and it
> > passes to the next stage.
> > In this case, M-mode runtime firmware gets a chance to update the
> > no-map property in DT that the kernel can parse.
> >
> > 2. User loads the DT from the boot loader (e.g EDK2, U-Boot proper).
> > Any DT patching done by the M-mode firmware is useless. If these DTBs
> > don't have the no-map
> > property, hibernation or EFI booting will have issues as well.
> >
>
> > We are trying to solve only one part of problem #1 in this patch.
>
> Correct.
>
> If someone's second stage is also providing an incorrect devicetree
> then, yeah, this approach would fall apart - but it's the firmware
> provided devicetree being incorrect that I am trying to account for
> here. If a person incorrectly constructed one, I am not really sure what
> we can do for them, they incorrect described their hardware /shrug
> My patch should of course help in some of the scenarios you mention above
> if the name of the reserved memory region from OpenSBI is propagated by
> the second-stage bootloader, but that is just an extension of case 1,
> not case 2.
>
> > I
> > don't think any other M-mode runtime firmware patches DT with no-map
> > property as well.
> > Please let me know if I am wrong about that. The problem is not
> > restricted to [v0.8 to v1.3) of OpenSBI.
>
> It comes down to Alex's question - do we want to fix this kind of
> firmware issue in the kernel? Ultimately this is a policy decision that
> "somebody" has to make. Maybe the list of firmwares that need this

IMO, we shouldn't as this is a slippery slope. Kernel can't fix every
firmware bug by having erratas.
I agree with your point below about firmware in shipping products. I
am not aware of any official products shipping anything other than
OpenSBI
either. However, I have seen users using other firmwares in their dev
environment.
IMHO, this approach sets a bad precedent for the future especially
when it only solves one part of the problem.
We shouldn't hide firmware bugs in the kernel when an upgraded
firmware is already available.

This bug is well documented in various threads and fixed in the latest
version of OpenSBI.
I am assuming other firmwares will follow it as well.

Anybody facing hibernation or efi related booting issues should just
upgrade to the latest version of firmware (e.g OpenSBI v1.3)
Latest version of Qemu will support(if not happened already) the
latest version of OpenSBI.

This issue will only manifest in kernels 6.4 or higher. Any user
facing these with the latest kernel can also upgrade the firmware.
Do you see any issue with that ?

> grows, but hopefully this bug will be fixed everywhere going forward,
> since, as you say, we now have a document.
> The reason I've only targeted OpenSBI here is that it's what is being
> shipped on people's SBCs etc by default & is also what is in the
> versions of QEMU that people seem to be using. Unless there's some
> example that I am missing, other firmwares are either not affected or
> not actually being shipped on to consumers?
>
> > The booting doc now says that no-map property must be set and any
> > usage of DTBs without that (via #1 or #2) will have unintended
> > consequences.
>
> Which is great. It unfortunately does not help one iota with stuff
> that's already in the wild.
>
> Thanks,
> Conor.
>
> > > Reported-by: Song Shuai <[e-mail]>
> > > Link: https://lore.kernel.org/all/CAAYs2=[e-mail]/
> > > Reported-by: JeeHeng Sia <[e-mail]>
> > > Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKfA6z8
> > > Reported-by: Petr Tesarik <[e-mail]>
> > > Closes: https://lore.kernel.org/linux-riscv/[e-mail]/ [1]
> > > CC: [e-mail]
> > > Signed-off-by: Conor Dooley <[e-mail]>
> > > ---
> > >  arch/riscv/include/asm/sbi.h |  5 +++++
> > >  arch/riscv/kernel/sbi.c      | 42 +++++++++++++++++++++++++++++++++++-
> > >  arch/riscv/mm/init.c         |  3 +++
> > >  3 files changed, 49 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > > index 5b4a1bf5f439..5360f3476278 100644
> > > --- a/arch/riscv/include/asm/sbi.h
> > > +++ b/arch/riscv/include/asm/sbi.h
> > > @@ -252,6 +252,9 @@ enum sbi_pmu_ctr_type {
> > >  #define SBI_ERR_ALREADY_STARTED -7
> > >  #define SBI_ERR_ALREADY_STOPPED -8
> > >
> > > +/* SBI implementation IDs */
> > > +#define SBI_IMP_OPENSBI        1
> > > +
> > >  extern unsigned long sbi_spec_version;
> > >  struct sbiret {
> > >         long error;
> > > @@ -259,6 +262,8 @@ struct sbiret {
> > >  };
> > >
> > >  void sbi_init(void);
> > > +void sbi_apply_reserved_mem_erratum(void *dtb_va);
> > > +
> > >  struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
> > >                         unsigned long arg1, unsigned long arg2,
> > >                         unsigned long arg3, unsigned long arg4,
> > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > index c672c8ba9a2a..aeb27263fa53 100644
> > > --- a/arch/riscv/kernel/sbi.c
> > > +++ b/arch/riscv/kernel/sbi.c
> > > @@ -5,8 +5,10 @@
> > >   * Copyright (c) 2020 Western Digital Corporation or its affiliates.
> > >   */
> > >
> > > +#include <linux/acpi.h>
> > >  #include <linux/bits.h>
> > >  #include <linux/init.h>
> > > +#include <linux/libfdt.h>
> > >  #include <linux/pm.h>
> > >  #include <linux/reboot.h>
> > >  #include <asm/sbi.h>
> > > @@ -583,6 +585,40 @@ long sbi_get_mimpid(void)
> > >  }
> > >  EXPORT_SYMBOL_GPL(sbi_get_mimpid);
> > >
> > > +static long sbi_firmware_id;
> > > +static long sbi_firmware_version;
> > > +
> > > +/*
> > > + * For devicetrees patched by OpenSBI a "mmode_resv" node is added to cover
> > > + * the region OpenSBI has protected by means of a PMP. Some versions of OpenSBI,
> > > + * [v0.8 to v1.3), omitted the "no-map" property, but this trips up hibernation
> > > + * among other things.
> > > + */
> > > +void __init sbi_apply_reserved_mem_erratum(void *dtb_pa)
> > > +{
> > > +       int child, reserved_mem;
> > > +
> > > +       if (sbi_firmware_id != SBI_IMP_OPENSBI)
> > > +               return;
> > > +
> > > +       if (!acpi_disabled)
> > > +               return;
> > > +
> > > +       if (sbi_firmware_version >= 0x10003 || sbi_firmware_version < 0x8)
> > > +               return;
> > > +
> > > +       reserved_mem = fdt_path_offset((void *)dtb_pa, "/reserved-memory");
> > > +       if (reserved_mem < 0)
> > > +               return;
> > > +
> > > +       fdt_for_each_subnode(child, (void *)dtb_pa, reserved_mem) {
> > > +               const char *name = fdt_get_name((void *)dtb_pa, child, NULL);
> > > +
> > > +               if (!strncmp(name, "mmode_resv", 10))
> > > +                       fdt_setprop((void *)dtb_pa, child, "no-map", NULL, 0);
> > > +       }
> > > +};
> > > +
> > >  void __init sbi_init(void)
> > >  {
> > >         int ret;
> > > @@ -596,8 +632,12 @@ void __init sbi_init(void)
> > >                 sbi_major_version(), sbi_minor_version());
> > >
> > >         if (!sbi_spec_is_0_1()) {
> > > +               sbi_firmware_id = sbi_get_firmware_id();
> > > +               sbi_firmware_version = sbi_get_firmware_version();
> > > +
> > >                 pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
> > > -                       sbi_get_firmware_id(), sbi_get_firmware_version());
> > > +                       sbi_firmware_id, sbi_firmware_version);
> > > +
> > >                 if (sbi_probe_extension(SBI_EXT_TIME)) {
> > >                         __sbi_set_timer = __sbi_set_timer_v02;
> > >                         pr_info("SBI TIME extension detected\n");
> > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > > index 70fb31960b63..cb16bfdeacdb 100644
> > > --- a/arch/riscv/mm/init.c
> > > +++ b/arch/riscv/mm/init.c
> > > @@ -29,6 +29,7 @@
> > >  #include <asm/tlbflush.h>
> > >  #include <asm/sections.h>
> > >  #include <asm/soc.h>
> > > +#include <asm/sbi.h>
> > >  #include <asm/io.h>
> > >  #include <asm/ptdump.h>
> > >  #include <asm/numa.h>
> > > @@ -253,6 +254,8 @@ static void __init setup_bootmem(void)
> > >          * in the device tree, otherwise the allocation could end up in a
> > >          * reserved region.
> > >          */
> > > +
> > > +       sbi_apply_reserved_mem_erratum(dtb_early_va);
> > >         early_init_fdt_scan_reserved_mem();
> > >
> > >         /*
> > > --
> > > 2.40.1
> > >

JeeHeng Sia Aug. 9, 2023, 10:24 a.m. UTC | #7

> -----Original Message-----
> From: Conor Dooley <[e-mail]>
> Sent: Tuesday, August 8, 2023 9:13 PM
> To: JeeHeng Sia <[e-mail]>
> Cc: Conor Dooley <[e-mail]>; [e-mail]; Paul Walmsley <[e-mail]>; Atish Patra
> <[e-mail]>; Anup Patel <[e-mail]>; Alexandre Ghiti <[e-mail]>; Björn Töpel
> <[e-mail]>; Song Shuai <[e-mail]>; Petr Tesarik <[e-mail]>; linux-
> [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [RFT 1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions
> 
> On Mon, Aug 07, 2023 at 12:44:07AM +0000, JeeHeng Sia wrote:
> 
> > > +/* SBI implementation IDs */
> > > +#define SBI_IMP_OPENSBI	1
> > I would suggest to create an enum struct for the SBI Imp ID in
> > the sbi.h file. What do you think?
> 
> I'm not really sure what the advantage of doing so is.
The macro SBI_IMP_OPENSBI seems weird (I would read it as "SBI Implementation OpenSBI"). However, if we implement an enum struct for SBI_IMP_ID (There are numerous IDs available), the macro can be abbreviated to OpenSBI. By doing this, the conditional checking of the implementation ID would be more readable, as shown below:
if (sbi_firmware_id != OPENSBI)

Conor Dooley Aug. 9, 2023, 11:04 a.m. UTC | #8

On Wed, Aug 09, 2023 at 10:24:57AM +0000, JeeHeng Sia wrote:
> 
> 
> > -----Original Message-----
> > From: Conor Dooley <[e-mail]>
> > Sent: Tuesday, August 8, 2023 9:13 PM
> > To: JeeHeng Sia <[e-mail]>
> > Cc: Conor Dooley <[e-mail]>; [e-mail]; Paul Walmsley <[e-mail]>; Atish Patra
> > <[e-mail]>; Anup Patel <[e-mail]>; Alexandre Ghiti <[e-mail]>; Björn Töpel
> > <[e-mail]>; Song Shuai <[e-mail]>; Petr Tesarik <[e-mail]>; linux-
> > [e-mail]; [e-mail]; [e-mail]
> > Subject: Re: [RFT 1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions
> > 
> > On Mon, Aug 07, 2023 at 12:44:07AM +0000, JeeHeng Sia wrote:
> > 
> > > > +/* SBI implementation IDs */
> > > > +#define SBI_IMP_OPENSBI	1
> > > I would suggest to create an enum struct for the SBI Imp ID in
> > > the sbi.h file. What do you think?
> > 
> > I'm not really sure what the advantage of doing so is.
> The macro SBI_IMP_OPENSBI seems weird (I would read it as "SBI Implementation OpenSBI").

That is what it is though, so I don't see what's weird about that.

> However, if we implement an enum struct for SBI_IMP_ID

> (There are numerous IDs available),

Ohh I know, but I didn't see the point adding those when I was only
focusing on a single implementation.

> the macro can be abbreviated to OpenSBI. By doing this, the conditional
> checking of the implementation ID would be more readable, as shown below:
> if (sbi_firmware_id != OPENSBI)

I don't see that it can become that simple, it'd still need to be
prefixed with SBI_ to be consistent with any other SBI related enum, and
at that point adding the extra IMP_ makes little odds.

Conor Dooley Aug. 10, 2023, 9:07 a.m. UTC | #9

On Wed, Aug 09, 2023 at 02:01:07AM -0700, Atish Kumar Patra wrote:
> On Tue, Aug 8, 2023 at 6:39 AM Conor Dooley <[e-mail]> wrote:
> >
> > On Tue, Aug 08, 2023 at 12:54:11AM -0700, Atish Kumar Patra wrote:
> > > On Wed, Aug 2, 2023 at 4:14 AM Conor Dooley <[e-mail]> wrote:
> > > >
> > > > Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> > > > the "no-map" property to the reserved memory nodes for the regions it
> > > > has protected using PMPs.
> > > >
> > > > Our existing fix sweeping hibernation under the carpet by marking it
> > > > NONPORTABLE is insufficient as there are other ways to generate
> > > > accesses to these reserved memory regions, as Petr discovered [1]
> > > > while testing crash kernels & kdump.
> > > >
> > > > Intercede during the boot process when the afflicted versions of OpenSBI
> > > > are present & set the "no-map" property in all "mmode_resv" nodes before
> > > > the kernel does its reserved memory region initialisation.
> > > >
> > >
> > > We have different mechanisms of DT being passed to the kernel.
> > >
> > > 1. A prior stage(e.g U-Boot SPL) to M-mode runtime firmware (e.g.
> > > OpenSBI, rustSBI) passes the DT to M-mode runtime firmware and it
> > > passes to the next stage.
> > > In this case, M-mode runtime firmware gets a chance to update the
> > > no-map property in DT that the kernel can parse.
> > >
> > > 2. User loads the DT from the boot loader (e.g EDK2, U-Boot proper).
> > > Any DT patching done by the M-mode firmware is useless. If these DTBs
> > > don't have the no-map
> > > property, hibernation or EFI booting will have issues as well.
> > >
> >
> > > We are trying to solve only one part of problem #1 in this patch.
> >
> > Correct.
> >
> > If someone's second stage is also providing an incorrect devicetree
> > then, yeah, this approach would fall apart - but it's the firmware
> > provided devicetree being incorrect that I am trying to account for
> > here. If a person incorrectly constructed one, I am not really sure what
> > we can do for them, they incorrect described their hardware /shrug
> > My patch should of course help in some of the scenarios you mention above
> > if the name of the reserved memory region from OpenSBI is propagated by
> > the second-stage bootloader, but that is just an extension of case 1,
> > not case 2.
> >
> > > I
> > > don't think any other M-mode runtime firmware patches DT with no-map
> > > property as well.
> > > Please let me know if I am wrong about that. The problem is not
> > > restricted to [v0.8 to v1.3) of OpenSBI.
> >
> > It comes down to Alex's question - do we want to fix this kind of
> > firmware issue in the kernel? Ultimately this is a policy decision that
> > "somebody" has to make. Maybe the list of firmwares that need this
> 
> IMO, we shouldn't as this is a slippery slope. Kernel can't fix every
> firmware bug by having erratas.
> I agree with your point below about firmware in shipping products. I
> am not aware of any official products shipping anything other than
> OpenSBI either.

> However, I have seen users using other firmwares in their dev
> environment.

If someone's already changed their boards firmware, I have less sympathy
for them, as they should be able to make further changes. Punters buying
SBCs to install Fedora or Debian w/o having to consider their firmware
are who I am more interested in helping.

> IMHO, this approach sets a bad precedent for the future especially
> when it only solves one part of the problem.

Yeah, I'm certainly wary of setting an unwise precedent here.
Inevitably we will need to have firmware-related errata and it'd be good
to have a policy for what is (or more importantly what isn't
acceptable). Certainly we have said that known-broken version of OpenSBI
that T-Head puts in their SDK is not supported by the mainline kernel.
On the latter part, I'm perfectly happy to expand the erratum to cover
all affected firmwares, but I wasn't even sure if my fix worked
properly, hence the request for testing from those who encountered the
problem.

> We shouldn't hide firmware bugs in the kernel when an upgraded
> firmware is already available.

Just to note, availability of an updated firmware upstream does not
necessarily mean that corresponding update is possible for affected
hardware.

> This bug is well documented in various threads and fixed in the latest
> version of OpenSBI.
> I am assuming other firmwares will follow it as well.
> 
> Anybody facing hibernation or efi related booting issues should just
> upgrade to the latest version of firmware (e.g OpenSBI v1.3)
> Latest version of Qemu will support(if not happened already) the
> latest version of OpenSBI.
> 
> This issue will only manifest in kernels 6.4 or higher. Any user
> facing these with the latest kernel can also upgrade the firmware.
> Do you see any issue with that ?

I don't think it is fair to compare the ease of upgrading the kernel
to that required to upgrade a boards firmware, with the latter being
far, far more inconvenient on pretty much all of the boards that I have.

I'm perfectly happy to drop this series though, if people generally are
of the opinion that this sort of firmware workaround is ill-advised.
We are unaffected by it, so I certainly have no pressure to have
something working here. It's my desire not to be user-hostile that
motivated this patch.

Palmer Dabbelt Dec. 6, 2023, 12:52 p.m. UTC | #10

On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
> On Wed, Aug 09, 2023 at 02:01:07AM -0700, Atish Kumar Patra wrote:
>> On Tue, Aug 8, 2023 at 6:39 AM Conor Dooley <[e-mail]> wrote:
>> >
>> > On Tue, Aug 08, 2023 at 12:54:11AM -0700, Atish Kumar Patra wrote:
>> > > On Wed, Aug 2, 2023 at 4:14 AM Conor Dooley <[e-mail]> wrote:
>> > > >
>> > > > Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
>> > > > the "no-map" property to the reserved memory nodes for the regions it
>> > > > has protected using PMPs.
>> > > >
>> > > > Our existing fix sweeping hibernation under the carpet by marking it
>> > > > NONPORTABLE is insufficient as there are other ways to generate
>> > > > accesses to these reserved memory regions, as Petr discovered [1]
>> > > > while testing crash kernels & kdump.
>> > > >
>> > > > Intercede during the boot process when the afflicted versions of OpenSBI
>> > > > are present & set the "no-map" property in all "mmode_resv" nodes before
>> > > > the kernel does its reserved memory region initialisation.
>> > > >
>> > >
>> > > We have different mechanisms of DT being passed to the kernel.
>> > >
>> > > 1. A prior stage(e.g U-Boot SPL) to M-mode runtime firmware (e.g.
>> > > OpenSBI, rustSBI) passes the DT to M-mode runtime firmware and it
>> > > passes to the next stage.
>> > > In this case, M-mode runtime firmware gets a chance to update the
>> > > no-map property in DT that the kernel can parse.
>> > >
>> > > 2. User loads the DT from the boot loader (e.g EDK2, U-Boot proper).
>> > > Any DT patching done by the M-mode firmware is useless. If these DTBs
>> > > don't have the no-map
>> > > property, hibernation or EFI booting will have issues as well.
>> > >
>> >
>> > > We are trying to solve only one part of problem #1 in this patch.
>> >
>> > Correct.
>> >
>> > If someone's second stage is also providing an incorrect devicetree
>> > then, yeah, this approach would fall apart - but it's the firmware
>> > provided devicetree being incorrect that I am trying to account for
>> > here. If a person incorrectly constructed one, I am not really sure what
>> > we can do for them, they incorrect described their hardware /shrug
>> > My patch should of course help in some of the scenarios you mention above
>> > if the name of the reserved memory region from OpenSBI is propagated by
>> > the second-stage bootloader, but that is just an extension of case 1,
>> > not case 2.
>> >
>> > > I
>> > > don't think any other M-mode runtime firmware patches DT with no-map
>> > > property as well.
>> > > Please let me know if I am wrong about that. The problem is not
>> > > restricted to [v0.8 to v1.3) of OpenSBI.
>> >
>> > It comes down to Alex's question - do we want to fix this kind of
>> > firmware issue in the kernel? Ultimately this is a policy decision that
>> > "somebody" has to make. Maybe the list of firmwares that need this
>> 
>> IMO, we shouldn't as this is a slippery slope. Kernel can't fix every
>> firmware bug by having erratas.
>> I agree with your point below about firmware in shipping products. I
>> am not aware of any official products shipping anything other than
>> OpenSBI either.
>
>> However, I have seen users using other firmwares in their dev
>> environment.
>
> If someone's already changed their boards firmware, I have less sympathy
> for them, as they should be able to make further changes. Punters buying
> SBCs to install Fedora or Debian w/o having to consider their firmware
> are who I am more interested in helping.
>
>> IMHO, this approach sets a bad precedent for the future especially
>> when it only solves one part of the problem.
>
> Yeah, I'm certainly wary of setting an unwise precedent here.
> Inevitably we will need to have firmware-related errata and it'd be good
> to have a policy for what is (or more importantly what isn't
> acceptable). Certainly we have said that known-broken version of OpenSBI
> that T-Head puts in their SDK is not supported by the mainline kernel.
> On the latter part, I'm perfectly happy to expand the erratum to cover
> all affected firmwares, but I wasn't even sure if my fix worked
> properly, hence the request for testing from those who encountered the
> problem.
>
>> We shouldn't hide firmware bugs in the kernel when an upgraded
>> firmware is already available.
>
> Just to note, availability of an updated firmware upstream does not
> necessarily mean that corresponding update is possible for affected
> hardware.

Yep.  I think we're been in a very hobbist-centric world in RISC-V land, 
but in general trying to get people to update firmware is hard.  Part of 
the whole "kernel updates don't break users" thing is what's underneath 
the kernel, it's not just a uABI thing.

>> This bug is well documented in various threads and fixed in the latest
>> version of OpenSBI.
>> I am assuming other firmwares will follow it as well.
>> 
>> Anybody facing hibernation or efi related booting issues should just
>> upgrade to the latest version of firmware (e.g OpenSBI v1.3)
>> Latest version of Qemu will support(if not happened already) the
>> latest version of OpenSBI.
>> 
>> This issue will only manifest in kernels 6.4 or higher. Any user
>> facing these with the latest kernel can also upgrade the firmware.
>> Do you see any issue with that ?
>
> I don't think it is fair to compare the ease of upgrading the kernel
> to that required to upgrade a boards firmware, with the latter being
> far, far more inconvenient on pretty much all of the boards that I have.

IMO we're in the same spot as every other port here, and generally they 
work around firmware bugs when they've rolled out into production 
somewhere that firmware updates aren't likely to happen quickly.  I'm 
not sure if there's any sort of exact rules written down anywhere, but 
IMO if the bug is going to impact users then we should deal with it.

That applies for hardware bugs, but also firmware bugs (at a certain 
point we won't be able to tell the difference).  We're sort of doing 
this with the misaligned access handling, for example.

> I'm perfectly happy to drop this series though, if people generally are
> of the opinion that this sort of firmware workaround is ill-advised.
> We are unaffected by it, so I certainly have no pressure to have
> something working here. It's my desire not to be user-hostile that
> motivated this patch.

IIUC you guys and Reneas are the only ones who have hardware that might 
be in a spot where users aren't able to update the firmware (ie, it's 
out in production somewhere).  So I'm adding Geert, though he probably 
saw this months ago...

On that note: It's been ~4 months and it look like nobody's tested 
anything (and the comments aren't really things that would preculde 
testing).  So maybe we just pick that second patch up into for-next and 
see what happens?  IIUC that will result in broken systems for users who 
haven't updated their firmware.

I agree that's a user-hostile way to do things, which is generally a bad 
way to go, but if it's really true that there's no users then we're 
safe.  Probably also worth calling it out on sw-dev just to be safe.

Conor Dooley Dec. 6, 2023, 2:23 p.m. UTC | #11

On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
> On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
> > On Wed, Aug 09, 2023 at 02:01:07AM -0700, Atish Kumar Patra wrote:
> > > On Tue, Aug 8, 2023 at 6:39 AM Conor Dooley <[e-mail]> wrote:
> > > >
> > > > On Tue, Aug 08, 2023 at 12:54:11AM -0700, Atish Kumar Patra wrote:
> > > > > On Wed, Aug 2, 2023 at 4:14 AM Conor Dooley <[e-mail]> wrote:
> > > > > >
> > > > > > Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> > > > > > the "no-map" property to the reserved memory nodes for the regions it
> > > > > > has protected using PMPs.
> > > > > >
> > > > > > Our existing fix sweeping hibernation under the carpet by marking it
> > > > > > NONPORTABLE is insufficient as there are other ways to generate
> > > > > > accesses to these reserved memory regions, as Petr discovered [1]
> > > > > > while testing crash kernels & kdump.
> > > > > >
> > > > > > Intercede during the boot process when the afflicted versions of OpenSBI
> > > > > > are present & set the "no-map" property in all "mmode_resv" nodes before
> > > > > > the kernel does its reserved memory region initialisation.
> > > > > >
> > > > >
> > > > > We have different mechanisms of DT being passed to the kernel.
> > > > >
> > > > > 1. A prior stage(e.g U-Boot SPL) to M-mode runtime firmware (e.g.
> > > > > OpenSBI, rustSBI) passes the DT to M-mode runtime firmware and it
> > > > > passes to the next stage.
> > > > > In this case, M-mode runtime firmware gets a chance to update the
> > > > > no-map property in DT that the kernel can parse.
> > > > >
> > > > > 2. User loads the DT from the boot loader (e.g EDK2, U-Boot proper).
> > > > > Any DT patching done by the M-mode firmware is useless. If these DTBs
> > > > > don't have the no-map
> > > > > property, hibernation or EFI booting will have issues as well.
> > > > >
> > > >
> > > > > We are trying to solve only one part of problem #1 in this patch.
> > > >
> > > > Correct.
> > > >
> > > > If someone's second stage is also providing an incorrect devicetree
> > > > then, yeah, this approach would fall apart - but it's the firmware
> > > > provided devicetree being incorrect that I am trying to account for
> > > > here. If a person incorrectly constructed one, I am not really sure what
> > > > we can do for them, they incorrect described their hardware /shrug
> > > > My patch should of course help in some of the scenarios you mention above
> > > > if the name of the reserved memory region from OpenSBI is propagated by
> > > > the second-stage bootloader, but that is just an extension of case 1,
> > > > not case 2.
> > > >
> > > > > I
> > > > > don't think any other M-mode runtime firmware patches DT with no-map
> > > > > property as well.
> > > > > Please let me know if I am wrong about that. The problem is not
> > > > > restricted to [v0.8 to v1.3) of OpenSBI.
> > > >
> > > > It comes down to Alex's question - do we want to fix this kind of
> > > > firmware issue in the kernel? Ultimately this is a policy decision that
> > > > "somebody" has to make. Maybe the list of firmwares that need this
> > > 
> > > IMO, we shouldn't as this is a slippery slope. Kernel can't fix every
> > > firmware bug by having erratas.
> > > I agree with your point below about firmware in shipping products. I
> > > am not aware of any official products shipping anything other than
> > > OpenSBI either.
> > 
> > > However, I have seen users using other firmwares in their dev
> > > environment.
> > 
> > If someone's already changed their boards firmware, I have less sympathy
> > for them, as they should be able to make further changes. Punters buying
> > SBCs to install Fedora or Debian w/o having to consider their firmware
> > are who I am more interested in helping.
> > 
> > > IMHO, this approach sets a bad precedent for the future especially
> > > when it only solves one part of the problem.
> > 
> > Yeah, I'm certainly wary of setting an unwise precedent here.
> > Inevitably we will need to have firmware-related errata and it'd be good
> > to have a policy for what is (or more importantly what isn't
> > acceptable). Certainly we have said that known-broken version of OpenSBI
> > that T-Head puts in their SDK is not supported by the mainline kernel.
> > On the latter part, I'm perfectly happy to expand the erratum to cover
> > all affected firmwares, but I wasn't even sure if my fix worked
> > properly, hence the request for testing from those who encountered the
> > problem.
> > 
> > > We shouldn't hide firmware bugs in the kernel when an upgraded
> > > firmware is already available.
> > 
> > Just to note, availability of an updated firmware upstream does not
> > necessarily mean that corresponding update is possible for affected
> > hardware.
> 
> Yep.  I think we're been in a very hobbist-centric world in RISC-V land, but
> in general trying to get people to update firmware is hard.  Part of the
> whole "kernel updates don't break users" thing is what's underneath the
> kernel, it's not just a uABI thing.

Yeah, there's certainly an attitude that I think needs to go away, that
updating firmware etc is something we can expect to be carried out on a
universal basis. Or that fixing things in the upstream version of
OpenSBI means it'll actually propagate down to system integrators.

> 
> > > This bug is well documented in various threads and fixed in the latest
> > > version of OpenSBI.
> > > I am assuming other firmwares will follow it as well.
> > > 
> > > Anybody facing hibernation or efi related booting issues should just
> > > upgrade to the latest version of firmware (e.g OpenSBI v1.3)
> > > Latest version of Qemu will support(if not happened already) the
> > > latest version of OpenSBI.
> > > 
> > > This issue will only manifest in kernels 6.4 or higher. Any user
> > > facing these with the latest kernel can also upgrade the firmware.
> > > Do you see any issue with that ?
> > 
> > I don't think it is fair to compare the ease of upgrading the kernel
> > to that required to upgrade a boards firmware, with the latter being
> > far, far more inconvenient on pretty much all of the boards that I have.
> 
> IMO we're in the same spot as every other port here, and generally they work
> around firmware bugs when they've rolled out into production somewhere that
> firmware updates aren't likely to happen quickly.  I'm not sure if there's
> any sort of exact rules written down anywhere, but IMO if the bug is going
> to impact users then we should deal with it.
> 
> That applies for hardware bugs, but also firmware bugs (at a certain point
> we won't be able to tell the difference).  We're sort of doing this with the
> misaligned access handling, for example.
> 
> > I'm perfectly happy to drop this series though, if people generally are
> > of the opinion that this sort of firmware workaround is ill-advised.
> > We are unaffected by it, so I certainly have no pressure to have
> > something working here. It's my desire not to be user-hostile that
> > motivated this patch.
> 
> IIUC you guys and Reneas are the only ones who have hardware that might be
> in a spot where users aren't able to update the firmware (ie, it's out in
> production somewhere).

I dunno if we can really keep thinking like that though. In terms of
people who have devicetrees in the kernel and stuff available in western
catalog distribution, sure.
I don't think we can assume that that covers all users though, certainly
the syntacore folks pop up every now and then, and I sure hope that
Andes etc have larger customer bases than the in-kernel users would
suggest.

> So I'm adding Geert, though he probably saw this
> months ago...

Prabhakar might be a good call on that front. I'm not sure if the
Renesas stuff works on affected versions of OpenSBI though, guess it
depends on the sequencing of the support for the non-coherent stuff and
when this bug was fixed.

> On that note: It's been ~4 months and it look like nobody's tested anything
> (and the comments aren't really things that would preculde testing).

Yeah, nobody seems to really have given a crap. I was hoping the
StarFive guys that actually added the support for this would be
interested in it, but alas they were not.
I don't really care all that much - the platform I support is not
affected by the problem and I just don't enable the option elsewhere.

> So
> maybe we just pick that second patch up into for-next and see what happens?
> IIUC that will result in broken systems for users who haven't updated their
> firmware.
> 
> I agree that's a user-hostile way to do things, which is generally a bad way
> to go, but if it's really true that there's no users then we're safe.
> Probably also worth calling it out on sw-dev just to be safe.

And if there are users, the fix is actually relatively straight-forward,
just apply patch #1.

Lad, Prabhakar Dec. 7, 2023, 1:02 p.m. UTC | #12

On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <[e-mail]> wrote:
>
> On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
> > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
> > > On Wed, Aug 09, 2023 at 02:01:07AM -0700, Atish Kumar Patra wrote:
> > > > On Tue, Aug 8, 2023 at 6:39 AM Conor Dooley <[e-mail]> wrote:
> > > > >
> > > > > On Tue, Aug 08, 2023 at 12:54:11AM -0700, Atish Kumar Patra wrote:
> > > > > > On Wed, Aug 2, 2023 at 4:14 AM Conor Dooley <[e-mail]> wrote:
> > > > > > >
> > > > > > > Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> > > > > > > the "no-map" property to the reserved memory nodes for the regions it
> > > > > > > has protected using PMPs.
> > > > > > >
> > > > > > > Our existing fix sweeping hibernation under the carpet by marking it
> > > > > > > NONPORTABLE is insufficient as there are other ways to generate
> > > > > > > accesses to these reserved memory regions, as Petr discovered [1]
> > > > > > > while testing crash kernels & kdump.
> > > > > > >
> > > > > > > Intercede during the boot process when the afflicted versions of OpenSBI
> > > > > > > are present & set the "no-map" property in all "mmode_resv" nodes before
> > > > > > > the kernel does its reserved memory region initialisation.
> > > > > > >
> > > > > >
> > > > > > We have different mechanisms of DT being passed to the kernel.
> > > > > >
> > > > > > 1. A prior stage(e.g U-Boot SPL) to M-mode runtime firmware (e.g.
> > > > > > OpenSBI, rustSBI) passes the DT to M-mode runtime firmware and it
> > > > > > passes to the next stage.
> > > > > > In this case, M-mode runtime firmware gets a chance to update the
> > > > > > no-map property in DT that the kernel can parse.
> > > > > >
> > > > > > 2. User loads the DT from the boot loader (e.g EDK2, U-Boot proper).
> > > > > > Any DT patching done by the M-mode firmware is useless. If these DTBs
> > > > > > don't have the no-map
> > > > > > property, hibernation or EFI booting will have issues as well.
> > > > > >
> > > > >
> > > > > > We are trying to solve only one part of problem #1 in this patch.
> > > > >
> > > > > Correct.
> > > > >
> > > > > If someone's second stage is also providing an incorrect devicetree
> > > > > then, yeah, this approach would fall apart - but it's the firmware
> > > > > provided devicetree being incorrect that I am trying to account for
> > > > > here. If a person incorrectly constructed one, I am not really sure what
> > > > > we can do for them, they incorrect described their hardware /shrug
> > > > > My patch should of course help in some of the scenarios you mention above
> > > > > if the name of the reserved memory region from OpenSBI is propagated by
> > > > > the second-stage bootloader, but that is just an extension of case 1,
> > > > > not case 2.
> > > > >
> > > > > > I
> > > > > > don't think any other M-mode runtime firmware patches DT with no-map
> > > > > > property as well.
> > > > > > Please let me know if I am wrong about that. The problem is not
> > > > > > restricted to [v0.8 to v1.3) of OpenSBI.
> > > > >
> > > > > It comes down to Alex's question - do we want to fix this kind of
> > > > > firmware issue in the kernel? Ultimately this is a policy decision that
> > > > > "somebody" has to make. Maybe the list of firmwares that need this
> > > >
> > > > IMO, we shouldn't as this is a slippery slope. Kernel can't fix every
> > > > firmware bug by having erratas.
> > > > I agree with your point below about firmware in shipping products. I
> > > > am not aware of any official products shipping anything other than
> > > > OpenSBI either.
> > >
> > > > However, I have seen users using other firmwares in their dev
> > > > environment.
> > >
> > > If someone's already changed their boards firmware, I have less sympathy
> > > for them, as they should be able to make further changes. Punters buying
> > > SBCs to install Fedora or Debian w/o having to consider their firmware
> > > are who I am more interested in helping.
> > >
> > > > IMHO, this approach sets a bad precedent for the future especially
> > > > when it only solves one part of the problem.
> > >
> > > Yeah, I'm certainly wary of setting an unwise precedent here.
> > > Inevitably we will need to have firmware-related errata and it'd be good
> > > to have a policy for what is (or more importantly what isn't
> > > acceptable). Certainly we have said that known-broken version of OpenSBI
> > > that T-Head puts in their SDK is not supported by the mainline kernel.
> > > On the latter part, I'm perfectly happy to expand the erratum to cover
> > > all affected firmwares, but I wasn't even sure if my fix worked
> > > properly, hence the request for testing from those who encountered the
> > > problem.
> > >
> > > > We shouldn't hide firmware bugs in the kernel when an upgraded
> > > > firmware is already available.
> > >
> > > Just to note, availability of an updated firmware upstream does not
> > > necessarily mean that corresponding update is possible for affected
> > > hardware.
> >
> > Yep.  I think we're been in a very hobbist-centric world in RISC-V land, but
> > in general trying to get people to update firmware is hard.  Part of the
> > whole "kernel updates don't break users" thing is what's underneath the
> > kernel, it's not just a uABI thing.
>
> Yeah, there's certainly an attitude that I think needs to go away, that
> updating firmware etc is something we can expect to be carried out on a
> universal basis. Or that fixing things in the upstream version of
> OpenSBI means it'll actually propagate down to system integrators.
>
> >
> > > > This bug is well documented in various threads and fixed in the latest
> > > > version of OpenSBI.
> > > > I am assuming other firmwares will follow it as well.
> > > >
> > > > Anybody facing hibernation or efi related booting issues should just
> > > > upgrade to the latest version of firmware (e.g OpenSBI v1.3)
> > > > Latest version of Qemu will support(if not happened already) the
> > > > latest version of OpenSBI.
> > > >
> > > > This issue will only manifest in kernels 6.4 or higher. Any user
> > > > facing these with the latest kernel can also upgrade the firmware.
> > > > Do you see any issue with that ?
> > >
> > > I don't think it is fair to compare the ease of upgrading the kernel
> > > to that required to upgrade a boards firmware, with the latter being
> > > far, far more inconvenient on pretty much all of the boards that I have.
> >
> > IMO we're in the same spot as every other port here, and generally they work
> > around firmware bugs when they've rolled out into production somewhere that
> > firmware updates aren't likely to happen quickly.  I'm not sure if there's
> > any sort of exact rules written down anywhere, but IMO if the bug is going
> > to impact users then we should deal with it.
> >
> > That applies for hardware bugs, but also firmware bugs (at a certain point
> > we won't be able to tell the difference).  We're sort of doing this with the
> > misaligned access handling, for example.
> >
> > > I'm perfectly happy to drop this series though, if people generally are
> > > of the opinion that this sort of firmware workaround is ill-advised.
> > > We are unaffected by it, so I certainly have no pressure to have
> > > something working here. It's my desire not to be user-hostile that
> > > motivated this patch.
> >
> > IIUC you guys and Reneas are the only ones who have hardware that might be
> > in a spot where users aren't able to update the firmware (ie, it's out in
> > production somewhere).
>
> I dunno if we can really keep thinking like that though. In terms of
> people who have devicetrees in the kernel and stuff available in western
> catalog distribution, sure.
> I don't think we can assume that that covers all users though, certainly
> the syntacore folks pop up every now and then, and I sure hope that
> Andes etc have larger customer bases than the in-kernel users would
> suggest.
>
> > So I'm adding Geert, though he probably saw this
> > months ago...
>
> Prabhakar might be a good call on that front. I'm not sure if the
> Renesas stuff works on affected versions of OpenSBI though, guess it
> depends on the sequencing of the support for the non-coherent stuff and
> when this bug was fixed.
>
ATM, I dont think there are any users who are using the upstream
kernel + OpenSBI (apart from me and Geert!). Currently the customers
are using the BSP releases.

Cheers,
Prabhakar

> > On that note: It's been ~4 months and it look like nobody's tested anything
> > (and the comments aren't really things that would preculde testing).
>
> Yeah, nobody seems to really have given a crap. I was hoping the
> StarFive guys that actually added the support for this would be
> interested in it, but alas they were not.
> I don't really care all that much - the platform I support is not
> affected by the problem and I just don't enable the option elsewhere.
>
> > So
> > maybe we just pick that second patch up into for-next and see what happens?
> > IIUC that will result in broken systems for users who haven't updated their
> > firmware.
> >
> > I agree that's a user-hostile way to do things, which is generally a bad way
> > to go, but if it's really true that there's no users then we're safe.
> > Probably also worth calling it out on sw-dev just to be safe.
>
> And if there are users, the fix is actually relatively straight-forward,
> just apply patch #1.
> _______________________________________________
> linux-riscv mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley Dec. 7, 2023, 1:11 p.m. UTC | #13

On Thu, Dec 07, 2023 at 01:02:00PM +0000, Lad, Prabhakar wrote:
> On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <[e-mail]> wrote:
> > On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
> > > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:

> > > > I'm perfectly happy to drop this series though, if people generally are
> > > > of the opinion that this sort of firmware workaround is ill-advised.
> > > > We are unaffected by it, so I certainly have no pressure to have
> > > > something working here. It's my desire not to be user-hostile that
> > > > motivated this patch.
> > >
> > > IIUC you guys and Reneas are the only ones who have hardware that might be
> > > in a spot where users aren't able to update the firmware (ie, it's out in
> > > production somewhere).
> >
> > I dunno if we can really keep thinking like that though. In terms of
> > people who have devicetrees in the kernel and stuff available in western
> > catalog distribution, sure.
> > I don't think we can assume that that covers all users though, certainly
> > the syntacore folks pop up every now and then, and I sure hope that
> > Andes etc have larger customer bases than the in-kernel users would
> > suggest.
> >
> > > So I'm adding Geert, though he probably saw this
> > > months ago...
> >
> > Prabhakar might be a good call on that front. I'm not sure if the
> > Renesas stuff works on affected versions of OpenSBI though, guess it
> > depends on the sequencing of the support for the non-coherent stuff and
> > when this bug was fixed.
> >
> ATM, I dont think there are any users who are using the upstream
> kernel + OpenSBI (apart from me and Geert!). Currently the customers
> are using the BSP releases.

That doesn't really answer whether or not you (and your customers) are
using an affected version of the vendor OpenSBI?
The affected range for OpenSBI itself is [v0.8 to v1.3).

Conor Dooley Dec. 19, 2023, 5:18 p.m. UTC | #14

Hey,

On Thu, Dec 07, 2023 at 01:11:23PM +0000, Conor Dooley wrote:
> On Thu, Dec 07, 2023 at 01:02:00PM +0000, Lad, Prabhakar wrote:
> > On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <[e-mail]> wrote:
> > > On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
> > > > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
> 
> > > > > I'm perfectly happy to drop this series though, if people generally are
> > > > > of the opinion that this sort of firmware workaround is ill-advised.
> > > > > We are unaffected by it, so I certainly have no pressure to have
> > > > > something working here. It's my desire not to be user-hostile that
> > > > > motivated this patch.
> > > >
> > > > IIUC you guys and Reneas are the only ones who have hardware that might be
> > > > in a spot where users aren't able to update the firmware (ie, it's out in
> > > > production somewhere).
> > >
> > > I dunno if we can really keep thinking like that though. In terms of
> > > people who have devicetrees in the kernel and stuff available in western
> > > catalog distribution, sure.
> > > I don't think we can assume that that covers all users though, certainly
> > > the syntacore folks pop up every now and then, and I sure hope that
> > > Andes etc have larger customer bases than the in-kernel users would
> > > suggest.
> > >
> > > > So I'm adding Geert, though he probably saw this
> > > > months ago...
> > >
> > > Prabhakar might be a good call on that front. I'm not sure if the
> > > Renesas stuff works on affected versions of OpenSBI though, guess it
> > > depends on the sequencing of the support for the non-coherent stuff and
> > > when this bug was fixed.
> > >
> > ATM, I dont think there are any users who are using the upstream
> > kernel + OpenSBI (apart from me and Geert!). Currently the customers
> > are using the BSP releases.
> 
> That doesn't really answer whether or not you (and your customers) are
> using an affected version of the vendor OpenSBI?
> The affected range for OpenSBI itself is [v0.8 to v1.3).

Did you perhaps miss this mail Prabhakar?

Cheers,
Conor.

Lad Prabhakar Dec. 19, 2023, 5:27 p.m. UTC | #15

Hi Conor,

> -----Original Message-----
> From: Conor Dooley <[e-mail]>
> Sent: Tuesday, December 19, 2023 5:18 PM
> To: Conor Dooley <[e-mail]>
> Cc: Lad, Prabhakar <[e-mail]>; Palmer Dabbelt <[e-mail]>;
> [e-mail]; Atish Patra <[e-mail]>; Paul Walmsley <[e-mail]>;
> [e-mail]; [e-mail]; Bjorn Topel <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail]; Prabhakar Mahadev Lad
> <[e-mail]>
> Subject: Re: [RFT 1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions
> 
> Hey,
> 
> On Thu, Dec 07, 2023 at 01:11:23PM +0000, Conor Dooley wrote:
> > On Thu, Dec 07, 2023 at 01:02:00PM +0000, Lad, Prabhakar wrote:
> > > On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <[e-mail]> wrote:
> > > > On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
> > > > > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
> >
> > > > > > I'm perfectly happy to drop this series though, if people
> > > > > > generally are of the opinion that this sort of firmware workaround is ill-advised.
> > > > > > We are unaffected by it, so I certainly have no pressure to
> > > > > > have something working here. It's my desire not to be
> > > > > > user-hostile that motivated this patch.
> > > > >
> > > > > IIUC you guys and Reneas are the only ones who have hardware
> > > > > that might be in a spot where users aren't able to update the
> > > > > firmware (ie, it's out in production somewhere).
> > > >
> > > > I dunno if we can really keep thinking like that though. In terms
> > > > of people who have devicetrees in the kernel and stuff available
> > > > in western catalog distribution, sure.
> > > > I don't think we can assume that that covers all users though,
> > > > certainly the syntacore folks pop up every now and then, and I
> > > > sure hope that Andes etc have larger customer bases than the
> > > > in-kernel users would suggest.
> > > >
> > > > > So I'm adding Geert, though he probably saw this months ago...
> > > >
> > > > Prabhakar might be a good call on that front. I'm not sure if the
> > > > Renesas stuff works on affected versions of OpenSBI though, guess
> > > > it depends on the sequencing of the support for the non-coherent
> > > > stuff and when this bug was fixed.
> > > >
> > > ATM, I dont think there are any users who are using the upstream
> > > kernel + OpenSBI (apart from me and Geert!). Currently the customers
> > > are using the BSP releases.
> >
> > That doesn't really answer whether or not you (and your customers) are
> > using an affected version of the vendor OpenSBI?
> > The affected range for OpenSBI itself is [v0.8 to v1.3).
> 
> Did you perhaps miss this mail Prabhakar?
> 
Oops sorry for that.

I can confirm the BSP version used by the customers is v1.0 [0].

[0] https://github.com/renesas-rz/rz_opensbi/commits/work/OpenSBI-PMA/

Cheers,
Prabhakar

Palmer Dabbelt Dec. 19, 2023, 6:06 p.m. UTC | #16

On Tue, 19 Dec 2023 09:27:42 PST (-0800), [e-mail] wrote:
> Hi Conor,
> 
>> -----Original Message-----
>> From: Conor Dooley <[e-mail]>
>> Sent: Tuesday, December 19, 2023 5:18 PM
>> To: Conor Dooley <[e-mail]>
>> Cc: Lad, Prabhakar <[e-mail]>; Palmer Dabbelt <[e-mail]>;
>> [e-mail]; Atish Patra <[e-mail]>; Paul Walmsley <[e-mail]>;
>> [e-mail]; [e-mail]; Bjorn Topel <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail]; [e-mail]; Prabhakar Mahadev Lad
>> <[e-mail]>
>> Subject: Re: [RFT 1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions
>> 
>> Hey,
>> 
>> On Thu, Dec 07, 2023 at 01:11:23PM +0000, Conor Dooley wrote:
>> > On Thu, Dec 07, 2023 at 01:02:00PM +0000, Lad, Prabhakar wrote:
>> > > On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <[e-mail]> wrote:
>> > > > On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
>> > > > > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
>> >
>> > > > > > I'm perfectly happy to drop this series though, if people
>> > > > > > generally are of the opinion that this sort of firmware workaround is ill-advised.
>> > > > > > We are unaffected by it, so I certainly have no pressure to
>> > > > > > have something working here. It's my desire not to be
>> > > > > > user-hostile that motivated this patch.
>> > > > >
>> > > > > IIUC you guys and Reneas are the only ones who have hardware
>> > > > > that might be in a spot where users aren't able to update the
>> > > > > firmware (ie, it's out in production somewhere).
>> > > >
>> > > > I dunno if we can really keep thinking like that though. In terms
>> > > > of people who have devicetrees in the kernel and stuff available
>> > > > in western catalog distribution, sure.
>> > > > I don't think we can assume that that covers all users though,
>> > > > certainly the syntacore folks pop up every now and then, and I
>> > > > sure hope that Andes etc have larger customer bases than the
>> > > > in-kernel users would suggest.
>> > > >
>> > > > > So I'm adding Geert, though he probably saw this months ago...
>> > > >
>> > > > Prabhakar might be a good call on that front. I'm not sure if the
>> > > > Renesas stuff works on affected versions of OpenSBI though, guess
>> > > > it depends on the sequencing of the support for the non-coherent
>> > > > stuff and when this bug was fixed.
>> > > >
>> > > ATM, I dont think there are any users who are using the upstream
>> > > kernel + OpenSBI (apart from me and Geert!). Currently the customers
>> > > are using the BSP releases.
>> >
>> > That doesn't really answer whether or not you (and your customers) are
>> > using an affected version of the vendor OpenSBI?
>> > The affected range for OpenSBI itself is [v0.8 to v1.3).
>> 
>> Did you perhaps miss this mail Prabhakar?
>> 
> Oops sorry for that.
> 
> I can confirm the BSP version used by the customers is v1.0 [0].
> 
> [0] https://github.com/renesas-rz/rz_opensbi/commits/work/OpenSBI-PMA/

OK, so sounds like would end up with broken systems from this bug, then?

IIRC we still have the Renesas systems as NONPORTABLE due to that DMA 
pool Kconfig conflict.  So if it's really only these Renesas systems 
that have the bug, maybe we can still remove hibernation from 
NONPORTABLE and just add in some sort of Kconfig to disable the 
Renesas+hibernation combinations that would break?

> 
> Cheers,
> Prabhakar

Lad Prabhakar Dec. 19, 2023, 6:38 p.m. UTC | #17

Hi Palmer,

> -----Original Message-----
> From: Palmer Dabbelt <[e-mail]>
> Sent: Tuesday, December 19, 2023 6:07 PM
> To: Prabhakar Mahadev Lad <[e-mail]>
> Cc: Conor Dooley <[e-mail]>; Conor Dooley <[e-mail]>;
> [e-mail]; [e-mail]; Atish Patra <[e-mail]>; Paul Walmsley
> <[e-mail]>; [e-mail]; [e-mail]; Bjorn Topel
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Subject: RE: [RFT 1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions
> 
> On Tue, 19 Dec 2023 09:27:42 PST (-0800), [e-mail] wrote:
> > Hi Conor,
> >
> >> -----Original Message-----
> >> From: Conor Dooley <[e-mail]>
> >> Sent: Tuesday, December 19, 2023 5:18 PM
> >> To: Conor Dooley <[e-mail]>
> >> Cc: Lad, Prabhakar <[e-mail]>; Palmer Dabbelt
> >> <[e-mail]>;
> >> [e-mail]; Atish Patra <[e-mail]>; Paul
> >> geert+Walmsley <[e-mail]>;
> >> [e-mail]; [e-mail]; Bjorn Topel
> >> <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; linux-
> >> [e-mail]; [e-mail];
> >> [e-mail]; Prabhakar Mahadev Lad
> >> <[e-mail]>
> >> Subject: Re: [RFT 1/2] RISC-V: handle missing "no-map" properties for
> >> OpenSBI's PMP protected regions
> >>
> >> Hey,
> >>
> >> On Thu, Dec 07, 2023 at 01:11:23PM +0000, Conor Dooley wrote:
> >> > On Thu, Dec 07, 2023 at 01:02:00PM +0000, Lad, Prabhakar wrote:
> >> > > On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <[e-mail]> wrote:
> >> > > > On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
> >> > > > > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
> >> >
> >> > > > > > I'm perfectly happy to drop this series though, if people
> >> > > > > > generally are of the opinion that this sort of firmware workaround is ill-advised.
> >> > > > > > We are unaffected by it, so I certainly have no pressure to
> >> > > > > > have something working here. It's my desire not to be
> >> > > > > > user-hostile that motivated this patch.
> >> > > > >
> >> > > > > IIUC you guys and Reneas are the only ones who have hardware
> >> > > > > that might be in a spot where users aren't able to update the
> >> > > > > firmware (ie, it's out in production somewhere).
> >> > > >
> >> > > > I dunno if we can really keep thinking like that though. In
> >> > > > terms of people who have devicetrees in the kernel and stuff
> >> > > > available in western catalog distribution, sure.
> >> > > > I don't think we can assume that that covers all users though,
> >> > > > certainly the syntacore folks pop up every now and then, and I
> >> > > > sure hope that Andes etc have larger customer bases than the
> >> > > > in-kernel users would suggest.
> >> > > >
> >> > > > > So I'm adding Geert, though he probably saw this months ago...
> >> > > >
> >> > > > Prabhakar might be a good call on that front. I'm not sure if
> >> > > > the Renesas stuff works on affected versions of OpenSBI though,
> >> > > > guess it depends on the sequencing of the support for the
> >> > > > non-coherent stuff and when this bug was fixed.
> >> > > >
> >> > > ATM, I dont think there are any users who are using the upstream
> >> > > kernel + OpenSBI (apart from me and Geert!). Currently the
> >> > > customers are using the BSP releases.
> >> >
> >> > That doesn't really answer whether or not you (and your customers)
> >> > are using an affected version of the vendor OpenSBI?
> >> > The affected range for OpenSBI itself is [v0.8 to v1.3).
> >>
> >> Did you perhaps miss this mail Prabhakar?
> >>
> > Oops sorry for that.
> >
> > I can confirm the BSP version used by the customers is v1.0 [0].
> >
> > [0]
> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> > ub.com%2Frenesas-rz%2Frz_opensbi%2Fcommits%2Fwork%2FOpenSBI-PMA%2F&dat
> > a=05%7C02%7Cprabhakar.mahadev-lad.rj%40bp.renesas.com%7C014cf4ddfd1e48
> > 1ff5bc08dc00bd467f%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638386
> > 060130410731%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=a0tcsXY4EQlODi
> > I34QygXS9QpJnVBqL8bNkxE8N5J2g%3D&reserved=0
> 
> OK, so sounds like would end up with broken systems from this bug, then?
> 
> IIRC we still have the Renesas systems as NONPORTABLE due to that DMA pool Kconfig conflict.  So if
> it's really only these Renesas systems that have the bug, maybe we can still remove hibernation from
> NONPORTABLE and just add in some sort of Kconfig to disable the
> Renesas+hibernation combinations that would break?
> 
Well customers using BSP uses v1.0 for OpenSBI and kernel 5.10-cip, and people wanting to run upstream kernel will have to only use the upstream OpenSBI as the OpenSBI used in BSP is not compatible with upstream kernel(Linux doesn’t bootup).

ATM I can say that its only me and Geert using upstream OpenBSI and upstream kernel.

With that in mind would we still require that change?

Cheers,
Prabhakar

Conor Dooley Dec. 19, 2023, 6:57 p.m. UTC | #18

On 19 December 2023 18:38:30 GMT, Prabhakar Mahadev Lad <[e-mail]> wrote:
>Hi Palmer,
>
>> -----Original Message-----
>> From: Palmer Dabbelt <[e-mail]>
>> Sent: Tuesday, December 19, 2023 6:07 PM
>> To: Prabhakar Mahadev Lad <[e-mail]>
>> Cc: Conor Dooley <[e-mail]>; Conor Dooley <[e-mail]>;
>> [e-mail]; [e-mail]; Atish Patra <[e-mail]>; Paul Walmsley
>> <[e-mail]>; [e-mail]; [e-mail]; Bjorn Topel
>> <[e-mail]>; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]
>> Subject: RE: [RFT 1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions
>> 
>> On Tue, 19 Dec 2023 09:27:42 PST (-0800), [e-mail] wrote:
>> > Hi Conor,
>> >
>> >> -----Original Message-----
>> >> From: Conor Dooley <[e-mail]>
>> >> Sent: Tuesday, December 19, 2023 5:18 PM
>> >> To: Conor Dooley <[e-mail]>
>> >> Cc: Lad, Prabhakar <[e-mail]>; Palmer Dabbelt
>> >> <[e-mail]>;
>> >> [e-mail]; Atish Patra <[e-mail]>; Paul
>> >> geert+Walmsley <[e-mail]>;
>> >> [e-mail]; [e-mail]; Bjorn Topel
>> >> <[e-mail]>; [e-mail];
>> >> [e-mail]; [e-mail]; linux-
>> >> [e-mail]; [e-mail];
>> >> [e-mail]; Prabhakar Mahadev Lad
>> >> <[e-mail]>
>> >> Subject: Re: [RFT 1/2] RISC-V: handle missing "no-map" properties for
>> >> OpenSBI's PMP protected regions
>> >>
>> >> Hey,
>> >>
>> >> On Thu, Dec 07, 2023 at 01:11:23PM +0000, Conor Dooley wrote:
>> >> > On Thu, Dec 07, 2023 at 01:02:00PM +0000, Lad, Prabhakar wrote:
>> >> > > On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <[e-mail]> wrote:
>> >> > > > On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
>> >> > > > > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
>> >> >
>> >> > > > > > I'm perfectly happy to drop this series though, if people
>> >> > > > > > generally are of the opinion that this sort of firmware workaround is ill-advised.
>> >> > > > > > We are unaffected by it, so I certainly have no pressure to
>> >> > > > > > have something working here. It's my desire not to be
>> >> > > > > > user-hostile that motivated this patch.
>> >> > > > >
>> >> > > > > IIUC you guys and Reneas are the only ones who have hardware
>> >> > > > > that might be in a spot where users aren't able to update the
>> >> > > > > firmware (ie, it's out in production somewhere).
>> >> > > >
>> >> > > > I dunno if we can really keep thinking like that though. In
>> >> > > > terms of people who have devicetrees in the kernel and stuff
>> >> > > > available in western catalog distribution, sure.
>> >> > > > I don't think we can assume that that covers all users though,
>> >> > > > certainly the syntacore folks pop up every now and then, and I
>> >> > > > sure hope that Andes etc have larger customer bases than the
>> >> > > > in-kernel users would suggest.
>> >> > > >
>> >> > > > > So I'm adding Geert, though he probably saw this months ago...
>> >> > > >
>> >> > > > Prabhakar might be a good call on that front. I'm not sure if
>> >> > > > the Renesas stuff works on affected versions of OpenSBI though,
>> >> > > > guess it depends on the sequencing of the support for the
>> >> > > > non-coherent stuff and when this bug was fixed.
>> >> > > >
>> >> > > ATM, I dont think there are any users who are using the upstream
>> >> > > kernel + OpenSBI (apart from me and Geert!). Currently the
>> >> > > customers are using the BSP releases.
>> >> >
>> >> > That doesn't really answer whether or not you (and your customers)
>> >> > are using an affected version of the vendor OpenSBI?
>> >> > The affected range for OpenSBI itself is [v0.8 to v1.3).
>> >>
>> >> Did you perhaps miss this mail Prabhakar?
>> >>
>> > Oops sorry for that.
>> >
>> > I can confirm the BSP version used by the customers is v1.0 [0].
>> >
>> > [0]
>> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
>> > ub.com%2Frenesas-rz%2Frz_opensbi%2Fcommits%2Fwork%2FOpenSBI-PMA%2F&dat
>> > a=05%7C02%7Cprabhakar.mahadev-lad.rj%40bp.renesas.com%7C014cf4ddfd1e48
>> > 1ff5bc08dc00bd467f%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638386
>> > 060130410731%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
>> > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=a0tcsXY4EQlODi
>> > I34QygXS9QpJnVBqL8bNkxE8N5J2g%3D&reserved=0
>> 
>> OK, so sounds like would end up with broken systems from this bug, then?
>> 
>> IIRC we still have the Renesas systems as NONPORTABLE due to that DMA pool Kconfig conflict.  So if
>> it's really only these Renesas systems that have the bug, maybe we can still remove hibernation from
>> NONPORTABLE and just add in some sort of Kconfig to disable the
>> Renesas+hibernation combinations that would break?
>> 
>Well customers using BSP uses v1.0 for OpenSBI and kernel 5.10-cip, and people wanting to run upstream kernel will have to only use the upstream OpenSBI as the OpenSBI used in BSP is not compatible with upstream kernel(Linux doesn’t bootup).
>
>ATM I can say that its only me and Geert using upstream OpenBSI and upstream kernel.
>
>With that in mind would we still require that change?

5.10 doesn't have hibernation support in it, although I'm not sure why anyone would really want to use a kernel that old with a RISC-V system.

The upstream versions of opensbi that support the renesas stuff have the no-map fix, right?
If that's the case, then nothing special config wise is likely required.

I'm still wary of other systems though, we are acting as if it is only Microchip and Renesas are the "real" users.

Lad Prabhakar Dec. 19, 2023, 7:46 p.m. UTC | #19

> -----Original Message-----
> From: Conor Dooley <[e-mail]>
> Sent: Tuesday, December 19, 2023 6:58 PM
> To: Prabhakar Mahadev Lad <[e-mail]>; Palmer Dabbelt
> <[e-mail]>
> Cc: Conor Dooley <[e-mail]>; [e-mail]; [e-mail];
> Atish Patra <[e-mail]>; Paul Walmsley <[e-mail]>; [e-mail];
> [e-mail]; Bjorn Topel <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail]
> Subject: RE: [RFT 1/2] RISC-V: handle missing "no-map" properties for OpenSBI's PMP protected regions
> 
> 
> 
> On 19 December 2023 18:38:30 GMT, Prabhakar Mahadev Lad <[e-mail]>
> wrote:
> >Hi Palmer,
> >
> >> -----Original Message-----
> >> From: Palmer Dabbelt <[e-mail]>
> >> Sent: Tuesday, December 19, 2023 6:07 PM
> >> To: Prabhakar Mahadev Lad <[e-mail]>
> >> Cc: Conor Dooley <[e-mail]>; Conor Dooley
> >> <[e-mail]>; [e-mail];
> >> [e-mail]; Atish Patra <[e-mail]>; Paul
> >> Walmsley <[e-mail]>; [e-mail];
> >> [e-mail]; Bjorn Topel <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: RE: [RFT 1/2] RISC-V: handle missing "no-map" properties for
> >> OpenSBI's PMP protected regions
> >>
> >> On Tue, 19 Dec 2023 09:27:42 PST (-0800), [e-mail] wrote:
> >> > Hi Conor,
> >> >
> >> >> -----Original Message-----
> >> >> From: Conor Dooley <[e-mail]>
> >> >> Sent: Tuesday, December 19, 2023 5:18 PM
> >> >> To: Conor Dooley <[e-mail]>
> >> >> Cc: Lad, Prabhakar <[e-mail]>; Palmer Dabbelt
> >> >> <[e-mail]>;
> >> >> [e-mail]; Atish Patra <[e-mail]>; Paul
> >> >> geert+Walmsley <[e-mail]>;
> >> >> [e-mail]; [e-mail]; Bjorn Topel
> >> >> <[e-mail]>; [e-mail];
> >> >> [e-mail]; [e-mail]; linux-
> >> >> [e-mail]; [e-mail];
> >> >> [e-mail]; Prabhakar Mahadev Lad
> >> >> <[e-mail]>
> >> >> Subject: Re: [RFT 1/2] RISC-V: handle missing "no-map" properties
> >> >> for OpenSBI's PMP protected regions
> >> >>
> >> >> Hey,
> >> >>
> >> >> On Thu, Dec 07, 2023 at 01:11:23PM +0000, Conor Dooley wrote:
> >> >> > On Thu, Dec 07, 2023 at 01:02:00PM +0000, Lad, Prabhakar wrote:
> >> >> > > On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <[e-mail]> wrote:
> >> >> > > > On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
> >> >> > > > > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
> >> >> >
> >> >> > > > > > I'm perfectly happy to drop this series though, if
> >> >> > > > > > people generally are of the opinion that this sort of firmware workaround is ill-
> advised.
> >> >> > > > > > We are unaffected by it, so I certainly have no pressure
> >> >> > > > > > to have something working here. It's my desire not to be
> >> >> > > > > > user-hostile that motivated this patch.
> >> >> > > > >
> >> >> > > > > IIUC you guys and Reneas are the only ones who have
> >> >> > > > > hardware that might be in a spot where users aren't able
> >> >> > > > > to update the firmware (ie, it's out in production somewhere).
> >> >> > > >
> >> >> > > > I dunno if we can really keep thinking like that though. In
> >> >> > > > terms of people who have devicetrees in the kernel and stuff
> >> >> > > > available in western catalog distribution, sure.
> >> >> > > > I don't think we can assume that that covers all users
> >> >> > > > though, certainly the syntacore folks pop up every now and
> >> >> > > > then, and I sure hope that Andes etc have larger customer
> >> >> > > > bases than the in-kernel users would suggest.
> >> >> > > >
> >> >> > > > > So I'm adding Geert, though he probably saw this months ago...
> >> >> > > >
> >> >> > > > Prabhakar might be a good call on that front. I'm not sure
> >> >> > > > if the Renesas stuff works on affected versions of OpenSBI
> >> >> > > > though, guess it depends on the sequencing of the support
> >> >> > > > for the non-coherent stuff and when this bug was fixed.
> >> >> > > >
> >> >> > > ATM, I dont think there are any users who are using the
> >> >> > > upstream kernel + OpenSBI (apart from me and Geert!).
> >> >> > > Currently the customers are using the BSP releases.
> >> >> >
> >> >> > That doesn't really answer whether or not you (and your
> >> >> > customers) are using an affected version of the vendor OpenSBI?
> >> >> > The affected range for OpenSBI itself is [v0.8 to v1.3).
> >> >>
> >> >> Did you perhaps miss this mail Prabhakar?
> >> >>
> >> > Oops sorry for that.
> >> >
> >> > I can confirm the BSP version used by the customers is v1.0 [0].
> >> >
> >> > [0]
> >> > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fg
> >> > ith%2F&data=05%7C02%7Cprabhakar.mahadev-lad.rj%40bp.renesas.com%7C6
> >> > 3259d3bbda343ccde3e08dc00c46054%7C53d82571da1947e49cb4625a166a4a2a%
> >> > 7C0%7C0%7C638386090629864750%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> >> > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7
> >> > C&sdata=xebpQgqY9W03HSDdoL0Si2taJ2RkOgTiR8H6koSKNq8%3D&reserved=0
> >> > ub.com%2Frenesas-rz%2Frz_opensbi%2Fcommits%2Fwork%2FOpenSBI-PMA%2F&
> >> > dat
> >> > a=05%7C02%7Cprabhakar.mahadev-lad.rj%40bp.renesas.com%7C014cf4ddfd1
> >> > e48
> >> > 1ff5bc08dc00bd467f%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C638
> >> > 386
> >> > 060130410731%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> >> > luM
> >> > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=a0tcsXY4EQl
> >> > ODi
> >> > I34QygXS9QpJnVBqL8bNkxE8N5J2g%3D&reserved=0
> >>
> >> OK, so sounds like would end up with broken systems from this bug, then?
> >>
> >> IIRC we still have the Renesas systems as NONPORTABLE due to that DMA
> >> pool Kconfig conflict.  So if it's really only these Renesas systems
> >> that have the bug, maybe we can still remove hibernation from
> >> NONPORTABLE and just add in some sort of Kconfig to disable the
> >> Renesas+hibernation combinations that would break?
> >>
> >Well customers using BSP uses v1.0 for OpenSBI and kernel 5.10-cip, and people wanting to run
> upstream kernel will have to only use the upstream OpenSBI as the OpenSBI used in BSP is not
> compatible with upstream kernel(Linux doesn’t bootup).
> >
> >ATM I can say that its only me and Geert using upstream OpenBSI and upstream kernel.
> >
> >With that in mind would we still require that change?
> 
> 5.10 doesn't have hibernation support in it, although I'm not sure why anyone would really want to use
> a kernel that old with a RISC-V system.
> 
At Renesas we have the BSPs based on the CIP kernel. Currently the BSPs are based on 5.10-cip [0] (we plan to upgrade it to 6.1-cip).

> The upstream versions of opensbi that support the renesas stuff have the no-map fix, right?
> If that's the case, then nothing special config wise is likely required.
> 
Yes it does.

> I'm still wary of other systems though, we are acting as if it is only Microchip and Renesas are the
> "real" users.

[0] https://gitlab.com/cip-project/cip-kernel/linux-cip/-/tree/linux-5.10.y-cip?ref_type=heads

Cheers,
Prabhakar

Patch

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 5b4a1bf5f439..5360f3476278 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -252,6 +252,9 @@  enum sbi_pmu_ctr_type {
 #define SBI_ERR_ALREADY_STARTED -7
 #define SBI_ERR_ALREADY_STOPPED -8
 
+/* SBI implementation IDs */
+#define SBI_IMP_OPENSBI	1
+
 extern unsigned long sbi_spec_version;
 struct sbiret {
 	long error;
@@ -259,6 +262,8 @@  struct sbiret {
 };
 
 void sbi_init(void);
+void sbi_apply_reserved_mem_erratum(void *dtb_va);
+
 struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
 			unsigned long arg1, unsigned long arg2,
 			unsigned long arg3, unsigned long arg4,
diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index c672c8ba9a2a..aeb27263fa53 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -5,8 +5,10 @@ 
  * Copyright (c) 2020 Western Digital Corporation or its affiliates.
  */
 
+#include <linux/acpi.h>
 #include <linux/bits.h>
 #include <linux/init.h>
+#include <linux/libfdt.h>
 #include <linux/pm.h>
 #include <linux/reboot.h>
 #include <asm/sbi.h>
@@ -583,6 +585,40 @@  long sbi_get_mimpid(void)
 }
 EXPORT_SYMBOL_GPL(sbi_get_mimpid);
 
+static long sbi_firmware_id;
+static long sbi_firmware_version;
+
+/*
+ * For devicetrees patched by OpenSBI a "mmode_resv" node is added to cover
+ * the region OpenSBI has protected by means of a PMP. Some versions of OpenSBI,
+ * [v0.8 to v1.3), omitted the "no-map" property, but this trips up hibernation
+ * among other things.
+ */
+void __init sbi_apply_reserved_mem_erratum(void *dtb_pa)
+{
+	int child, reserved_mem;
+
+	if (sbi_firmware_id != SBI_IMP_OPENSBI)
+		return;
+
+	if (!acpi_disabled)
+		return;
+
+	if (sbi_firmware_version >= 0x10003 || sbi_firmware_version < 0x8)
+		return;
+
+	reserved_mem = fdt_path_offset((void *)dtb_pa, "/reserved-memory");
+	if (reserved_mem < 0)
+		return;
+
+	fdt_for_each_subnode(child, (void *)dtb_pa, reserved_mem) {
+		const char *name = fdt_get_name((void *)dtb_pa, child, NULL);
+
+		if (!strncmp(name, "mmode_resv", 10))
+			fdt_setprop((void *)dtb_pa, child, "no-map", NULL, 0);
+	}
+};
+
 void __init sbi_init(void)
 {
 	int ret;
@@ -596,8 +632,12 @@  void __init sbi_init(void)
 		sbi_major_version(), sbi_minor_version());
 
 	if (!sbi_spec_is_0_1()) {
+		sbi_firmware_id = sbi_get_firmware_id();
+		sbi_firmware_version = sbi_get_firmware_version();
+
 		pr_info("SBI implementation ID=0x%lx Version=0x%lx\n",
-			sbi_get_firmware_id(), sbi_get_firmware_version());
+			sbi_firmware_id, sbi_firmware_version);
+
 		if (sbi_probe_extension(SBI_EXT_TIME)) {
 			__sbi_set_timer = __sbi_set_timer_v02;
 			pr_info("SBI TIME extension detected\n");
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 70fb31960b63..cb16bfdeacdb 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -29,6 +29,7 @@ 
 #include <asm/tlbflush.h>
 #include <asm/sections.h>
 #include <asm/soc.h>
+#include <asm/sbi.h>
 #include <asm/io.h>
 #include <asm/ptdump.h>
 #include <asm/numa.h>
@@ -253,6 +254,8 @@  static void __init setup_bootmem(void)
 	 * in the device tree, otherwise the allocation could end up in a
 	 * reserved region.
 	 */
+
+	sbi_apply_reserved_mem_erratum(dtb_early_va);
 	early_init_fdt_scan_reserved_mem();
 
 	/*