Re: [RFC patch 4/4] riscv: sifive: apply errata "cip-453" patch

Re: [RFC patch 4/4] riscv: sifive: apply errata "cip-453" patch

[Ruinland ChuanTzu Tsai]

Hi Vincent,

Thanks for introducing the alternative mechanism to RISC-V, with which
vendors could provide fixes for each erratum in a more elegant way.

Somehow, I'm a bit sketchy about these parts of your proposal :

>     /* Exception vector table */
>  ENTRY(excp_vect_table)
>     RISCV_PTR do_trap_insn_misaligned
> -   RISCV_PTR do_trap_insn_fault
> +   ALTERNATIVE(__stringify(RISCV_PTR do_trap_insn_fault),
> +           __stringify(RISCV_PTR do_trap_insn_fault_trampoline),
> +           SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
>     RISCV_PTR do_trap_insn_illegal
>     RISCV_PTR do_trap_break
>     RISCV_PTR do_trap_load_misaligned
> @@ -461,7 +466,10 @@ ENTRY(excp_vect_table)
>     RISCV_PTR do_trap_ecall_s
>     RISCV_PTR do_trap_unknown
>     RISCV_PTR do_trap_ecall_m
> -   RISCV_PTR do_page_fault   /* instruction page fault */
> +   /* instruciton page fault */
> +   ALTERNATIVE(__stringify(RISCV_PTR do_page_fault),
> +           __stringify(RISCV_PTR do_page_fault_trampoline),
> +           SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
>     RISCV_PTR do_page_fault   /* load page fault */

As far as I can tell, `ALTERNATIVE(...)` seems a bit like a mixture of
ARM's version of ALTERNATIVE and `alternative_insn`. However, ARM's
ALTERNATIVE takes a vardatic macro and yours here doesn't, which makes
me wonder if another vendor needs to patch the same location as yours,
will they be able to multiplex the same probe ?


Secondly, I think it's a bit intrusive to patch directly on exception
vector table here.

I'm not sure about whether it's possible to introduce your alternative
probe inside do_trap_insn_fault() & do_page_fault(), do the inspection
the reason of trap (e.g. instruction/load/store page fault) there and
then, perform the software workaround.

If that's not feasible, maybe we shall make a new macro with a name
like "RISCV_TRAP_ENTRY" which encompass the alternative probes ?

Last but not least, is it possible that in the near future,
`alternative_if` macro family from ARM could be ported to RISC-V ?

Best regards,
Ruinland

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC patch 4/4] riscv: sifive: apply errata "cip-453" patch

[Vincent Chen]

On Thu, Mar 11, 2021 at 5:58 AM Ruinland ChuanTzu Tsai
<ruinland@andestech.com> wrote:
>
> Hi Vincent,
>
> Thanks for introducing the alternative mechanism to RISC-V, with which
> vendors could provide fixes for each erratum in a more elegant way.
>
> Somehow, I'm a bit sketchy about these parts of your proposal :
>
> >     /* Exception vector table */
> >  ENTRY(excp_vect_table)
> >     RISCV_PTR do_trap_insn_misaligned
> > -   RISCV_PTR do_trap_insn_fault
> > +   ALTERNATIVE(__stringify(RISCV_PTR do_trap_insn_fault),
> > +           __stringify(RISCV_PTR do_trap_insn_fault_trampoline),
> > +           SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
> >     RISCV_PTR do_trap_insn_illegal
> >     RISCV_PTR do_trap_break
> >     RISCV_PTR do_trap_load_misaligned
> > @@ -461,7 +466,10 @@ ENTRY(excp_vect_table)
> >     RISCV_PTR do_trap_ecall_s
> >     RISCV_PTR do_trap_unknown
> >     RISCV_PTR do_trap_ecall_m
> > -   RISCV_PTR do_page_fault   /* instruction page fault */
> > +   /* instruciton page fault */
> > +   ALTERNATIVE(__stringify(RISCV_PTR do_page_fault),
> > +           __stringify(RISCV_PTR do_page_fault_trampoline),
> > +           SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
> >     RISCV_PTR do_page_fault   /* load page fault */
>
> As far as I can tell, `ALTERNATIVE(...)` seems a bit like a mixture of
> ARM's version of ALTERNATIVE and `alternative_insn`. However, ARM's
> ALTERNATIVE takes a vardatic macro and yours here doesn't, which makes
> me wonder if another vendor needs to patch the same location as yours,
> will they be able to multiplex the same probe ?
>
This is a good question. For this case, I think the current
ALTERNATIVE(...) can not be used.
In my thoughts, we need to define a new MACRO like

#define ALTERNATIVE2("old inst", "vendor-1's new inst","vendor-1
id","errata_id","CONFIG_ERRATA_vendor-1" , "vendor-2's new
inst","vendor-2 id","errata_id","CONFIG_ERRATA_vendor-2" ) \
       "886 :\n"                                                       \
       oldinsn "\n"                                                    \
       ALT("vendor-1's new inst", "vendor-1
id","errata_id","CONFIG_ERRATA_vendor-1") \
       ALT("vendor-2's new inst", "vendor-2
id","errata_id","CONFIG_ERRATA_vendor-2")

where ALT is defined as below:
+#define ALT (altinsn, vendor_id, errata_id, enable) \
+       ".if " __stringify(enable) " == 1\n"                            \
+       "887 :\n"                                                       \
+       ".pushsection .alternative, \"a\"\n"                            \
+       ALT_ENTRY("886b", "888f", __stringify(vendor_id),
__stringify(errata_id), "889f - 888f") \
+       ".popsection\n"                                                 \
+       ".subsection 1\n"                                               \
+       "888 :\n"                                                       \
+       altinsn "\n"                                                    \
+       "889 :\n"                                                       \
+       ".previous\n"                                                   \
+       ".org   . - (887b - 886b) + (889b - 888b)\n"                    \
+       ".org   . - (889b - 888b) + (887b - 886b)\n"                    \,
+       ".endif\n"
+
By using ALTERNATIVE2, vendor-1 and vendor-2 (more precisely, CPU-1
and CPU-2) can replace the same instruction with different errata
patches. I will create a sample code in my next version patch for
vendor reference.

>
> Secondly, I think it's a bit intrusive to patch directly on exception
> vector table here.
>
> I'm not sure about whether it's possible to introduce your alternative
> probe inside do_trap_insn_fault() & do_page_fault(), do the inspection
> the reason of trap (e.g. instruction/load/store page fault) there and
> then, perform the software workaround.
>
> If that's not feasible, maybe we shall make a new macro with a name
> like "RISCV_TRAP_ENTRY" which encompass the alternative probes ?
>
The do_page_fault() will deal with all page fault exceptions such as
load/store page fault. However, our errata is only for the instruction
page fault. Therefore, not only we need an additional if condition to
distinguish the incoming exception type, but also all page fault will
suffer the performance impact from the if conditions. Therefore, I
decided to replace the exception handler directly.

I think your idea is good. I will follow your suggestions to create a
new macro for them to improve the readability.


> Last but not least, is it possible that in the near future,
> `alternative_if` macro family from ARM could be ported to RISC-V ?
>
No, it is not in my current plan.


Thank you for your feedback.


> Best regards,
> Ruinland
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC patch 0/4] riscv: introduce alternative mechanism to apply errata patches

[Vincent Chen]

With the emergence of more and more RISC-V CPUs, the request for how to
upstream the vendor errata patch may gradually appear. In order to resolve
this issue, this patch introduces the alternative mechanism from ARM64 and
x86 to enable the kernel to patch code at runtime according to the
manufacturer information of the running CPU. The main purpose of this patch
set is to propose a framework to apply vendor's errata solutions. Based on
this framework, it can be ensured that the errata only applies to the
specified CPU cores. Other CPU cores do not be affected. Therefore, some
complicated scenarios are unsupported in this patch set, such as patching
code to the kernel module, doing relocation in patching code, and
heterogeneous CPU topology.

In the "alternative" scheme, Users could use the macro ALTERNATIVE to apply
an errata to the existing code flow. In the macro ALTERNATIVE, users need
to specify the manufacturer information (vendor id, arch id, and implement
id) for this errata. Therefore, kernel will know this errata is suitable
for which CPU core. During the booting procedure, kernel will select the
errata required by the CPU core and then patch it. It means that the kernel
only applies the errata to the specified CPU core. In this case, the
vendor's errata does not affect each other at runtime. The above patching
procedure only occurs during the booting phase, so we only take the
overhead of the "alternative" mechanism once.

This "alternative" mechanism is enabled by default to ensure that all
required errata will be applied. However, users can disable this feature by
the Kconfig "CONFIG_RISCV_ERRATA_ALTERNATIVE".

The last patch is to apply the SiFive CIP-453 errata by this "alternative"
scheme. Therefore, It can be regarded as an example. According to the
results of running this image on the QEMU virt platform, kernel does not
apply this errata at run-time because the CPU manufacturer information
does not match the specified SiFive CPU core. Therefore, this errata does
not affect any CPU core except for the specified SiFive cores.

Vincent Chen (4):
  riscv: Add 3 SBI wrapper functions to get cpu manufacturer information
  riscv: Get CPU manufacturer information
  riscv: Introduce alternative mechanism to apply errata solution
  riscv: sifive: apply errata "cip-453" patch

 arch/riscv/Kconfig                          |   1 +
 arch/riscv/Kconfig.erratas                  |  32 ++++++++
 arch/riscv/Kconfig.socs                     |   1 +
 arch/riscv/Makefile                         |   1 +
 arch/riscv/errata/Makefile                  |   2 +
 arch/riscv/errata/alternative.c             |  69 +++++++++++++++++
 arch/riscv/errata/sifive/Makefile           |   2 +
 arch/riscv/errata/sifive/errata.c           |  56 ++++++++++++++
 arch/riscv/errata/sifive/errata_cip_453.S   |  34 +++++++++
 arch/riscv/include/asm/alternative-macros.h | 110 ++++++++++++++++++++++++++++
 arch/riscv/include/asm/alternative.h        |  44 +++++++++++
 arch/riscv/include/asm/asm.h                |   1 +
 arch/riscv/include/asm/csr.h                |   3 +
 arch/riscv/include/asm/errata_list.h        |   9 +++
 arch/riscv/include/asm/hwcap.h              |   6 ++
 arch/riscv/include/asm/processor.h          |   2 +
 arch/riscv/include/asm/sbi.h                |   3 +
 arch/riscv/include/asm/sections.h           |   2 +
 arch/riscv/include/asm/soc.h                |   1 +
 arch/riscv/include/asm/vendorid_list.h      |   6 ++
 arch/riscv/kernel/cpufeature.c              |  17 +++++
 arch/riscv/kernel/entry.S                   |  12 ++-
 arch/riscv/kernel/sbi.c                     |  15 ++++
 arch/riscv/kernel/setup.c                   |   2 +
 arch/riscv/kernel/smpboot.c                 |   4 +
 arch/riscv/kernel/soc.c                     |   1 +
 arch/riscv/kernel/vmlinux.lds.S             |  14 ++++
 27 files changed, 448 insertions(+), 2 deletions(-)
 create mode 100644 arch/riscv/Kconfig.erratas
 create mode 100644 arch/riscv/errata/Makefile
 create mode 100644 arch/riscv/errata/alternative.c
 create mode 100644 arch/riscv/errata/sifive/Makefile
 create mode 100644 arch/riscv/errata/sifive/errata.c
 create mode 100644 arch/riscv/errata/sifive/errata_cip_453.S
 create mode 100644 arch/riscv/include/asm/alternative-macros.h
 create mode 100644 arch/riscv/include/asm/alternative.h
 create mode 100644 arch/riscv/include/asm/errata_list.h
 create mode 100644 arch/riscv/include/asm/vendorid_list.h

-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC patch 4/4] riscv: sifive: apply errata "cip-453" patch

[Vincent Chen]

Add sign extension to the $badaddr before addressing the instruction page
fault and instruction access fault to workaround the issue "cip-453".

To avoid affecting the existing code sequence, this patch will creates two
trampolines to add sign extension to the $badaddr. By the "alternative"
mechanism, these two trampolines will replace the original exception
handler of instruction page fault and instruction access fault in the
excp_vect_table. In this case, only the specific SiFive CPU core jumps to
the do_page_fault and do_trap_insn_fault through these two trampolines.
Other CPUs are not affected.

Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/Kconfig.erratas                | 20 +++++++++++
 arch/riscv/Kconfig.socs                   |  1 +
 arch/riscv/errata/Makefile                |  1 +
 arch/riscv/errata/sifive/Makefile         |  2 ++
 arch/riscv/errata/sifive/errata.c         | 56 +++++++++++++++++++++++++++++++
 arch/riscv/errata/sifive/errata_cip_453.S | 34 +++++++++++++++++++
 arch/riscv/include/asm/errata_list.h      |  3 +-
 arch/riscv/kernel/entry.S                 | 12 +++++--
 8 files changed, 126 insertions(+), 3 deletions(-)
 create mode 100644 arch/riscv/errata/sifive/Makefile
 create mode 100644 arch/riscv/errata/sifive/errata.c
 create mode 100644 arch/riscv/errata/sifive/errata_cip_453.S

diff --git a/arch/riscv/Kconfig.erratas b/arch/riscv/Kconfig.erratas
index 4d0bafc536df..907fa6b13ee2 100644
--- a/arch/riscv/Kconfig.erratas
+++ b/arch/riscv/Kconfig.erratas
@@ -9,4 +9,24 @@ config RISCV_ERRATA_ALTERNATIVE
 	  code patching is performed once in the boot stages. It means
 	  that the overhead from this mechanism is just taken once.
 
+config ERRATA_SIFIVE
+	bool "SiFive errata"
+	help
+	  All SiFive errata Kconfig depend on this Kconfig. Please say "Y"
+	  here if your platform uses SiFive CPU cores.
+
+	  Otherwise, please say "N" here to avoid unnecessary overhead.
+
+config ERRATA_SIFIVE_CIP_453
+	bool "Apply SiFive errata CIP-453"
+	depends on ERRATA_SIFIVE
+	depends on RISCV_ERRATA_ALTERNATIVE
+	default y
+	help
+	  This will apply the SiFive CIP-453 errata to add sign extension
+	  to the $badaddr when exception type is instruction page fault
+	  and instruction access fault.
+
+	  If you don't know what to do here, say "Y".
+
 endmenu
diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index 7efcece8896c..b9eda857fc87 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -7,6 +7,7 @@ config SOC_SIFIVE
 	select CLK_SIFIVE
 	select CLK_SIFIVE_PRCI
 	select SIFIVE_PLIC
+	select ERRATA_SIFIVE
 	help
 	  This enables support for SiFive SoC platform hardware.
 
diff --git a/arch/riscv/errata/Makefile b/arch/riscv/errata/Makefile
index 43e6d5424367..b8f8740a3e44 100644
--- a/arch/riscv/errata/Makefile
+++ b/arch/riscv/errata/Makefile
@@ -1 +1,2 @@
 obj-y	+= alternative.o
+obj-$(CONFIG_ERRATA_SIFIVE) += sifive/
diff --git a/arch/riscv/errata/sifive/Makefile b/arch/riscv/errata/sifive/Makefile
new file mode 100644
index 000000000000..b7f4cd7ee185
--- /dev/null
+++ b/arch/riscv/errata/sifive/Makefile
@@ -0,0 +1,2 @@
+obj-y += altern_ops.o
+obj-y += errata_cip_453.o
diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c
new file mode 100644
index 000000000000..0b0a9af42a55
--- /dev/null
+++ b/arch/riscv/errata/sifive/errata.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2021 Sifive.
+ */
+
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/bug.h>
+#include <asm/alternative.h>
+#include <asm/vendorid_list.h>
+#include <asm/errata_list.h>
+
+#define MAX_ERRATA_IMPID 5
+struct errata_info_t {
+	char name[ERRATA_STRING_LENGTH_MAX];
+	unsigned long arch_id;
+	unsigned long imp_id[MAX_ERRATA_IMPID];
+} errata_info;
+
+struct errata_info_t sifive_errata_list[ERRATA_NUMBER] = {
+	{
+		.name = "cip-453",
+		.arch_id = 0x8000000000000007,
+		.imp_id = {
+			0x20181004, 0x00200504
+		},
+	},
+};
+
+static inline bool __init cpu_info_cmp(struct errata_info_t *errata, struct alt_entry *alt)
+{
+	int i;
+
+	if (errata->arch_id != cpu_mfr_info.arch_id)
+		return false;
+
+	for (i = 0; i < MAX_ERRATA_IMPID && errata->imp_id[i]; i++)
+		if (errata->imp_id[i] == cpu_mfr_info.imp_id)
+			return true;
+
+	return false;
+}
+
+static bool __init sifive_errata_check(struct alt_entry *alt)
+{
+	if (cpu_mfr_info.vendor_id != alt->vendor_id)
+		return false;
+
+	if (likely(alt->errata_id < ERRATA_NUMBER))
+		return cpu_info_cmp(&sifive_errata_list[alt->errata_id], alt);
+
+	WARN_ON(1);
+	return false;
+}
+
+REGISTER_ERRATA_CHECKFUNC(sifive_errata_check, SIFIVE_VENDOR_ID);
diff --git a/arch/riscv/errata/sifive/errata_cip_453.S b/arch/riscv/errata/sifive/errata_cip_453.S
new file mode 100644
index 000000000000..34d0fe26172e
--- /dev/null
+++ b/arch/riscv/errata/sifive/errata_cip_453.S
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 SiFive
+ */
+
+#include <linux/linkage.h>
+#include <asm/asm.h>
+#include <asm/asm-offsets.h>
+#include <asm/alternative.h>
+
+.macro ADD_SIGN_EXT pt_reg badaddr tmp_reg
+	REG_L \badaddr, PT_BADADDR(\pt_reg)
+	li \tmp_reg,1
+	slli \tmp_reg,\tmp_reg,0x26
+	and \tmp_reg,\tmp_reg,\badaddr
+	beqz \tmp_reg, 1f
+	li \tmp_reg,-1
+	slli \tmp_reg,\tmp_reg,0x27
+	or \badaddr,\tmp_reg,\badaddr
+	REG_S \badaddr, PT_BADADDR(\pt_reg)
+1:
+.endm
+
+ENTRY(do_page_fault_trampoline)
+	ADD_SIGN_EXT a0, t0, t1
+	la t0, do_page_fault
+	jr t0
+END(do_page_fault_trampoline)
+
+ENTRY(do_trap_insn_fault_trampoline)
+	ADD_SIGN_EXT a0, t0, t1
+	la t0, do_trap_insn_fault
+	jr t0
+END(do_trap_insn_fault_trampoline)
diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
index 5e5a1fcd90ba..d3752c8eff1f 100644
--- a/arch/riscv/include/asm/errata_list.h
+++ b/arch/riscv/include/asm/errata_list.h
@@ -4,5 +4,6 @@
  */
 
 #ifdef CONFIG_SOC_SIFIVE
-#define	ERRATA_NUMBER 0
+#define	ERRATA_CIP_453 0
+#define	ERRATA_NUMBER 1
 #endif
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 744f3209c48d..821e86ee67e4 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -12,6 +12,9 @@
 #include <asm/unistd.h>
 #include <asm/thread_info.h>
 #include <asm/asm-offsets.h>
+#include <asm/alternative.h>
+#include <asm/vendorid_list.h>
+#include <asm/errata_list.h>
 
 #if !IS_ENABLED(CONFIG_PREEMPTION)
 .set resume_kernel, restore_all
@@ -450,7 +453,9 @@ ENDPROC(__switch_to)
 	/* Exception vector table */
 ENTRY(excp_vect_table)
 	RISCV_PTR do_trap_insn_misaligned
-	RISCV_PTR do_trap_insn_fault
+	ALTERNATIVE(__stringify(RISCV_PTR do_trap_insn_fault),
+		    __stringify(RISCV_PTR do_trap_insn_fault_trampoline),
+		    SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
 	RISCV_PTR do_trap_insn_illegal
 	RISCV_PTR do_trap_break
 	RISCV_PTR do_trap_load_misaligned
@@ -461,7 +466,10 @@ ENTRY(excp_vect_table)
 	RISCV_PTR do_trap_ecall_s
 	RISCV_PTR do_trap_unknown
 	RISCV_PTR do_trap_ecall_m
-	RISCV_PTR do_page_fault   /* instruction page fault */
+	/* instruciton page fault */
+	ALTERNATIVE(__stringify(RISCV_PTR do_page_fault),
+		    __stringify(RISCV_PTR do_page_fault_trampoline),
+		    SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
 	RISCV_PTR do_page_fault   /* load page fault */
 	RISCV_PTR do_trap_unknown
 	RISCV_PTR do_page_fault   /* store page fault */
-- 
2.7.4


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC patch 4/4] riscv: sifive: apply errata "cip-453" patch

[Palmer Dabbelt]

On Sun, 07 Mar 2021 19:58:17 PST (-0800), vincent.chen@sifive.com wrote:
> Add sign extension to the $badaddr before addressing the instruction page
> fault and instruction access fault to workaround the issue "cip-453".

The documentation I see quite explicitly says that this bug doesn't manifest in 
Linux.  IDK if I'm just reading an old one, but if not that should be fixed as 
presumably it does actually manifest?

> To avoid affecting the existing code sequence, this patch will creates two
> trampolines to add sign extension to the $badaddr. By the "alternative"
> mechanism, these two trampolines will replace the original exception
> handler of instruction page fault and instruction access fault in the
> excp_vect_table. In this case, only the specific SiFive CPU core jumps to
> the do_page_fault and do_trap_insn_fault through these two trampolines.
> Other CPUs are not affected.
>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  arch/riscv/Kconfig.erratas                | 20 +++++++++++
>  arch/riscv/Kconfig.socs                   |  1 +
>  arch/riscv/errata/Makefile                |  1 +
>  arch/riscv/errata/sifive/Makefile         |  2 ++
>  arch/riscv/errata/sifive/errata.c         | 56 +++++++++++++++++++++++++++++++
>  arch/riscv/errata/sifive/errata_cip_453.S | 34 +++++++++++++++++++
>  arch/riscv/include/asm/errata_list.h      |  3 +-
>  arch/riscv/kernel/entry.S                 | 12 +++++--
>  8 files changed, 126 insertions(+), 3 deletions(-)
>  create mode 100644 arch/riscv/errata/sifive/Makefile
>  create mode 100644 arch/riscv/errata/sifive/errata.c
>  create mode 100644 arch/riscv/errata/sifive/errata_cip_453.S
>
> diff --git a/arch/riscv/Kconfig.erratas b/arch/riscv/Kconfig.erratas
> index 4d0bafc536df..907fa6b13ee2 100644
> --- a/arch/riscv/Kconfig.erratas
> +++ b/arch/riscv/Kconfig.erratas
> @@ -9,4 +9,24 @@ config RISCV_ERRATA_ALTERNATIVE
>  	  code patching is performed once in the boot stages. It means
>  	  that the overhead from this mechanism is just taken once.
>
> +config ERRATA_SIFIVE
> +	bool "SiFive errata"
> +	help
> +	  All SiFive errata Kconfig depend on this Kconfig. Please say "Y"
> +	  here if your platform uses SiFive CPU cores.
> +
> +	  Otherwise, please say "N" here to avoid unnecessary overhead.
> +
> +config ERRATA_SIFIVE_CIP_453
> +	bool "Apply SiFive errata CIP-453"
> +	depends on ERRATA_SIFIVE
> +	depends on RISCV_ERRATA_ALTERNATIVE
> +	default y
> +	help
> +	  This will apply the SiFive CIP-453 errata to add sign extension
> +	  to the $badaddr when exception type is instruction page fault
> +	  and instruction access fault.
> +
> +	  If you don't know what to do here, say "Y".
> +
>  endmenu
> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> index 7efcece8896c..b9eda857fc87 100644
> --- a/arch/riscv/Kconfig.socs
> +++ b/arch/riscv/Kconfig.socs
> @@ -7,6 +7,7 @@ config SOC_SIFIVE
>  	select CLK_SIFIVE
>  	select CLK_SIFIVE_PRCI
>  	select SIFIVE_PLIC
> +	select ERRATA_SIFIVE
>  	help
>  	  This enables support for SiFive SoC platform hardware.
>
> diff --git a/arch/riscv/errata/Makefile b/arch/riscv/errata/Makefile
> index 43e6d5424367..b8f8740a3e44 100644
> --- a/arch/riscv/errata/Makefile
> +++ b/arch/riscv/errata/Makefile
> @@ -1 +1,2 @@
>  obj-y	+= alternative.o
> +obj-$(CONFIG_ERRATA_SIFIVE) += sifive/
> diff --git a/arch/riscv/errata/sifive/Makefile b/arch/riscv/errata/sifive/Makefile
> new file mode 100644
> index 000000000000..b7f4cd7ee185
> --- /dev/null
> +++ b/arch/riscv/errata/sifive/Makefile
> @@ -0,0 +1,2 @@
> +obj-y += altern_ops.o
> +obj-y += errata_cip_453.o
> diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c
> new file mode 100644
> index 000000000000..0b0a9af42a55
> --- /dev/null
> +++ b/arch/riscv/errata/sifive/errata.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Sifive.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +#include <linux/bug.h>
> +#include <asm/alternative.h>
> +#include <asm/vendorid_list.h>
> +#include <asm/errata_list.h>
> +
> +#define MAX_ERRATA_IMPID 5

Why 5?  I only see 2.

> +struct errata_info_t {
> +	char name[ERRATA_STRING_LENGTH_MAX];
> +	unsigned long arch_id;
> +	unsigned long imp_id[MAX_ERRATA_IMPID];
> +} errata_info;
> +
> +struct errata_info_t sifive_errata_list[ERRATA_NUMBER] = {
> +	{
> +		.name = "cip-453",
> +		.arch_id = 0x8000000000000007,
> +		.imp_id = {
> +			0x20181004, 0x00200504

Is there any way to figure these out from the documentation?

> +		},
> +	},
> +};
> +
> +static inline bool __init cpu_info_cmp(struct errata_info_t *errata, struct alt_entry *alt)
> +{
> +	int i;
> +
> +	if (errata->arch_id != cpu_mfr_info.arch_id)
> +		return false;
> +
> +	for (i = 0; i < MAX_ERRATA_IMPID && errata->imp_id[i]; i++)
> +		if (errata->imp_id[i] == cpu_mfr_info.imp_id)
> +			return true;
> +
> +	return false;
> +}
> +
> +static bool __init sifive_errata_check(struct alt_entry *alt)
> +{
> +	if (cpu_mfr_info.vendor_id != alt->vendor_id)
> +		return false;
> +
> +	if (likely(alt->errata_id < ERRATA_NUMBER))
> +		return cpu_info_cmp(&sifive_errata_list[alt->errata_id], alt);
> +
> +	WARN_ON(1);
> +	return false;
> +}
> +
> +REGISTER_ERRATA_CHECKFUNC(sifive_errata_check, SIFIVE_VENDOR_ID);
> diff --git a/arch/riscv/errata/sifive/errata_cip_453.S b/arch/riscv/errata/sifive/errata_cip_453.S
> new file mode 100644
> index 000000000000..34d0fe26172e
> --- /dev/null
> +++ b/arch/riscv/errata/sifive/errata_cip_453.S
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 SiFive
> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/asm.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/alternative.h>
> +
> +.macro ADD_SIGN_EXT pt_reg badaddr tmp_reg
> +	REG_L \badaddr, PT_BADADDR(\pt_reg)
> +	li \tmp_reg,1
> +	slli \tmp_reg,\tmp_reg,0x26
> +	and \tmp_reg,\tmp_reg,\badaddr
> +	beqz \tmp_reg, 1f
> +	li \tmp_reg,-1
> +	slli \tmp_reg,\tmp_reg,0x27
> +	or \badaddr,\tmp_reg,\badaddr
> +	REG_S \badaddr, PT_BADADDR(\pt_reg)
> +1:
> +.endm
> +
> +ENTRY(do_page_fault_trampoline)
> +	ADD_SIGN_EXT a0, t0, t1
> +	la t0, do_page_fault
> +	jr t0
> +END(do_page_fault_trampoline)
> +
> +ENTRY(do_trap_insn_fault_trampoline)
> +	ADD_SIGN_EXT a0, t0, t1
> +	la t0, do_trap_insn_fault
> +	jr t0
> +END(do_trap_insn_fault_trampoline)

These should get names that are more specific to the errata in question, as 
they're global symbols.

> diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> index 5e5a1fcd90ba..d3752c8eff1f 100644
> --- a/arch/riscv/include/asm/errata_list.h
> +++ b/arch/riscv/include/asm/errata_list.h
> @@ -4,5 +4,6 @@
>   */
>
>  #ifdef CONFIG_SOC_SIFIVE
> -#define	ERRATA_NUMBER 0
> +#define	ERRATA_CIP_453 0
> +#define	ERRATA_NUMBER 1
>  #endif
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 744f3209c48d..821e86ee67e4 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -12,6 +12,9 @@
>  #include <asm/unistd.h>
>  #include <asm/thread_info.h>
>  #include <asm/asm-offsets.h>
> +#include <asm/alternative.h>
> +#include <asm/vendorid_list.h>
> +#include <asm/errata_list.h>
>
>  #if !IS_ENABLED(CONFIG_PREEMPTION)
>  .set resume_kernel, restore_all
> @@ -450,7 +453,9 @@ ENDPROC(__switch_to)
>  	/* Exception vector table */
>  ENTRY(excp_vect_table)
>  	RISCV_PTR do_trap_insn_misaligned
> -	RISCV_PTR do_trap_insn_fault
> +	ALTERNATIVE(__stringify(RISCV_PTR do_trap_insn_fault),
> +		    __stringify(RISCV_PTR do_trap_insn_fault_trampoline),
> +		    SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)

You've only got the instruction patching function in the errata framework, but 
this is data.  It doesn't really matter, as we fence before the other CPUs come 
online, but a comment in there that both are OK would be good.

>  	RISCV_PTR do_trap_insn_illegal
>  	RISCV_PTR do_trap_break
>  	RISCV_PTR do_trap_load_misaligned
> @@ -461,7 +466,10 @@ ENTRY(excp_vect_table)
>  	RISCV_PTR do_trap_ecall_s
>  	RISCV_PTR do_trap_unknown
>  	RISCV_PTR do_trap_ecall_m
> -	RISCV_PTR do_page_fault   /* instruction page fault */
> +	/* instruciton page fault */
> +	ALTERNATIVE(__stringify(RISCV_PTR do_page_fault),
> +		    __stringify(RISCV_PTR do_page_fault_trampoline),
> +		    SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
>  	RISCV_PTR do_page_fault   /* load page fault */
>  	RISCV_PTR do_trap_unknown
>  	RISCV_PTR do_page_fault   /* store page fault */

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC patch 4/4] riscv: sifive: apply errata "cip-453" patch

[Vincent Chen]

On Wed, Mar 10, 2021 at 12:39 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Sun, 07 Mar 2021 19:58:17 PST (-0800), vincent.chen@sifive.com wrote:
> > Add sign extension to the $badaddr before addressing the instruction page
> > fault and instruction access fault to workaround the issue "cip-453".
>
> The documentation I see quite explicitly says that this bug doesn't manifest in
> Linux.  IDK if I'm just reading an old one, but if not that should be fixed as
> presumably it does actually manifest?
>
> > To avoid affecting the existing code sequence, this patch will creates two
> > trampolines to add sign extension to the $badaddr. By the "alternative"
> > mechanism, these two trampolines will replace the original exception
> > handler of instruction page fault and instruction access fault in the
> > excp_vect_table. In this case, only the specific SiFive CPU core jumps to
> > the do_page_fault and do_trap_insn_fault through these two trampolines.
> > Other CPUs are not affected.
> >
> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > ---
> >  arch/riscv/Kconfig.erratas                | 20 +++++++++++
> >  arch/riscv/Kconfig.socs                   |  1 +
> >  arch/riscv/errata/Makefile                |  1 +
> >  arch/riscv/errata/sifive/Makefile         |  2 ++
> >  arch/riscv/errata/sifive/errata.c         | 56 +++++++++++++++++++++++++++++++
> >  arch/riscv/errata/sifive/errata_cip_453.S | 34 +++++++++++++++++++
> >  arch/riscv/include/asm/errata_list.h      |  3 +-
> >  arch/riscv/kernel/entry.S                 | 12 +++++--
> >  8 files changed, 126 insertions(+), 3 deletions(-)
> >  create mode 100644 arch/riscv/errata/sifive/Makefile
> >  create mode 100644 arch/riscv/errata/sifive/errata.c
> >  create mode 100644 arch/riscv/errata/sifive/errata_cip_453.S
> >
> > diff --git a/arch/riscv/Kconfig.erratas b/arch/riscv/Kconfig.erratas
> > index 4d0bafc536df..907fa6b13ee2 100644
> > --- a/arch/riscv/Kconfig.erratas
> > +++ b/arch/riscv/Kconfig.erratas
> > @@ -9,4 +9,24 @@ config RISCV_ERRATA_ALTERNATIVE
> >         code patching is performed once in the boot stages. It means
> >         that the overhead from this mechanism is just taken once.
> >
> > +config ERRATA_SIFIVE
> > +     bool "SiFive errata"
> > +     help
> > +       All SiFive errata Kconfig depend on this Kconfig. Please say "Y"
> > +       here if your platform uses SiFive CPU cores.
> > +
> > +       Otherwise, please say "N" here to avoid unnecessary overhead.
> > +
> > +config ERRATA_SIFIVE_CIP_453
> > +     bool "Apply SiFive errata CIP-453"
> > +     depends on ERRATA_SIFIVE
> > +     depends on RISCV_ERRATA_ALTERNATIVE
> > +     default y
> > +     help
> > +       This will apply the SiFive CIP-453 errata to add sign extension
> > +       to the $badaddr when exception type is instruction page fault
> > +       and instruction access fault.
> > +
> > +       If you don't know what to do here, say "Y".
> > +
> >  endmenu
> > diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> > index 7efcece8896c..b9eda857fc87 100644
> > --- a/arch/riscv/Kconfig.socs
> > +++ b/arch/riscv/Kconfig.socs
> > @@ -7,6 +7,7 @@ config SOC_SIFIVE
> >       select CLK_SIFIVE
> >       select CLK_SIFIVE_PRCI
> >       select SIFIVE_PLIC
> > +     select ERRATA_SIFIVE
> >       help
> >         This enables support for SiFive SoC platform hardware.
> >
> > diff --git a/arch/riscv/errata/Makefile b/arch/riscv/errata/Makefile
> > index 43e6d5424367..b8f8740a3e44 100644
> > --- a/arch/riscv/errata/Makefile
> > +++ b/arch/riscv/errata/Makefile
> > @@ -1 +1,2 @@
> >  obj-y        += alternative.o
> > +obj-$(CONFIG_ERRATA_SIFIVE) += sifive/
> > diff --git a/arch/riscv/errata/sifive/Makefile b/arch/riscv/errata/sifive/Makefile
> > new file mode 100644
> > index 000000000000..b7f4cd7ee185
> > --- /dev/null
> > +++ b/arch/riscv/errata/sifive/Makefile
> > @@ -0,0 +1,2 @@
> > +obj-y += altern_ops.o
> > +obj-y += errata_cip_453.o
> > diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c
> > new file mode 100644
> > index 000000000000..0b0a9af42a55
> > --- /dev/null
> > +++ b/arch/riscv/errata/sifive/errata.c
> > @@ -0,0 +1,56 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2021 Sifive.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/bug.h>
> > +#include <asm/alternative.h>
> > +#include <asm/vendorid_list.h>
> > +#include <asm/errata_list.h>
> > +
> > +#define MAX_ERRATA_IMPID 5
>
> Why 5?  I only see 2.
>
This is my mistake. I forgot to modify it after I completed some
testing. I will modify it.

> > +struct errata_info_t {
> > +     char name[ERRATA_STRING_LENGTH_MAX];
> > +     unsigned long arch_id;
> > +     unsigned long imp_id[MAX_ERRATA_IMPID];
> > +} errata_info;
> > +
> > +struct errata_info_t sifive_errata_list[ERRATA_NUMBER] = {
> > +     {
> > +             .name = "cip-453",
> > +             .arch_id = 0x8000000000000007,
> > +             .imp_id = {
> > +                     0x20181004, 0x00200504
>
> Is there any way to figure these out from the documentation?
>
Sorry. Currently, there is no public document to record the information.
> > +             },
> > +     },
> > +};
> > +
> > +static inline bool __init cpu_info_cmp(struct errata_info_t *errata, struct alt_entry *alt)
> > +{
> > +     int i;
> > +
> > +     if (errata->arch_id != cpu_mfr_info.arch_id)
> > +             return false;
> > +
> > +     for (i = 0; i < MAX_ERRATA_IMPID && errata->imp_id[i]; i++)
> > +             if (errata->imp_id[i] == cpu_mfr_info.imp_id)
> > +                     return true;
> > +
> > +     return false;
> > +}
> > +
> > +static bool __init sifive_errata_check(struct alt_entry *alt)
> > +{
> > +     if (cpu_mfr_info.vendor_id != alt->vendor_id)
> > +             return false;
> > +
> > +     if (likely(alt->errata_id < ERRATA_NUMBER))
> > +             return cpu_info_cmp(&sifive_errata_list[alt->errata_id], alt);
> > +
> > +     WARN_ON(1);
> > +     return false;
> > +}
> > +
> > +REGISTER_ERRATA_CHECKFUNC(sifive_errata_check, SIFIVE_VENDOR_ID);
> > diff --git a/arch/riscv/errata/sifive/errata_cip_453.S b/arch/riscv/errata/sifive/errata_cip_453.S
> > new file mode 100644
> > index 000000000000..34d0fe26172e
> > --- /dev/null
> > +++ b/arch/riscv/errata/sifive/errata_cip_453.S
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2021 SiFive
> > + */
> > +
> > +#include <linux/linkage.h>
> > +#include <asm/asm.h>
> > +#include <asm/asm-offsets.h>
> > +#include <asm/alternative.h>
> > +
> > +.macro ADD_SIGN_EXT pt_reg badaddr tmp_reg
> > +     REG_L \badaddr, PT_BADADDR(\pt_reg)
> > +     li \tmp_reg,1
> > +     slli \tmp_reg,\tmp_reg,0x26
> > +     and \tmp_reg,\tmp_reg,\badaddr
> > +     beqz \tmp_reg, 1f
> > +     li \tmp_reg,-1
> > +     slli \tmp_reg,\tmp_reg,0x27
> > +     or \badaddr,\tmp_reg,\badaddr
> > +     REG_S \badaddr, PT_BADADDR(\pt_reg)
> > +1:
> > +.endm
> > +
> > +ENTRY(do_page_fault_trampoline)
> > +     ADD_SIGN_EXT a0, t0, t1
> > +     la t0, do_page_fault
> > +     jr t0
> > +END(do_page_fault_trampoline)
> > +
> > +ENTRY(do_trap_insn_fault_trampoline)
> > +     ADD_SIGN_EXT a0, t0, t1
> > +     la t0, do_trap_insn_fault
> > +     jr t0
> > +END(do_trap_insn_fault_trampoline)
>
> These should get names that are more specific to the errata in question, as
> they're global symbols.
>
OK
> > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> > index 5e5a1fcd90ba..d3752c8eff1f 100644
> > --- a/arch/riscv/include/asm/errata_list.h
> > +++ b/arch/riscv/include/asm/errata_list.h
> > @@ -4,5 +4,6 @@
> >   */
> >
> >  #ifdef CONFIG_SOC_SIFIVE
> > -#define      ERRATA_NUMBER 0
> > +#define      ERRATA_CIP_453 0
> > +#define      ERRATA_NUMBER 1
> >  #endif
> > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> > index 744f3209c48d..821e86ee67e4 100644
> > --- a/arch/riscv/kernel/entry.S
> > +++ b/arch/riscv/kernel/entry.S
> > @@ -12,6 +12,9 @@
> >  #include <asm/unistd.h>
> >  #include <asm/thread_info.h>
> >  #include <asm/asm-offsets.h>
> > +#include <asm/alternative.h>
> > +#include <asm/vendorid_list.h>
> > +#include <asm/errata_list.h>
> >
> >  #if !IS_ENABLED(CONFIG_PREEMPTION)
> >  .set resume_kernel, restore_all
> > @@ -450,7 +453,9 @@ ENDPROC(__switch_to)
> >       /* Exception vector table */
> >  ENTRY(excp_vect_table)
> >       RISCV_PTR do_trap_insn_misaligned
> > -     RISCV_PTR do_trap_insn_fault
> > +     ALTERNATIVE(__stringify(RISCV_PTR do_trap_insn_fault),
> > +                 __stringify(RISCV_PTR do_trap_insn_fault_trampoline),
> > +                 SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
>
> You've only got the instruction patching function in the errata framework, but
> this is data.  It doesn't really matter, as we fence before the other CPUs come
> online, but a comment in there that both are OK would be good.
>
OK. I got it. Thanks
> >       RISCV_PTR do_trap_insn_illegal
> >       RISCV_PTR do_trap_break
> >       RISCV_PTR do_trap_load_misaligned
> > @@ -461,7 +466,10 @@ ENTRY(excp_vect_table)
> >       RISCV_PTR do_trap_ecall_s
> >       RISCV_PTR do_trap_unknown
> >       RISCV_PTR do_trap_ecall_m
> > -     RISCV_PTR do_page_fault   /* instruction page fault */
> > +     /* instruciton page fault */
> > +     ALTERNATIVE(__stringify(RISCV_PTR do_page_fault),
> > +                 __stringify(RISCV_PTR do_page_fault_trampoline),
> > +                 SIFIVE_VENDOR_ID, ERRATA_CIP_453, CONFIG_ERRATA_SIFIVE_CIP_453)
> >       RISCV_PTR do_page_fault   /* load page fault */
> >       RISCV_PTR do_trap_unknown
> >       RISCV_PTR do_page_fault   /* store page fault */

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv