[PATCH 0/7] Protect M-mode only MMIO devices

[PATCH 0/7] Protect M-mode only MMIO devices

[Anup Patel]

We should protect the M-mode only MMIO devices (such as CLINT) using
the OpenSBI domain support. This can be easily achieved by having APIs
to allow platform support (and drivers) to add memregions for the root
domain.

This series extends OpenSBI domain support to protect M-mode only
devices.

These patches can be found in domain_root_region_v1 branch at:
https://github.com/avpatel/opensbi

Anup Patel (7):
  lib: sbi: Domains can be registered only before finalizing domains
  lib: sbi: Add sbi_domain_memregion_init() API
  lib: sbi: Add sbi_domain_root_add_memregion() API
  lib: utils/sys: Add CLINT memregion in the root domain
  lib: sbi: Make the root domain instance global variable
  lib: utils: Copy over restricted root domain memregions to FDT domains
  lib: sbi: Make sbi_domain_memregion_initfw() a local function

 include/sbi/sbi_domain.h   |  29 ++++++++-
 lib/sbi/sbi_domain.c       | 118 ++++++++++++++++++++++++++++++-------
 lib/utils/fdt/fdt_domain.c |  26 ++++++--
 lib/utils/sys/clint.c      |  17 +++++-
 4 files changed, 160 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH 1/7] lib: sbi: Domains can be registered only before finalizing domains

[Anup Patel]

The domains are boot-time system-level partitions so we should
allow platform support to register domains only before hart
domain assignments are finalized.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 lib/sbi/sbi_domain.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 195c941..2849241 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -19,8 +19,8 @@
 
 struct sbi_domain *hartid_to_domain_table[SBI_HARTMASK_MAX_BITS] = { 0 };
 struct sbi_domain *domidx_to_domain_table[SBI_DOMAIN_MAX_INDEX] = { 0 };
-
 static u32 domain_count = 0;
+static bool domain_finalized = false;
 
 static struct sbi_hartmask root_hmask = { 0 };
 
@@ -376,7 +376,8 @@ int sbi_domain_register(struct sbi_domain *dom,
 	u32 cold_hartid = current_hartid();
 	const struct sbi_platform *plat = sbi_platform_thishart_ptr();
 
-	if (!dom || !assign_mask)
+	/* Sanity checks */
+	if (!dom || !assign_mask || domain_finalized)
 		return SBI_EINVAL;
 
 	/* Check if domain already discovered */
@@ -490,6 +491,12 @@ int sbi_domain_finalize(struct sbi_scratch *scratch, u32 cold_hartid)
 		}
 	}
 
+	/*
+	 * Set the finalized flag so that the root domain
+	 * regions can't be changed.
+	 */
+	domain_finalized = true;
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH 2/7] lib: sbi: Add sbi_domain_memregion_init() API

[Anup Patel]

This patch adds sbi_domain_memregion_init() helper API which can
be used by platform support to initialize a domain memory region
before adding it to the root domain.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 include/sbi/sbi_domain.h | 14 +++++++++++++
 lib/sbi/sbi_domain.c     | 45 ++++++++++++++++++++++++++++++++--------
 2 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index 1f8b942..f9f4f7d 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -127,6 +127,20 @@ ulong sbi_domain_get_assigned_hartmask(const struct sbi_domain *dom,
 /** Initialize a domain memory region as firmware region */
 void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg);
 
+/**
+ * Initialize a domain memory region based on it's physical
+ * address and size.
+ *
+ * @param addr start physical address of memory region
+ * @param size physical size of memory region
+ * @param flags memory region flags
+ * @param reg pointer to memory region being initialized
+ */
+void sbi_domain_memregion_init(unsigned long addr,
+				unsigned long size,
+				unsigned long flags,
+				struct sbi_domain_memregion *reg);
+
 /**
  * Check whether we can access specified address for given mode and
  * memory region flags under a domain
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 2849241..164f35c 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -72,6 +72,35 @@ void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg)
 	sbi_memcpy(reg, &root_memregs[ROOT_FW_REGION], sizeof(*reg));
 }
 
+void sbi_domain_memregion_init(unsigned long addr,
+				unsigned long size,
+				unsigned long flags,
+				struct sbi_domain_memregion *reg)
+{
+	unsigned long base = 0, order;
+
+	for (order = log2roundup(size) ; order <= __riscv_xlen; order++) {
+		if (order < __riscv_xlen) {
+			base = addr & ~((1UL << order) - 1UL);
+			if ((base <= addr) &&
+			    (addr < (base + (1UL << order))) &&
+			    (base <= (addr + size - 1UL)) &&
+			    ((addr + size - 1UL) < (base + (1UL << order))))
+				break;
+		} else {
+			base = 0;
+			break;
+		}
+
+	}
+
+	if (reg) {
+		reg->base = base;
+		reg->order = order;
+		reg->flags = flags;
+	}
+}
+
 bool sbi_domain_check_addr(const struct sbi_domain *dom,
 			   unsigned long addr, unsigned long mode,
 			   unsigned long access_flags)
@@ -507,17 +536,15 @@ int sbi_domain_init(struct sbi_scratch *scratch, u32 cold_hartid)
 	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
 
 	/* Root domain firmware memory region */
-	root_memregs[ROOT_FW_REGION].order = log2roundup(scratch->fw_size);
-	root_memregs[ROOT_FW_REGION].base = scratch->fw_start &
-				~((1UL << root_memregs[0].order) - 1UL);
-	root_memregs[ROOT_FW_REGION].flags = 0;
+	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size, 0,
+				  &root_memregs[ROOT_FW_REGION]);
 
 	/* Root domain allow everything memory region */
-	root_memregs[ROOT_ALL_REGION].order = __riscv_xlen;
-	root_memregs[ROOT_ALL_REGION].base = 0;
-	root_memregs[ROOT_ALL_REGION].flags = (SBI_DOMAIN_MEMREGION_READABLE |
-						SBI_DOMAIN_MEMREGION_WRITEABLE |
-						SBI_DOMAIN_MEMREGION_EXECUTABLE);
+	sbi_domain_memregion_init(0, ~0UL,
+				  (SBI_DOMAIN_MEMREGION_READABLE |
+				   SBI_DOMAIN_MEMREGION_WRITEABLE |
+				   SBI_DOMAIN_MEMREGION_EXECUTABLE),
+				  &root_memregs[ROOT_ALL_REGION]);
 
 	/* Root domain memory region end */
 	root_memregs[ROOT_END_REGION].order = 0;
-- 
2.25.1

[PATCH 3/7] lib: sbi: Add sbi_domain_root_add_memregion() API

[Anup Patel]

We should allow platform support to add more root memory regions
before domains are finalized. This will help platform support to
protect critical M-mode only resources.

This patch adds sbi_domain_root_add_memregion() API for above
described purpose.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 include/sbi/sbi_domain.h | 11 +++++++
 lib/sbi/sbi_domain.c     | 62 +++++++++++++++++++++++++++++++++-------
 2 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index f9f4f7d..b5f2f38 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -170,6 +170,17 @@ void sbi_domain_dump_all(const char *suffix);
 int sbi_domain_register(struct sbi_domain *dom,
 			const struct sbi_hartmask *assign_mask);
 
+/**
+ * Add a memory region to the root domain
+ * @param reg pointer to the memory region to be added
+ * @param skip_conflict do nothing if a conflicting region is already
+ * present in the root domain
+ *
+ * @return 0 on success and negative error code on failure
+ */
+int sbi_domain_root_add_memregion(const struct sbi_domain_memregion *reg,
+				   bool skip_conflict);
+
 /** Finalize domain tables and startup non-root domains */
 int sbi_domain_finalize(struct sbi_scratch *scratch, u32 cold_hartid);
 
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 164f35c..8afdb5d 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -24,10 +24,10 @@ static bool domain_finalized = false;
 
 static struct sbi_hartmask root_hmask = { 0 };
 
-#define ROOT_FW_REGION		0
-#define ROOT_ALL_REGION	1
-#define ROOT_END_REGION	2
-static struct sbi_domain_memregion root_memregs[ROOT_END_REGION + 1] = { 0 };
+#define ROOT_REGION_MAX	16
+static u32 root_memregs_count = 0;
+static struct sbi_domain_memregion root_fw_region;
+static struct sbi_domain_memregion root_memregs[ROOT_REGION_MAX + 1] = { 0 };
 
 static struct sbi_domain root = {
 	.name = "root",
@@ -69,7 +69,7 @@ void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg)
 	if (!reg)
 		return;
 
