public inbox for opensbi@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Initial ESWIN/EIC7700 support
@ 2025-11-17  5:48 Bo Gan
  2025-11-17  5:48 ` [PATCH v2 1/5] lib: sbi: allow platform to override PMP (un)configuration Bo Gan
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Bo Gan @ 2025-11-17  5:48 UTC (permalink / raw)
  To: opensbi; +Cc: linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

EIC7700 is the SoC used in HiFive P550 and Milk-V Megrez. This SoC is
currently one of the only off-the-shelf board/chips that support H
extension, although it's v0.6.1. It also supports pre-ratified N-trace.
Add support for it so people can benefit from latest OpenSBI features.

The device-tree of HiFive P550 has been upstreamed to Linux:
https://lore.kernel.org/all/20250825132427.1618089-1-pinkesh.vaghela@einfochips.com/
However U-boot is not, and there are bugs in vendor U-boot device-tree,
and also inconsistencies between the two. Thus, this patch is coded with
the upstreamed device-tree as the reference, but tested with the patched
vendor U-boot device tree as `FW_FDT_PATH`. The patched vendor U-boot is
hosted here: https://github.com/ganboing/u-boot-eic7x/tree/eic7x-dt-fix
Refer to PATCH 5/5 for the instructions on building the firmware blob
and launch it through UART boot.

The major complication of this chip is that it requires certain memory
regions to be blocked with PMP entries to prevent speculative execution
or HW prefetcher from touching them to avoid bus errors. Also due to the
fact that this SoC handles cache incoherent DMA by mapping memory twice,
one as cached, and the other as uncached, we also need an extra PMP to
protect the OpenSBI in the uncached portion in address space. Following
changes are made to lib/ and firmware/ to make it possible:

 - Allow platform to override pmp_(un)configure
 - Add helper function for settings PMP TOR
 - Add helper function to check if memregions are disjoint
 - Introduce FIRMWARE_PACKED_RXRW for disabling power-of-2 RW split

The defconfig of EIC770X is separated out, because it has to require
CONFIG_FIRMWARE_PACKED_RXRW to not run out of PMP entries. It'll
change the layout of firmware sections to not power-of-2 align the
RW sections, so do not by default enable this for other platforms.

Signed-off-by: Bo Gan <ganboing@gmail.com>
---
Changes in v2:
- Major enhancement of PMP consolidation logic. Also fixed a Linux
  Panic bug due to the mismatch between PMP settings and reserved
  memory regions passed to Linux via FDT.
- Also protects the OpenSBI firmware in uncached memory portion of
  address space.
- More detailed documentation on EIC770X/P550

---
Bo Gan (5):
  lib: sbi: allow platform to override PMP configuration
  lib: sbi: Add __pmp_set_tor for setting TOR regions
  firmware: add CONFIG_FIRMWARE_PACKED_RXRW
  include: sbi: Add helpers for sbi_domain_memregion
  platform: generic: eswin: add EIC7700

 firmware/Kconfig                           |  11 +
 firmware/fw_base.ldS                       |  14 +-
 include/sbi/riscv_asm.h                    |   7 +
 include/sbi/sbi_domain.h                   |  23 ++
 include/sbi/sbi_hart.h                     |   9 +
 include/sbi/sbi_platform.h                 |  53 +++
 lib/sbi/riscv_asm.c                        |  75 ++++-
 lib/sbi/sbi_domain.c                       |  11 +
 lib/sbi/sbi_domain_context.c               |  11 +-
 lib/sbi/sbi_hart.c                         |  97 ++++--
 platform/generic/Kconfig                   |   6 +
 platform/generic/configs/eic770x_defconfig |  28 ++
 platform/generic/eswin/Kconfig             |  29 ++
 platform/generic/eswin/eic770x.c           | 361 +++++++++++++++++++++
 platform/generic/eswin/objects.mk          |  11 +
 platform/generic/include/eswin/eic770x.h   |  66 ++++
 16 files changed, 752 insertions(+), 60 deletions(-)
 create mode 100644 platform/generic/configs/eic770x_defconfig
 create mode 100644 platform/generic/eswin/Kconfig
 create mode 100644 platform/generic/eswin/eic770x.c
 create mode 100644 platform/generic/eswin/objects.mk
 create mode 100644 platform/generic/include/eswin/eic770x.h

-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH v2 1/5] lib: sbi: allow platform to override PMP (un)configuration
  2025-11-17  5:48 [PATCH v2 0/5] Initial ESWIN/EIC7700 support Bo Gan
@ 2025-11-17  5:48 ` Bo Gan
  2025-11-17  5:48 ` [PATCH v2 2/5] lib: sbi: Add __pmp_set_tor for setting TOR regions Bo Gan
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Bo Gan @ 2025-11-17  5:48 UTC (permalink / raw)
  To: opensbi; +Cc: linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

Platform sometimes wants to override the entire PMP (un)config phase,
not just performing additional work for each individual PMP entry. This
allows platforms to insert SoC/core specific PMP entries in a way that
works together with the existing ones set by lib/ code, not conflicting.
platform can also choose to merge or skip memory regions in a reasonable
way in case there's a shortage of PMP entries.

In addition, logic in `sbi_hart_oldpmp_configure` is abstracted out as
`sbi_hart_oldpmp_set`, and made public, so platform with traditional PMP
can leverage it in its own `pmp_configure`

Signed-off-by: Bo Gan <ganboing@gmail.com>
---
 include/sbi/sbi_hart.h       |  9 ++++
 include/sbi/sbi_platform.h   | 53 ++++++++++++++++++++
 lib/sbi/sbi_domain_context.c | 11 +---
 lib/sbi/sbi_hart.c           | 97 ++++++++++++++++++++++++------------
 4 files changed, 129 insertions(+), 41 deletions(-)

diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
index e66dd52f..8ece6342 100644
--- a/include/sbi/sbi_hart.h
+++ b/include/sbi/sbi_hart.h
@@ -133,6 +133,8 @@ struct sbi_hart_features {
 };
 
 struct sbi_scratch;
+struct sbi_domain;
+struct sbi_domain_memregion;
 
 int sbi_hart_reinit(struct sbi_scratch *scratch);
 int sbi_hart_init(struct sbi_scratch *scratch, bool cold_boot);
@@ -147,7 +149,14 @@ unsigned int sbi_hart_pmp_log2gran(struct sbi_scratch *scratch);
 unsigned int sbi_hart_pmp_addrbits(struct sbi_scratch *scratch);
 unsigned int sbi_hart_mhpm_bits(struct sbi_scratch *scratch);
 bool sbi_hart_smepmp_is_fw_region(unsigned int pmp_idx);
+void sbi_hart_pmp_unconfigure(struct sbi_scratch *scratch);
 int sbi_hart_pmp_configure(struct sbi_scratch *scratch);
+void sbi_hart_oldpmp_set(struct sbi_scratch *scratch,
+			 struct sbi_domain *dom,
+			 struct sbi_domain_memregion *reg,
+			 unsigned int pmp_idx,
+			 unsigned int pmp_log2gran,
+			 unsigned long pmp_addr_max);
 int sbi_hart_map_saddr(unsigned long base, unsigned long size);
 int sbi_hart_unmap_saddr(void);
 int sbi_hart_priv_version(struct sbi_scratch *scratch);
diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
index d75c12de..a53e1797 100644
--- a/include/sbi/sbi_platform.h
+++ b/include/sbi/sbi_platform.h
@@ -146,6 +146,18 @@ struct sbi_platform_operations {
 			unsigned long log2len);
 	/** platform specific pmp disable on current HART */
 	void (*pmp_disable)(unsigned int n);
+
+	/** platform pmp configure override on current HART */
+	int (*pmp_configure)(unsigned int pmp_count,
+			     unsigned int pmp_log2gran,
+			     unsigned long pmp_addr_max);
+	/**
+	 * You need both pmp_configure/unconfigure to properly
+	 * provide platform override
+	 */
+
+	/** platform pmp unconfigure override on current HART */
+	void (*pmp_unconfigure)(void);
 };
 
 /** Platform default per-HART stack size for exception/interrupt handling */
@@ -666,6 +678,47 @@ static inline void sbi_platform_pmp_disable(const struct sbi_platform *plat,
 		sbi_platform_ops(plat)->pmp_disable(n);
 }
 
+/**
+ * Check if platform wants to override PMP (un)configuration
+ *
+ * @param plat pointer to struct sbi_platform
+ */
+static inline bool sbi_platform_pmp_override(const struct sbi_platform *plat)
+{
+	return plat &&
+		sbi_platform_ops(plat)->pmp_configure &&
+		sbi_platform_ops(plat)->pmp_unconfigure;
+}
+
+/**
+ * Platform PMP configuration override
+ *
+ * @param plat pointer to struct sbi_platform
+ * @param pmp_count number of PMP entries
+ * @param pmp_log2gran PMP granularity
+ * @param pmp_addr_max largest value pmpaddr(x) can hold
+ */
+static inline int sbi_platform_pmp_configure(const struct sbi_platform *plat,
+					     unsigned int pmp_count,
+					     unsigned int pmp_log2gran,
+					     unsigned long pmp_addr_max)
+{
+	return sbi_platform_ops(plat)->pmp_configure(pmp_count,
+						     pmp_log2gran,
+						     pmp_addr_max);
+}
+
+/**
+ * Platform PMP unconfiguration override
+ *
+ * @param plat pointer to struct sbi_platform
+ */
+static inline void sbi_platform_pmp_unconfigure(
+					const struct sbi_platform *plat)
+{
+	return sbi_platform_ops(plat)->pmp_unconfigure();
+}
+
 #endif
 
 #endif
diff --git a/lib/sbi/sbi_domain_context.c b/lib/sbi/sbi_domain_context.c
index 74ad25e8..ea7f741b 100644
--- a/lib/sbi/sbi_domain_context.c
+++ b/lib/sbi/sbi_domain_context.c
@@ -102,7 +102,6 @@ static int switch_to_next_domain_context(struct hart_context *ctx,
 	struct sbi_trap_context *trap_ctx;
 	struct sbi_domain *current_dom, *target_dom;
 	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
-	unsigned int pmp_count = sbi_hart_pmp_count(scratch);
 
 	if (!ctx || !dom_ctx || ctx == dom_ctx)
 		return SBI_EINVAL;
@@ -120,15 +119,7 @@ static int switch_to_next_domain_context(struct hart_context *ctx,
 	sbi_hartmask_set_hartindex(hartindex, &target_dom->assigned_harts);
 	spin_unlock(&target_dom->assigned_harts_lock);
 
-	/* Reconfigure PMP settings for the new domain */
-	for (int i = 0; i < pmp_count; i++) {
-		/* Don't revoke firmware access permissions */
-		if (sbi_hart_smepmp_is_fw_region(i))
-			continue;
-
-		sbi_platform_pmp_disable(sbi_platform_thishart_ptr(), i);
-		pmp_disable(i);
-	}
+	sbi_hart_pmp_unconfigure(scratch);
 	sbi_hart_pmp_configure(scratch);
 
 	/* Save current CSR context and restore target domain's CSR context */
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index a91703b4..b68b18d4 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -433,6 +433,44 @@ static int sbi_hart_smepmp_configure(struct sbi_scratch *scratch,
 	return 0;
 }
 
+void sbi_hart_oldpmp_set(struct sbi_scratch *scratch,
+			 struct sbi_domain *dom,
+			 struct sbi_domain_memregion *reg,
+			 unsigned int pmp_idx,
+			 unsigned int pmp_log2gran,
+			 unsigned long pmp_addr_max)
+{
+	unsigned int pmp_flags = 0;
+	unsigned long pmp_addr;
+
+	/*
+	 * If permissions are to be enforced for all modes on
+	 * this region, the lock bit should be set.
+	 */
+	if (reg->flags & SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS)
+		pmp_flags |= PMP_L;
+
+	if (reg->flags & SBI_DOMAIN_MEMREGION_SU_READABLE)
+		pmp_flags |= PMP_R;
+	if (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE)
+		pmp_flags |= PMP_W;
+	if (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)
+		pmp_flags |= PMP_X;
+
+	pmp_addr = reg->base >> PMP_SHIFT;
+	if (pmp_log2gran <= reg->order && pmp_addr < pmp_addr_max) {
+		sbi_platform_pmp_set(sbi_platform_ptr(scratch),
+				     pmp_idx, reg->flags, pmp_flags,
+				     reg->base, reg->order);
+		pmp_set(pmp_idx, pmp_flags, reg->base, reg->order);
+	} else {
+		sbi_printf("Can not configure pmp for domain %s because"
+			   " memory region address 0x%lx or size 0x%lx "
+			   "is not in range.\n", dom->name, reg->base,
+			   reg->order);
+	}
+}
+
 static int sbi_hart_oldpmp_configure(struct sbi_scratch *scratch,
 				     unsigned int pmp_count,
 				     unsigned int pmp_log2gran,
@@ -441,41 +479,13 @@ static int sbi_hart_oldpmp_configure(struct sbi_scratch *scratch,
 	struct sbi_domain_memregion *reg;
 	struct sbi_domain *dom = sbi_domain_thishart_ptr();
 	unsigned int pmp_idx = 0;
-	unsigned int pmp_flags;
-	unsigned long pmp_addr;
 
 	sbi_domain_for_each_memregion(dom, reg) {
 		if (!is_valid_pmp_idx(pmp_count, pmp_idx))
 			return SBI_EFAIL;
 
-		pmp_flags = 0;
-
-		/*
-		 * If permissions are to be enforced for all modes on
-		 * this region, the lock bit should be set.
-		 */
-		if (reg->flags & SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS)
-			pmp_flags |= PMP_L;
-
-		if (reg->flags & SBI_DOMAIN_MEMREGION_SU_READABLE)
-			pmp_flags |= PMP_R;
-		if (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE)
-			pmp_flags |= PMP_W;
-		if (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)
-			pmp_flags |= PMP_X;
-
-		pmp_addr = reg->base >> PMP_SHIFT;
-		if (pmp_log2gran <= reg->order && pmp_addr < pmp_addr_max) {
-			sbi_platform_pmp_set(sbi_platform_ptr(scratch),
-					     pmp_idx, reg->flags, pmp_flags,
-					     reg->base, reg->order);
-			pmp_set(pmp_idx++, pmp_flags, reg->base, reg->order);
-		} else {
-			sbi_printf("Can not configure pmp for domain %s because"
-				   " memory region address 0x%lx or size 0x%lx "
-				   "is not in range.\n", dom->name, reg->base,
-				   reg->order);
-		}
+		sbi_hart_oldpmp_set(scratch, dom, reg, pmp_idx++,
+				    pmp_log2gran, pmp_addr_max);
 	}
 
 	return 0;
@@ -528,12 +538,34 @@ int sbi_hart_unmap_saddr(void)
 	return pmp_disable(SBI_SMEPMP_RESV_ENTRY);
 }
 
+void sbi_hart_pmp_unconfigure(struct sbi_scratch *scratch)
+{
+	unsigned int pmp_count = sbi_hart_pmp_count(scratch);
+	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
+
+	if (sbi_platform_pmp_override(plat)) {
+		sbi_platform_pmp_unconfigure(plat);
+		return;
+	}
+
+	/* Reconfigure PMP settings for the new domain */
+	for (unsigned int i = 0; i < pmp_count; i++) {
+		/* Don't revoke firmware access permissions */
+		if (sbi_hart_smepmp_is_fw_region(i))
+			continue;
+
+		sbi_platform_pmp_disable(sbi_platform_thishart_ptr(), i);
+		pmp_disable(i);
+	}
+}
+
 int sbi_hart_pmp_configure(struct sbi_scratch *scratch)
 {
 	int rc;
 	unsigned int pmp_bits, pmp_log2gran;
 	unsigned int pmp_count = sbi_hart_pmp_count(scratch);
 	unsigned long pmp_addr_max;
+	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
 
 	if (!pmp_count)
 		return 0;
@@ -542,7 +574,10 @@ int sbi_hart_pmp_configure(struct sbi_scratch *scratch)
 	pmp_bits = sbi_hart_pmp_addrbits(scratch) - 1;
 	pmp_addr_max = (1UL << pmp_bits) | ((1UL << pmp_bits) - 1);
 
-	if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMEPMP))
+	if (sbi_platform_pmp_override(plat))
+		rc = sbi_platform_pmp_configure(plat, pmp_count,
+						pmp_log2gran, pmp_addr_max);
+	else if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMEPMP))
 		rc = sbi_hart_smepmp_configure(scratch, pmp_count,
 						pmp_log2gran, pmp_addr_max);
 	else
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 2/5] lib: sbi: Add __pmp_set_tor for setting TOR regions
  2025-11-17  5:48 [PATCH v2 0/5] Initial ESWIN/EIC7700 support Bo Gan
  2025-11-17  5:48 ` [PATCH v2 1/5] lib: sbi: allow platform to override PMP (un)configuration Bo Gan
