riscv: support EIC770X/JH7110 noncoherent devices with XPbmtUC

SoCs with pre-Svpbmt Sifive cores, e.g., Starfive JH7110 and ESWIN EIC770X both have non cache-coherent peripherals. On JH7110[1], video subsystem (GPU/VOUT/VPU/ISP) is routed to the sys port, making them not cache-coherent. On EIC770X, all peripherals are routed to the sys port, and none is cache-coherent. Instead of Svpbmt, these SoCs map system memory twice -- the conventional cached region (through front port), and the uncached alias (through sys port) at different base addresses. The uncached alias implicitly applies the uncacheable PMA. Drivers working with noncoherent devices can utilize the uncache alias to map DMA buffers, without doing explicit cache flushes.

This feature is not an ISA standard, and the cache/uncache base can be configured by the SoC vendor. To expose it properly, introduce a Sifive "errata", namely "XPbmtUC", to model the setup as a customized version of Svpbmt. It choses a single, artificial bit in PTE at runtime for cache/uncache control, effectively offsetting the PPN by power-of-2. On JH7110, it aligns perfectly with the HW: it maps the cached region at 0x40000000, and the uncached alias at 0x4_40000000. Chosing bit 32 (PPN bit 34) as the UC bit matches HW exactly.

Starfive JH7110 (Sifive U74 core) memory map:

          [0x0,   0x40000000) Low MMIO

[0x40000000, 0x2_40000000) Cached Mem
[0x4_40000000, 0x6_40000000) Uncached Mem (UC+)
[0x9_00000000, 0x9_d0000000) High MMIO

On EIC770X, the aliased UC region is put to a offset not power-of-2. There can also be 2 NUMA node (dual-die) with 2 separate memory regions and their UC alias counterparts are offsetted differently. We detect if the firmware has the capability to re-arrange the memory map, using G-stage pagetable, making the the offsets power-of-2 again.

            [0x0,     0x20000000) Core Internal
     [0x20000000,     0x40000000) Core Internal (Die 1)
     [0x40000000,     0x60000000) Low MMIO
     [0x60000000,     0x80000000) Low MMIO (Die 1)
     [0x80000000,  0x10_80000000) Cached Mem
  [0x20_00000000,  0x30_00000000) Cached Mem (Die 1)
  [0x80_00000000,  0xa0_00000000) High MMIO
  [0xa0_00000000,  0xc0_00000000) High MMIO (Die 1)
  [0xc0_00000000,  0xd0_00000000) Uncached Mem <----------.
  [0xe0_00000000,  0xf0_00000000) Uncached Mem (Die 1) <--+--.
with firmware/hypervisor re-mapping:                      |  |
------------------------------------                      |  |
 [0x100_80000000, 0x110_80000000) Mem UC+ ----------------'  |

[0x120_00000000, 0x130_00000000) Mem UC+ (Die 1) -----------'

The "XPbmtUC" alternative PTE format is the cleanest solution I can think of to solve the non-coherent device enablement w/o Svpbmt from kernel side. Drivers can do explicit cache flushes to workaround the problem, but a. it pushes the burden of cache flushes to driver code, and we don't want to complicate them if it's already written with the cache coherent assumption in mind. b. complex drivers like GPU could allow user-space to mmap DMA pages, but userspace can't flush caches due to the lack of Zicbom on these SoCs. I'm aware there's an ongoing series[2] that Samuel sent for physical memory aliases, which is essentially a superset of my patch. I don't mean to step ahead of him, but try to find a middle ground if the community still worries about his change touching too many areas. My change is very minimal and local. It's fairly easy to remove, too.

----------------------------------------
Notes about PoC firmware implementation on EIC7700X[3]:

The OpenSBI is augmented to provide a very thin layer hypervisor, where it runs the entire host OS in VS-mode, and provide the aforementioned remapping. I remap UC+ memory to 2^40+ to make the 2-stage translation efficient, where I can utilize Sv39x4 G-stage scheme to map the entire physical address space at bottom-half, and the uncache counterparts to system memory at top-half. I also make use of the largest page in Sv39 -- 1GB page, to map everything, keeping the G-stage page-table minimal, only 16KB in size, while also minimizing TLB misses. A very slight, unavoidable, slow down is with the external interrupt delivery. Due to the lack of AIA in EIC770X, all device irq now needs to trap to M mode first, before forwarding to VS mode. The overhead of running KVM in such setup is yet unknown, and may well be noticeable. All HS-qualified instructions will trap to M mode, which is costly. The NACL extension, if implemented, will alleviate it, but there's also the extra cost of flushing G/VS-stage TLBs. I'm analyzing it in parallel.