-	sbi_memcpy(reg, &root_memregs[ROOT_FW_REGION], sizeof(*reg));
+	sbi_memcpy(reg, &root_fw_region, sizeof(*reg));
 }
 
 void sbi_domain_memregion_init(unsigned long addr,
@@ -236,9 +236,9 @@ static int sanitize_domain(const struct sbi_platform *plat,
 	count = 0;
 	have_fw_reg = FALSE;
 	sbi_domain_for_each_memregion(dom, reg) {
-		if (reg->order == root_memregs[ROOT_FW_REGION].order &&
-		    reg->base == root_memregs[ROOT_FW_REGION].base &&
-		    reg->flags == root_memregs[ROOT_FW_REGION].flags)
+		if (reg->order == root_fw_region.order &&
+		    reg->base == root_fw_region.base &&
+		    reg->flags == root_fw_region.flags)
 			have_fw_reg = TRUE;
 		count++;
 	}
@@ -468,6 +468,45 @@ int sbi_domain_register(struct sbi_domain *dom,
 	return 0;
 }
 
+int sbi_domain_root_add_memregion(const struct sbi_domain_memregion *reg,
+				   bool skip_conflict)
+{
+	int rc;
+	struct sbi_domain_memregion *nreg;
+	const struct sbi_platform *plat = sbi_platform_thishart_ptr();
+
+	/* Sanity checks */
+	if (!reg || domain_finalized ||
+	    (root.regions != root_memregs) ||
+	    (ROOT_REGION_MAX <= root_memregs_count))
+		return SBI_EINVAL;
+
+	/* Check for conflicts */
+	sbi_domain_for_each_memregion(&root, nreg) {
+		if (is_region_conflict(reg, nreg))
+			return (skip_conflict) ? 0 : SBI_EINVAL;
+	}
+
+	/* Append the region to root regions */
+	nreg = &root_memregs[root_memregs_count];
+	sbi_memcpy(nreg, reg, sizeof(*reg));
+	root_memregs_count++;
+	root_memregs[root_memregs_count].order = 0;
+
+	/* Sanitize the root domain so that root regions are sorted */
+	rc = sanitize_domain(plat, &root);
+	if (rc) {
+		sbi_printf("%s: sanity checks failed for"
+			   " %s (error %d)\n", __func__,
+			   root.name, rc);
+		nreg->order = 0;
+		root_memregs_count--;
+		return rc;
+	}
+
+	return 0;
+}
+
 int sbi_domain_finalize(struct sbi_scratch *scratch, u32 cold_hartid)
 {
 	int rc;
@@ -537,17 +576,18 @@ int sbi_domain_init(struct sbi_scratch *scratch, u32 cold_hartid)
 
 	/* Root domain firmware memory region */
 	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size, 0,
-				  &root_memregs[ROOT_FW_REGION]);
+				  &root_fw_region);
+	sbi_domain_memregion_initfw(&root_memregs[root_memregs_count++]);
 
 	/* Root domain allow everything memory region */
 	sbi_domain_memregion_init(0, ~0UL,
 				  (SBI_DOMAIN_MEMREGION_READABLE |
 				   SBI_DOMAIN_MEMREGION_WRITEABLE |
 				   SBI_DOMAIN_MEMREGION_EXECUTABLE),
-				  &root_memregs[ROOT_ALL_REGION]);
+				  &root_memregs[root_memregs_count++]);
 
 	/* Root domain memory region end */
-	root_memregs[ROOT_END_REGION].order = 0;
+	root_memregs[root_memregs_count].order = 0;
 
 	/* Use platform specific root memory regions when available */
 	memregs = sbi_platform_domains_root_regions(plat);
-- 
2.25.1

[PATCH 4/7] lib: utils/sys: Add CLINT memregion in the root domain

[Anup Patel]

The CLINT memory should not be accessed by the supervisor-mode
software so let's protect it by adding CLINT memregion to the
root domain.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 lib/utils/sys/clint.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/utils/sys/clint.c b/lib/utils/sys/clint.c
index 7a392aa..fe98cc4 100644
--- a/lib/utils/sys/clint.c
+++ b/lib/utils/sys/clint.c
@@ -10,6 +10,7 @@
 #include <sbi/riscv_asm.h>
 #include <sbi/riscv_atomic.h>
 #include <sbi/riscv_io.h>
+#include <sbi/sbi_domain.h>
 #include <sbi/sbi_error.h>
 #include <sbi/sbi_hartmask.h>
 #include <sbi_utils/sys/clint.h>
@@ -17,9 +18,19 @@
 #define CLINT_IPI_OFF		0
 #define CLINT_TIME_CMP_OFF	0x4000
 #define CLINT_TIME_VAL_OFF	0xbff8
+#define CLINT_SIZE		0xc000
 
 static struct clint_data *clint_ipi_hartid2data[SBI_HARTMASK_MAX_BITS];
 
+static int clint_add_root_memregion(struct clint_data *clint)
+{
+	struct sbi_domain_memregion reg;
+
+	sbi_domain_memregion_init(clint->addr, CLINT_SIZE,
+				  SBI_DOMAIN_MEMREGION_MMIO, &reg);
+	return sbi_domain_root_add_memregion(&reg, true);
+}
+
 void clint_ipi_send(u32 target_hart)
 {
 	struct clint_data *clint;
@@ -70,7 +81,8 @@ int clint_cold_ipi_init(struct clint_data *clint)
 	for (i = 0; i < clint->hart_count; i++)
 		clint_ipi_hartid2data[clint->first_hartid + i] = clint;
 
-	return 0;
+	/* Add CLINT region to the root domain */
+	return clint_add_root_memregion(clint);
 }
 
 static struct clint_data *clint_timer_hartid2data[SBI_HARTMASK_MAX_BITS];
@@ -199,5 +211,6 @@ int clint_cold_timer_init(struct clint_data *clint,
 	for (i = 0; i < clint->hart_count; i++)
 		clint_timer_hartid2data[clint->first_hartid + i] = clint;
 
-	return 0;
+	/* Add CLINT region to the root domain */
+	return clint_add_root_memregion(clint);
 }
-- 
2.25.1

[PATCH 5/7] lib: sbi: Make the root domain instance global variable

[Anup Patel]

We make the the root domain instance global variable so that
platform support and drivers can iterate over the root domain
regions.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 include/sbi/sbi_domain.h | 3 +++
 lib/sbi/sbi_domain.c     | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index b5f2f38..ef53ca5 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -80,6 +80,9 @@ struct sbi_domain {
 	bool system_reset_allowed;
 };
 
+/** The root domain instance */
+extern struct sbi_domain root;
+
 /** HART id to domain table */
 extern struct sbi_domain *hartid_to_domain_table[];
 
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 8afdb5d..5e3a30d 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -29,7 +29,7 @@ static u32 root_memregs_count = 0;
 static struct sbi_domain_memregion root_fw_region;
 static struct sbi_domain_memregion root_memregs[ROOT_REGION_MAX + 1] = { 0 };
 
-static struct sbi_domain root = {
+struct sbi_domain root = {
 	.name = "root",
 	.possible_harts = &root_hmask,
 	.regions = root_memregs,
-- 
2.25.1

[PATCH 6/7] lib: utils: Copy over restricted root domain memregions to FDT domains

[Anup Patel]

We should copy over all restricted memregions from the root domain
to the domains populated from FDT. These restricted root memregions
are typically firmware memregion and M-mode only mmio memregions.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 lib/utils/fdt/fdt_domain.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 09615e5..95c195d 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -222,7 +222,7 @@ static u32 fdt_domains_count;
 static struct sbi_domain fdt_domains[FDT_DOMAIN_MAX_COUNT];
 static struct sbi_hartmask fdt_masks[FDT_DOMAIN_MAX_COUNT];
 static struct sbi_domain_memregion
-	fdt_regions[FDT_DOMAIN_MAX_COUNT][FDT_DOMAIN_REGION_MAX_COUNT + 2];
+	fdt_regions[FDT_DOMAIN_MAX_COUNT][FDT_DOMAIN_REGION_MAX_COUNT + 1];
 
 static int __fdt_parse_region(void *fdt, int domain_offset,
 			      int region_offset, u32 region_access,
@@ -276,7 +276,7 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque)
 	struct sbi_hartmask *mask;
 	struct sbi_hartmask assign_mask;
 	int *cold_domain_offset = opaque;
-	struct sbi_domain_memregion *regions;
+	struct sbi_domain_memregion *reg, *regions;
 	int i, err, len, cpus_offset, cpu_offset, doffset;
 
 	/* Sanity check on maximum domains we can handle */
@@ -314,13 +314,31 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque)
 	/* Setup memregions from DT */
 	val32 = 0;
 	sbi_memset(regions, 0,
-		   sizeof(*regions) * (FDT_DOMAIN_REGION_MAX_COUNT + 2));
+		   sizeof(*regions) * (FDT_DOMAIN_REGION_MAX_COUNT + 1));
 	dom->regions = regions;
 	err = fdt_iterate_each_memregion(fdt, domain_offset, &val32,
 					 __fdt_parse_region);
 	if (err)
 		return err;
-	sbi_domain_memregion_initfw(&regions[val32]);
+
+	/*
+	 * Copy over root domain memregions which don't allow
+	 * read, write and execute from lower privilege modes.
+	 *
+	 * These root domain memregions without read, write,
+	 * and execute permissions include:
+	 * 1) firmware region protecting the firmware memory
+	 * 2) mmio regions protecting M-mode only mmio devices
+	 */
+	sbi_domain_for_each_memregion(&root, reg) {
+		if ((reg->flags & SBI_DOMAIN_MEMREGION_READABLE) ||
+		    (reg->flags & SBI_DOMAIN_MEMREGION_WRITEABLE) ||
+		    (reg->flags & SBI_DOMAIN_MEMREGION_EXECUTABLE))
+			continue;
+		if (FDT_DOMAIN_REGION_MAX_COUNT <= val32)
+			return SBI_EINVAL;
+		sbi_memcpy(&regions[val32++], reg, sizeof(*reg));
+	}
 
 	/* Read "boot-hart" DT property */
 	val32 = -1U;
-- 
2.25.1

[PATCH 7/7] lib: sbi: Make sbi_domain_memregion_initfw() a local function

[Anup Patel]

The sbi_domain_memregion_initfw() is no longer used outside
sbi_domain.c so let's make it a local function.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
---
 include/sbi/sbi_domain.h | 3 ---
 lib/sbi/sbi_domain.c     | 4 ++--
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index ef53ca5..3de5e46 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -127,9 +127,6 @@ bool sbi_domain_is_assigned_hart(const struct sbi_domain *dom, u32 hartid);
 ulong sbi_domain_get_assigned_hartmask(const struct sbi_domain *dom,
 				       ulong hbase);
 