@ 2025-11-17  5:48 ` Bo Gan
  2025-11-17  5:48 ` [PATCH v2 3/5] firmware: add CONFIG_FIRMWARE_PACKED_RXRW Bo Gan
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Bo Gan @ 2025-11-17  5:48 UTC (permalink / raw)
  To: opensbi; +Cc: linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

TOR can be utilized to cover memory regions that are not aligned with
their sizes. Given that the address matching is formed bt 2 consecutive
pmpaddr, i.e., pmpaddr(i-1) and pmpaddr(i), TOR should not be used
generically to avoid pmpaddr conflict with other NA4/NAPOT regions.

Only use them in platform PMP (un)configure override code where the
caller can ensure the index and order of every pmp region especially when
there's a mixture of TOR/NA4/NAPOT. Don't use TOR in lib/ code. For lib/
code, maintain the 1:1 PMP entry <-> memory region correspondence.

Signed-off-by: Bo Gan <ganboing@gmail.com>
---
 include/sbi/riscv_asm.h |  7 ++++
 lib/sbi/riscv_asm.c     | 75 +++++++++++++++++++++++++++++++----------
 2 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/include/sbi/riscv_asm.h b/include/sbi/riscv_asm.h
index ef48dc89..04ecf5c1 100644
--- a/include/sbi/riscv_asm.h
+++ b/include/sbi/riscv_asm.h
@@ -215,6 +215,13 @@ int pmp_disable(unsigned int n);
 /* Check if the matching field is set */
 int is_pmp_entry_mapped(unsigned long entry);
 
+/**
+ * Top of range (TOR) matching mode. pmpaddr(n-1) will also be changed.
+ * Use it *very* carefully.
+ */
+int __pmp_set_tor(unsigned int n, unsigned long prot, unsigned long addr,
+		  unsigned long size);
+
 int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
 	    unsigned long log2len);
 
diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
index 3e44320f..557e1c82 100644
--- a/lib/sbi/riscv_asm.c
+++ b/lib/sbi/riscv_asm.c
@@ -330,16 +330,10 @@ int is_pmp_entry_mapped(unsigned long entry)
 	return false;
 }
 
-int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
-	    unsigned long log2len)
+static void pmp_set_prot(unsigned int n, unsigned long prot)
 {
-	int pmpcfg_csr, pmpcfg_shift, pmpaddr_csr;
+	int pmpcfg_csr, pmpcfg_shift;
 	unsigned long cfgmask, pmpcfg;
-	unsigned long addrmask, pmpaddr;
-
-	/* check parameters */
-	if (n >= PMP_COUNT || log2len > __riscv_xlen || log2len < PMP_SHIFT)
-		return SBI_EINVAL;
 
 	/* calculate PMP register and offset */
 #if __riscv_xlen == 32
@@ -351,15 +345,29 @@ int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
 #else
 # error "Unexpected __riscv_xlen"
 #endif
-	pmpaddr_csr = CSR_PMPADDR0 + n;
-
-	/* encode PMP config */
-	prot &= ~PMP_A;
-	prot |= (log2len == PMP_SHIFT) ? PMP_A_NA4 : PMP_A_NAPOT;
 	cfgmask = ~(0xffUL << pmpcfg_shift);
 	pmpcfg	= (csr_read_num(pmpcfg_csr) & cfgmask);
 	pmpcfg |= ((prot << pmpcfg_shift) & ~cfgmask);
 
+	csr_write_num(pmpcfg_csr, pmpcfg);
+}
+
+static void pmp_set_addr(unsigned int n, unsigned long pmpaddr)
+{
+	int pmpaddr_csr = CSR_PMPADDR0 + n;
+
+	csr_write_num(pmpaddr_csr, pmpaddr);
+}
+
+int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
+	    unsigned long log2len)
+{
+	unsigned long addrmask, pmpaddr;
+
+	/* check parameters */
+	if (n >= PMP_COUNT || log2len > __riscv_xlen || log2len < PMP_SHIFT)
+		return SBI_EINVAL;
+
 	/* encode PMP address */
 	if (log2len == PMP_SHIFT) {
 		pmpaddr = (addr >> PMP_SHIFT);
@@ -373,10 +381,41 @@ int pmp_set(unsigned int n, unsigned long prot, unsigned long addr,
 		}
 	}
 
+	/* encode PMP config */
+	prot &= ~PMP_A;
+	prot |= (log2len == PMP_SHIFT) ? PMP_A_NA4 : PMP_A_NAPOT;
+
 	/* write csrs */
-	csr_write_num(pmpaddr_csr, pmpaddr);
-	csr_write_num(pmpcfg_csr, pmpcfg);
+	pmp_set_addr(n, pmpaddr);
+	pmp_set_prot(n, prot);
+	return 0;
+}
+
+int __pmp_set_tor(unsigned int n, unsigned long prot, unsigned long addr,
+		  unsigned long size)
+{
+	unsigned long pmpaddr, pmpaddrp;
+
+	/* check parameters */
+	if (n >= PMP_COUNT)
+		return SBI_EINVAL;
+
+	if (n == 0 && addr != 0)
+		return SBI_EINVAL;
+
+	/* encode PMP address */
+	pmpaddrp = addr >> PMP_SHIFT;
+	pmpaddr = (addr + size) >> PMP_SHIFT;
 
+	/* encode PMP config */
+	prot &= ~PMP_A;
+	prot |= PMP_A_TOR;
+
+	/* write csrs */
+	if (n)
+		pmp_set_addr(n - 1, pmpaddrp);
+	pmp_set_addr(n, pmpaddr);
+	pmp_set_prot(n, prot);
 	return 0;
 }
 
@@ -420,10 +459,12 @@ int pmp_get(unsigned int n, unsigned long *prot_out, unsigned long *addr_out,
 			addr	= (addr & ~((1UL << t1) - 1)) << PMP_SHIFT;
 			len	= (t1 + PMP_SHIFT + 1);
 		}
-	} else {
+	} else if ((prot & PMP_A) == PMP_A_NA4) {
 		addr	= csr_read_num(pmpaddr_csr) << PMP_SHIFT;
 		len	= PMP_SHIFT;
-	}
+	} else
+		/* Error out for TOR region */
+		return SBI_EINVAL;
 
 	/* return details */
 	*prot_out    = prot;
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 3/5] firmware: add CONFIG_FIRMWARE_PACKED_RXRW
  2025-11-17  5:48 [PATCH v2 0/5] Initial ESWIN/EIC7700 support Bo Gan
  2025-11-17  5:48 ` [PATCH v2 1/5] lib: sbi: allow platform to override PMP (un)configuration Bo Gan
  2025-11-17  5:48 ` [PATCH v2 2/5] lib: sbi: Add __pmp_set_tor for setting TOR regions Bo Gan
@ 2025-11-17  5:48 ` Bo Gan
  2025-11-17  5:48 ` [PATCH v2 4/5] include: sbi: Add helpers for sbi_domain_memregion Bo Gan
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Bo Gan @ 2025-11-17  5:48 UTC (permalink / raw)
  To: opensbi; +Cc: linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

By default we power-of-2 align the rw sections of firmware. Provide
an option to disable this behavior. Platforms w/o Smepmp won't be
able to enforce RX/RW in M mode, so this 2^n align only perhaps
saves some memory by covering OpenSBI with 2 finer grained PMP
entries, instead of 1. For platforms that are really in short of
PMP, consider enabling this knob.

Signed-off-by: Bo Gan <ganboing@gmail.com>
---
 firmware/Kconfig     | 11 +++++++++++
 firmware/fw_base.ldS | 14 ++++++++++++--
 lib/sbi/sbi_domain.c | 11 +++++++++++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/firmware/Kconfig b/firmware/Kconfig
index c1fee190..30e4ec7d 100644
--- a/firmware/Kconfig
+++ b/firmware/Kconfig
@@ -25,4 +25,15 @@ config STACK_PROTECTOR_ALL
 	  Turn on the "stack-protector" with "-fstack-protector-all" option.
 	  Like -fstack-protector except that all functions are protected.
 
+config FIRMWARE_PACKED_RXRW
+	bool "Do not power-of-2 align the RW portion"
+	default n
+	help
+	  By default we align the beginning of read-write data sections to
+	  2^n. This facilitates the setting of NAPOT PMP entries to cover
+	  text, rodata, data... sections with proper permissions. For those
+	  platforms that're in short of PMP entries, and not using Smepmp,
+	  they may choose to disable this alignment and the entire firmware
+	  can be covered by a single PMP entry.
+
 endmenu
diff --git a/firmware/fw_base.ldS b/firmware/fw_base.ldS
index 12c7a844..8b60a68a 100644
--- a/firmware/fw_base.ldS
+++ b/firmware/fw_base.ldS
@@ -56,12 +56,22 @@
 
 	/* End of the read-only data sections */
 
+#ifdef CONFIG_FIRMWARE_PACKED_RXRW
+	. = ALIGN(0x1000); /* Ensure next section is page aligned */
+#else
 	/*
-	 * PMP regions must be to be power-of-2. RX/RW will have separate
-	 * regions, so ensure that the split is power-of-2.
+	 * Align the start of RW sections to power-of-2, so READ/EXEC
+	 * and READ/WRITE sections can be covered by different PMP
+	 * entries. Platforms with Smepmp can utilize it to enfore RX/RW
+	 * permission on different portions of the firmware. For platforms
+	 * using traditional PMPs, there's no enforcement of permissions
+	 * in M mode, so NAPOT aligning the RW sections has no security
+	 * benefits other than perhaps saving some memory for SU mode to
+	 * use. It does require 2 PMP entries to cover OpenSBI, not 1.
 	 */
 	. = ALIGN(1 << LOG2CEIL((SIZEOF(.rodata) + SIZEOF(.text)
 				+ SIZEOF(.dynsym) + SIZEOF(.rela.dyn))));