Use [4] if you have a Hifive Premier P550 to try it out.

[1] https://github.com/starfive-tech/JH7100_Docs/blob/main/JH7100%20Cache%20Coherence%20V1.0.pdf
[2] https://lore.kernel.org/all/[email protected]/
[3] https://github.com/ganboing/opensbi/tree/eic77x-vspt-physalias-wip
[4] https://github.com/ganboing/linux-eic77/tree/ganboing-xpbmt-uc-v2-eic77-clk-v15

---
v2:
- Move the core logic to Sifive errata to address Conor's comments

v1: https://lore.kernel.org/linux-riscv/[email protected]

Bo Gan (3):
riscv: alternatives: support auipc+load pair
riscv: errata: sifive: support auipc/load pair in patched alternatives
riscv: errata: sifive: Add an "errata" to simulate Svpbmt on cores
without

 arch/riscv/Kconfig.errata                    | 13 ++++
 arch/riscv/errata/sifive/errata.c            | 80 +++++++++++++++++++-
 arch/riscv/include/asm/errata_list.h         | 19 ++++-
 arch/riscv/include/asm/errata_list_vendors.h |  3 +-
 arch/riscv/include/asm/insn.h                |  8 ++
 arch/riscv/include/asm/pgtable-64.h          |  9 ++-
 arch/riscv/kernel/alternative.c              | 11 +--
 7 files changed, 132 insertions(+), 11 deletions(-)

2.34.1

Previously only auipc+jalr pair is supported. Add auipc+load pair to support PC-relative memory load instruction as well.

---
 arch/riscv/include/asm/insn.h   |  8 ++++++++
 arch/riscv/kernel/alternative.c | 11 ++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

index c3005573e8c99..1c791a8732efc 100644
@@ -135,6 +135,8 @@
 #define RVC_C2_RS1_MASK		GENMASK(4, 0)
 
 /* parts of opcode for RVG*/
+#define RVG_OPCODE_LOAD		0x03
+#define RVG_OPCODE_STORE	0x23
 #define RVG_OPCODE_FENCE	0x0f
 #define RVG_OPCODE_AUIPC	0x17
 #define RVG_OPCODE_BRANCH	0x63

@@ -198,6 +200,8 @@
 #define RVG_MATCH_BGE		(RV_ENCODE_FUNCT3(BGE) | RVG_OPCODE_BRANCH)
 #define RVG_MATCH_BLTU		(RV_ENCODE_FUNCT3(BLTU) | RVG_OPCODE_BRANCH)
 #define RVG_MATCH_BGEU		(RV_ENCODE_FUNCT3(BGEU) | RVG_OPCODE_BRANCH)
+#define RVG_MATCH_LOAD		(RVG_OPCODE_LOAD)
+#define RVG_MATCH_STORE		(RVG_OPCODE_STORE)
 #define RVG_MATCH_EBREAK	(RV_ENCODE_FUNCT12(EBREAK) | RVG_OPCODE_SYSTEM)
 #define RVG_MATCH_SRET		(RV_ENCODE_FUNCT12(SRET) | RVG_OPCODE_SYSTEM)
 #define RVC_MATCH_C_BEQZ	(RVC_ENCODE_FUNCT3(C_BEQZ) | RVC_OPCODE_C1)

@@ -222,6 +226,8 @@
 #define RVG_MASK_BGE		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
 #define RVG_MASK_BLTU		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
 #define RVG_MASK_BGEU		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
+#define RVG_MASK_LOAD		(RV_INSN_OPCODE_MASK)
+#define RVG_MASK_STORE		(RV_INSN_OPCODE_MASK)
 #define RVC_MASK_C_BEQZ		(RVC_INSN_FUNCT3_MASK | RVC_INSN_OPCODE_MASK)
 #define RVC_MASK_C_BNEZ		(RVC_INSN_FUNCT3_MASK | RVC_INSN_OPCODE_MASK)
 #define RVC_MASK_C_EBREAK	0xffff

@@ -262,6 +268,8 @@ __RISCV_INSN_FUNCS(c_ebreak, RVC_MASK_C_EBREAK, RVC_MATCH_C_EBREAK)
 __RISCV_INSN_FUNCS(ebreak, RVG_MASK_EBREAK, RVG_MATCH_EBREAK)
 __RISCV_INSN_FUNCS(sret, RVG_MASK_SRET, RVG_MATCH_SRET)
 __RISCV_INSN_FUNCS(fence, RVG_MASK_FENCE, RVG_MATCH_FENCE);