-/** Initialize a domain memory region as firmware region */
-void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg);
-
 /**
  * Initialize a domain memory region based on it's physical
  * address and size.
diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 5e3a30d..069e2a7 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -64,7 +64,7 @@ ulong sbi_domain_get_assigned_hartmask(const struct sbi_domain *dom,
 	return ret;
 }
 
-void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg)
+static void domain_memregion_initfw(struct sbi_domain_memregion *reg)
 {
 	if (!reg)
 		return;
@@ -577,7 +577,7 @@ int sbi_domain_init(struct sbi_scratch *scratch, u32 cold_hartid)
 	/* Root domain firmware memory region */
 	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size, 0,
 				  &root_fw_region);
-	sbi_domain_memregion_initfw(&root_memregs[root_memregs_count++]);
+	domain_memregion_initfw(&root_memregs[root_memregs_count++]);
 
 	/* Root domain allow everything memory region */
 	sbi_domain_memregion_init(0, ~0UL,
-- 
2.25.1

[PATCH 1/7] lib: sbi: Domains can be registered only before finalizing domains

[Xiang W]

? 2021-04-10?? 12:48 +0530?Anup Patel???
> The domains are boot-time system-level partitions so we should
> allow platform support to register domains only before hart
> domain assignments are finalized.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
looks good to me.

Reviewed-by: Xiang W <wxjstz@126.com>

Regards,
Xiang W
> ---
>  lib/sbi/sbi_domain.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 195c941..2849241 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -19,8 +19,8 @@
>  
>  struct sbi_domain *hartid_to_domain_table[SBI_HARTMASK_MAX_BITS] = {
> 0 };
>  struct sbi_domain *domidx_to_domain_table[SBI_DOMAIN_MAX_INDEX] = {
> 0 };
> -
>  static u32 domain_count = 0;
> +static bool domain_finalized = false;
>  
>  static struct sbi_hartmask root_hmask = { 0 };
>  
> @@ -376,7 +376,8 @@ int sbi_domain_register(struct sbi_domain *dom,
>  	u32 cold_hartid = current_hartid();
>  	const struct sbi_platform *plat = sbi_platform_thishart_ptr();
>  
> -	if (!dom || !assign_mask)
> +	/* Sanity checks */
> +	if (!dom || !assign_mask || domain_finalized)
>  		return SBI_EINVAL;
>  
>  	/* Check if domain already discovered */
> @@ -490,6 +491,12 @@ int sbi_domain_finalize(struct sbi_scratch
> *scratch, u32 cold_hartid)
>  		}
>  	}
>  
> +	/*
> +	 * Set the finalized flag so that the root domain
> +	 * regions can't be changed.
> +	 */
> +	domain_finalized = true;
> +
>  	return 0;
>  }
>  
> -- 
> 2.25.1
> 
> 

[PATCH 2/7] lib: sbi: Add sbi_domain_memregion_init() API

[Xiang W]

? 2021-04-10?? 12:48 +0530?Anup Patel???
> This patch adds sbi_domain_memregion_init() helper API which can
> be used by platform support to initialize a domain memory region
> before adding it to the root domain.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
looks good to me.

Reviewed-by: Xiang W <wxjstz@126.com>

Regards,
Xiang W
> ---
>  include/sbi/sbi_domain.h | 14 +++++++++++++
>  lib/sbi/sbi_domain.c     | 45 ++++++++++++++++++++++++++++++++----
> ----
>  2 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index 1f8b942..f9f4f7d 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -127,6 +127,20 @@ ulong sbi_domain_get_assigned_hartmask(const
> struct sbi_domain *dom,
>  /** Initialize a domain memory region as firmware region */
>  void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg);
>  
> +/**
> + * Initialize a domain memory region based on it's physical
> + * address and size.
> + *
> + * @param addr start physical address of memory region
> + * @param size physical size of memory region
> + * @param flags memory region flags
> + * @param reg pointer to memory region being initialized
> + */
> +void sbi_domain_memregion_init(unsigned long addr,
> +				unsigned long size,
> +				unsigned long flags,
> +				struct sbi_domain_memregion *reg);
> +
>  /**
>   * Check whether we can access specified address for given mode and
>   * memory region flags under a domain
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 2849241..164f35c 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -72,6 +72,35 @@ void sbi_domain_memregion_initfw(struct
> sbi_domain_memregion *reg)
>  	sbi_memcpy(reg, &root_memregs[ROOT_FW_REGION], sizeof(*reg));
>  }
>  
> +void sbi_domain_memregion_init(unsigned long addr,
> +				unsigned long size,
> +				unsigned long flags,
> +				struct sbi_domain_memregion *reg)
> +{
> +	unsigned long base = 0, order;
> +
> +	for (order = log2roundup(size) ; order <= __riscv_xlen;
> order++) {
> +		if (order < __riscv_xlen) {
> +			base = addr & ~((1UL << order) - 1UL);
> +			if ((base <= addr) &&
> +			    (addr < (base + (1UL << order))) &&
> +			    (base <= (addr + size - 1UL)) &&
> +			    ((addr + size - 1UL) < (base + (1UL <<
> order))))
> +				break;
> +		} else {
> +			base = 0;
> +			break;
> +		}
> +
> +	}
> +
> +	if (reg) {
> +		reg->base = base;
> +		reg->order = order;
> +		reg->flags = flags;
> +	}
> +}
> +
>  bool sbi_domain_check_addr(const struct sbi_domain *dom,
>  			   unsigned long addr, unsigned long mode,
>  			   unsigned long access_flags)
> @@ -507,17 +536,15 @@ int sbi_domain_init(struct sbi_scratch
> *scratch, u32 cold_hartid)
>  	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>  
>  	/* Root domain firmware memory region */
> -	root_memregs[ROOT_FW_REGION].order = log2roundup(scratch-
> >fw_size);
> -	root_memregs[ROOT_FW_REGION].base = scratch->fw_start &
> -				~((1UL << root_memregs[0].order) -
> 1UL);
> -	root_memregs[ROOT_FW_REGION].flags = 0;
> +	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
> 0,
> +				  &root_memregs[ROOT_FW_REGION]);
>  
>  	/* Root domain allow everything memory region */
> -	root_memregs[ROOT_ALL_REGION].order = __riscv_xlen;
> -	root_memregs[ROOT_ALL_REGION].base = 0;
> -	root_memregs[ROOT_ALL_REGION].flags =
> (SBI_DOMAIN_MEMREGION_READABLE |
> -						SBI_DOMAIN_MEMREGION_WR
> ITEABLE |
> -						SBI_DOMAIN_MEMREGION_EX
> ECUTABLE);
> +	sbi_domain_memregion_init(0, ~0UL,
> +				  (SBI_DOMAIN_MEMREGION_READABLE |
> +				   SBI_DOMAIN_MEMREGION_WRITEABLE |
> +				   SBI_DOMAIN_MEMREGION_EXECUTABLE),
> +				  &root_memregs[ROOT_ALL_REGION]);
>  
>  	/* Root domain memory region end */
>  	root_memregs[ROOT_END_REGION].order = 0;
> -- 
> 2.25.1
> 
> 

[PATCH 3/7] lib: sbi: Add sbi_domain_root_add_memregion() API

[Xiang W]

? 2021-04-10?? 12:48 +0530?Anup Patel???
> We should allow platform support to add more root memory regions
> before domains are finalized. This will help platform support to
> protect critical M-mode only resources.
> 
> This patch adds sbi_domain_root_add_memregion() API for above
> described purpose.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
looks good to me.

Reviewed-by: Xiang W <wxjstz@126.com>