+#endif
 
 	PROVIDE(_fw_rw_start = .);
 
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index da0f0557..7307f11e 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -865,6 +865,7 @@ int sbi_domain_init(struct sbi_scratch *scratch, u32 cold_hartid)
 
 	SBI_INIT_LIST_HEAD(&domain_list);
 
+#ifndef CONFIG_FIRMWARE_PACKED_RXRW
 	if (scratch->fw_rw_offset == 0 ||
 	    (scratch->fw_rw_offset & (scratch->fw_rw_offset - 1)) != 0) {
 		sbi_printf("%s: fw_rw_offset is not a power of 2 (0x%lx)\n",
@@ -877,6 +878,7 @@ int sbi_domain_init(struct sbi_scratch *scratch, u32 cold_hartid)
 			   __func__);
 		return SBI_EINVAL;
 	}
+#endif
 
 	domain_hart_ptr_offset = sbi_scratch_alloc_type_offset(void *);
 	if (!domain_hart_ptr_offset)
@@ -904,6 +906,14 @@ int sbi_domain_init(struct sbi_scratch *scratch, u32 cold_hartid)
 	root.possible_harts = root_hmask;
 
 	/* Root domain firmware memory region */
+#ifdef CONFIG_FIRMWARE_PACKED_RXRW
+	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
+				  (SBI_DOMAIN_MEMREGION_M_READABLE |
+				   SBI_DOMAIN_MEMREGION_M_WRITABLE |
+				   SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
+				   SBI_DOMAIN_MEMREGION_FW),
+				  &root_memregs[root_memregs_count++]);
+#else
 	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_rw_offset,
 				  (SBI_DOMAIN_MEMREGION_M_READABLE |
 				   SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
@@ -916,6 +926,7 @@ int sbi_domain_init(struct sbi_scratch *scratch, u32 cold_hartid)
 				   SBI_DOMAIN_MEMREGION_M_WRITABLE |
 				   SBI_DOMAIN_MEMREGION_FW),
 				  &root_memregs[root_memregs_count++]);
+#endif
 
 	root.fw_region_inited = true;
 
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 4/5] include: sbi: Add helpers for sbi_domain_memregion
  2025-11-17  5:48 [PATCH v2 0/5] Initial ESWIN/EIC7700 support Bo Gan
                   ` (2 preceding siblings ...)
  2025-11-17  5:48 ` [PATCH v2 3/5] firmware: add CONFIG_FIRMWARE_PACKED_RXRW Bo Gan