+__RISCV_INSN_FUNCS(load, RVG_MASK_LOAD, RVG_MATCH_LOAD);
+__RISCV_INSN_FUNCS(store, RVG_MASK_STORE, RVG_MATCH_STORE);
 
 /* special case to catch _any_ system instruction */
 static __always_inline bool riscv_insn_is_system(u32 code)

index 7642704c7f184..04a9d3aed4647 100644
@@ -74,7 +74,7 @@ static u32 riscv_instruction_at(void *p)
 	return (u32)parcel[0] | (u32)parcel[1] << 16;
 }
 
-static void riscv_alternative_fix_auipc_jalr(void *ptr, u32 auipc_insn,
+static void riscv_alternative_fix_auipc_pair(void *ptr, u32 auipc_insn,
 					     u32 jalr_insn, int patch_offset)
 {
 	u32 call[2] = { auipc_insn, jalr_insn };

@@ -123,14 +123,15 @@ void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len,
 		if (riscv_insn_is_auipc(insn) && i < num_insn - 1) {
 			u32 insn2 = riscv_instruction_at(alt_ptr + (i + 1) * sizeof(u32));
 
-			if (!riscv_insn_is_jalr(insn2))
+			if (!riscv_insn_is_jalr(insn2) &&
+			    !riscv_insn_is_load(insn2))
 				continue;
 
-			/* if instruction pair is a call, it will use the ra register */
-			if (RV_EXTRACT_RD_REG(insn) != 1)
+			if (RV_EXTRACT_RD_REG(insn) != RV_EXTRACT_RS1_REG(insn2))
 				continue;
 
-			riscv_alternative_fix_auipc_jalr(alt_ptr + i * sizeof(u32),
+			/* insn2 use rd of insn as rs1, patch it */
+			riscv_alternative_fix_auipc_pair(alt_ptr + i * sizeof(u32),
 							 insn, insn2, patch_offset);
 			i++;
 		}

2.34.1

Enhance the errata/sifive patching function to support auipc/load insn pair by fixing the offsets in immediate, just like in function riscv_cpufeature_patch_func. Refer to commit 27c653c06505 ("RISC-V: fix auipc-jalr addresses in patched alternatives")

---
 arch/riscv/errata/sifive/errata.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index d0c61f86cba33..f26c997e04e59 100644
@@ -80,6 +80,7 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end,
 			      unsigned int stage)
 {
 	struct alt_entry *alt;
+	void *oldptr, *altptr;
 	u32 cpu_req_errata;
 	u32 tmp;
 

@@ -100,9 +101,12 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end,
 
 		tmp = (1U << alt->patch_id);
 		if (cpu_req_errata & tmp) {
+			oldptr = ALT_OLD_PTR(alt);
+			altptr = ALT_ALT_PTR(alt);
+
 			mutex_lock(&text_mutex);
-			patch_text_nosync(ALT_OLD_PTR(alt), ALT_ALT_PTR(alt),
-					  alt->alt_len);
+			patch_text_nosync(oldptr, altptr, alt->alt_len);
+			riscv_alternative_fix_offsets(oldptr, alt->alt_len, oldptr - altptr);
 			mutex_unlock(&text_mutex);
 		}
 	}

2.34.1

On some platforms with pre-Svpbmt Sifive cores, they map the system memory twice in physical address space, one as cached (through the front port), and the other as uncached (through the system port), to enable drivers working with non-cache-coherent devices. Drivers can map the DMA buffers uncached through the uncached alias, and don't have to flush caches explicitly. It eases the driver programming. For complex device drivers, such as the GPU, this is a must have, as DMA pages can be mmap'ed to user space, and the userspace can't do caches flushes due to the lack of Zicbom on these older cores.

Introduce a Sifive "errata" to model such setup with a customized version of Svpbmt, "XPbmtUC", where a single, artificial bit in the PTE is used for cache/uncache control (UC), effectively offsetting the PPN by power-of-2. I.e.,

Starfive JH7110 (Sifive U74):

           [0x0,   0x40000000) Low MMIO
    [0x40000000, 0x2_40000000) Cached Mem

[0x4_40000000, 0x6_40000000) Uncached Mem UC+
[0x9_00000000, 0x9_d0000000) High MMIO

Using PTE bit 32 (PPN bit 34) as UC (uncache) control perfectly matches the memory map of the SoC.

Other SoCs like ESWIN EIC770X is not directly compatible to this model, as the uncached regions are not power-of-2 offseted, and the offsets are different between Dies in the dual-die version (EIC7702). The firmware, however, could use G-stage page table to transparently re-map, and make the address space suitable for XPbmtUC scheme to be applied:

             [0x0,     0x20000000) Core Internal
      [0x20000000,     0x40000000) Core Internal (Die 1)
      [0x40000000,     0x60000000) Low MMIO
      [0x60000000,     0x80000000) Low MMIO (Die 1)
      [0x80000000,  0x10_80000000) Cached Mem
   [0x20_00000000,  0x30_00000000) Cached Mem (Die 1)
   [0x80_00000000,  0xa0_00000000) High MMIO
   [0xa0_00000000,  0xc0_00000000) High MMIO (Die 1)
   [0xc0_00000000,  0xd0_00000000) Uncached Mem <----------.
   [0xe0_00000000,  0xf0_00000000) Uncached Mem (Die 1) <--+--.
 with firmware/hypervisor re-mapping:                      |  |
 ------------------------------------                      |  |
  [0x100_80000000, 0x110_80000000) Mem UC+ ----------------'  |

[0x120_00000000, 0x130_00000000) Mem UC+ (Die 1) -----------'

Such firmware capability is detected at boot time by sbi ecalls. The firmware will provide us the UC bit position if re-map is in effect.

Note: currently this feature is gated by JH7110 and EIC770X SoCs to avoid unnecessary sbi ecalls. There's also no IO bit in such XPbmtUC scheme, as it's assumed that the PMA (hard-wired on these SoCs) will convey the strongly-ordered, non-idempotent attribute of MMIO regions.

---
 arch/riscv/Kconfig.errata                    | 13 ++++
 arch/riscv/errata/sifive/errata.c            | 72 ++++++++++++++++++++
 arch/riscv/include/asm/errata_list.h         | 19 +++++-
 arch/riscv/include/asm/errata_list_vendors.h |  3 +-
 arch/riscv/include/asm/pgtable-64.h          |  9 ++-
 5 files changed, 112 insertions(+), 4 deletions(-)

index 3c945d086c7d0..0722dc8df4a9e 100644
@@ -76,6 +76,19 @@ config ERRATA_SIFIVE_CIP_1200
 
 	  If you don't know what to do here, say "Y".
 
+config ERRATA_SIFIVE_XPBMTUC
+	bool "Support XPbmtUC (customized uncache bit)"
+	depends on ERRATA_SIFIVE && 64BIT && MMU
+	default y
+	select DMA_DIRECT_REMAP
+	help
+	  This will detect and enable the XPbmtUC, where a bit in PTE
+	  is chosen as the UC (uncache) control bit to emulate Svpbmt
+	  on supported SoCs. For SoCs with non-cache-coherent devices,
+	  enabling XPbmtUC allows drivers to map DMA buffers uncached.
+
+	  If you don't know what to do here, say "Y".
+
 config ERRATA_STARFIVE_JH7100
 	bool "StarFive JH7100 support"
 	depends on ARCH_STARFIVE

index 6694b5ccdcf85..a05086f05a0cf 100644
@@ -53,18 +53,33 @@ asm(ALTERNATIVE(	\
 	: /* no inputs */	\
 	: "memory")
 
+#ifdef CONFIG_64BIT
+#define ALT_PAGE_CUST_BIT(_bit)						\
+asm(ALTERNATIVE("li %0, 0\t\nnop",					\
+		"1: auipc %0, %%pcrel_hi(riscv_xpbmtuc_mask)\t\n"	\
+		      "ld %0, %%pcrel_lo(1b)(%0)", SIFIVE_VENDOR_ID,	\
+			ERRATA_SIFIVE_XPBMTUC,				\
+			CONFIG_ERRATA_SIFIVE_XPBMTUC)			\
+		: "=r"(_bit))
+#endif
+
 /*
  * _val is marked as "will be overwritten", so need to set it to 0
  * in the default case.
  */
 #define ALT_SVPBMT_SHIFT 61
 #define ALT_THEAD_MAE_SHIFT 59
+#define HAS_XPBMTUC_PAGE_NOCACHE	CONFIG_ERRATA_SIFIVE_XPBMTUC
+#define HAS_XPBMTUC_PAGE_MTMASK		CONFIG_ERRATA_SIFIVE_XPBMTUC
 #define ALT_SVPBMT(_val, prot)						\
-asm(ALTERNATIVE_2("li %0, 0\t\nnop",					\
+asm(ALTERNATIVE_3("li %0, 0\t\nnop",					\
 		  "li %0, %1\t\nslli %0,%0,%3", 0,			\
 			RISCV_ISA_EXT_SVPBMT, CONFIG_RISCV_ISA_SVPBMT,	\
 		  "li %0, %2\t\nslli %0,%0,%4", THEAD_VENDOR_ID,	\
-			ERRATA_THEAD_MAE, CONFIG_ERRATA_THEAD_MAE)	\
+			ERRATA_THEAD_MAE, CONFIG_ERRATA_THEAD_MAE,	\
+		  "1: auipc %0, %%pcrel_hi(riscv_xpbmtuc_mask)\t\n"	\
+			"ld %0, %%pcrel_lo(1b)(%0)", SIFIVE_VENDOR_ID,	\
+			ERRATA_SIFIVE_XPBMTUC, HAS_XPBMTUC##prot)	\
 		: "=r"(_val)						\
 		: "I"(prot##_SVPBMT >> ALT_SVPBMT_SHIFT),		\
 		  "I"(prot##_THEAD >> ALT_THEAD_MAE_SHIFT),		\

index ec7eba3734371..b2bf8d7a52c30 100644
@@ -11,7 +11,8 @@
 #ifdef CONFIG_ERRATA_SIFIVE
 #define	ERRATA_SIFIVE_CIP_453 0
 #define	ERRATA_SIFIVE_CIP_1200 1
-#define	ERRATA_SIFIVE_NUMBER 2
+#define	ERRATA_SIFIVE_XPBMTUC 2
+#define	ERRATA_SIFIVE_NUMBER 3
 #endif
 
 #ifdef CONFIG_ERRATA_THEAD

index 6e789fa58514c..7f47a361d8003 100644
@@ -76,7 +76,14 @@ typedef struct {
  * | 63 | 62 61 | 60 54 | 53  10 | 9             8 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
  *   N      MT     RSV    PFN      reserved for SW   D   A   G   U   X   W   R   V
  */
-#define _PAGE_PFN_MASK  GENMASK(53, 10)
+static inline u64 riscv_pfn_mask(void)
+{
+	u64 cust_bit;
+
+	ALT_PAGE_CUST_BIT(cust_bit);
+	return GENMASK(53, 10) ^ cust_bit;
+}
+#define _PAGE_PFN_MASK  riscv_pfn_mask()
 
 /*
  * [63] Svnapot definitions:

2.34.1

Hi All,

Any suggestions/comments on this?

Bo

I'll get to it at some point, just busy sorry!

Hi Conor,

It's been quite a while, and I almost forgot about this. Any new suggestions? Comments?

Bo

Oh god, I completely forgot about this - sorry.

I'm unfortunately not available really until the 22nd, but I'll do my best to take a look before LPC.

Hey,

Once again, I am sorry for how long this has taken me. I've had this sitting at the base of my review mailbox and kept meaning to get to it, but due to being at the bottom I always got here when I was already tired and could never face into actually sitting down and looking at what the EIC7700 opensbi changes actually did.

he other day Icenowy submitted some patches for the drm driver used on the JH7110 that are not actually that intrusive:
https://lore.kernel.org/all/[email protected]/

No idea if the DRM maintainers will find this kind of thing acceptable though, there's every chance that they hate the idea. On the JH7110 I don't mind what's done in this series - especially given that what the JH7100 is doing with reserved memory is something Rob considered to be a bit of an abuse. But if the DRM guys are okay with Icenowy's work, the value of this is obviously much reduced. One new alternative (that could be avoided entirely if the erratum config is disabled) isn't much of a price to pay here if the userspace flushing is important (I really dunno anything at all about how useful that is to GPU stuff).

I dunno, it's not that easy to remove I don't think. Switching from this to Samuel's approach requires a DT change, so you're looking at regressions if this is removed.

The problem with this idea though is how it works on EIC7700. This is very invasive and the impact this has on hypervisor use (on one of the only hypervisor capable boards) probably makes this unacceptable to most users, compared to a solution like Samuel's that has nothing like that.

On top of that, it has firmware changes that really would need to be either upstream or shipped by the vendor to make this acceptable. I know vendors are willing to ship all sorts of things, but with that in mind, they could just ship Samuel's series downstream without the downsides the opensbi change here has. Upstreaming it may not be possible, which would leave things in a really weird state.

I know Samuel's series has been stalled for a long time, but I expect it to get revived when the eic7700 support patches progress past the simpler peripherals.
Without the vendors (be that sifive or eswin or einfochips) showing interest in this method it's really hard to sit here and tell you to pursue this further, since you're probably gonna face resistance getting the opensbi stuff anywhere.

Thanks,
Conor.