Regards,
Xiang W
> ---
>  include/sbi/sbi_domain.h | 11 +++++++
>  lib/sbi/sbi_domain.c     | 62 +++++++++++++++++++++++++++++++++-----
> --
>  2 files changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index f9f4f7d..b5f2f38 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -170,6 +170,17 @@ void sbi_domain_dump_all(const char *suffix);
>  int sbi_domain_register(struct sbi_domain *dom,
>  			const struct sbi_hartmask *assign_mask);
>  
> +/**
> + * Add a memory region to the root domain
> + * @param reg pointer to the memory region to be added
> + * @param skip_conflict do nothing if a conflicting region is
> already
> + * present in the root domain
> + *
> + * @return 0 on success and negative error code on failure
> + */
> +int sbi_domain_root_add_memregion(const struct sbi_domain_memregion
> *reg,
> +				   bool skip_conflict);
> +
>  /** Finalize domain tables and startup non-root domains */
>  int sbi_domain_finalize(struct sbi_scratch *scratch, u32
> cold_hartid);
>  
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 164f35c..8afdb5d 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -24,10 +24,10 @@ static bool domain_finalized = false;
>  
>  static struct sbi_hartmask root_hmask = { 0 };
>  
> -#define ROOT_FW_REGION		0
> -#define ROOT_ALL_REGION	1
> -#define ROOT_END_REGION	2
> -static struct sbi_domain_memregion root_memregs[ROOT_END_REGION + 1]
> = { 0 };
> +#define ROOT_REGION_MAX	16
> +static u32 root_memregs_count = 0;
> +static struct sbi_domain_memregion root_fw_region;
> +static struct sbi_domain_memregion root_memregs[ROOT_REGION_MAX + 1]
> = { 0 };
>  
>  static struct sbi_domain root = {
>  	.name = "root",
> @@ -69,7 +69,7 @@ void sbi_domain_memregion_initfw(struct
> sbi_domain_memregion *reg)
>  	if (!reg)
>  		return;
>  
> -	sbi_memcpy(reg, &root_memregs[ROOT_FW_REGION], sizeof(*reg));
> +	sbi_memcpy(reg, &root_fw_region, sizeof(*reg));
>  }
>  
>  void sbi_domain_memregion_init(unsigned long addr,
> @@ -236,9 +236,9 @@ static int sanitize_domain(const struct
> sbi_platform *plat,
>  	count = 0;
>  	have_fw_reg = FALSE;
>  	sbi_domain_for_each_memregion(dom, reg) {
> -		if (reg->order == root_memregs[ROOT_FW_REGION].order &&
> -		    reg->base == root_memregs[ROOT_FW_REGION].base &&
> -		    reg->flags == root_memregs[ROOT_FW_REGION].flags)
> +		if (reg->order == root_fw_region.order &&
> +		    reg->base == root_fw_region.base &&
> +		    reg->flags == root_fw_region.flags)
>  			have_fw_reg = TRUE;
>  		count++;
>  	}
> @@ -468,6 +468,45 @@ int sbi_domain_register(struct sbi_domain *dom,
>  	return 0;
>  }
>  
> +int sbi_domain_root_add_memregion(const struct sbi_domain_memregion
> *reg,
> +				   bool skip_conflict)
> +{
> +	int rc;
> +	struct sbi_domain_memregion *nreg;
> +	const struct sbi_platform *plat = sbi_platform_thishart_ptr();
> +
> +	/* Sanity checks */
> +	if (!reg || domain_finalized ||
> +	    (root.regions != root_memregs) ||
> +	    (ROOT_REGION_MAX <= root_memregs_count))
> +		return SBI_EINVAL;
> +
> +	/* Check for conflicts */
> +	sbi_domain_for_each_memregion(&root, nreg) {
> +		if (is_region_conflict(reg, nreg))
> +			return (skip_conflict) ? 0 : SBI_EINVAL;
> +	}
> +
> +	/* Append the region to root regions */
> +	nreg = &root_memregs[root_memregs_count];
> +	sbi_memcpy(nreg, reg, sizeof(*reg));
> +	root_memregs_count++;
> +	root_memregs[root_memregs_count].order = 0;
> +
> +	/* Sanitize the root domain so that root regions are sorted */
> +	rc = sanitize_domain(plat, &root);
> +	if (rc) {
> +		sbi_printf("%s: sanity checks failed for"
> +			   " %s (error %d)\n", __func__,
> +			   root.name, rc);
> +		nreg->order = 0;
> +		root_memregs_count--;
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  int sbi_domain_finalize(struct sbi_scratch *scratch, u32
> cold_hartid)
>  {
>  	int rc;
> @@ -537,17 +576,18 @@ int sbi_domain_init(struct sbi_scratch
> *scratch, u32 cold_hartid)
>  
>  	/* Root domain firmware memory region */
>  	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
> 0,
> -				  &root_memregs[ROOT_FW_REGION]);
> +				  &root_fw_region);
> +	sbi_domain_memregion_initfw(&root_memregs[root_memregs_count++]
> );
>  
>  	/* Root domain allow everything memory region */
>  	sbi_domain_memregion_init(0, ~0UL,
>  				  (SBI_DOMAIN_MEMREGION_READABLE |
>  				   SBI_DOMAIN_MEMREGION_WRITEABLE |
>  				   SBI_DOMAIN_MEMREGION_EXECUTABLE),
> -				  &root_memregs[ROOT_ALL_REGION]);
> +				  &root_memregs[root_memregs_count++]);
>  
>  	/* Root domain memory region end */
> -	root_memregs[ROOT_END_REGION].order = 0;
> +	root_memregs[root_memregs_count].order = 0;
>  
>  	/* Use platform specific root memory regions when available */
>  	memregs = sbi_platform_domains_root_regions(plat);
> -- 
> 2.25.1
> 
> 

[PATCH 4/7] lib: utils/sys: Add CLINT memregion in the root domain

[Xiang W]

? 2021-04-10?? 12:48 +0530?Anup Patel???
> The CLINT memory should not be accessed by the supervisor-mode
> software so let's protect it by adding CLINT memregion to the
> root domain.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
clint_cold_timer_init and clint_cold_ipi_init may use the same clint,
which will call clint_add_root_memregion twice and generate an error.
I suggest to separate

In clint_cold_timer_init execute
sbi_domain_memregion_init(clint->addr, 0x4000,
SBI_DOMAIN_MEMREGION_MMIO, &reg);

In clint_cold_ipi_init execute
sbi_domain_memregion_init(clint->addr + 0x4000, 0x8000,
SBI_DOMAIN_MEMREGION_MMIO, &reg);

Regards,
Xiang W
> ---
>  lib/utils/sys/clint.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/utils/sys/clint.c b/lib/utils/sys/clint.c
> index 7a392aa..fe98cc4 100644
> --- a/lib/utils/sys/clint.c
> +++ b/lib/utils/sys/clint.c
> @@ -10,6 +10,7 @@
>  #include <sbi/riscv_asm.h>
>  #include <sbi/riscv_atomic.h>
>  #include <sbi/riscv_io.h>
> +#include <sbi/sbi_domain.h>
>  #include <sbi/sbi_error.h>
>  #include <sbi/sbi_hartmask.h>
>  #include <sbi_utils/sys/clint.h>
> @@ -17,9 +18,19 @@
>  #define CLINT_IPI_OFF		0
>  #define CLINT_TIME_CMP_OFF	0x4000
>  #define CLINT_TIME_VAL_OFF	0xbff8
> +#define CLINT_SIZE		0xc000
>  
>  static struct clint_data
> *clint_ipi_hartid2data[SBI_HARTMASK_MAX_BITS];
>  
> +static int clint_add_root_memregion(struct clint_data *clint)
> +{
> +	struct sbi_domain_memregion reg;
> +
> +	sbi_domain_memregion_init(clint->addr, CLINT_SIZE,
> +				  SBI_DOMAIN_MEMREGION_MMIO, &reg);
> +	return sbi_domain_root_add_memregion(&reg, true);
> +}
> +
>  void clint_ipi_send(u32 target_hart)
>  {
>  	struct clint_data *clint;
> @@ -70,7 +81,8 @@ int clint_cold_ipi_init(struct clint_data *clint)
>  	for (i = 0; i < clint->hart_count; i++)
>  		clint_ipi_hartid2data[clint->first_hartid + i] = clint;
>  
> -	return 0;
> +	/* Add CLINT region to the root domain */
> +	return clint_add_root_memregion(clint);
>  }
>  
>  static struct clint_data
> *clint_timer_hartid2data[SBI_HARTMASK_MAX_BITS];
> @@ -199,5 +211,6 @@ int clint_cold_timer_init(struct clint_data
> *clint,
>  	for (i = 0; i < clint->hart_count; i++)
>  		clint_timer_hartid2data[clint->first_hartid + i] =
> clint;
>  
> -	return 0;
> +	/* Add CLINT region to the root domain */
> +	return clint_add_root_memregion(clint);
>  }
> -- 
> 2.25.1
> 
> 

[PATCH 5/7] lib: sbi: Make the root domain instance global variable

[Xiang W]

? 2021-04-10?? 12:48 +0530?Anup Patel???
> We make the the root domain instance global variable so that
> platform support and drivers can iterate over the root domain
> regions.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
looks good to me.

Reviewed-by: Xiang W <wxjstz@126.com>

Regards,
Xiang W
> ---
>  include/sbi/sbi_domain.h | 3 +++
>  lib/sbi/sbi_domain.c     | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index b5f2f38..ef53ca5 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -80,6 +80,9 @@ struct sbi_domain {
>  	bool system_reset_allowed;
>  };
>  
> +/** The root domain instance */
> +extern struct sbi_domain root;
> +
>  /** HART id to domain table */
>  extern struct sbi_domain *hartid_to_domain_table[];
>  
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 8afdb5d..5e3a30d 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -29,7 +29,7 @@ static u32 root_memregs_count = 0;
>  static struct sbi_domain_memregion root_fw_region;
>  static struct sbi_domain_memregion root_memregs[ROOT_REGION_MAX + 1]
> = { 0 };
>  
> -static struct sbi_domain root = {
> +struct sbi_domain root = {
>  	.name = "root",
>  	.possible_harts = &root_hmask,
>  	.regions = root_memregs,
> -- 
> 2.25.1
> 
> 

[PATCH 6/7] lib: utils: Copy over restricted root domain memregions to FDT domains

[Xiang W]

? 2021-04-10?? 12:48 +0530?Anup Patel???
> We should copy over all restricted memregions from the root domain
> to the domains populated from FDT. These restricted root memregions
> are typically firmware memregion and M-mode only mmio memregions.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
looks good to me.

Reviewed-by: Xiang W <wxjstz@126.com>

Regards,
Xiang W
> ---
>  lib/utils/fdt/fdt_domain.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 09615e5..95c195d 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -222,7 +222,7 @@ static u32 fdt_domains_count;
>  static struct sbi_domain fdt_domains[FDT_DOMAIN_MAX_COUNT];
>  static struct sbi_hartmask fdt_masks[FDT_DOMAIN_MAX_COUNT];
>  static struct sbi_domain_memregion
> -	fdt_regions[FDT_DOMAIN_MAX_COUNT][FDT_DOMAIN_REGION_MAX_COUNT +
> 2];
> +	fdt_regions[FDT_DOMAIN_MAX_COUNT][FDT_DOMAIN_REGION_MAX_COUNT +
> 1];
>  
>  static int __fdt_parse_region(void *fdt, int domain_offset,
>  			      int region_offset, u32 region_access,
> @@ -276,7 +276,7 @@ static int __fdt_parse_domain(void *fdt, int
> domain_offset, void *opaque)
>  	struct sbi_hartmask *mask;
>  	struct sbi_hartmask assign_mask;
>  	int *cold_domain_offset = opaque;
> -	struct sbi_domain_memregion *regions;
> +	struct sbi_domain_memregion *reg, *regions;
>  	int i, err, len, cpus_offset, cpu_offset, doffset;
>  
>  	/* Sanity check on maximum domains we can handle */
> @@ -314,13 +314,31 @@ static int __fdt_parse_domain(void *fdt, int
> domain_offset, void *opaque)
>  	/* Setup memregions from DT */
>  	val32 = 0;
>  	sbi_memset(regions, 0,
> -		   sizeof(*regions) * (FDT_DOMAIN_REGION_MAX_COUNT +
> 2));
> +		   sizeof(*regions) * (FDT_DOMAIN_REGION_MAX_COUNT +
> 1));
>  	dom->regions = regions;
>  	err = fdt_iterate_each_memregion(fdt, domain_offset, &val32,
>  					 __fdt_parse_region);
>  	if (err)
>  		return err;
> -	sbi_domain_memregion_initfw(&regions[val32]);
> +
> +	/*
> +	 * Copy over root domain memregions which don't allow
> +	 * read, write and execute from lower privilege modes.
> +	 *
> +	 * These root domain memregions without read, write,
> +	 * and execute permissions include:
> +	 * 1) firmware region protecting the firmware memory
> +	 * 2) mmio regions protecting M-mode only mmio devices
> +	 */
> +	sbi_domain_for_each_memregion(&root, reg) {
> +		if ((reg->flags & SBI_DOMAIN_MEMREGION_READABLE) ||
> +		    (reg->flags & SBI_DOMAIN_MEMREGION_WRITEABLE) ||
> +		    (reg->flags & SBI_DOMAIN_MEMREGION_EXECUTABLE))
> +			continue;
> +		if (FDT_DOMAIN_REGION_MAX_COUNT <= val32)
> +			return SBI_EINVAL;
> +		sbi_memcpy(&regions[val32++], reg, sizeof(*reg));
> +	}
>  
>  	/* Read "boot-hart" DT property */
>  	val32 = -1U;
> -- 
> 2.25.1
> 
> 

[PATCH 7/7] lib: sbi: Make sbi_domain_memregion_initfw() a local function

[Xiang W]

? 2021-04-10?? 12:48 +0530?Anup Patel???
> The sbi_domain_memregion_initfw() is no longer used outside
> sbi_domain.c so let's make it a local function.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
looks good to me.

Reviewed-by: Xiang W <wxjstz@126.com>

Regards,
Xiang W
> ---
>  include/sbi/sbi_domain.h | 3 ---
>  lib/sbi/sbi_domain.c     | 4 ++--
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index ef53ca5..3de5e46 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -127,9 +127,6 @@ bool sbi_domain_is_assigned_hart(const struct
> sbi_domain *dom, u32 hartid);
>  ulong sbi_domain_get_assigned_hartmask(const struct sbi_domain *dom,
>  				       ulong hbase);
>  
> -/** Initialize a domain memory region as firmware region */
> -void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg);
> -
>  /**
>   * Initialize a domain memory region based on it's physical
>   * address and size.
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 5e3a30d..069e2a7 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -64,7 +64,7 @@ ulong sbi_domain_get_assigned_hartmask(const struct
> sbi_domain *dom,
>  	return ret;
>  }
>  
> -void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg)
> +static void domain_memregion_initfw(struct sbi_domain_memregion
> *reg)
>  {
>  	if (!reg)
>  		return;
> @@ -577,7 +577,7 @@ int sbi_domain_init(struct sbi_scratch *scratch,
> u32 cold_hartid)
>  	/* Root domain firmware memory region */
>  	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
> 0,
>  				  &root_fw_region);
> -	sbi_domain_memregion_initfw(&root_memregs[root_memregs_count++]
> );
> +	domain_memregion_initfw(&root_memregs[root_memregs_count++]);
>  
>  	/* Root domain allow everything memory region */
>  	sbi_domain_memregion_init(0, ~0UL,
> -- 
> 2.25.1
> 
> 

[PATCH 1/7] lib: sbi: Domains can be registered only before finalizing domains

[Alistair Francis]

On Sat, 2021-04-10 at 12:48 +0530, Anup Patel wrote:
> The domains are boot-time system-level partitions so we should
> allow platform support to register domains only before hart
> domain assignments are finalized.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> ?lib/sbi/sbi_domain.c | 11 +++++++++--
> ?1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 195c941..2849241 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -19,8 +19,8 @@
> ?
> ?struct sbi_domain *hartid_to_domain_table[SBI_HARTMASK_MAX_BITS] = {
> 0 };
> ?struct sbi_domain *domidx_to_domain_table[SBI_DOMAIN_MAX_INDEX] = {
> 0 };
> -
> ?static u32 domain_count = 0;
> +static bool domain_finalized = false;
> ?
> ?static struct sbi_hartmask root_hmask = { 0 };
> ?
> @@ -376,7 +376,8 @@ int sbi_domain_register(struct sbi_domain *dom,
> ????????u32 cold_hartid = current_hartid();
> ????????const struct sbi_platform *plat =
> sbi_platform_thishart_ptr();
> ?
> -???????if (!dom || !assign_mask)
> +???????/* Sanity checks */
> +???????if (!dom || !assign_mask || domain_finalized)
> ????????????????return SBI_EINVAL;
> ?
> ????????/* Check if domain already discovered */
> @@ -490,6 +491,12 @@ int sbi_domain_finalize(struct sbi_scratch
> *scratch, u32 cold_hartid)
> ????????????????}
> ????????}
> ?
> +???????/*
> +??????? * Set the finalized flag so that the root domain
> +??????? * regions can't be changed.
> +??????? */
> +???????domain_finalized = true;
> +
> ????????return 0;
> ?}
> ?

[PATCH 2/7] lib: sbi: Add sbi_domain_memregion_init() API

[Alistair Francis]

On Sat, 2021-04-10 at 12:48 +0530, Anup Patel wrote:
> This patch adds sbi_domain_memregion_init() helper API which can
> be used by platform support to initialize a domain memory region
> before adding it to the root domain.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> ?include/sbi/sbi_domain.h | 14 +++++++++++++
> ?lib/sbi/sbi_domain.c???? | 45 ++++++++++++++++++++++++++++++++--------
> ?2 files changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index 1f8b942..f9f4f7d 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -127,6 +127,20 @@ ulong sbi_domain_get_assigned_hartmask(const
> struct sbi_domain *dom,
> ?/** Initialize a domain memory region as firmware region */
> ?void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg);
> ?
> +/**
> + * Initialize a domain memory region based on it's physical
> + * address and size.
> + *
> + * @param addr start physical address of memory region
> + * @param size physical size of memory region
> + * @param flags memory region flags
> + * @param reg pointer to memory region being initialized
> + */
> +void sbi_domain_memregion_init(unsigned long addr,
> +???????????????????????????????unsigned long size,
> +???????????????????????????????unsigned long flags,
> +???????????????????????????????struct sbi_domain_memregion *reg);
> +
> ?/**
> ? * Check whether we can access specified address for given mode and
> ? * memory region flags under a domain
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 2849241..164f35c 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -72,6 +72,35 @@ void sbi_domain_memregion_initfw(struct
> sbi_domain_memregion *reg)
> ????????sbi_memcpy(reg, &root_memregs[ROOT_FW_REGION], sizeof(*reg));
> ?}
> ?
> +void sbi_domain_memregion_init(unsigned long addr,
> +???????????????????????????????unsigned long size,
> +???????????????????????????????unsigned long flags,
> +???????????????????????????????struct sbi_domain_memregion *reg)
> +{
> +???????unsigned long base = 0, order;
> +
> +???????for (order = log2roundup(size) ; order <= __riscv_xlen;
> order++) {
> +???????????????if (order < __riscv_xlen) {
> +???????????????????????base = addr & ~((1UL << order) - 1UL);
> +???????????????????????if ((base <= addr) &&
> +?????????????????????????? (addr < (base + (1UL << order))) &&
> +?????????????????????????? (base <= (addr + size - 1UL)) &&
> +?????????????????????????? ((addr + size - 1UL) < (base + (1UL <<
> order))))
> +???????????????????????????????break;
> +???????????????} else {
> +???????????????????????base = 0;
> +???????????????????????break;
> +???????????????}
> +
> +???????}
> +
> +???????if (reg) {
> +???????????????reg->base = base;
> +???????????????reg->order = order;
> +???????????????reg->flags = flags;
> +???????}
> +}
> +
> ?bool sbi_domain_check_addr(const struct sbi_domain *dom,
> ?????????????????????????? unsigned long addr, unsigned long mode,
> ?????????????????????????? unsigned long access_flags)
> @@ -507,17 +536,15 @@ int sbi_domain_init(struct sbi_scratch *scratch,
> u32 cold_hartid)
> ????????const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> ?
> ????????/* Root domain firmware memory region */
> -???????root_memregs[ROOT_FW_REGION].order = log2roundup(scratch-
> >fw_size);
> -???????root_memregs[ROOT_FW_REGION].base = scratch->fw_start &
> -???????????????????????????????~((1UL << root_memregs[0].order) -
> 1UL);
> -???????root_memregs[ROOT_FW_REGION].flags = 0;
> +???????sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
> 0,
> +???????????????????????????????? &root_memregs[ROOT_FW_REGION]);
> ?
> ????????/* Root domain allow everything memory region */
> -???????root_memregs[ROOT_ALL_REGION].order = __riscv_xlen;
> -???????root_memregs[ROOT_ALL_REGION].base = 0;
> -???????root_memregs[ROOT_ALL_REGION].flags =
> (SBI_DOMAIN_MEMREGION_READABLE |
> -
> ???????????????????????????????????????????????SBI_DOMAIN_MEMREGION_WRIT
> EABLE |
> -
> ???????????????????????????????????????????????SBI_DOMAIN_MEMREGION_EXEC
> UTABLE);
> +???????sbi_domain_memregion_init(0, ~0UL,
> +???????????????????????????????? (SBI_DOMAIN_MEMREGION_READABLE |
> +????????????????????????????????? SBI_DOMAIN_MEMREGION_WRITEABLE |
> +????????????????????????????????? SBI_DOMAIN_MEMREGION_EXECUTABLE),
> +???????????????????????????????? &root_memregs[ROOT_ALL_REGION]);
> ?
> ????????/* Root domain memory region end */
> ????????root_memregs[ROOT_END_REGION].order = 0;