@ 2025-11-17  5:48 ` Bo Gan
  2025-11-17  5:48 ` [PATCH v2 5/5] platform: generic: eswin: add EIC7700 Bo Gan
  2025-11-17  8:04 ` [PATCH v2 0/5] Initial ESWIN/EIC7700 support Anup Patel
  5 siblings, 0 replies; 12+ messages in thread
From: Bo Gan @ 2025-11-17  5:48 UTC (permalink / raw)
  To: opensbi; +Cc: linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

New helpers:

- sbi_domain_memregion_disjoint:
  Check if two memregion are disjoint

- sbi_domain_for_each_memregion_idx:
  Iterate domain memregion with index

Signed-off-by: Bo Gan <ganboing@gmail.com>
---
 include/sbi/sbi_domain.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index 1196d609..0d1b095f 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -164,6 +164,26 @@ struct sbi_domain_memregion {
 	unsigned long flags;
 };
 
+/**
+ * Check if two memregion A/B are disjoint.
+ * @param a memregion A
+ * @param b memregion B
+ * @return true if disjoint
+ */
+static inline bool sbi_domain_memregion_disjoint(
+				struct sbi_domain_memregion* a,
+				struct sbi_domain_memregion* b)
+{
+	/* Bail out early to avoid overflow */
+	if (a->order >= __riscv_xlen || b->order >= __riscv_xlen)
+		return false;
+
+	if (a->base < b->base)
+		return b->base - a->base >= (1UL << a->order);
+	else
+		return a->base - b->base >= (1UL << b->order);
+}
+
 /** Representation of OpenSBI domain */
 struct sbi_domain {
 	/** Node in linked list of domains */
@@ -222,6 +242,9 @@ extern struct sbi_dlist domain_list;
 #define sbi_domain_for_each_memregion(__d, __r) \
 	for ((__r) = (__d)->regions; (__r)->order; (__r)++)
 
+#define sbi_domain_for_each_memregion_idx(__d, __r, __i) \
+	for ((__r) = (__d)->regions, (__i) = 0; (__r)->order; (__r)++, (__i)++)
+
 /**
  * Check whether given HART is assigned to specified domain
  * @param dom pointer to domain
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH v2 5/5] platform: generic: eswin: add EIC7700
  2025-11-17  5:48 [PATCH v2 0/5] Initial ESWIN/EIC7700 support Bo Gan
                   ` (3 preceding siblings ...)
  2025-11-17  5:48 ` [PATCH v2 4/5] include: sbi: Add helpers for sbi_domain_memregion Bo Gan
@ 2025-11-17  5:48 ` Bo Gan
  2025-11-17  8:04 ` [PATCH v2 0/5] Initial ESWIN/EIC7700 support Anup Patel
  5 siblings, 0 replies; 12+ messages in thread
From: Bo Gan @ 2025-11-17  5:48 UTC (permalink / raw)
  To: opensbi; +Cc: linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

Initial platform support for ESWIN Computing EIC7700 based on public SoC
datasheet[1] and tested on HiFive Premier P550. Vendor U-boot/Linux boots
fine, and I've tested Geekbench 6.5.0 Preview and got scores on par with
the vendor OpenSBI. System shutdown is not implemented due to the lack of
public doc regarding the UART communication protocol between the SoC and
the onboard BMC, which controls ATX power on HiFive P550. [2]

The files and functions are intentionally named as eic770x in many places
for future enhancements to support the 2 die version of the same SoC,
namely EIC7702, seen on DC-ROMA AI PC FML13V03 [3]. This patch set only
deals with the single die version, and the assumption is we can be either
die with id=0 or id=1, but there's only a single die in the system, or we
are only using a single die out of 2. However, the way the SoC handles 2-
die greatly affects how we configure it in a 1-die setup. EIC770X address
map has die 0/1 memory regions interleaved (see comments in eic770x.c).
If only 1 die is connected or active, it creates holes in the address map
for those regions corresponding to the remote die. Although not mentioned
in the datasheet, experiment shows speculative or HW prefetch can trigger
bus errors when touching those "holes". We need to use PMP to block the
access to those "holes", necessitating very tricky PMP configuration.

To make matters worse, EIC770X doesn't have cache coherent DMA, and due
to the fact that the P550 core lacks Svpbmt, the SoC maps main memory
twice as different regions in address map, so it can bypass cache and
fetch data directly from memory. In address space, we have two memory
regions, one as cached, the other as uncached. Thus, we also need an
extra PMP entry to protect OpenSBI blob from the uncached window. To do
this, we require FIRMWARE_PACKED_RXRW, otherwise, we'll run out of PMP
entries. Currently we barely made it to cover everything with the total
8 avilable PMP:

For root domain harts:

 PMP[0] NAPOT: Protect OpenSBI in cached memory window
 PMP[1] NAPOT: Protect OpenSBI in uncached memory window
 PMP[2] NAPOT: Protect CLINT
 PMP[3] NAPOT: Enable cached memory (die 1 only)
 PMP[4] NAPOT: Block P550 internal on remote die
 PMP[5-6] TOR: Block holes in memory port on remote die
 PMP[7] NAPOT: Allow everything

EIC770X also have several feature disable/enable CSRs accessible in M
mode. By default many core features such as speculation and HW prefetch
are disabled, and M mode software is responsible of enabling. Hence,
introduce 4 new build time tunable parameters to Kconfig, which reflects
the values get updated to those CSRs:
 - ESWIN_EIC770X_FEAT0_CFG
 - ESWIN_EIC770X_FEAT1_CFG
 - ESWIN_EIC770X_L1_HWPF_CFG
 - ESWIN_EIC770X_L2_HWPF_CFG

The default values are somewhat optimal for generic workloads. They are
dumped when running SiFive's vendor OpenSBI, and in addition, with my
own tuning to address the perf regression reported by drmpeg [4]

To build the firmware+u-boot blob, Use the following, and docs [5] for
testing it with UART boot without flashing:

make PLATFORM_DEFCONFIG=eic770x_defconfig \
     FW_TEXT_START=0x80000000 \
     FW_PAYLOAD_OFFSET=0x200000 \
     FW_PAYLOAD_PATH=u-boot-nodtb.bin \
     FW_PAYLOAD_FDT_ADDR=0xf8000000 \
     FW_FDT_PATH=u-boot.dtb

[1] https://github.com/eswincomputing/EIC7700X-SoC-Technical-Reference-Manual
[2] https://www.sifive.com/boards/hifive-premier-p550#documentation
[3] https://github.com/geerlingguy/sbc-reviews/issues/82
[4] https://forums.sifive.com/t/low-1-core-stream-bandwidth/7274/15
[5] https://github.com/ganboing/EIC770x-Docs/blob/main/p550/bootchain/UART-Boot.md

Signed-off-by: Bo Gan <ganboing@gmail.com>
---
 platform/generic/Kconfig                   |   6 +
 platform/generic/configs/eic770x_defconfig |  28 ++
 platform/generic/eswin/Kconfig             |  29 ++
 platform/generic/eswin/eic770x.c           | 361 +++++++++++++++++++++
 platform/generic/eswin/objects.mk          |  11 +
 platform/generic/include/eswin/eic770x.h   |  66 ++++
 6 files changed, 501 insertions(+)
 create mode 100644 platform/generic/configs/eic770x_defconfig
 create mode 100644 platform/generic/eswin/Kconfig
 create mode 100644 platform/generic/eswin/eic770x.c
 create mode 100644 platform/generic/eswin/objects.mk
 create mode 100644 platform/generic/include/eswin/eic770x.h

diff --git a/platform/generic/Kconfig b/platform/generic/Kconfig
index aedc59a5..adffd5c9 100644
--- a/platform/generic/Kconfig
+++ b/platform/generic/Kconfig
@@ -88,7 +88,13 @@ config PLATFORM_SPACEMIT_K1
 	select FDT_HSM_SPACEMIT
 	default n
 
+config PLATFORM_ESWIN_EIC770X
+	bool "ESWIN EIC770X support"
+	depends on FIRMWARE_PACKED_RXRW
+	default n
+
 source "$(OPENSBI_SRC_DIR)/platform/generic/andes/Kconfig"
+source "$(OPENSBI_SRC_DIR)/platform/generic/eswin/Kconfig"
 source "$(OPENSBI_SRC_DIR)/platform/generic/thead/Kconfig"
 
 endif
diff --git a/platform/generic/configs/eic770x_defconfig b/platform/generic/configs/eic770x_defconfig
new file mode 100644
index 00000000..f33778c9
--- /dev/null
+++ b/platform/generic/configs/eic770x_defconfig
@@ -0,0 +1,28 @@
+CONFIG_PLATFORM_ESWIN_EIC770X=y
+CONFIG_FIRMWARE_PACKED_RXRW=y
+CONFIG_FDT_CACHE=y
+CONFIG_FDT_CACHE_SIFIVE_CCACHE=y
+CONFIG_FDT_CPPC=y
+CONFIG_FDT_CPPC_RPMI=y
+CONFIG_FDT_HSM=y
+CONFIG_FDT_HSM_RPMI=y
+CONFIG_FDT_IPI=y
+CONFIG_FDT_IPI_MSWI=y
+CONFIG_FDT_IRQCHIP=y
+CONFIG_FDT_IRQCHIP_PLIC=y
+CONFIG_FDT_MAILBOX=y
+CONFIG_RPMI_MAILBOX=y
+CONFIG_FDT_MAILBOX_RPMI_SHMEM=y
+CONFIG_FDT_SERIAL=y
+CONFIG_FDT_SERIAL_UART8250=y
+CONFIG_FDT_SUSPEND=y
+CONFIG_FDT_SUSPEND_RPMI=y
+CONFIG_FDT_TIMER=y
+CONFIG_FDT_TIMER_MTIMER=y
+CONFIG_FDT_MPXY=y
+CONFIG_FDT_MPXY_RPMI_MBOX=y
+CONFIG_FDT_MPXY_RPMI_CLOCK=y
+CONFIG_FDT_MPXY_RPMI_VOLTAGE=y
+CONFIG_FDT_MPXY_RPMI_DEVICE_POWER=y
+CONFIG_FDT_MPXY_RPMI_PERFORMANCE=y
+CONFIG_FDT_MPXY_RPMI_SYSMSI=y
diff --git a/platform/generic/eswin/Kconfig b/platform/generic/eswin/Kconfig
new file mode 100644
index 00000000..84d0f43a
--- /dev/null
+++ b/platform/generic/eswin/Kconfig
@@ -0,0 +1,29 @@
+# SPDX-License-Identifier: BSD-2-Clause
+
+config ESWIN_EIC770X_FEAT0_CFG
+	int "ESWIN EIC7700X Feature Disable 0 CSR Configuration"
+	default 0x4000
+	help
+	 CSR Value to initialize EIC770X_FEAT0 (0x7c1) with.
+	 Refer to EIC770X SoC TRM for recommendations.
+
+config ESWIN_EIC770X_FEAT1_CFG
+	int "ESWIN EIC7700X Feature Disable 1 CSR Configuration"
+	default 0x80
+	help
+	 CSR Value to initialize EIC770X_FEAT1 (0x7c2) with.
+	 Refer to EIC770X SoC TRM for recommendations.
+
+config ESWIN_EIC770X_L1_HWPF_CFG
+	int "ESWIN EIC7700X L1 HW Prefetcher CSR Configuration"
+	default 0x1005c1be649
+	help
+	 CSR Value to initialize EIC770X_L1_HWPF (0x7c3) with.
+	 Refer to EIC770X SoC TRM for recommendations.
+
+config ESWIN_EIC770X_L2_HWPF_CFG
+	int "ESWIN EIC7700X L2 HW Prefetcher CSR Configuration"
+	default 0x929f
+	help
+	 CSR Value to initialize EIC770X_L2_HWPF (0x7c4) with.
+	 Refer to EIC770X SoC TRM for recommendations.
diff --git a/platform/generic/eswin/eic770x.c b/platform/generic/eswin/eic770x.c
new file mode 100644
index 00000000..be8894ad
--- /dev/null
+++ b/platform/generic/eswin/eic770x.c
@@ -0,0 +1,361 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Authors:
+ *   Bo Gan <ganboing@gmail.com>
+ *
+ */
+
+#include <platform_override.h>
+#include <sbi/riscv_io.h>
+#include <sbi/sbi_console.h>
+#include <sbi/sbi_system.h>
+#include <sbi/sbi_math.h>
+#include <eswin/eic770x.h>
+
+static int eic770x_system_reset_check(u32 type, u32 reason)
+{
+	switch (type) {
+	case SBI_SRST_RESET_TYPE_COLD_REBOOT:
+	case SBI_SRST_RESET_TYPE_WARM_REBOOT:
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+static void eic770x_system_reset(u32 type, u32 reason)
+{
+	switch (type) {
+	case SBI_SRST_RESET_TYPE_COLD_REBOOT:
+	case SBI_SRST_RESET_TYPE_WARM_REBOOT:
+		writel(EIC770X_SYSCRG_RST_VAL, (void *)EIC770X_SYSCRG_RST);
+	}
+
+	sbi_printf("%s: Unable to reset system\n", __func__);
+	sbi_hart_hang();
+}
+
+static struct sbi_system_reset_device eic770x_reset = {
+	.name = "eic770x_reset",
+	.system_reset_check = eic770x_system_reset_check,
+	.system_reset = eic770x_system_reset
+};
+
+static int eswin_eic7700_early_init(bool cold_boot)
+{
+	if (!cold_boot)
+		return generic_early_init(cold_boot);
+
+	/* Enable bus blocker */
+	writel(1, (void*)EIC770X_TL64D2D_OUT);
+	writel(1, (void*)EIC770X_TL256D2D_OUT);
+	writel(1, (void*)EIC770X_TL256D2D_IN);
+	asm volatile ("fence o, rw");
+
+	return generic_early_init(cold_boot);
+}
+
+/**
+ * EIC7700 special arrangement of PMP entries:
+ *
+ * We have to use extra PMPs to block Memory Port regions that doesn't
+ * belong to the current hart's die in order to prevent speculative
+ * accesses or HW prefetcher (perhaps?) from generating bus error:
+ *
+ * 	bus error of cause event: 9, accrued: 0x220,
+ *	physical address: 0x24ffffffa0
+ *
+ * The extra PMPs need LOCK bit to be set in order to be effective
+ * in M mode as well.
+ *
+ * We also have to to use 1 extra PMP to protect OpenSBI in uncached
+ * memory. EIC770X maps main memory (DRAM) twice -- one in memory
+ * port (cached), the other in system port (uncached). P550 doesn't
+ * support Svpbmt, so EIC770X use the uncached window to handle DMA
+ * that are cache incoherent -- pretty much all peripherals
+ *
+ * EIC770X memory port map:
+ * P550 Internal
+ *   ├─ 0x00_0000_0000 - 0x00_2000_0000 die 0 internal
+ *   └─ 0x00_2000_0000 - 0x00_4000_0000 die 1 internal
+ * System Port 0
+ *   ├─ 0x00_4000_0000 - 0x00_6000_0000 die 0 low MMIO
+ *   └─ 0x00_6000_0000 - 0x00_8000_0000 die 1 low MMIO
+ * Memory Port
+ *   ├─ 0x00_8000_0000 - 0x10_8000_0000 die 0 memory (cached)
+ *   ├─ 0x20_0000_0000 - 0x30_0000_0000 die 1 memory (cached)
+ *   └─ 0x40_0000_0000 - 0x60_0000_0000 interleaved memory (cached)
+ * System Port 1
+ *   ├─ 0x80_0000_0000 - 0xa0_0000_0000 die 0 high MMIO
+ *   ├─ 0xa0_0000_0000 - 0xc0_0000_0000 die 1 high MMIO
+ *   ├─ 0xc0_0000_0000 - 0xd0_0000_0000 die 0 memory (uncached)
+ *   ├─ 0xe0_0000_0000 - 0xf0_0000_0000 die 1 memory (uncached)
+ *   ├─ 0x100_0000_0000 - 0x120_0000_0000 interleaved memory (uncached)
+ *   └─ ...
+ *
+ * Refer to comment below for PMP entries setup in different scenarios:
+ */
+static int eswin_eic7700_final_init(bool cold_boot)
+{
+	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
+	struct sbi_domain_memregion fw_memreg;
+	int rc;
+
+	if (cold_boot)
+		sbi_system_reset_add_device(&eic770x_reset);
+
+	/**
+	 * Do generic_final_init stuff first, because it touchs FDT.
+	 * The PMPs blocking memory regions have LOCK bit set, so
+	 * before pmp_configure, we can't access memory outside of
+	 * [fw_start, fw_start + fw_size). The FDT could very well
+	 * reside outside of firmware region.
+	 */
+	rc = generic_final_init(cold_boot);
+	if (rc)
+		return rc;
+
+	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
+				  (SBI_DOMAIN_MEMREGION_M_READABLE |
+				   SBI_DOMAIN_MEMREGION_M_WRITABLE |
+				   SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
+				   SBI_DOMAIN_MEMREGION_FW), &fw_memreg);
+
+	pmp_set(0, 0, fw_memreg.base, fw_memreg.order);
+	pmp_set(1, 0, EIC770X_TO_UNCACHED(fw_memreg.base), fw_memreg.order);
+
+#define PMP_FREE_START 2
+#define PMP_FREE_COUNT 2
+#define PMP_LAST 7
+	if (current_hart_die()) {
+		/**
+		 * Die 1:
+		 *
+		 *  PMP[0] NAPOT: Protect OpenSBI in cached memory
+		 *  PMP[1] NAPOT: Protect OpenSBI in uncached memory
+		 *  PMP[2-3]    : Free
+		 *  PMP[4] NAPOT: Disallow die 0 P550 internal
+		 *  PMP[5-6] TOR: Disallow memory port
+		 *  PMP[7] NAPOT: Match all (optional)
+		 */
+		pmp_set(4, PMP_L, EIC770X_P550INT_BASE(0),
+		      log2roundup(EIC770X_P550INT_SIZE));
+		__pmp_set_tor(6, PMP_L, EIC770X_MEMPORT_BASE,
+					EIC770X_MEMPORT_SIZE);
+	} else {
+		/**
+		 * Die 0:
+		 *
+		 *  PMP[0] NAPOT: Protect OpenSBI in cached memory
+		 *  PMP[1] NAPOT: Protect OpenSBI in uncached memory
+		 *  PMP[2-3]    : Free
+		 *  PMP[4] NAPOT: Disallow die 1 P550 internal
+		 *  PMP[5-6] TOR: Disallow memory port except die 0 cached
+		 *  PMP[7] NAPOT: Match all (optional)
+		 */
+		pmp_set(4, PMP_L, EIC770X_P550INT_BASE(1),
+		      log2roundup(EIC770X_P550INT_SIZE));
+		__pmp_set_tor(6, PMP_L,
+			      EIC770X_D0_MEM_BASE + EIC770X_D0_MEM_SIZE,
+			      EIC770X_MEMPORT_SIZE - EIC770X_D0_MEM_SIZE);
+	}
+	/**
+	 * These must come after the setup of PMP, as we are about to
+	 * enable speculation and HW prefetcher bits
+	 */
+	csr_write(EIC770X_CSR_FEAT0, CONFIG_ESWIN_EIC770X_FEAT0_CFG);
+	csr_write(EIC770X_CSR_FEAT1, CONFIG_ESWIN_EIC770X_FEAT1_CFG);
+	csr_write(EIC770X_CSR_L1_HWPF, CONFIG_ESWIN_EIC770X_L1_HWPF_CFG);
+	csr_write(EIC770X_CSR_L2_HWPF, CONFIG_ESWIN_EIC770X_L2_HWPF_CFG);
+
+	return 0;
+}
+
+/**
+ * Check if memregion A supersedes B, or vice versa. It helps to save
+ * precious PMP entries. The policy is as follows (given A/B contains
+ * each other). If A is superset of B, then B is useless (PMP A will
+ * match before B) Else, B is a strict super set of A:
+ *
+ *  First normalize permissions on M/SU based on ENF_PERMISSIONS, then
+ *  check if both M and SU permission are exactly the same. Only then
+ *  can we say B supersedes A. We make one relaxation that we treat A
+ *  as having SU Executable (X) if A has SURW and B has SURWX. We
+ *  assume S-mode OS does its job by enforcing with page-tables.
+ *  This'll get us more savings.
+ *
+ * Returns: >0: A supersedes B, <0: B supersedes A, ==0: neither
+ */
+static int eswin_eic770x_memreg_superseded(struct sbi_domain_memregion *a,
+					    struct sbi_domain_memregion *b)
+{
+	unsigned long
+		mflags_a = (a->flags & SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS) ?
+			    a->flags & SBI_DOMAIN_MEMREGION_M_ACCESS_MASK :
+			    SBI_DOMAIN_MEMREGION_M_RWX,
+		mflags_b = (b->flags & SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS) ?
+			    b->flags & SBI_DOMAIN_MEMREGION_M_ACCESS_MASK :
+			    SBI_DOMAIN_MEMREGION_M_RWX,
+		suflags_a = a->flags & SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK,
+		suflags_b = b->flags & SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK;
+
+	if (a->order >= b->order)
+		return 1;
+
+	if (mflags_a != mflags_b)
+		return 0;
+
+	if (suflags_a == suflags_b ||
+	   (suflags_a | SBI_DOMAIN_MEMREGION_SU_EXECUTABLE) == suflags_b)
+		return -1;
+
+	return 0;
+}
+
+/* Returns a bitmask of memregions superseded and skipped */
+static uint64_t eswin_eic770x_memreg_consolidate(struct sbi_domain *dom)
+{
+	struct sbi_domain_memregion *rega, *regb;
+	uint64_t skipped = 0;
+	unsigned i, j;
+	int rc;
+
+	/* Ignore all FIRMWARE regions (handled separately) */
+	sbi_domain_for_each_memregion_idx(dom, rega, i) {
+		if (SBI_DOMAIN_MEMREGION_IS_FIRMWARE(rega->flags))
+			skipped |= BIT(i);
+	}
+
+	sbi_domain_for_each_memregion_idx(dom, rega, i) {
+		if (skipped & BIT(i))
+			continue;
+
+		sbi_domain_for_each_memregion_idx(dom, regb, j) {
+			if (j <= i || (skipped & BIT(j)))
+				continue;
+
+			if (sbi_domain_memregion_disjoint(rega, regb))
+				continue;
+			/**
+			 * Now A and B must fully contain each other
+			 * or they are exactly the same range. Check
+			 * if one can supersede the other. If not,
+			 * we must break futher processing. In essense
+			 * it's logically wrong to compare two regions
+			 * when there's a third region in between that
+			 * also covers part or all of either of them.
+			 */
+			rc = eswin_eic770x_memreg_superseded(rega, regb);
+			if (!rc)
+				break;
+
+			if (rc > 0) {
+				skipped |= BIT(j);
+				continue;
+			}
+			skipped |= BIT(i);
+			break;
+		}
+	}
+
+	return skipped;
+}
+
+static int eswin_eic7700_pmp_configure(unsigned int pmp_count,
+					unsigned int pmp_log2gran,
+					unsigned long pmp_addr_max)
+{
+	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
+	struct sbi_domain *dom = sbi_domain_thishart_ptr();
+	struct sbi_domain_memregion temp, *reg;
+	unsigned int pmp_idx = PMP_FREE_START,
+		     pmp_max = pmp_idx + PMP_FREE_COUNT;
+	unsigned int i;
+	uint64_t memreg_skipped;
+
+	/* Check if some memreg can be safely ignored */
+	memreg_skipped = eswin_eic770x_memreg_consolidate(dom);
+
+	/* Special handling of match all region [0, -1] if present */
+	sbi_domain_for_each_memregion_idx(dom, reg, i) {
+		if (memreg_skipped & BIT(i))
+			continue;
+
+		if (reg->order >= __riscv_xlen) {
+			/**
+			 * For die 1, when there is a match all region,
+			 * such as the [0, -1] in root domain, we need
+			 * to use PMP[3] with NAPOT to reenable die 1
+			 * cached memory from the memory port region we
+			 * blocked earlier. This is not required for die
+			 * 0, as we didn't block die 0 cached memory, as
+			 * it happens to be at the bottom of memory port,
+			 * thus excluded.
+			 */
+			if (current_hart_die())
+				pmp_max--;
+			break;
+		}
+	}
+
+	/* Actually set PMPs */
+	sbi_domain_for_each_memregion_idx(dom, reg, i) {
+		if (memreg_skipped & BIT(i))
+			continue;
+
+		if (reg->order < __riscv_xlen) {
+			if (pmp_idx >= pmp_max)
+				goto no_more_pmp;
+
+			sbi_hart_oldpmp_set(scratch, dom, reg, pmp_idx++,
+					    pmp_log2gran, pmp_addr_max);
+			continue;
+		}
+
+		if (current_hart_die()) {
+			sbi_domain_memregion_init(EIC770X_D1_MEM_BASE,
+						  EIC770X_D1_MEM_SIZE,
+						  reg->flags, &temp);
+			sbi_hart_oldpmp_set(scratch, dom, &temp, pmp_max,
+					    pmp_log2gran, pmp_addr_max);
+		}
+		sbi_hart_oldpmp_set(scratch, dom, reg, PMP_LAST,
+				    pmp_log2gran, pmp_addr_max);
+		break;
+	}
+
+	return 0;
+no_more_pmp:
+	sbi_printf("%s: insufficient PMP entries\n", __func__);
+	return SBI_EFAIL;
+}
+
+static void eswin_eic7700_pmp_unconfigure(void)
+{
+	for (unsigned i = 0; i < PMP_FREE_COUNT; i++)
+		pmp_disable(i + PMP_FREE_START);
+
+	pmp_disable(PMP_LAST);
+}
+
+static int eswin_eic7700_platform_init(const void *fdt, int nodeoff,
+					const struct fdt_match *match)
+{
+	generic_platform_ops.early_init = eswin_eic7700_early_init;
+	generic_platform_ops.final_init = eswin_eic7700_final_init;
+	generic_platform_ops.pmp_configure = eswin_eic7700_pmp_configure;
+	generic_platform_ops.pmp_unconfigure = eswin_eic7700_pmp_unconfigure;
+
+	return 0;
+}
+
+static const struct fdt_match eswin_eic7700_match[] = {
+	{ .compatible = "eswin,eic7700" },
+	{ },
+};
+
+const struct fdt_driver eswin_eic7700 = {
+	.match_table = eswin_eic7700_match,
+	.init = eswin_eic7700_platform_init,
+};
diff --git a/platform/generic/eswin/objects.mk b/platform/generic/eswin/objects.mk
new file mode 100644
index 00000000..6942a107
--- /dev/null
+++ b/platform/generic/eswin/objects.mk
@@ -0,0 +1,11 @@
+#
+# SPDX-License-Identifier: BSD-2-Clause
+#
+# Copyright (C) 2025 Bo Gan <ganboing@gmail.com>
+#
+
+carray-platform_override_modules-$(CONFIG_PLATFORM_ESWIN_EIC770X) += eswin_eic7700
+platform-objs-$(CONFIG_PLATFORM_ESWIN_EIC770X) += eswin/eic770x.o
+
+FW_PAYLOAD=y
+FW_PAYLOAD_OFFSET=0x200000
diff --git a/platform/generic/include/eswin/eic770x.h b/platform/generic/include/eswin/eic770x.h
new file mode 100644
index 00000000..4eb61546
--- /dev/null
+++ b/platform/generic/include/eswin/eic770x.h
@@ -0,0 +1,66 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2025 Bo Gan <ganboing@gmail.com>
+ *
+ */
+
+#ifndef __EIC770X_H__
+#define __EIC770X_H__
+
+/* CSRs */
+#define EIC770X_CSR_BRPREDICT	0x7c0
+#define EIC770X_CSR_FEAT0	0x7c1
+#define EIC770X_CSR_FEAT1	0x7c2
+#define EIC770X_CSR_L1_HWPF	0x7c3
+#define EIC770X_CSR_L2_HWPF	0x7c4
+
+/* Hart ID to core/die conversion */
+#define CPU_CORE_BITS		2
+#define CPU_CORE_MASK		((1 << CPU_CORE_BITS) - 1)
+#define CPU_DIE_SHIFT		CPU_CORE_BITS
+#define CPU_DIE_BITS		1
+#define CPU_DIE_MASK		((1 << CPU_DIE_SHIFT) - 1)
+
+#define hart_core(i)		((i) & CPU_CORE_MASK)
+#define hart_die(i)		(((i) >> CPU_DIE_SHIFT) & CPU_DIE_MASK)
+#define current_hart_core()	hart_core(current_hartid())
+#define current_hart_die()	hart_die(current_hartid())
+
+/* P550 Internal and System Port 0 */
+#define EIC770X_P550INT_SIZE	0x20000000UL
+#define EIC770X_P550INT_BASE(d)	(0UL + EIC770X_P550INT_SIZE * (d))
+#define EIC770X_P550INT_LOCAL	EIC770X_P550INT_BASE(current_hart_die())
+#define EIC770X_TL64D2D_OUT	(EIC770X_P550INT_LOCAL + 0x200000)
+#define EIC770X_TL256D2D_OUT	(EIC770X_P550INT_LOCAL + 0x202000)
+#define EIC770X_TL256D2D_IN	(EIC770X_P550INT_LOCAL + 0x204000)
+
+#define EIC770X_SYSPORT_SIZE	0x20000000UL
+#define EIC770X_SYSPORT_BASE(d)	(0x40000000UL + EIC770X_SYSPORT_SIZE * (d))
+#define EIC770X_SYSPORT_LOCAL	EIC770X_SYSPORT_BASE(current_hart_die())
+#define EIC770X_SYSCRG		(EIC770X_SYSPORT_LOCAL + 0x11828000UL)
+#define EIC770X_SYSCRG_RST	(EIC770X_SYSCRG + 0x300UL)
+#define EIC770X_SYSCRG_RST_VAL	0x1AC0FFE6UL
+
+/* Memory Ports */
+#define EIC770X_MEMPORT_BASE	0x0080000000UL // 2G
+#define EIC770X_MEMPORT_SIZE	0x7f80000000UL // +510G
+#define EIC770X_D0_MEM_BASE	0x0080000000UL // 2G
+#define EIC770X_D0_MEM_SIZE	0x0f80000000UL // +62G
+#define EIC770X_D1_MEM_BASE	0x2000000000UL // 128G
+#define EIC770X_D1_MEM_SIZE	0x1000000000UL // +64G
+#define EIC770X_CACHED_BASE	(current_hart_die() ? \
+				EIC770X_D1_MEM_BASE : \
+				EIC770X_D0_MEM_BASE)
+
+/* Uncached memory mapped in System Port 1 */
+#define EIC770X_D0_UC_BASE	0xc000000000UL
+#define EIC770X_D1_UC_BASE	0xe000000000UL
+#define EIC770X_UNCACHED_BASE	(current_hart_die() ? \
+				EIC770X_D1_UC_BASE : \
+				EIC770X_D0_UC_BASE)
+
+#define EIC770X_TO_UNCACHED(x)	((x) - EIC770X_CACHED_BASE + \
+				EIC770X_UNCACHED_BASE)
+
+#endif
-- 
2.34.1


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/5] Initial ESWIN/EIC7700 support
  2025-11-17  5:48 [PATCH v2 0/5] Initial ESWIN/EIC7700 support Bo Gan
                   ` (4 preceding siblings ...)
  2025-11-17  5:48 ` [PATCH v2 5/5] platform: generic: eswin: add EIC7700 Bo Gan
@ 2025-11-17  8:04 ` Anup Patel
  2025-11-17  9:29   ` Bo Gan
  5 siblings, 1 reply; 12+ messages in thread
From: Anup Patel @ 2025-11-17  8:04 UTC (permalink / raw)
  To: Bo Gan; +Cc: opensbi, linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

On Mon, Nov 17, 2025 at 11:20 AM Bo Gan <ganboing@gmail.com> wrote:
>
> EIC7700 is the SoC used in HiFive P550 and Milk-V Megrez. This SoC is
> currently one of the only off-the-shelf board/chips that support H
> extension, although it's v0.6.1. It also supports pre-ratified N-trace.
> Add support for it so people can benefit from latest OpenSBI features.
>
> The device-tree of HiFive P550 has been upstreamed to Linux:
> https://lore.kernel.org/all/20250825132427.1618089-1-pinkesh.vaghela@einfochips.com/
> However U-boot is not, and there are bugs in vendor U-boot device-tree,
> and also inconsistencies between the two. Thus, this patch is coded with
> the upstreamed device-tree as the reference, but tested with the patched
> vendor U-boot device tree as `FW_FDT_PATH`. The patched vendor U-boot is
> hosted here: https://github.com/ganboing/u-boot-eic7x/tree/eic7x-dt-fix
> Refer to PATCH 5/5 for the instructions on building the firmware blob
> and launch it through UART boot.
>
> The major complication of this chip is that it requires certain memory
> regions to be blocked with PMP entries to prevent speculative execution
> or HW prefetcher from touching them to avoid bus errors. Also due to the
> fact that this SoC handles cache incoherent DMA by mapping memory twice,
> one as cached, and the other as uncached, we also need an extra PMP to
> protect the OpenSBI in the uncached portion in address space. Following
> changes are made to lib/ and firmware/ to make it possible:
>
>  - Allow platform to override pmp_(un)configure
>  - Add helper function for settings PMP TOR
>  - Add helper function to check if memregions are disjoint
>  - Introduce FIRMWARE_PACKED_RXRW for disabling power-of-2 RW split

PMP TOR entries are not going to fly. Also, adding separate config
option FIRMWARE_PACKED_RXRW and separate defconfig for
EIC770X  means distros can't use the same fw_dyanmic.bin on
EIC770X  and other platforms.

If PMP TOR is being used only to save one PMP entry then there
are better ways to achieve this.

For example, you can do the following ...

diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
index d75c12de..9932fe0c 100644
--- a/include/sbi/sbi_platform.h
+++ b/include/sbi/sbi_platform.h
@@ -76,6 +76,9 @@ struct sbi_platform_operations {
     /* Check if specified HART is allowed to do cold boot */
     bool (*cold_boot_allowed)(u32 hartid);

+    /* Check if platform requires single firmware region */
+    bool (*single_fw_region)(void);
+
     /* Platform nascent initialization */
     int (*nascent_init)(void);

@@ -347,6 +350,23 @@ static inline bool sbi_platform_cold_boot_allowed(
     return true;
 }

+/**
+ * Check whether platform requires single firmware region
+ *
+ * Note: Single firmware region only works with legacy PMP because with
+ * Smepmp M-mode only regions can't have RWX permissions.
+ *
+ * @param plat pointer to struct sbi_platform
+ *
+ * @return true if single firmware region required and false otherwise
+ */
+static inline bool sbi_platform_single_fw_region(const struct
sbi_platform *plat)
+{
+    if (plat && sbi_platform_ops(plat)->single_fw_region)
+        return sbi_platform_ops(plat)->single_fw_region();
+    return false;
+}
+
 /**
  * Nascent (very early) initialization for current HART
  *
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index da0f0557..bf74ba67 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -904,18 +904,27 @@ int sbi_domain_init(struct sbi_scratch *scratch,
u32 cold_hartid)
     root.possible_harts = root_hmask;

     /* Root domain firmware memory region */
-    sbi_domain_memregion_init(scratch->fw_start, scratch->fw_rw_offset,
-                  (SBI_DOMAIN_MEMREGION_M_READABLE |
-                   SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
-                   SBI_DOMAIN_MEMREGION_FW),
-                  &root_memregs[root_memregs_count++]);
-
-    sbi_domain_memregion_init((scratch->fw_start + scratch->fw_rw_offset),
-                  (scratch->fw_size - scratch->fw_rw_offset),
-                  (SBI_DOMAIN_MEMREGION_M_READABLE |
-                   SBI_DOMAIN_MEMREGION_M_WRITABLE |
-                   SBI_DOMAIN_MEMREGION_FW),
-                  &root_memregs[root_memregs_count++]);
+    if (sbi_platform_single_fw_region(sbi_platform_ptr(scratch))) {
+        sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
+                      (SBI_DOMAIN_MEMREGION_M_READABLE |
+                       SBI_DOMAIN_MEMREGION_M_WRITABLE |
+                       SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
+                       SBI_DOMAIN_MEMREGION_FW),
+                      &root_memregs[root_memregs_count++]);
+    } else{
+        sbi_domain_memregion_init(scratch->fw_start, scratch->fw_rw_offset,
+                      (SBI_DOMAIN_MEMREGION_M_READABLE |
+                       SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
+                       SBI_DOMAIN_MEMREGION_FW),
+                      &root_memregs[root_memregs_count++]);
+
+        sbi_domain_memregion_init((scratch->fw_start + scratch->fw_rw_offset),
+                      (scratch->fw_size - scratch->fw_rw_offset),
+                      (SBI_DOMAIN_MEMREGION_M_READABLE |
+                       SBI_DOMAIN_MEMREGION_M_WRITABLE |
+                       SBI_DOMAIN_MEMREGION_FW),
+                      &root_memregs[root_memregs_count++]);
+    }

     root.fw_region_inited = true;

Regards,
Anup

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/5] Initial ESWIN/EIC7700 support
  2025-11-17  8:04 ` [PATCH v2 0/5] Initial ESWIN/EIC7700 support Anup Patel