[PATCH 3/7] lib: sbi: Add sbi_domain_root_add_memregion() API

[Alistair Francis]

On Sat, 2021-04-10 at 12:48 +0530, Anup Patel wrote:
> We should allow platform support to add more root memory regions
> before domains are finalized. This will help platform support to
> protect critical M-mode only resources.
> 
> This patch adds sbi_domain_root_add_memregion() API for above
> described purpose.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> ?include/sbi/sbi_domain.h | 11 +++++++
> ?lib/sbi/sbi_domain.c???? | 62 +++++++++++++++++++++++++++++++++-------
> ?2 files changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index f9f4f7d..b5f2f38 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -170,6 +170,17 @@ void sbi_domain_dump_all(const char *suffix);
> ?int sbi_domain_register(struct sbi_domain *dom,
> ????????????????????????const struct sbi_hartmask *assign_mask);
> ?
> +/**
> + * Add a memory region to the root domain
> + * @param reg pointer to the memory region to be added
> + * @param skip_conflict do nothing if a conflicting region is already
> + * present in the root domain
> + *
> + * @return 0 on success and negative error code on failure
> + */
> +int sbi_domain_root_add_memregion(const struct sbi_domain_memregion
> *reg,
> +????????????????????????????????? bool skip_conflict);
> +
> ?/** Finalize domain tables and startup non-root domains */
> ?int sbi_domain_finalize(struct sbi_scratch *scratch, u32 cold_hartid);
> ?
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 164f35c..8afdb5d 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -24,10 +24,10 @@ static bool domain_finalized = false;
> ?
> ?static struct sbi_hartmask root_hmask = { 0 };
> ?
> -#define ROOT_FW_REGION?????????0
> -#define ROOT_ALL_REGION????????1
> -#define ROOT_END_REGION????????2
> -static struct sbi_domain_memregion root_memregs[ROOT_END_REGION + 1] =
> { 0 };
> +#define ROOT_REGION_MAX????????16
> +static u32 root_memregs_count = 0;
> +static struct sbi_domain_memregion root_fw_region;
> +static struct sbi_domain_memregion root_memregs[ROOT_REGION_MAX + 1] =
> { 0 };
> ?
> ?static struct sbi_domain root = {
> ????????.name = "root",
> @@ -69,7 +69,7 @@ void sbi_domain_memregion_initfw(struct
> sbi_domain_memregion *reg)
> ????????if (!reg)
> ????????????????return;
> ?
> -???????sbi_memcpy(reg, &root_memregs[ROOT_FW_REGION], sizeof(*reg));
> +???????sbi_memcpy(reg, &root_fw_region, sizeof(*reg));
> ?}
> ?
> ?void sbi_domain_memregion_init(unsigned long addr,
> @@ -236,9 +236,9 @@ static int sanitize_domain(const struct
> sbi_platform *plat,
> ????????count = 0;
> ????????have_fw_reg = FALSE;
> ????????sbi_domain_for_each_memregion(dom, reg) {
> -???????????????if (reg->order == root_memregs[ROOT_FW_REGION].order &&
> -?????????????????? reg->base == root_memregs[ROOT_FW_REGION].base &&
> -?????????????????? reg->flags == root_memregs[ROOT_FW_REGION].flags)
> +???????????????if (reg->order == root_fw_region.order &&
> +?????????????????? reg->base == root_fw_region.base &&
> +?????????????????? reg->flags == root_fw_region.flags)
> ????????????????????????have_fw_reg = TRUE;
> ????????????????count++;
> ????????}
> @@ -468,6 +468,45 @@ int sbi_domain_register(struct sbi_domain *dom,
> ????????return 0;
> ?}
> ?
> +int sbi_domain_root_add_memregion(const struct sbi_domain_memregion
> *reg,
> +????????????????????????????????? bool skip_conflict)
> +{
> +???????int rc;
> +???????struct sbi_domain_memregion *nreg;
> +???????const struct sbi_platform *plat = sbi_platform_thishart_ptr();
> +
> +???????/* Sanity checks */
> +???????if (!reg || domain_finalized ||
> +?????????? (root.regions != root_memregs) ||
> +?????????? (ROOT_REGION_MAX <= root_memregs_count))
> +???????????????return SBI_EINVAL;
> +
> +???????/* Check for conflicts */
> +???????sbi_domain_for_each_memregion(&root, nreg) {
> +???????????????if (is_region_conflict(reg, nreg))
> +???????????????????????return (skip_conflict) ? 0 : SBI_EINVAL;
> +???????}
> +
> +???????/* Append the region to root regions */
> +???????nreg = &root_memregs[root_memregs_count];
> +???????sbi_memcpy(nreg, reg, sizeof(*reg));
> +???????root_memregs_count++;
> +???????root_memregs[root_memregs_count].order = 0;
> +
> +???????/* Sanitize the root domain so that root regions are sorted */
> +???????rc = sanitize_domain(plat, &root);
> +???????if (rc) {
> +???????????????sbi_printf("%s: sanity checks failed for"
> +????????????????????????? " %s (error %d)\n", __func__,
> +????????????????????????? root.name, rc);
> +???????????????nreg->order = 0;
> +???????????????root_memregs_count--;
> +???????????????return rc;
> +???????}
> +
> +???????return 0;
> +}
> +
> ?int sbi_domain_finalize(struct sbi_scratch *scratch, u32 cold_hartid)
> ?{
> ????????int rc;
> @@ -537,17 +576,18 @@ int sbi_domain_init(struct sbi_scratch *scratch,
> u32 cold_hartid)
> ?
> ????????/* Root domain firmware memory region */
> ????????sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
> 0,
> -???????????????????????????????? &root_memregs[ROOT_FW_REGION]);
> +???????????????????????????????? &root_fw_region);
> +???????sbi_domain_memregion_initfw(&root_memregs[root_memregs_count++]
> );
> ?
> ????????/* Root domain allow everything memory region */
> ????????sbi_domain_memregion_init(0, ~0UL,
> ????????????????????????????????? (SBI_DOMAIN_MEMREGION_READABLE |
> ?????????????????????????????????? SBI_DOMAIN_MEMREGION_WRITEABLE |
> ?????????????????????????????????? SBI_DOMAIN_MEMREGION_EXECUTABLE),
> -???????????????????????????????? &root_memregs[ROOT_ALL_REGION]);
> +???????????????????????????????? &root_memregs[root_memregs_count++]);
> ?
> ????????/* Root domain memory region end */
> -???????root_memregs[ROOT_END_REGION].order = 0;
> +???????root_memregs[root_memregs_count].order = 0;
> ?
> ????????/* Use platform specific root memory regions when available */
> ????????memregs = sbi_platform_domains_root_regions(plat);