@ 2025-11-17  9:29   ` Bo Gan
  2025-11-17 15:09     ` Anup Patel
  0 siblings, 1 reply; 12+ messages in thread
From: Bo Gan @ 2025-11-17  9:29 UTC (permalink / raw)
  To: Anup Patel; +Cc: opensbi, linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

Hi Anup,

Thanks for the prompt reply. See inline.

On 11/17/25 00:04, Anup Patel wrote:
> On Mon, Nov 17, 2025 at 11:20 AM Bo Gan <ganboing@gmail.com> wrote:
>>
>> EIC7700 is the SoC used in HiFive P550 and Milk-V Megrez. This SoC is
>> currently one of the only off-the-shelf board/chips that support H
>> extension, although it's v0.6.1. It also supports pre-ratified N-trace.
>> Add support for it so people can benefit from latest OpenSBI features.
>>
>> The device-tree of HiFive P550 has been upstreamed to Linux:
>> https://lore.kernel.org/all/20250825132427.1618089-1-pinkesh.vaghela@einfochips.com/
>> However U-boot is not, and there are bugs in vendor U-boot device-tree,
>> and also inconsistencies between the two. Thus, this patch is coded with
>> the upstreamed device-tree as the reference, but tested with the patched
>> vendor U-boot device tree as `FW_FDT_PATH`. The patched vendor U-boot is
>> hosted here: https://github.com/ganboing/u-boot-eic7x/tree/eic7x-dt-fix
>> Refer to PATCH 5/5 for the instructions on building the firmware blob
>> and launch it through UART boot.
>>
>> The major complication of this chip is that it requires certain memory
>> regions to be blocked with PMP entries to prevent speculative execution
>> or HW prefetcher from touching them to avoid bus errors. Also due to the
>> fact that this SoC handles cache incoherent DMA by mapping memory twice,
>> one as cached, and the other as uncached, we also need an extra PMP to
>> protect the OpenSBI in the uncached portion in address space. Following
>> changes are made to lib/ and firmware/ to make it possible:
>>
>>   - Allow platform to override pmp_(un)configure
>>   - Add helper function for settings PMP TOR
>>   - Add helper function to check if memregions are disjoint
>>   - Introduce FIRMWARE_PACKED_RXRW for disabling power-of-2 RW split
> 
> PMP TOR entries are not going to fly. Also, adding separate config
> option FIRMWARE_PACKED_RXRW and separate defconfig for
> EIC770X  means distros can't use the same fw_dyanmic.bin on
> EIC770X  and other platforms.
> 
> If PMP TOR is being used only to save one PMP entry then there
> are better ways to achieve this.
> 

Okay. I think I've found a way to use NAPOT to achieve the same effect.

Memory Port (die 0)
   ├─ 0x00_8000_0000 - 0x10_8000_0000 die 0 memory (cached)
   └─ 0x10_8000_0000 - 0x80_0000_0000 (hole)

Memory Port (die 1)
   ├─ 0x00_8000_0000 - 0x20_0000_0000 (hole)
   ├─ 0x20_0000_0000 - 0x30_0000_0000 die 1 memory (cached)
   └─ 0x30_0000_0000 - 0x80_0000_0000 (hole)

By default, for both die 0/1:

   NAPOT[0]   ......   MRWX,SU()  ____ OpenSBI in cached mem
   NAPOT[1]   ......   MRWX,SU()  ____ OpenSBI in uncached mem
   NAPOT[2-5]                     <free>
   NAPOT[6] [0x0, 0x80_0000_0000) L___
   NAPOT[7]       ......          <reserved>

For die 0 in root domain:

   NAPOT[0]       ......          M(RWX)SU() ____ OpenSBI in cached mem
   NAPOT[1]       ......          M(RWX)SU() ____ OpenSBI in uncached mem
   NAPOT[2]         [0x200_0000, 0x201_0000) ____ M(RW)SU() CLINT
   NAPOT[3]       [0x2000_0000, 0x4000_0000) L___ for die 1 p550
   NAPOT[4]            [0x0, 0x10_0000_0000) _RWX M() SU(RWX)
   NAPOT[5] [0x10_0000_0000, 0x10_8000_0000) _RWX M() SU(RWX)
   NAPOT[6]            [0x0, 0x80_0000_0000) L___
   NAPOT[7]        [0x0, 0xffffffffffffffff) _RWX M() SU(RWX)

Similarly, for die 1, use:


  NAPOT[0]       ......          M(RWX)SU() ____ OpenSBI in cached mem
  NAPOT[1]       ......          M(RWX)SU() ____ OpenSBI in uncached mem
  NAPOT[2]         [0x400_0000, 0x401_0000) ____ M(RW)SU() CLINT
  NAPOT[3]               [0x0, 0x2000_0000) L___ for die 0 p550
  NAPOT[4]               [0x0, 0x8000_0000) _RWX M() SU(RWX)
  NAPOT[5] [0x20_0000_0000, 0x30_8000_0000) _RWX M() SU(RWX)
  NAPOT[6]            [0x0, 0x80_0000_0000) L___
  NAPOT[7]        [0x0, 0xffffffffffffffff) _RWX M() SU(RWX)

It'll work also for non-root domain harts, and actually better if it
doesn't contain a "catch all" region (order == __riscv_xlen). However,
compared to the TOR approach, it consumes 1 more PMP on die 0 for root
domain or any domain with a "catch all" region, and we've used up all
entries. There's no room for more, so we can't do something like a
trusted/untrusted-domain scenario as documented in domain_support.md,
because we can't block the access of tdomain memory region in udomain.
I'm not sure if this is an issue at this point. Perhaps it could be
when someone tries out TEE? I don't have the answer today.

> For example, you can do the following ...
> 
> diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> index d75c12de..9932fe0c 100644
> --- a/include/sbi/sbi_platform.h
> +++ b/include/sbi/sbi_platform.h
> @@ -76,6 +76,9 @@ struct sbi_platform_operations {
>       /* Check if specified HART is allowed to do cold boot */
>       bool (*cold_boot_allowed)(u32 hartid);
> 
> +    /* Check if platform requires single firmware region */
> +    bool (*single_fw_region)(void);
> +

Sounds good. I'll make the change in next version.

Bo

>       /* Platform nascent initialization */
>       int (*nascent_init)(void);
> 
> @@ -347,6 +350,23 @@ static inline bool sbi_platform_cold_boot_allowed(
>       return true;
>   }
> 
> +/**
> + * Check whether platform requires single firmware region
> + *
> + * Note: Single firmware region only works with legacy PMP because with
> + * Smepmp M-mode only regions can't have RWX permissions.
> + *
> + * @param plat pointer to struct sbi_platform
> + *
> + * @return true if single firmware region required and false otherwise
> + */
> +static inline bool sbi_platform_single_fw_region(const struct
> sbi_platform *plat)
> +{
> +    if (plat && sbi_platform_ops(plat)->single_fw_region)
> +        return sbi_platform_ops(plat)->single_fw_region();
> +    return false;
> +}
> +
>   /**
>    * Nascent (very early) initialization for current HART
>    *
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index da0f0557..bf74ba67 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -904,18 +904,27 @@ int sbi_domain_init(struct sbi_scratch *scratch,
> u32 cold_hartid)
>       root.possible_harts = root_hmask;
> 
>       /* Root domain firmware memory region */
> -    sbi_domain_memregion_init(scratch->fw_start, scratch->fw_rw_offset,
> -                  (SBI_DOMAIN_MEMREGION_M_READABLE |
> -                   SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
> -                   SBI_DOMAIN_MEMREGION_FW),
> -                  &root_memregs[root_memregs_count++]);
> -
> -    sbi_domain_memregion_init((scratch->fw_start + scratch->fw_rw_offset),
> -                  (scratch->fw_size - scratch->fw_rw_offset),
> -                  (SBI_DOMAIN_MEMREGION_M_READABLE |
> -                   SBI_DOMAIN_MEMREGION_M_WRITABLE |
> -                   SBI_DOMAIN_MEMREGION_FW),
> -                  &root_memregs[root_memregs_count++]);
> +    if (sbi_platform_single_fw_region(sbi_platform_ptr(scratch))) {
> +        sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
> +                      (SBI_DOMAIN_MEMREGION_M_READABLE |
> +                       SBI_DOMAIN_MEMREGION_M_WRITABLE |
> +                       SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
> +                       SBI_DOMAIN_MEMREGION_FW),
> +                      &root_memregs[root_memregs_count++]);
> +    } else{
> +        sbi_domain_memregion_init(scratch->fw_start, scratch->fw_rw_offset,
> +                      (SBI_DOMAIN_MEMREGION_M_READABLE |
> +                       SBI_DOMAIN_MEMREGION_M_EXECUTABLE |
> +                       SBI_DOMAIN_MEMREGION_FW),
> +                      &root_memregs[root_memregs_count++]);
> +
> +        sbi_domain_memregion_init((scratch->fw_start + scratch->fw_rw_offset),
> +                      (scratch->fw_size - scratch->fw_rw_offset),
> +                      (SBI_DOMAIN_MEMREGION_M_READABLE |
> +                       SBI_DOMAIN_MEMREGION_M_WRITABLE |
> +                       SBI_DOMAIN_MEMREGION_FW),
> +                      &root_memregs[root_memregs_count++]);
> +    }
> 
>       root.fw_region_inited = true;
> 
> Regards,
> Anup


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/5] Initial ESWIN/EIC7700 support
  2025-11-17  9:29   ` Bo Gan
@ 2025-11-17 15:09     ` Anup Patel
  2025-11-18  7:03       ` Bo Gan
  0 siblings, 1 reply; 12+ messages in thread
From: Anup Patel @ 2025-11-17 15:09 UTC (permalink / raw)
  To: Bo Gan; +Cc: opensbi, linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

Hi Bo Gan,

On Mon, Nov 17, 2025 at 3:00 PM Bo Gan <ganboing@gmail.com> wrote:
>
> Hi Anup,
>
> Thanks for the prompt reply. See inline.
>
> On 11/17/25 00:04, Anup Patel wrote:
> > On Mon, Nov 17, 2025 at 11:20 AM Bo Gan <ganboing@gmail.com> wrote:
> >>
> >> EIC7700 is the SoC used in HiFive P550 and Milk-V Megrez. This SoC is
> >> currently one of the only off-the-shelf board/chips that support H
> >> extension, although it's v0.6.1. It also supports pre-ratified N-trace.
> >> Add support for it so people can benefit from latest OpenSBI features.
> >>
> >> The device-tree of HiFive P550 has been upstreamed to Linux:
> >> https://lore.kernel.org/all/20250825132427.1618089-1-pinkesh.vaghela@einfochips.com/
> >> However U-boot is not, and there are bugs in vendor U-boot device-tree,
> >> and also inconsistencies between the two. Thus, this patch is coded with
> >> the upstreamed device-tree as the reference, but tested with the patched
> >> vendor U-boot device tree as `FW_FDT_PATH`. The patched vendor U-boot is
> >> hosted here: https://github.com/ganboing/u-boot-eic7x/tree/eic7x-dt-fix
> >> Refer to PATCH 5/5 for the instructions on building the firmware blob
> >> and launch it through UART boot.
> >>
> >> The major complication of this chip is that it requires certain memory
> >> regions to be blocked with PMP entries to prevent speculative execution
> >> or HW prefetcher from touching them to avoid bus errors. Also due to the
> >> fact that this SoC handles cache incoherent DMA by mapping memory twice,
> >> one as cached, and the other as uncached, we also need an extra PMP to
> >> protect the OpenSBI in the uncached portion in address space. Following
> >> changes are made to lib/ and firmware/ to make it possible:
> >>
> >>   - Allow platform to override pmp_(un)configure
> >>   - Add helper function for settings PMP TOR
> >>   - Add helper function to check if memregions are disjoint
> >>   - Introduce FIRMWARE_PACKED_RXRW for disabling power-of-2 RW split
> >
> > PMP TOR entries are not going to fly. Also, adding separate config
> > option FIRMWARE_PACKED_RXRW and separate defconfig for
> > EIC770X  means distros can't use the same fw_dyanmic.bin on
> > EIC770X  and other platforms.
> >
> > If PMP TOR is being used only to save one PMP entry then there
> > are better ways to achieve this.
> >
>
> Okay. I think I've found a way to use NAPOT to achieve the same effect.
>
> Memory Port (die 0)
>    ├─ 0x00_8000_0000 - 0x10_8000_0000 die 0 memory (cached)
>    └─ 0x10_8000_0000 - 0x80_0000_0000 (hole)
>
> Memory Port (die 1)
>    ├─ 0x00_8000_0000 - 0x20_0000_0000 (hole)
>    ├─ 0x20_0000_0000 - 0x30_0000_0000 die 1 memory (cached)
>    └─ 0x30_0000_0000 - 0x80_0000_0000 (hole)
>
> By default, for both die 0/1:
>
>    NAPOT[0]   ......   MRWX,SU()  ____ OpenSBI in cached mem
>    NAPOT[1]   ......   MRWX,SU()  ____ OpenSBI in uncached mem
>    NAPOT[2-5]                     <free>
>    NAPOT[6] [0x0, 0x80_0000_0000) L___
>    NAPOT[7]       ......          <reserved>
>
> For die 0 in root domain:
>
>    NAPOT[0]       ......          M(RWX)SU() ____ OpenSBI in cached mem
>    NAPOT[1]       ......          M(RWX)SU() ____ OpenSBI in uncached mem
>    NAPOT[2]         [0x200_0000, 0x201_0000) ____ M(RW)SU() CLINT
>    NAPOT[3]       [0x2000_0000, 0x4000_0000) L___ for die 1 p550
>    NAPOT[4]            [0x0, 0x10_0000_0000) _RWX M() SU(RWX)
>    NAPOT[5] [0x10_0000_0000, 0x10_8000_0000) _RWX M() SU(RWX)
>    NAPOT[6]            [0x0, 0x80_0000_0000) L___
>    NAPOT[7]        [0x0, 0xffffffffffffffff) _RWX M() SU(RWX)
>
> Similarly, for die 1, use:
>
>
>   NAPOT[0]       ......          M(RWX)SU() ____ OpenSBI in cached mem
>   NAPOT[1]       ......          M(RWX)SU() ____ OpenSBI in uncached mem
>   NAPOT[2]         [0x400_0000, 0x401_0000) ____ M(RW)SU() CLINT
>   NAPOT[3]               [0x0, 0x2000_0000) L___ for die 0 p550
>   NAPOT[4]               [0x0, 0x8000_0000) _RWX M() SU(RWX)
>   NAPOT[5] [0x20_0000_0000, 0x30_8000_0000) _RWX M() SU(RWX)
>   NAPOT[6]            [0x0, 0x80_0000_0000) L___
>   NAPOT[7]        [0x0, 0xffffffffffffffff) _RWX M() SU(RWX)
>
> It'll work also for non-root domain harts, and actually better if it
> doesn't contain a "catch all" region (order == __riscv_xlen). However,
> compared to the TOR approach, it consumes 1 more PMP on die 0 for root
> domain or any domain with a "catch all" region, and we've used up all
> entries. There's no room for more, so we can't do something like a
> trusted/untrusted-domain scenario as documented in domain_support.md,
> because we can't block the access of tdomain memory region in udomain.
> I'm not sure if this is an issue at this point. Perhaps it could be
> when someone tries out TEE? I don't have the answer today.

There is one more approach which can save PMP entries for
you where you can create a large region in platform early init
covering multiple regions added by OpenSBI drivers.

For example, you can create a single region for all M-mode
only MMIO devices if platform address space allows you.

Regards,
Anup

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/5] Initial ESWIN/EIC7700 support
  2025-11-17 15:09     ` Anup Patel