[PATCH 5/7] lib: sbi: Make the root domain instance global variable

[Alistair Francis]

On Sat, 2021-04-10 at 12:48 +0530, Anup Patel wrote:
> We make the the root domain instance global variable so that
> platform support and drivers can iterate over the root domain
> regions.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> ?include/sbi/sbi_domain.h | 3 +++
> ?lib/sbi/sbi_domain.c???? | 2 +-
> ?2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index b5f2f38..ef53ca5 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -80,6 +80,9 @@ struct sbi_domain {
> ????????bool system_reset_allowed;
> ?};
> ?
> +/** The root domain instance */
> +extern struct sbi_domain root;
> +
> ?/** HART id to domain table */
> ?extern struct sbi_domain *hartid_to_domain_table[];
> ?
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 8afdb5d..5e3a30d 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -29,7 +29,7 @@ static u32 root_memregs_count = 0;
> ?static struct sbi_domain_memregion root_fw_region;
> ?static struct sbi_domain_memregion root_memregs[ROOT_REGION_MAX + 1] =
> { 0 };
> ?
> -static struct sbi_domain root = {
> +struct sbi_domain root = {
> ????????.name = "root",
> ????????.possible_harts = &root_hmask,
> ????????.regions = root_memregs,

[PATCH 6/7] lib: utils: Copy over restricted root domain memregions to FDT domains

[Alistair Francis]

On Sat, 2021-04-10 at 12:48 +0530, Anup Patel wrote:
> We should copy over all restricted memregions from the root domain
> to the domains populated from FDT. These restricted root memregions
> are typically firmware memregion and M-mode only mmio memregions.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> ?lib/utils/fdt/fdt_domain.c | 26 ++++++++++++++++++++++----
> ?1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 09615e5..95c195d 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -222,7 +222,7 @@ static u32 fdt_domains_count;
> ?static struct sbi_domain fdt_domains[FDT_DOMAIN_MAX_COUNT];
> ?static struct sbi_hartmask fdt_masks[FDT_DOMAIN_MAX_COUNT];
> ?static struct sbi_domain_memregion
> -???????fdt_regions[FDT_DOMAIN_MAX_COUNT][FDT_DOMAIN_REGION_MAX_COUNT +
> 2];
> +???????fdt_regions[FDT_DOMAIN_MAX_COUNT][FDT_DOMAIN_REGION_MAX_COUNT +
> 1];
> ?
> ?static int __fdt_parse_region(void *fdt, int domain_offset,
> ????????????????????????????? int region_offset, u32 region_access,
> @@ -276,7 +276,7 @@ static int __fdt_parse_domain(void *fdt, int
> domain_offset, void *opaque)
> ????????struct sbi_hartmask *mask;
> ????????struct sbi_hartmask assign_mask;
> ????????int *cold_domain_offset = opaque;
> -???????struct sbi_domain_memregion *regions;
> +???????struct sbi_domain_memregion *reg, *regions;
> ????????int i, err, len, cpus_offset, cpu_offset, doffset;
> ?
> ????????/* Sanity check on maximum domains we can handle */
> @@ -314,13 +314,31 @@ static int __fdt_parse_domain(void *fdt, int
> domain_offset, void *opaque)
> ????????/* Setup memregions from DT */
> ????????val32 = 0;
> ????????sbi_memset(regions, 0,
> -????????????????? sizeof(*regions) * (FDT_DOMAIN_REGION_MAX_COUNT +
> 2));
> +????????????????? sizeof(*regions) * (FDT_DOMAIN_REGION_MAX_COUNT +
> 1));
> ????????dom->regions = regions;
> ????????err = fdt_iterate_each_memregion(fdt, domain_offset, &val32,
> ???????????????????????????????????????? __fdt_parse_region);
> ????????if (err)
> ????????????????return err;
> -???????sbi_domain_memregion_initfw(&regions[val32]);
> +
> +???????/*
> +??????? * Copy over root domain memregions which don't allow
> +??????? * read, write and execute from lower privilege modes.
> +??????? *
> +??????? * These root domain memregions without read, write,
> +??????? * and execute permissions include:
> +??????? * 1) firmware region protecting the firmware memory
> +??????? * 2) mmio regions protecting M-mode only mmio devices
> +??????? */
> +???????sbi_domain_for_each_memregion(&root, reg) {
> +???????????????if ((reg->flags & SBI_DOMAIN_MEMREGION_READABLE) ||
> +?????????????????? (reg->flags & SBI_DOMAIN_MEMREGION_WRITEABLE) ||
> +?????????????????? (reg->flags & SBI_DOMAIN_MEMREGION_EXECUTABLE))
> +???????????????????????continue;
> +???????????????if (FDT_DOMAIN_REGION_MAX_COUNT <= val32)
> +???????????????????????return SBI_EINVAL;
> +???????????????sbi_memcpy(&regions[val32++], reg, sizeof(*reg));
> +???????}
> ?
> ????????/* Read "boot-hart" DT property */
> ????????val32 = -1U;

[PATCH 7/7] lib: sbi: Make sbi_domain_memregion_initfw() a local function

[Alistair Francis]

On Sat, 2021-04-10 at 12:48 +0530, Anup Patel wrote:
> The sbi_domain_memregion_initfw() is no longer used outside
> sbi_domain.c so let's make it a local function.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> ?include/sbi/sbi_domain.h | 3 ---
> ?lib/sbi/sbi_domain.c???? | 4 ++--
> ?2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index ef53ca5..3de5e46 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -127,9 +127,6 @@ bool sbi_domain_is_assigned_hart(const struct
> sbi_domain *dom, u32 hartid);
> ?ulong sbi_domain_get_assigned_hartmask(const struct sbi_domain *dom,
> ?????????????????????????????????????? ulong hbase);
> ?
> -/** Initialize a domain memory region as firmware region */
> -void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg);
> -
> ?/**
> ? * Initialize a domain memory region based on it's physical
> ? * address and size.
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 5e3a30d..069e2a7 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -64,7 +64,7 @@ ulong sbi_domain_get_assigned_hartmask(const struct
> sbi_domain *dom,
> ????????return ret;
> ?}
> ?
> -void sbi_domain_memregion_initfw(struct sbi_domain_memregion *reg)
> +static void domain_memregion_initfw(struct sbi_domain_memregion *reg)
> ?{
> ????????if (!reg)
> ????????????????return;
> @@ -577,7 +577,7 @@ int sbi_domain_init(struct sbi_scratch *scratch,
> u32 cold_hartid)
> ????????/* Root domain firmware memory region */
> ????????sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
> 0,
> ????????????????????????????????? &root_fw_region);
> -
> ???????sbi_domain_memregion_initfw(&root_memregs[root_memregs_count++]);
> +???????domain_memregion_initfw(&root_memregs[root_memregs_count++]);
> ?
> ????????/* Root domain allow everything memory region */
> ????????sbi_domain_memregion_init(0, ~0UL,

[PATCH 4/7] lib: utils/sys: Add CLINT memregion in the root domain

[Anup Patel]

On Sun, Apr 11, 2021 at 2:35 PM Xiang W <wxjstz@126.com> wrote:
>
> ? 2021-04-10?? 12:48 +0530?Anup Patel???
> > The CLINT memory should not be accessed by the supervisor-mode
> > software so let's protect it by adding CLINT memregion to the
> > root domain.
> >
> > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> clint_cold_timer_init and clint_cold_ipi_init may use the same clint,
> which will call clint_add_root_memregion twice and generate an error.
> I suggest to separate
>
> In clint_cold_timer_init execute
> sbi_domain_memregion_init(clint->addr, 0x4000,
> SBI_DOMAIN_MEMREGION_MMIO, &reg);
>
> In clint_cold_ipi_init execute
> sbi_domain_memregion_init(clint->addr + 0x4000, 0x8000,
> SBI_DOMAIN_MEMREGION_MMIO, &reg);

I had though about this but the only issue in having two
separate memregions for CLINT is that it will take-up more
PMP CSRs. The "skip_conflt" flag was added as parameter
to sbi_domain_root_add_memregion() so that CLINT driver
can call it multiple time.

I think we can have a better implementation of the
sbi_domain_root_add_memregion() which will merge
consecutive regions with matching flags. This will allow
use to add separate regions for CLINT driver but
sbi_domain_root_add_memregion() will merge them
into single region.

Regards,
Anup

>
> Regards,
> Xiang W
> > ---
> >  lib/utils/sys/clint.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/utils/sys/clint.c b/lib/utils/sys/clint.c
> > index 7a392aa..fe98cc4 100644
> > --- a/lib/utils/sys/clint.c
> > +++ b/lib/utils/sys/clint.c
> > @@ -10,6 +10,7 @@
> >  #include <sbi/riscv_asm.h>
> >  #include <sbi/riscv_atomic.h>
> >  #include <sbi/riscv_io.h>
> > +#include <sbi/sbi_domain.h>
> >  #include <sbi/sbi_error.h>
> >  #include <sbi/sbi_hartmask.h>
> >  #include <sbi_utils/sys/clint.h>
> > @@ -17,9 +18,19 @@
> >  #define CLINT_IPI_OFF                0
> >  #define CLINT_TIME_CMP_OFF   0x4000
> >  #define CLINT_TIME_VAL_OFF   0xbff8
> > +#define CLINT_SIZE           0xc000
> >
> >  static struct clint_data
> > *clint_ipi_hartid2data[SBI_HARTMASK_MAX_BITS];
> >
> > +static int clint_add_root_memregion(struct clint_data *clint)
> > +{
> > +     struct sbi_domain_memregion reg;
> > +
> > +     sbi_domain_memregion_init(clint->addr, CLINT_SIZE,
> > +                               SBI_DOMAIN_MEMREGION_MMIO, &reg);
> > +     return sbi_domain_root_add_memregion(&reg, true);
> > +}
> > +
> >  void clint_ipi_send(u32 target_hart)
> >  {
> >       struct clint_data *clint;
> > @@ -70,7 +81,8 @@ int clint_cold_ipi_init(struct clint_data *clint)
> >       for (i = 0; i < clint->hart_count; i++)
> >               clint_ipi_hartid2data[clint->first_hartid + i] = clint;
> >
> > -     return 0;
> > +     /* Add CLINT region to the root domain */
> > +     return clint_add_root_memregion(clint);
> >  }
> >
> >  static struct clint_data
> > *clint_timer_hartid2data[SBI_HARTMASK_MAX_BITS];
> > @@ -199,5 +211,6 @@ int clint_cold_timer_init(struct clint_data
> > *clint,
> >       for (i = 0; i < clint->hart_count; i++)
> >               clint_timer_hartid2data[clint->first_hartid + i] =
> > clint;
> >
> > -     return 0;
> > +     /* Add CLINT region to the root domain */
> > +     return clint_add_root_memregion(clint);
> >  }
> > --
> > 2.25.1
> >
> >
>

[PATCH 4/7] lib: utils/sys: Add CLINT memregion in the root domain

[Xiang W]

? 2021-04-12?? 10:53 +0530?Anup Patel???
> On Sun, Apr 11, 2021 at 2:35 PM Xiang W <wxjstz@126.com> wrote:
> > ? 2021-04-10?? 12:48 +0530?Anup Patel???
> > > The CLINT memory should not be accessed by the supervisor-mode
> > > software so let's protect it by adding CLINT memregion to the
> > > root domain.
> > > 
> > > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > clint_cold_timer_init and clint_cold_ipi_init may use the same
> > clint,
> > which will call clint_add_root_memregion twice and generate an
> > error.
> > I suggest to separate
> > 
> > In clint_cold_timer_init execute
> > sbi_domain_memregion_init(clint->addr, 0x4000,
> > SBI_DOMAIN_MEMREGION_MMIO, &reg);
> > 
> > In clint_cold_ipi_init execute
> > sbi_domain_memregion_init(clint->addr + 0x4000, 0x8000,
> > SBI_DOMAIN_MEMREGION_MMIO, &reg);
> 
> I had though about this but the only issue in having two
> separate memregions for CLINT is that it will take-up more
> PMP CSRs. The "skip_conflt" flag was added as parameter
> to sbi_domain_root_add_memregion() so that CLINT driver
> can call it multiple time.
> 
> I think we can have a better implementation of the
> sbi_domain_root_add_memregion() which will merge
> consecutive regions with matching flags. This will allow
> use to add separate regions for CLINT driver but
> sbi_domain_root_add_memregion() will merge them
> into single region.
Merge regions is a good idea. I think the merger can be performed by
default. So we can separate this side first, and then modify
sbi_domain_root_add_memregion to add the merge function.

Regards,
Xiang W
> 
> Regards,
> Anup
> 
> > Regards,
> > Xiang W
> > > ---
> > >  lib/utils/sys/clint.c | 17 +++++++++++++++--
> > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/utils/sys/clint.c b/lib/utils/sys/clint.c
> > > index 7a392aa..fe98cc4 100644
> > > --- a/lib/utils/sys/clint.c
> > > +++ b/lib/utils/sys/clint.c
> > > @@ -10,6 +10,7 @@
> > >  #include <sbi/riscv_asm.h>
> > >  #include <sbi/riscv_atomic.h>
> > >  #include <sbi/riscv_io.h>
> > > +#include <sbi/sbi_domain.h>
> > >  #include <sbi/sbi_error.h>
> > >  #include <sbi/sbi_hartmask.h>
> > >  #include <sbi_utils/sys/clint.h>
> > > @@ -17,9 +18,19 @@
> > >  #define CLINT_IPI_OFF                0
> > >  #define CLINT_TIME_CMP_OFF   0x4000
> > >  #define CLINT_TIME_VAL_OFF   0xbff8
> > > +#define CLINT_SIZE           0xc000
> > > 
> > >  static struct clint_data
> > > *clint_ipi_hartid2data[SBI_HARTMASK_MAX_BITS];
> > > 
> > > +static int clint_add_root_memregion(struct clint_data *clint)
> > > +{
> > > +     struct sbi_domain_memregion reg;
> > > +
> > > +     sbi_domain_memregion_init(clint->addr, CLINT_SIZE,
> > > +                               SBI_DOMAIN_MEMREGION_MMIO, &reg);
> > > +     return sbi_domain_root_add_memregion(&reg, true);
> > > +}
> > > +
> > >  void clint_ipi_send(u32 target_hart)
> > >  {
> > >       struct clint_data *clint;
> > > @@ -70,7 +81,8 @@ int clint_cold_ipi_init(struct clint_data
> > > *clint)
> > >       for (i = 0; i < clint->hart_count; i++)
> > >               clint_ipi_hartid2data[clint->first_hartid + i] =
> > > clint;
> > > 
> > > -     return 0;
> > > +     /* Add CLINT region to the root domain */
> > > +     return clint_add_root_memregion(clint);
> > >  }
> > > 
> > >  static struct clint_data
> > > *clint_timer_hartid2data[SBI_HARTMASK_MAX_BITS];
> > > @@ -199,5 +211,6 @@ int clint_cold_timer_init(struct clint_data
> > > *clint,
> > >       for (i = 0; i < clint->hart_count; i++)
> > >               clint_timer_hartid2data[clint->first_hartid + i] =
> > > clint;
> > > 
> > > -     return 0;
> > > +     /* Add CLINT region to the root domain */
> > > +     return clint_add_root_memregion(clint);
> > >  }
> > > --
> > > 2.25.1
> > > 
> > > 

[PATCH 4/7] lib: utils/sys: Add CLINT memregion in the root domain

[Anup Patel]

On Mon, Apr 12, 2021 at 11:12 AM Xiang W <wxjstz@126.com> wrote:
>
> ? 2021-04-12?? 10:53 +0530?Anup Patel???
> > On Sun, Apr 11, 2021 at 2:35 PM Xiang W <wxjstz@126.com> wrote:
> > > ? 2021-04-10?? 12:48 +0530?Anup Patel???
> > > > The CLINT memory should not be accessed by the supervisor-mode
> > > > software so let's protect it by adding CLINT memregion to the
> > > > root domain.
> > > >
> > > > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > > clint_cold_timer_init and clint_cold_ipi_init may use the same
> > > clint,
> > > which will call clint_add_root_memregion twice and generate an
> > > error.
> > > I suggest to separate
> > >
> > > In clint_cold_timer_init execute
> > > sbi_domain_memregion_init(clint->addr, 0x4000,
> > > SBI_DOMAIN_MEMREGION_MMIO, &reg);
> > >
> > > In clint_cold_ipi_init execute
> > > sbi_domain_memregion_init(clint->addr + 0x4000, 0x8000,
> > > SBI_DOMAIN_MEMREGION_MMIO, &reg);
> >
> > I had though about this but the only issue in having two
> > separate memregions for CLINT is that it will take-up more
> > PMP CSRs. The "skip_conflt" flag was added as parameter
> > to sbi_domain_root_add_memregion() so that CLINT driver
> > can call it multiple time.
> >
> > I think we can have a better implementation of the
> > sbi_domain_root_add_memregion() which will merge
> > consecutive regions with matching flags. This will allow
> > use to add separate regions for CLINT driver but
> > sbi_domain_root_add_memregion() will merge them
> > into single region.
> Merge regions is a good idea. I think the merger can be performed by
> default. So we can separate this side first, and then modify
> sbi_domain_root_add_memregion to add the merge function.

I will be updating both PATCH3 and PATCH4 (this patch) for this
so please review these patches again in next revision.

Regards,
Anup

>
> Regards,
> Xiang W
> >
> > Regards,
> > Anup
> >
> > > Regards,
> > > Xiang W
> > > > ---
> > > >  lib/utils/sys/clint.c | 17 +++++++++++++++--
> > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/utils/sys/clint.c b/lib/utils/sys/clint.c
> > > > index 7a392aa..fe98cc4 100644
> > > > --- a/lib/utils/sys/clint.c
> > > > +++ b/lib/utils/sys/clint.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include <sbi/riscv_asm.h>
> > > >  #include <sbi/riscv_atomic.h>
> > > >  #include <sbi/riscv_io.h>
> > > > +#include <sbi/sbi_domain.h>
> > > >  #include <sbi/sbi_error.h>
> > > >  #include <sbi/sbi_hartmask.h>
> > > >  #include <sbi_utils/sys/clint.h>
> > > > @@ -17,9 +18,19 @@
> > > >  #define CLINT_IPI_OFF                0
> > > >  #define CLINT_TIME_CMP_OFF   0x4000
> > > >  #define CLINT_TIME_VAL_OFF   0xbff8
> > > > +#define CLINT_SIZE           0xc000
> > > >
> > > >  static struct clint_data
> > > > *clint_ipi_hartid2data[SBI_HARTMASK_MAX_BITS];
> > > >
> > > > +static int clint_add_root_memregion(struct clint_data *clint)
> > > > +{
> > > > +     struct sbi_domain_memregion reg;
> > > > +
> > > > +     sbi_domain_memregion_init(clint->addr, CLINT_SIZE,
> > > > +                               SBI_DOMAIN_MEMREGION_MMIO, &reg);
> > > > +     return sbi_domain_root_add_memregion(&reg, true);
> > > > +}
> > > > +
> > > >  void clint_ipi_send(u32 target_hart)
> > > >  {
> > > >       struct clint_data *clint;
> > > > @@ -70,7 +81,8 @@ int clint_cold_ipi_init(struct clint_data
> > > > *clint)
> > > >       for (i = 0; i < clint->hart_count; i++)
> > > >               clint_ipi_hartid2data[clint->first_hartid + i] =
> > > > clint;
> > > >
> > > > -     return 0;
> > > > +     /* Add CLINT region to the root domain */
> > > > +     return clint_add_root_memregion(clint);
> > > >  }
> > > >
> > > >  static struct clint_data
> > > > *clint_timer_hartid2data[SBI_HARTMASK_MAX_BITS];
> > > > @@ -199,5 +211,6 @@ int clint_cold_timer_init(struct clint_data
> > > > *clint,
> > > >       for (i = 0; i < clint->hart_count; i++)
> > > >               clint_timer_hartid2data[clint->first_hartid + i] =
> > > > clint;
> > > >
> > > > -     return 0;
> > > > +     /* Add CLINT region to the root domain */
> > > > +     return clint_add_root_memregion(clint);
> > > >  }
> > > > --
> > > > 2.25.1
> > > >
> > > >
>