@ 2025-11-18  7:03       ` Bo Gan
  2025-11-18 17:23         ` Anup Patel
  0 siblings, 1 reply; 12+ messages in thread
From: Bo Gan @ 2025-11-18  7:03 UTC (permalink / raw)
  To: Anup Patel, Bo Gan
  Cc: opensbi, linmin, pinkesh.vaghela, gaohan, samuel, wangxiang

Hi Anup,

On 11/17/25 07:09, Anup Patel wrote:
> Hi Bo Gan,
> 
> On Mon, Nov 17, 2025 at 3:00 PM Bo Gan <ganboing@gmail.com> wrote:
>>
>> Hi Anup,
>>
>> Thanks for the prompt reply. See inline.
>>
>> On 11/17/25 00:04, Anup Patel wrote:
>>> On Mon, Nov 17, 2025 at 11:20 AM Bo Gan <ganboing@gmail.com> wrote:
>>>>
>>>> EIC7700 is the SoC used in HiFive P550 and Milk-V Megrez. This SoC is
>>>> currently one of the only off-the-shelf board/chips that support H
>>>> extension, although it's v0.6.1. It also supports pre-ratified N-trace.
>>>> Add support for it so people can benefit from latest OpenSBI features.
>>>>
>>>> The device-tree of HiFive P550 has been upstreamed to Linux:
>>>> https://lore.kernel.org/all/20250825132427.1618089-1-pinkesh.vaghela@einfochips.com/
>>>> However U-boot is not, and there are bugs in vendor U-boot device-tree,
>>>> and also inconsistencies between the two. Thus, this patch is coded with
>>>> the upstreamed device-tree as the reference, but tested with the patched
>>>> vendor U-boot device tree as `FW_FDT_PATH`. The patched vendor U-boot is
>>>> hosted here: https://github.com/ganboing/u-boot-eic7x/tree/eic7x-dt-fix
>>>> Refer to PATCH 5/5 for the instructions on building the firmware blob
>>>> and launch it through UART boot.
>>>>
>>>> The major complication of this chip is that it requires certain memory
>>>> regions to be blocked with PMP entries to prevent speculative execution
>>>> or HW prefetcher from touching them to avoid bus errors. Also due to the
>>>> fact that this SoC handles cache incoherent DMA by mapping memory twice,
>>>> one as cached, and the other as uncached, we also need an extra PMP to
>>>> protect the OpenSBI in the uncached portion in address space. Following
>>>> changes are made to lib/ and firmware/ to make it possible:
>>>>
>>>>    - Allow platform to override pmp_(un)configure
>>>>    - Add helper function for settings PMP TOR
>>>>    - Add helper function to check if memregions are disjoint
>>>>    - Introduce FIRMWARE_PACKED_RXRW for disabling power-of-2 RW split
>>>
>>> PMP TOR entries are not going to fly. Also, adding separate config
>>> option FIRMWARE_PACKED_RXRW and separate defconfig for
>>> EIC770X  means distros can't use the same fw_dyanmic.bin on
>>> EIC770X  and other platforms.
>>>
>>> If PMP TOR is being used only to save one PMP entry then there
>>> are better ways to achieve this.
>>>
>>
>> Okay. I think I've found a way to use NAPOT to achieve the same effect.
>>
>> Memory Port (die 0)
>>     ├─ 0x00_8000_0000 - 0x10_8000_0000 die 0 memory (cached)
>>     └─ 0x10_8000_0000 - 0x80_0000_0000 (hole)
>>
>> Memory Port (die 1)
>>     ├─ 0x00_8000_0000 - 0x20_0000_0000 (hole)
>>     ├─ 0x20_0000_0000 - 0x30_0000_0000 die 1 memory (cached)
>>     └─ 0x30_0000_0000 - 0x80_0000_0000 (hole)
>>
>> By default, for both die 0/1:
>>
>>     NAPOT[0]   ......   MRWX,SU()  ____ OpenSBI in cached mem
>>     NAPOT[1]   ......   MRWX,SU()  ____ OpenSBI in uncached mem
>>     NAPOT[2-5]                     <free>
>>     NAPOT[6] [0x0, 0x80_0000_0000) L___
>>     NAPOT[7]       ......          <reserved>
>>
>> For die 0 in root domain:
>>
>>     NAPOT[0]       ......          M(RWX)SU() ____ OpenSBI in cached mem
>>     NAPOT[1]       ......          M(RWX)SU() ____ OpenSBI in uncached mem
>>     NAPOT[2]         [0x200_0000, 0x201_0000) ____ M(RW)SU() CLINT
>>     NAPOT[3]       [0x2000_0000, 0x4000_0000) L___ for die 1 p550
>>     NAPOT[4]            [0x0, 0x10_0000_0000) _RWX M() SU(RWX)
>>     NAPOT[5] [0x10_0000_0000, 0x10_8000_0000) _RWX M() SU(RWX)
>>     NAPOT[6]            [0x0, 0x80_0000_0000) L___
>>     NAPOT[7]        [0x0, 0xffffffffffffffff) _RWX M() SU(RWX)
>>
>> Similarly, for die 1, use:
>>
>>
>>    NAPOT[0]       ......          M(RWX)SU() ____ OpenSBI in cached mem
>>    NAPOT[1]       ......          M(RWX)SU() ____ OpenSBI in uncached mem
>>    NAPOT[2]         [0x400_0000, 0x401_0000) ____ M(RW)SU() CLINT
>>    NAPOT[3]               [0x0, 0x2000_0000) L___ for die 0 p550
>>    NAPOT[4]               [0x0, 0x8000_0000) _RWX M() SU(RWX)
>>    NAPOT[5] [0x20_0000_0000, 0x30_8000_0000) _RWX M() SU(RWX)
>>    NAPOT[6]            [0x0, 0x80_0000_0000) L___
>>    NAPOT[7]        [0x0, 0xffffffffffffffff) _RWX M() SU(RWX)
>>
>> It'll work also for non-root domain harts, and actually better if it
>> doesn't contain a "catch all" region (order == __riscv_xlen). However,
>> compared to the TOR approach, it consumes 1 more PMP on die 0 for root
>> domain or any domain with a "catch all" region, and we've used up all
>> entries. There's no room for more, so we can't do something like a
>> trusted/untrusted-domain scenario as documented in domain_support.md,
>> because we can't block the access of tdomain memory region in udomain.
>> I'm not sure if this is an issue at this point. Perhaps it could be
>> when someone tries out TEE? I don't have the answer today.
> 
> There is one more approach which can save PMP entries for
> you where you can create a large region in platform early init
> covering multiple regions added by OpenSBI drivers.
> 
> For example, you can create a single region for all M-mode
> only MMIO devices if platform address space allows you.
> 

Unfortunately, the address space layout makes it impossible. we need to
block CLINT from S/U, but allow the Trace components, PLIC and L3 Cache
Controller from S/U.

Die 0 P550 internal region (quoting from datasheet):
R:Read, W:Write, X:Exec, I:Inst Cacheable, D:Data Cacheable, A:Atomics

0x00000000 - 0x00000FFF Debug
0x00001000 - 0x00003FFF RWX  A Error Device
0x00004000 - 0x00004FFF RW   A Test Indicator
0x00018000 - 0x00018FFF RWX  A Trace Funnel
0x00100000 - 0x00100FFF RWX  A Trace Encoder 0
0x00101000 - 0x00101FFF RWX  A Trace Encoder 1
0x00102000 - 0x00102FFF RWX  A Trace Encoder 2
0x00103000 - 0x00103FFF RWX  A Trace Encoder 3
0x00104000 - 0x00107FFF RWX  A Hart 0 Private L2 Cache
0x00108000 - 0x0010BFFF RWX  A Hart 1 Private L2 Cache
0x0010C000 - 0x0010FFFF RWX  A Hart 2 Private L2 Cache
0x00110000 - 0x00113FFF RWX  A Hart 3 Private L2 Cache
0x00200000 - 0x00200FFF RW   A Bus blocker TL64D2D_Out
0x00202000 - 0x00202FFF RW   A Bus blocker TL256D2D_Out
0x00204000 - 0x00204FFF RW   A Bus blocker TL256D2D_In
0x01700000 - 0x01700FFF RWX  A Hart 0 Bus-Error Unit
0x01701000 - 0x01701FFF RWX  A Hart 1 Bus-Error Unit
0x01702000 - 0x01702FFF RWX  A Hart 2 Bus-Error Unit
0x01703000 - 0x01703FFF RWX  A Hart 3 Bus-Error Unit
0x02000000 - 0x0200FFFF RWX  A CLINT
0x02010000 - 0x02013FFF RWX  A L3 Cache Controller
0x08000000 - 0x083FFFFF RWX  A L3 LIM
0x0C000000 - 0x0FFFFFFF RWX  A PLIC
0x10010000 - 0x10010FFF RWX  A Burst Bundler on Front Port
0x10030000 - 0x10033FFF RWX  A TL-UH to TL-C Adapter
0x1A000000 - 0x1A3FFFFF RWXIDA L3 Zero Device

I re-read the EIC7700 datasheet, and I finally got a theory on the reason
behind bus errors. There are 3 regions having data cacheable attribute in
the chip hard-coded PMA:

- [0x00_1a00_0000, 0x00_1a40_0000) -- L3 zero device for die 0
- [0x00_3a00_0000, 0x00_3a40_0000) -- L3 zero device for die 1
- [0x00_8000_0000, 0x80_0000_0000) -- entire memory port

They can be speculatively accessed or HW prefetched, so if there's only
1 die and there're holes in these regions, you'll get bus error from
time to time if the core happens to touch it. Thus, whatever belongs to
the other (remote) die needs to be blocked. Hopefully this only applies
for data cacheable regions, otherwise, I'd say it's horribly broken. I
already dislike the fact that we have to use PMP to do it, but it's what
it is. In the PMP configuration above, I chose to block die 1 L3 zero
device from die 0 by disallowing the entire internal port for die 1. Or
I can do it with finer granularity. Either way, it can't be omitted.

What I can do is configure the PMPs with NAPOTs as you suggested. It'll
be like the detailed list above. This should cover 99% of use cases. For
users with a udomain-tdomain like setup, they can still try it, but put
a note in the comment and say due to HW limitation, it's experimental
only and you can't properly protect tdomain from udomain.

Does it sound good?

Bo

> Regards,
> Anup


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/5] Initial ESWIN/EIC7700 support
  2025-11-18  7:03       ` Bo Gan
@ 2025-11-18 17:23         ` Anup Patel
  2025-11-20  9:39           ` Bo Gan
  0 siblings, 1 reply; 12+ messages in thread
From: Anup Patel @ 2025-11-18 17:23 UTC (permalink / raw)
  To: Bo Gan
  Cc: Anup Patel, opensbi, linmin, pinkesh.vaghela, gaohan, samuel,
	wangxiang

On Tue, Nov 18, 2025 at 12:35 PM Bo Gan <ganboing@gmail.com> wrote:
>
> Hi Anup,
>
> On 11/17/25 07:09, Anup Patel wrote:
> > Hi Bo Gan,
> >
> > On Mon, Nov 17, 2025 at 3:00 PM Bo Gan <ganboing@gmail.com> wrote:
> >>
> >> Hi Anup,
> >>
> >> Thanks for the prompt reply. See inline.
> >>
> >> On 11/17/25 00:04, Anup Patel wrote:
> >>> On Mon, Nov 17, 2025 at 11:20 AM Bo Gan <ganboing@gmail.com> wrote:
> >>>>
> >>>> EIC7700 is the SoC used in HiFive P550 and Milk-V Megrez. This SoC is
> >>>> currently one of the only off-the-shelf board/chips that support H
> >>>> extension, although it's v0.6.1. It also supports pre-ratified N-trace.
> >>>> Add support for it so people can benefit from latest OpenSBI features.
> >>>>
> >>>> The device-tree of HiFive P550 has been upstreamed to Linux:
> >>>> https://lore.kernel.org/all/20250825132427.1618089-1-pinkesh.vaghela@einfochips.com/
> >>>> However U-boot is not, and there are bugs in vendor U-boot device-tree,
> >>>> and also inconsistencies between the two. Thus, this patch is coded with
> >>>> the upstreamed device-tree as the reference, but tested with the patched
> >>>> vendor U-boot device tree as `FW_FDT_PATH`. The patched vendor U-boot is
> >>>> hosted here: https://github.com/ganboing/u-boot-eic7x/tree/eic7x-dt-fix
> >>>> Refer to PATCH 5/5 for the instructions on building the firmware blob
> >>>> and launch it through UART boot.
> >>>>
> >>>> The major complication of this chip is that it requires certain memory
> >>>> regions to be blocked with PMP entries to prevent speculative execution
> >>>> or HW prefetcher from touching them to avoid bus errors. Also due to the
> >>>> fact that this SoC handles cache incoherent DMA by mapping memory twice,
> >>>> one as cached, and the other as uncached, we also need an extra PMP to
> >>>> protect the OpenSBI in the uncached portion in address space. Following
> >>>> changes are made to lib/ and firmware/ to make it possible:
> >>>>
> >>>>    - Allow platform to override pmp_(un)configure
> >>>>    - Add helper function for settings PMP TOR
> >>>>    - Add helper function to check if memregions are disjoint
> >>>>    - Introduce FIRMWARE_PACKED_RXRW for disabling power-of-2 RW split
> >>>
> >>> PMP TOR entries are not going to fly. Also, adding separate config
> >>> option FIRMWARE_PACKED_RXRW and separate defconfig for
> >>> EIC770X  means distros can't use the same fw_dyanmic.bin on
> >>> EIC770X  and other platforms.
> >>>
> >>> If PMP TOR is being used only to save one PMP entry then there
> >>> are better ways to achieve this.
> >>>
> >>
> >> Okay. I think I've found a way to use NAPOT to achieve the same effect.
> >>
> >> Memory Port (die 0)
> >>     ├─ 0x00_8000_0000 - 0x10_8000_0000 die 0 memory (cached)
> >>     └─ 0x10_8000_0000 - 0x80_0000_0000 (hole)
> >>
> >> Memory Port (die 1)
> >>     ├─ 0x00_8000_0000 - 0x20_0000_0000 (hole)
> >>     ├─ 0x20_0000_0000 - 0x30_0000_0000 die 1 memory (cached)
> >>     └─ 0x30_0000_0000 - 0x80_0000_0000 (hole)
> >>
> >> By default, for both die 0/1:
> >>
> >>     NAPOT[0]   ......   MRWX,SU()  ____ OpenSBI in cached mem
> >>     NAPOT[1]   ......   MRWX,SU()  ____ OpenSBI in uncached mem
> >>     NAPOT[2-5]                     <free>
> >>     NAPOT[6] [0x0, 0x80_0000_0000) L___
> >>     NAPOT[7]       ......          <reserved>
> >>
> >> For die 0 in root domain:
> >>
> >>     NAPOT[0]       ......          M(RWX)SU() ____ OpenSBI in cached mem
> >>     NAPOT[1]       ......          M(RWX)SU() ____ OpenSBI in uncached mem
> >>     NAPOT[2]         [0x200_0000, 0x201_0000) ____ M(RW)SU() CLINT
> >>     NAPOT[3]       [0x2000_0000, 0x4000_0000) L___ for die 1 p550
> >>     NAPOT[4]            [0x0, 0x10_0000_0000) _RWX M() SU(RWX)
> >>     NAPOT[5] [0x10_0000_0000, 0x10_8000_0000) _RWX M() SU(RWX)
> >>     NAPOT[6]            [0x0, 0x80_0000_0000) L___
> >>     NAPOT[7]        [0x0, 0xffffffffffffffff) _RWX M() SU(RWX)
> >>
> >> Similarly, for die 1, use:
> >>
> >>
> >>    NAPOT[0]       ......          M(RWX)SU() ____ OpenSBI in cached mem
> >>    NAPOT[1]       ......          M(RWX)SU() ____ OpenSBI in uncached mem
> >>    NAPOT[2]         [0x400_0000, 0x401_0000) ____ M(RW)SU() CLINT
> >>    NAPOT[3]               [0x0, 0x2000_0000) L___ for die 0 p550
> >>    NAPOT[4]               [0x0, 0x8000_0000) _RWX M() SU(RWX)
> >>    NAPOT[5] [0x20_0000_0000, 0x30_8000_0000) _RWX M() SU(RWX)
> >>    NAPOT[6]            [0x0, 0x80_0000_0000) L___
> >>    NAPOT[7]        [0x0, 0xffffffffffffffff) _RWX M() SU(RWX)
> >>
> >> It'll work also for non-root domain harts, and actually better if it
> >> doesn't contain a "catch all" region (order == __riscv_xlen). However,
> >> compared to the TOR approach, it consumes 1 more PMP on die 0 for root
> >> domain or any domain with a "catch all" region, and we've used up all
> >> entries. There's no room for more, so we can't do something like a
> >> trusted/untrusted-domain scenario as documented in domain_support.md,
> >> because we can't block the access of tdomain memory region in udomain.
> >> I'm not sure if this is an issue at this point. Perhaps it could be
> >> when someone tries out TEE? I don't have the answer today.
> >
> > There is one more approach which can save PMP entries for
> > you where you can create a large region in platform early init
> > covering multiple regions added by OpenSBI drivers.
> >
> > For example, you can create a single region for all M-mode
> > only MMIO devices if platform address space allows you.
> >
>
> Unfortunately, the address space layout makes it impossible. we need to
> block CLINT from S/U, but allow the Trace components, PLIC and L3 Cache
> Controller from S/U.
>
> Die 0 P550 internal region (quoting from datasheet):
> R:Read, W:Write, X:Exec, I:Inst Cacheable, D:Data Cacheable, A:Atomics
>
> 0x00000000 - 0x00000FFF Debug
> 0x00001000 - 0x00003FFF RWX  A Error Device
> 0x00004000 - 0x00004FFF RW   A Test Indicator
> 0x00018000 - 0x00018FFF RWX  A Trace Funnel
> 0x00100000 - 0x00100FFF RWX  A Trace Encoder 0
> 0x00101000 - 0x00101FFF RWX  A Trace Encoder 1
> 0x00102000 - 0x00102FFF RWX  A Trace Encoder 2
> 0x00103000 - 0x00103FFF RWX  A Trace Encoder 3
> 0x00104000 - 0x00107FFF RWX  A Hart 0 Private L2 Cache
> 0x00108000 - 0x0010BFFF RWX  A Hart 1 Private L2 Cache
> 0x0010C000 - 0x0010FFFF RWX  A Hart 2 Private L2 Cache
> 0x00110000 - 0x00113FFF RWX  A Hart 3 Private L2 Cache
> 0x00200000 - 0x00200FFF RW   A Bus blocker TL64D2D_Out
> 0x00202000 - 0x00202FFF RW   A Bus blocker TL256D2D_Out
> 0x00204000 - 0x00204FFF RW   A Bus blocker TL256D2D_In
> 0x01700000 - 0x01700FFF RWX  A Hart 0 Bus-Error Unit
> 0x01701000 - 0x01701FFF RWX  A Hart 1 Bus-Error Unit
> 0x01702000 - 0x01702FFF RWX  A Hart 2 Bus-Error Unit
> 0x01703000 - 0x01703FFF RWX  A Hart 3 Bus-Error Unit
> 0x02000000 - 0x0200FFFF RWX  A CLINT
> 0x02010000 - 0x02013FFF RWX  A L3 Cache Controller
> 0x08000000 - 0x083FFFFF RWX  A L3 LIM
> 0x0C000000 - 0x0FFFFFFF RWX  A PLIC
> 0x10010000 - 0x10010FFF RWX  A Burst Bundler on Front Port
> 0x10030000 - 0x10033FFF RWX  A TL-UH to TL-C Adapter
> 0x1A000000 - 0x1A3FFFFF RWXIDA L3 Zero Device
>
> I re-read the EIC7700 datasheet, and I finally got a theory on the reason
> behind bus errors. There are 3 regions having data cacheable attribute in
> the chip hard-coded PMA:
>
> - [0x00_1a00_0000, 0x00_1a40_0000) -- L3 zero device for die 0
> - [0x00_3a00_0000, 0x00_3a40_0000) -- L3 zero device for die 1
> - [0x00_8000_0000, 0x80_0000_0000) -- entire memory port
>
> They can be speculatively accessed or HW prefetched, so if there's only
> 1 die and there're holes in these regions, you'll get bus error from
> time to time if the core happens to touch it. Thus, whatever belongs to
> the other (remote) die needs to be blocked. Hopefully this only applies
> for data cacheable regions, otherwise, I'd say it's horribly broken. I
> already dislike the fact that we have to use PMP to do it, but it's what
> it is. In the PMP configuration above, I chose to block die 1 L3 zero
> device from die 0 by disallowing the entire internal port for die 1. Or
> I can do it with finer granularity. Either way, it can't be omitted.

PMP is a scarce resource across platforms and using it for
work-arounds is an expensive affair.

>
> What I can do is configure the PMPs with NAPOTs as you suggested. It'll
> be like the detailed list above. This should cover 99% of use cases. For
> users with a udomain-tdomain like setup, they can still try it, but put
> a note in the comment and say due to HW limitation, it's experimental
> only and you can't properly protect tdomain from udomain.
>
> Does it sound good?
>

Yes, that should be fine.

Regards,
Anup

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/5] Initial ESWIN/EIC7700 support
  2025-11-18 17:23         ` Anup Patel
@ 2025-11-20  9:39           ` Bo Gan
  0 siblings, 0 replies; 12+ messages in thread
From: Bo Gan @ 2025-11-20  9:39 UTC (permalink / raw)
  To: Anup Patel
  Cc: Anup Patel, opensbi, linmin, pinkesh.vaghela, gaohan, samuel,
	wangxiang

Hi Anup,

Actually I managed to do a tdomain/udomain like setup on die 0. There's
a free PMP entry there. I've also simplified the PMP consolidation logic
and refactored the comments to make it more readable. Please see v3.

Thanks!
Bo

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-11-20  9:41 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-17  5:48 [PATCH v2 0/5] Initial ESWIN/EIC7700 support Bo Gan
2025-11-17  5:48 ` [PATCH v2 1/5] lib: sbi: allow platform to override PMP (un)configuration Bo Gan
2025-11-17  5:48 ` [PATCH v2 2/5] lib: sbi: Add __pmp_set_tor for setting TOR regions Bo Gan
2025-11-17  5:48 ` [PATCH v2 3/5] firmware: add CONFIG_FIRMWARE_PACKED_RXRW Bo Gan
2025-11-17  5:48 ` [PATCH v2 4/5] include: sbi: Add helpers for sbi_domain_memregion Bo Gan
2025-11-17  5:48 ` [PATCH v2 5/5] platform: generic: eswin: add EIC7700 Bo Gan
2025-11-17  8:04 ` [PATCH v2 0/5] Initial ESWIN/EIC7700 support Anup Patel
2025-11-17  9:29   ` Bo Gan
2025-11-17 15:09     ` Anup Patel
2025-11-18  7:03       ` Bo Gan
2025-11-18 17:23         ` Anup Patel
2025-11-20  9:39           ` Bo Gan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox