[PATCH 0/9] Split region permissions into M-mode and SU-mode

[PATCH 0/9] Split region permissions into M-mode and SU-mode

[Himanshu Chauhan]

This is first series in the series of forthcoming patches that
would add support for different access permission for different
sections of OpenSBI (RO, RX, RW). Eventually, this would lead
to support of smepmp extension.

This patch series splits the current R/W/X permissions to
M-mode R/W/X and SU-mode R/W/X. The split is in such a way
that current permission semantics are not broken.


Himanshu Chauhan (9):
  include: sbi: Fine grain the permissions for M and SU modes
  lib: sbi: Use finer permission semantics for address validation
  lib: sbi: Add permissions for the firmware start till end
  lib: sbi: Use finer permission sematics to decide on PMP bits
  lib: sbi: Modify the boot time region flag prints
  lib: utils: Use SU-{R/W/X} flags for region permissions during parsing
  lib: utils: Disallow non-root domains from adding M-mode regions
  lib: utils: Add M-mode {R/W} flags to the MMIO regions
  docs: Update domain's region permissions and requirements

 docs/domain_support.md          |  7 +++--
 include/sbi/sbi_domain.h        | 45 ++++++++++++++++++++++++++++----
 lib/sbi/sbi_domain.c            | 46 +++++++++++++++++++++++----------
 lib/sbi/sbi_hart.c              | 16 ++++++++----
 lib/utils/fdt/fdt_domain.c      | 20 +++++++++++---
 lib/utils/ipi/aclint_mswi.c     |  4 ++-
 lib/utils/irqchip/aplic.c       |  4 ++-
 lib/utils/irqchip/imsic.c       |  4 ++-
 lib/utils/timer/aclint_mtimer.c | 16 +++++++++---
 9 files changed, 127 insertions(+), 35 deletions(-)

-- 
2.39.0

[PATCH 1/9] include: sbi: Fine grain the permissions for M and SU modes

[Himanshu Chauhan]

Split the permissions for M-mode and SU-mode. This would
help if different sections of OpenSBI need to be given
different permissions and if M-mode has different permisssions
than the SU-mode over a region.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 include/sbi/sbi_domain.h | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index 5553d21..955ffa3 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -36,11 +36,41 @@ struct sbi_domain_memregion {
 	 */
 	unsigned long base;
 	/** Flags representing memory region attributes */
-#define SBI_DOMAIN_MEMREGION_READABLE		(1UL << 0)
-#define SBI_DOMAIN_MEMREGION_WRITEABLE		(1UL << 1)
-#define SBI_DOMAIN_MEMREGION_EXECUTABLE		(1UL << 2)
-#define SBI_DOMAIN_MEMREGION_MMODE		(1UL << 3)
-#define SBI_DOMAIN_MEMREGION_ACCESS_MASK	(0xfUL)
+#define SBI_DOMAIN_MEMREGION_M_READABLE		(1UL << 0)
+#define SBI_DOMAIN_MEMREGION_M_WRITABLE		(1UL << 1)
+#define SBI_DOMAIN_MEMREGION_M_EXECUTABLE	(1UL << 2)
+#define SBI_DOMAIN_MEMREGION_SU_READABLE	(1UL << 3)
+#define SBI_DOMAIN_MEMREGION_SU_WRITABLE	(1UL << 4)
+#define SBI_DOMAIN_MEMREGION_SU_EXECUTABLE	(1UL << 5)
+
+	/** Bit to control if permissions are enforced on all modes */
+#define SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS	(1UL << 6)
+
+#define SBI_DOMAIN_MEMREGION_M_RWX		(SBI_DOMAIN_MEMREGION_M_READABLE | \
+						 SBI_DOMAIN_MEMREGION_M_WRITABLE | \
+						 SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
+
+	/* Unrestricted M-mode accesses but enfoced on SU-mode */
+#define SBI_DOMAIN_MEMREGION_READABLE		(SBI_DOMAIN_MEMREGION_SU_READABLE | \
+						 SBI_DOMAIN_MEMREGION_M_RWX)
+#define SBI_DOMAIN_MEMREGION_WRITEABLE		(SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
+						 SBI_DOMAIN_MEMREGION_M_RWX)
+#define SBI_DOMAIN_MEMREGION_EXECUTABLE		(SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
+						 SBI_DOMAIN_MEMREGION_M_RWX)
+
+	/* Enforced accesses across all modes */
+#define SBI_DOMAIN_MEMREGION_ENF_READABLE	(SBI_DOMAIN_MEMREGION_SU_READABLE | \
+						 SBI_DOMAIN_MEMREGION_M_READABLE)
+#define SBI_DOMAIN_MEMREGION_ENF_WRITABLE	(SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
+						 SBI_DOMAIN_MEMREGION_M_WRITABLE)
+#define SBI_DOMAIN_MEMREGION_ENF_EXECUTABLE	(SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
+						 SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
+
+#define SBI_DOMAIN_MEMREGION_ACCESS_MASK	(0x3fUL)
+#define SBI_DOMAIN_MEMREGION_M_ACCESS_MASK	(0x7UL)
+#define SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK	(0x38UL)
+
+#define SBI_DOMAIN_MEMREGION_SU_ACCESS_SHIFT	(3)
 
 #define SBI_DOMAIN_MEMREGION_MMIO		(1UL << 31)
 	unsigned long flags;
-- 
2.39.0

[PATCH 2/9] lib: sbi: Use finer permission semantics for address validation

[Himanshu Chauhan]

Use the fine grained permisssion semantics for address validation
of a given region.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 lib/sbi/sbi_domain.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 3205595..8f9306c 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -107,24 +107,33 @@ bool sbi_domain_check_addr(const struct sbi_domain *dom,
 {
 	bool rmmio, mmio = FALSE;
 	struct sbi_domain_memregion *reg;
-	unsigned long rstart, rend, rflags, rwx = 0;
+	unsigned long rstart, rend, rflags, rwx = 0, rrwx = 0;
 
 	if (!dom)
 		return FALSE;
 
+	/*
+	 * Use M_{R/W/X} bits because the SU-bits are at the
+	 * same relative offsets. If the mode is not M, the SU
+	 * bits will fall at same offsets after the shift.
+	 */
 	if (access_flags & SBI_DOMAIN_READ)
-		rwx |= SBI_DOMAIN_MEMREGION_READABLE;
+		rwx |= SBI_DOMAIN_MEMREGION_M_READABLE;
+
 	if (access_flags & SBI_DOMAIN_WRITE)
-		rwx |= SBI_DOMAIN_MEMREGION_WRITEABLE;
+		rwx |= SBI_DOMAIN_MEMREGION_M_WRITABLE;
+
 	if (access_flags & SBI_DOMAIN_EXECUTE)
-		rwx |= SBI_DOMAIN_MEMREGION_EXECUTABLE;
+		rwx |= SBI_DOMAIN_MEMREGION_M_EXECUTABLE;
+
 	if (access_flags & SBI_DOMAIN_MMIO)
 		mmio = TRUE;
 
 	sbi_domain_for_each_memregion(dom, reg) {
 		rflags = reg->flags;
-		if (mode == PRV_M && !(rflags & SBI_DOMAIN_MEMREGION_MMODE))
-			continue;
+		rrwx = (mode == PRV_M ? (rflags & SBI_DOMAIN_MEMREGION_M_ACCESS_MASK)
+			: (rflags & SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK)
+			>> SBI_DOMAIN_MEMREGION_SU_ACCESS_SHIFT);
 
 		rstart = reg->base;
 		rend = (reg->order < __riscv_xlen) ?
@@ -133,7 +142,7 @@ bool sbi_domain_check_addr(const struct sbi_domain *dom,
 			rmmio = (rflags & SBI_DOMAIN_MEMREGION_MMIO) ? TRUE : FALSE;
 			if (mmio != rmmio)
 				return FALSE;
-			return ((rflags & rwx) == rwx) ? TRUE : FALSE;
+			return ((rrwx & rwx) == rwx) ? TRUE : FALSE;
 		}
 	}
 
-- 
2.39.0

[PATCH 3/9] lib: sbi: Add permissions for the firmware start till end

[Himanshu Chauhan]

Change the zero flag to M-mode R/W/X flag for the firmware
region.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 lib/sbi/sbi_domain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 8f9306c..01eff18 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -628,7 +628,8 @@ 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 */
-	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size, 0,
+	sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
+				  SBI_DOMAIN_MEMREGION_M_RWX,
 				  &root_fw_region);
 	domain_memregion_initfw(&root_memregs[root_memregs_count++]);
 
-- 
2.39.0

[PATCH 4/9] lib: sbi: Use finer permission sematics to decide on PMP bits

[Himanshu Chauhan]

Use the fine grained permission bits to decide if the region
permissions are to enforced on all modes. Also use the new
permission bits for deciding on R/W/X bits in pmpcfg register.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 lib/sbi/sbi_hart.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index 5447c52..2ded55b 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -303,14 +303,20 @@ int sbi_hart_pmp_configure(struct sbi_scratch *scratch)
 			break;
 
 		pmp_flags = 0;
-		if (reg->flags & SBI_DOMAIN_MEMREGION_READABLE)
+
+		/*
+		 * 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_WRITEABLE)
+		if (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE)
 			pmp_flags |= PMP_W;
-		if (reg->flags & SBI_DOMAIN_MEMREGION_EXECUTABLE)
+		if (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)
 			pmp_flags |= PMP_X;
-		if (reg->flags & SBI_DOMAIN_MEMREGION_MMODE)
-			pmp_flags |= PMP_L;
 
 		pmp_addr =  reg->base >> PMP_SHIFT;
 		if (pmp_gran_log2 <= reg->order && pmp_addr < pmp_addr_max)
-- 
2.39.0

[PATCH 5/9] lib: sbi: Modify the boot time region flag prints

[Himanshu Chauhan]

With the finer permission semantics, the region access
permissions must be displayed separately for M and SU mode.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 lib/sbi/sbi_domain.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 01eff18..39a00f3 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -344,15 +344,25 @@ void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix)
 			   dom->index, i, suffix, rstart, rend);
 
 		k = 0;
-		if (reg->flags & SBI_DOMAIN_MEMREGION_MMODE)
-			sbi_printf("%cM", (k++) ? ',' : '(');
+
+		sbi_printf("M: ");
 		if (reg->flags & SBI_DOMAIN_MEMREGION_MMIO)
 			sbi_printf("%cI", (k++) ? ',' : '(');
-		if (reg->flags & SBI_DOMAIN_MEMREGION_READABLE)
+		if (reg->flags & SBI_DOMAIN_MEMREGION_M_READABLE)
+			sbi_printf("%cR", (k++) ? ',' : '(');
+		if (reg->flags & SBI_DOMAIN_MEMREGION_M_WRITABLE)
+			sbi_printf("%cW", (k++) ? ',' : '(');
+		if (reg->flags & SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
+			sbi_printf("%cX", (k++) ? ',' : '(');
+		sbi_printf("%s ", (k++) ? ")" : "()");
+
+		k = 0;
+		sbi_printf("S/U: ");
+		if (reg->flags & SBI_DOMAIN_MEMREGION_SU_READABLE)
 			sbi_printf("%cR", (k++) ? ',' : '(');
-		if (reg->flags & SBI_DOMAIN_MEMREGION_WRITEABLE)
+		if (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE)
 			sbi_printf("%cW", (k++) ? ',' : '(');
-		if (reg->flags & SBI_DOMAIN_MEMREGION_EXECUTABLE)
+		if (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)
 			sbi_printf("%cX", (k++) ? ',' : '(');
 		sbi_printf("%s\n", (k++) ? ")" : "()");
 
-- 
2.39.0

[PATCH 6/9] lib: utils: Use SU-{R/W/X} flags for region permissions during parsing

[Himanshu Chauhan]

Use the newer SU-{R/W/X} flags for checking and assigning region
permissions.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 lib/utils/fdt/fdt_domain.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 0568603..f979343 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -338,9 +338,9 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque)
 	 * 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))
+		if ((reg->flags & SBI_DOMAIN_MEMREGION_SU_READABLE) ||
+		    (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE) ||
+		    (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE))
 			continue;
 		if (FDT_DOMAIN_REGION_MAX_COUNT <= val32)
 			return SBI_EINVAL;
-- 
2.39.0

[PATCH 7/9] lib: utils: Disallow non-root domains from adding M-mode regions

[Himanshu Chauhan]

The M-mode regions can only be added by root domain. The non-root
domains shouldn't be able to add them from FDT.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 include/sbi/sbi_domain.h   |  5 +++++
 lib/utils/fdt/fdt_domain.c | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
index 955ffa3..3f5d28e 100644
--- a/include/sbi/sbi_domain.h
+++ b/include/sbi/sbi_domain.h
@@ -50,6 +50,11 @@ struct sbi_domain_memregion {
 						 SBI_DOMAIN_MEMREGION_M_WRITABLE | \
 						 SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
 
+#define SBI_DOMAIN_MEMREGION_SU_RWX		(SBI_DOMAIN_MEMREGION_SU_READABLE | \
+						 SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
+						 SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)
+
+
 	/* Unrestricted M-mode accesses but enfoced on SU-mode */
 #define SBI_DOMAIN_MEMREGION_READABLE		(SBI_DOMAIN_MEMREGION_SU_READABLE | \
 						 SBI_DOMAIN_MEMREGION_M_RWX)
diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index f979343..838aeca 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -239,6 +239,20 @@ static int __fdt_parse_region(void *fdt, int domain_offset,
 	u32 *region_count = opaque;
 	struct sbi_domain_memregion *region;
 
+	/*
+	 * Non-root domains cannot add a region with only M-mode
+	 * access permissions. M-mode regions can only be part of
+	 * root domain.
+	 *
+	 * SU permission bits can't be all zeroes and M-mode permission
+	 * bits must be all set.
+	 */
+	if (!((region_access & SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK)
+	     & SBI_DOMAIN_MEMREGION_SU_RWX)
+	    && ((region_access & SBI_DOMAIN_MEMREGION_M_ACCESS_MASK)
+		& SBI_DOMAIN_MEMREGION_M_RWX))
+		return SBI_EINVAL;
+
 	/* Find next region of the domain */
 	if (FDT_DOMAIN_REGION_MAX_COUNT <= *region_count)
 		return SBI_EINVAL;
-- 
2.39.0

[PATCH 8/9] lib: utils: Add M-mode {R/W} flags to the MMIO regions

[Himanshu Chauhan]

Add the M-mode readable/writable flags to mmio regions
of various drivers.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 lib/utils/ipi/aclint_mswi.c     |  4 +++-
 lib/utils/irqchip/aplic.c       |  4 +++-
 lib/utils/irqchip/imsic.c       |  4 +++-
 lib/utils/timer/aclint_mtimer.c | 16 ++++++++++++----
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
index 832e223..c714a89 100644
--- a/lib/utils/ipi/aclint_mswi.c
+++ b/lib/utils/ipi/aclint_mswi.c
@@ -88,7 +88,9 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
 		region_size = ((mswi->size - pos) < ACLINT_MSWI_ALIGN) ?
 			      (mswi->size - pos) : ACLINT_MSWI_ALIGN;
 		sbi_domain_memregion_init(mswi->addr + pos, region_size,
-					  SBI_DOMAIN_MEMREGION_MMIO, &reg);
+					  (SBI_DOMAIN_MEMREGION_MMIO |
+					   SBI_DOMAIN_MEMREGION_M_READABLE |
+					   SBI_DOMAIN_MEMREGION_M_WRITABLE), &reg);
 		rc = sbi_domain_root_add_memregion(&reg);
 		if (rc)
 			return rc;
diff --git a/lib/utils/irqchip/aplic.c b/lib/utils/irqchip/aplic.c
index 0a8469b..858e9b2 100644
--- a/lib/utils/irqchip/aplic.c
+++ b/lib/utils/irqchip/aplic.c
@@ -269,7 +269,9 @@ int aplic_cold_irqchip_init(struct aplic_data *aplic)
 	    (last_deleg_irq == aplic->num_source) &&
 	    (first_deleg_irq == 1))) {
 		sbi_domain_memregion_init(aplic->addr, aplic->size,
-					  SBI_DOMAIN_MEMREGION_MMIO, &reg);
+					  (SBI_DOMAIN_MEMREGION_MMIO |
+					   SBI_DOMAIN_MEMREGION_M_READABLE |
+					   SBI_DOMAIN_MEMREGION_M_WRITABLE), &reg);
 		rc = sbi_domain_root_add_memregion(&reg);
 		if (rc)
 			return rc;
diff --git a/lib/utils/irqchip/imsic.c b/lib/utils/irqchip/imsic.c
index 98f2cb6..ada4f3b 100644
--- a/lib/utils/irqchip/imsic.c
+++ b/lib/utils/irqchip/imsic.c
@@ -313,7 +313,9 @@ int imsic_cold_irqchip_init(struct imsic_data *imsic)
 	for (i = 0; i < IMSIC_MAX_REGS && imsic->regs[i].size; i++) {
 		sbi_domain_memregion_init(imsic->regs[i].addr,
 					  imsic->regs[i].size,
-					  SBI_DOMAIN_MEMREGION_MMIO, &reg);
+					  (SBI_DOMAIN_MEMREGION_MMIO |
+					   SBI_DOMAIN_MEMREGION_M_READABLE |
+					   SBI_DOMAIN_MEMREGION_M_WRITABLE), &reg);
 		rc = sbi_domain_root_add_memregion(&reg);
 		if (rc)
 			return rc;
diff --git a/lib/utils/timer/aclint_mtimer.c b/lib/utils/timer/aclint_mtimer.c
index 1846a9a..84ded4e 100644
--- a/lib/utils/timer/aclint_mtimer.c
+++ b/lib/utils/timer/aclint_mtimer.c
@@ -188,26 +188,34 @@ int aclint_mtimer_cold_init(struct aclint_mtimer_data *mt,
 		rc = sbi_domain_root_add_memrange(mt->mtimecmp_addr,
 					mt->mtime_size + mt->mtimecmp_size,
 					MTIMER_REGION_ALIGN,
-					SBI_DOMAIN_MEMREGION_MMIO);
+					(SBI_DOMAIN_MEMREGION_MMIO |
+					 SBI_DOMAIN_MEMREGION_M_READABLE |
+					 SBI_DOMAIN_MEMREGION_M_WRITABLE));
 		if (rc)
 			return rc;
 	} else if (mt->mtimecmp_addr == (mt->mtime_addr + mt->mtime_size)) {
 		rc = sbi_domain_root_add_memrange(mt->mtime_addr,
 					mt->mtime_size + mt->mtimecmp_size,
 					MTIMER_REGION_ALIGN,
-					SBI_DOMAIN_MEMREGION_MMIO);
+					(SBI_DOMAIN_MEMREGION_MMIO |
+					 SBI_DOMAIN_MEMREGION_M_READABLE |
+					 SBI_DOMAIN_MEMREGION_M_WRITABLE));
 		if (rc)
 			return rc;
 	} else {
 		rc = sbi_domain_root_add_memrange(mt->mtime_addr,
 						mt->mtime_size, MTIMER_REGION_ALIGN,
-						SBI_DOMAIN_MEMREGION_MMIO);
+						(SBI_DOMAIN_MEMREGION_MMIO |
+						 SBI_DOMAIN_MEMREGION_M_READABLE |
+						 SBI_DOMAIN_MEMREGION_M_WRITABLE));
 		if (rc)
 			return rc;
 
 		rc = sbi_domain_root_add_memrange(mt->mtimecmp_addr,
 						mt->mtimecmp_size, MTIMER_REGION_ALIGN,
-						SBI_DOMAIN_MEMREGION_MMIO);
+						(SBI_DOMAIN_MEMREGION_MMIO |
+						 SBI_DOMAIN_MEMREGION_M_READABLE |
+						 SBI_DOMAIN_MEMREGION_M_WRITABLE));
 		if (rc)
 			return rc;
 	}
-- 
2.39.0

[PATCH 9/9] docs: Update domain's region permissions and requirements

[Himanshu Chauhan]

Updated the various permissions bits available for domains
defined in DT node and restrictions on them.

Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
 docs/domain_support.md | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/docs/domain_support.md b/docs/domain_support.md
index 8963b57..e533bf7 100644
--- a/docs/domain_support.md
+++ b/docs/domain_support.md
@@ -160,8 +160,11 @@ The DT properties of a domain instance DT node are as follows:
 * **regions** (Optional) - The list of domain memory region DT node phandle
   and access permissions for the domain instance. Each list entry is a pair
   of DT node phandle and access permissions. The access permissions are
-  represented as a 32bit bitmask having bits: **readable** (BIT[0]),
-  **writeable** (BIT[1]), **executable** (BIT[2]), and **m-mode** (BIT[3]).
+  represented as a 32bit bitmask having bits: **M readable** (BIT[0]),
+  **M writeable** (BIT[1]), **M executable** (BIT[2]), **SU readable**
+  (BIT[3]), **SU writable** (BIT[4]), and **SU executable** (BIT[5]).
+  Any region of a domain defined in DT node cannot have only M-bits set
+  in access permissions i.e. it cannot be an m-mode only accessible region.
 * **boot-hart** (Optional) - The DT node phandle of the HART booting the
   domain instance. If coldboot HART is assigned to the domain instance then
   this DT property is ignored and the coldboot HART is assumed to be the
-- 
2.39.0

[PATCH 1/9] include: sbi: Fine grain the permissions for M and SU modes

[Anup Patel]

On Tue, Dec 20, 2022 at 4:16 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> Split the permissions for M-mode and SU-mode. This would
> help if different sections of OpenSBI need to be given
> different permissions and if M-mode has different permisssions
> than the SU-mode over a region.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  include/sbi/sbi_domain.h | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index 5553d21..955ffa3 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -36,11 +36,41 @@ struct sbi_domain_memregion {
>          */
>         unsigned long base;
>         /** Flags representing memory region attributes */
> -#define SBI_DOMAIN_MEMREGION_READABLE          (1UL << 0)
> -#define SBI_DOMAIN_MEMREGION_WRITEABLE         (1UL << 1)
> -#define SBI_DOMAIN_MEMREGION_EXECUTABLE                (1UL << 2)
> -#define SBI_DOMAIN_MEMREGION_MMODE             (1UL << 3)
> -#define SBI_DOMAIN_MEMREGION_ACCESS_MASK       (0xfUL)
> +#define SBI_DOMAIN_MEMREGION_M_READABLE                (1UL << 0)
> +#define SBI_DOMAIN_MEMREGION_M_WRITABLE                (1UL << 1)
> +#define SBI_DOMAIN_MEMREGION_M_EXECUTABLE      (1UL << 2)
> +#define SBI_DOMAIN_MEMREGION_SU_READABLE       (1UL << 3)
> +#define SBI_DOMAIN_MEMREGION_SU_WRITABLE       (1UL << 4)
> +#define SBI_DOMAIN_MEMREGION_SU_EXECUTABLE     (1UL << 5)
> +
> +       /** Bit to control if permissions are enforced on all modes */
> +#define SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS   (1UL << 6)
> +
> +#define SBI_DOMAIN_MEMREGION_M_RWX             (SBI_DOMAIN_MEMREGION_M_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
> +
> +       /* Unrestricted M-mode accesses but enfoced on SU-mode */
> +#define SBI_DOMAIN_MEMREGION_READABLE          (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> +#define SBI_DOMAIN_MEMREGION_WRITEABLE         (SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> +#define SBI_DOMAIN_MEMREGION_EXECUTABLE                (SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> +
> +       /* Enforced accesses across all modes */
> +#define SBI_DOMAIN_MEMREGION_ENF_READABLE      (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_READABLE)
> +#define SBI_DOMAIN_MEMREGION_ENF_WRITABLE      (SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE)
> +#define SBI_DOMAIN_MEMREGION_ENF_EXECUTABLE    (SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_EXECUTABLE)

Small nit: Please try to keep each line within 80 characters

> +
> +#define SBI_DOMAIN_MEMREGION_ACCESS_MASK       (0x3fUL)
> +#define SBI_DOMAIN_MEMREGION_M_ACCESS_MASK     (0x7UL)
> +#define SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK    (0x38UL)
> +
> +#define SBI_DOMAIN_MEMREGION_SU_ACCESS_SHIFT   (3)
>
>  #define SBI_DOMAIN_MEMREGION_MMIO              (1UL << 31)
>         unsigned long flags;
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Otherwise, it looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

[PATCH 2/9] lib: sbi: Use finer permission semantics for address validation

[Anup Patel]

On Tue, Dec 20, 2022 at 4:17 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> Use the fine grained permisssion semantics for address validation
> of a given region.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  lib/sbi/sbi_domain.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 3205595..8f9306c 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -107,24 +107,33 @@ bool sbi_domain_check_addr(const struct sbi_domain *dom,
>  {
>         bool rmmio, mmio = FALSE;
>         struct sbi_domain_memregion *reg;
> -       unsigned long rstart, rend, rflags, rwx = 0;
> +       unsigned long rstart, rend, rflags, rwx = 0, rrwx = 0;
>
>         if (!dom)
>                 return FALSE;
>
> +       /*
> +        * Use M_{R/W/X} bits because the SU-bits are at the
> +        * same relative offsets. If the mode is not M, the SU
> +        * bits will fall at same offsets after the shift.
> +        */
>         if (access_flags & SBI_DOMAIN_READ)
> -               rwx |= SBI_DOMAIN_MEMREGION_READABLE;
> +               rwx |= SBI_DOMAIN_MEMREGION_M_READABLE;
> +
>         if (access_flags & SBI_DOMAIN_WRITE)
> -               rwx |= SBI_DOMAIN_MEMREGION_WRITEABLE;
> +               rwx |= SBI_DOMAIN_MEMREGION_M_WRITABLE;
> +
>         if (access_flags & SBI_DOMAIN_EXECUTE)
> -               rwx |= SBI_DOMAIN_MEMREGION_EXECUTABLE;
> +               rwx |= SBI_DOMAIN_MEMREGION_M_EXECUTABLE;
> +
>         if (access_flags & SBI_DOMAIN_MMIO)
>                 mmio = TRUE;
>
>         sbi_domain_for_each_memregion(dom, reg) {
>                 rflags = reg->flags;
> -               if (mode == PRV_M && !(rflags & SBI_DOMAIN_MEMREGION_MMODE))
> -                       continue;
> +               rrwx = (mode == PRV_M ? (rflags & SBI_DOMAIN_MEMREGION_M_ACCESS_MASK)
> +                       : (rflags & SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK)
> +                       >> SBI_DOMAIN_MEMREGION_SU_ACCESS_SHIFT);
>
>                 rstart = reg->base;
>                 rend = (reg->order < __riscv_xlen) ?
> @@ -133,7 +142,7 @@ bool sbi_domain_check_addr(const struct sbi_domain *dom,
>                         rmmio = (rflags & SBI_DOMAIN_MEMREGION_MMIO) ? TRUE : FALSE;
>                         if (mmio != rmmio)
>                                 return FALSE;
> -                       return ((rflags & rwx) == rwx) ? TRUE : FALSE;
> +                       return ((rrwx & rwx) == rwx) ? TRUE : FALSE;
>                 }
>         }
>
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 3/9] lib: sbi: Add permissions for the firmware start till end

[Anup Patel]

On Tue, Dec 20, 2022 at 4:17 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> Change the zero flag to M-mode R/W/X flag for the firmware
> region.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  lib/sbi/sbi_domain.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 8f9306c..01eff18 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -628,7 +628,8 @@ 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 */
> -       sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size, 0,
> +       sbi_domain_memregion_init(scratch->fw_start, scratch->fw_size,
> +                                 SBI_DOMAIN_MEMREGION_M_RWX,
>                                   &root_fw_region);
>         domain_memregion_initfw(&root_memregs[root_memregs_count++]);
>
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 4/9] lib: sbi: Use finer permission sematics to decide on PMP bits

[Anup Patel]

On Tue, Dec 20, 2022 at 4:17 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> Use the fine grained permission bits to decide if the region
> permissions are to enforced on all modes. Also use the new

s/are to/are to be/

> permission bits for deciding on R/W/X bits in pmpcfg register.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>

Otherwise, looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  lib/sbi/sbi_hart.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
> index 5447c52..2ded55b 100644
> --- a/lib/sbi/sbi_hart.c
> +++ b/lib/sbi/sbi_hart.c
> @@ -303,14 +303,20 @@ int sbi_hart_pmp_configure(struct sbi_scratch *scratch)
>                         break;
>
>                 pmp_flags = 0;
> -               if (reg->flags & SBI_DOMAIN_MEMREGION_READABLE)
> +
> +               /*
> +                * 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_WRITEABLE)
> +               if (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE)
>                         pmp_flags |= PMP_W;
> -               if (reg->flags & SBI_DOMAIN_MEMREGION_EXECUTABLE)
> +               if (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)
>                         pmp_flags |= PMP_X;
> -               if (reg->flags & SBI_DOMAIN_MEMREGION_MMODE)
> -                       pmp_flags |= PMP_L;
>
>                 pmp_addr =  reg->base >> PMP_SHIFT;
>                 if (pmp_gran_log2 <= reg->order && pmp_addr < pmp_addr_max)
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 5/9] lib: sbi: Modify the boot time region flag prints

[Anup Patel]

On Tue, Dec 20, 2022 at 4:17 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> With the finer permission semantics, the region access
> permissions must be displayed separately for M and SU mode.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  lib/sbi/sbi_domain.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
> index 01eff18..39a00f3 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -344,15 +344,25 @@ void sbi_domain_dump(const struct sbi_domain *dom, const char *suffix)
>                            dom->index, i, suffix, rstart, rend);
>
>                 k = 0;
> -               if (reg->flags & SBI_DOMAIN_MEMREGION_MMODE)
> -                       sbi_printf("%cM", (k++) ? ',' : '(');
> +
> +               sbi_printf("M: ");
>                 if (reg->flags & SBI_DOMAIN_MEMREGION_MMIO)
>                         sbi_printf("%cI", (k++) ? ',' : '(');
> -               if (reg->flags & SBI_DOMAIN_MEMREGION_READABLE)
> +               if (reg->flags & SBI_DOMAIN_MEMREGION_M_READABLE)
> +                       sbi_printf("%cR", (k++) ? ',' : '(');
> +               if (reg->flags & SBI_DOMAIN_MEMREGION_M_WRITABLE)
> +                       sbi_printf("%cW", (k++) ? ',' : '(');
> +               if (reg->flags & SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
> +                       sbi_printf("%cX", (k++) ? ',' : '(');
> +               sbi_printf("%s ", (k++) ? ")" : "()");
> +
> +               k = 0;
> +               sbi_printf("S/U: ");
> +               if (reg->flags & SBI_DOMAIN_MEMREGION_SU_READABLE)
>                         sbi_printf("%cR", (k++) ? ',' : '(');
> -               if (reg->flags & SBI_DOMAIN_MEMREGION_WRITEABLE)
> +               if (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE)
>                         sbi_printf("%cW", (k++) ? ',' : '(');
> -               if (reg->flags & SBI_DOMAIN_MEMREGION_EXECUTABLE)
> +               if (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)
>                         sbi_printf("%cX", (k++) ? ',' : '(');
>                 sbi_printf("%s\n", (k++) ? ")" : "()");
>
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 6/9] lib: utils: Use SU-{R/W/X} flags for region permissions during parsing

[Anup Patel]

On Tue, Dec 20, 2022 at 4:17 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> Use the newer SU-{R/W/X} flags for checking and assigning region
> permissions.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  lib/utils/fdt/fdt_domain.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 0568603..f979343 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -338,9 +338,9 @@ static int __fdt_parse_domain(void *fdt, int domain_offset, void *opaque)
>          * 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))
> +               if ((reg->flags & SBI_DOMAIN_MEMREGION_SU_READABLE) ||
> +                   (reg->flags & SBI_DOMAIN_MEMREGION_SU_WRITABLE) ||
> +                   (reg->flags & SBI_DOMAIN_MEMREGION_SU_EXECUTABLE))
>                         continue;
>                 if (FDT_DOMAIN_REGION_MAX_COUNT <= val32)
>                         return SBI_EINVAL;
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 7/9] lib: utils: Disallow non-root domains from adding M-mode regions

[Anup Patel]

On Tue, Dec 20, 2022 at 4:17 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> The M-mode regions can only be added by root domain. The non-root

s/be added by/be added to/

> domains shouldn't be able to add them from FDT.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  include/sbi/sbi_domain.h   |  5 +++++
>  lib/utils/fdt/fdt_domain.c | 14 ++++++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h
> index 955ffa3..3f5d28e 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -50,6 +50,11 @@ struct sbi_domain_memregion {
>                                                  SBI_DOMAIN_MEMREGION_M_WRITABLE | \
>                                                  SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
>
> +#define SBI_DOMAIN_MEMREGION_SU_RWX            (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> +                                                SBI_DOMAIN_MEMREGION_SU_EXECUTABLE)

Small nit: Please ensure line is within 80 characters

> +
> +

Redundant new line.

>         /* Unrestricted M-mode accesses but enfoced on SU-mode */
>  #define SBI_DOMAIN_MEMREGION_READABLE          (SBI_DOMAIN_MEMREGION_SU_READABLE | \
>                                                  SBI_DOMAIN_MEMREGION_M_RWX)
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index f979343..838aeca 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -239,6 +239,20 @@ static int __fdt_parse_region(void *fdt, int domain_offset,
>         u32 *region_count = opaque;
>         struct sbi_domain_memregion *region;
>
> +       /*
> +        * Non-root domains cannot add a region with only M-mode
> +        * access permissions. M-mode regions can only be part of
> +        * root domain.
> +        *
> +        * SU permission bits can't be all zeroes and M-mode permission
> +        * bits must be all set.
> +        */
> +       if (!((region_access & SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK)
> +            & SBI_DOMAIN_MEMREGION_SU_RWX)
> +           && ((region_access & SBI_DOMAIN_MEMREGION_M_ACCESS_MASK)
> +               & SBI_DOMAIN_MEMREGION_M_RWX))
> +               return SBI_EINVAL;
> +
>         /* Find next region of the domain */
>         if (FDT_DOMAIN_REGION_MAX_COUNT <= *region_count)
>                 return SBI_EINVAL;
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Otherwise, it looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

[PATCH 8/9] lib: utils: Add M-mode {R/W} flags to the MMIO regions

[Anup Patel]

On Tue, Dec 20, 2022 at 4:17 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> Add the M-mode readable/writable flags to mmio regions
> of various drivers.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  lib/utils/ipi/aclint_mswi.c     |  4 +++-
>  lib/utils/irqchip/aplic.c       |  4 +++-
>  lib/utils/irqchip/imsic.c       |  4 +++-
>  lib/utils/timer/aclint_mtimer.c | 16 ++++++++++++----
>  4 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
> index 832e223..c714a89 100644
> --- a/lib/utils/ipi/aclint_mswi.c
> +++ b/lib/utils/ipi/aclint_mswi.c
> @@ -88,7 +88,9 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
>                 region_size = ((mswi->size - pos) < ACLINT_MSWI_ALIGN) ?
>                               (mswi->size - pos) : ACLINT_MSWI_ALIGN;
>                 sbi_domain_memregion_init(mswi->addr + pos, region_size,
> -                                         SBI_DOMAIN_MEMREGION_MMIO, &reg);
> +                                         (SBI_DOMAIN_MEMREGION_MMIO |
> +                                          SBI_DOMAIN_MEMREGION_M_READABLE |
> +                                          SBI_DOMAIN_MEMREGION_M_WRITABLE), &reg);
>                 rc = sbi_domain_root_add_memregion(&reg);
>                 if (rc)
>                         return rc;
> diff --git a/lib/utils/irqchip/aplic.c b/lib/utils/irqchip/aplic.c
> index 0a8469b..858e9b2 100644
> --- a/lib/utils/irqchip/aplic.c
> +++ b/lib/utils/irqchip/aplic.c
> @@ -269,7 +269,9 @@ int aplic_cold_irqchip_init(struct aplic_data *aplic)
>             (last_deleg_irq == aplic->num_source) &&
>             (first_deleg_irq == 1))) {
>                 sbi_domain_memregion_init(aplic->addr, aplic->size,
> -                                         SBI_DOMAIN_MEMREGION_MMIO, &reg);
> +                                         (SBI_DOMAIN_MEMREGION_MMIO |
> +                                          SBI_DOMAIN_MEMREGION_M_READABLE |
> +                                          SBI_DOMAIN_MEMREGION_M_WRITABLE), &reg);
>                 rc = sbi_domain_root_add_memregion(&reg);
>                 if (rc)
>                         return rc;
> diff --git a/lib/utils/irqchip/imsic.c b/lib/utils/irqchip/imsic.c
> index 98f2cb6..ada4f3b 100644
> --- a/lib/utils/irqchip/imsic.c
> +++ b/lib/utils/irqchip/imsic.c
> @@ -313,7 +313,9 @@ int imsic_cold_irqchip_init(struct imsic_data *imsic)
>         for (i = 0; i < IMSIC_MAX_REGS && imsic->regs[i].size; i++) {
>                 sbi_domain_memregion_init(imsic->regs[i].addr,
>                                           imsic->regs[i].size,
> -                                         SBI_DOMAIN_MEMREGION_MMIO, &reg);
> +                                         (SBI_DOMAIN_MEMREGION_MMIO |
> +                                          SBI_DOMAIN_MEMREGION_M_READABLE |
> +                                          SBI_DOMAIN_MEMREGION_M_WRITABLE), &reg);
>                 rc = sbi_domain_root_add_memregion(&reg);
>                 if (rc)
>                         return rc;
> diff --git a/lib/utils/timer/aclint_mtimer.c b/lib/utils/timer/aclint_mtimer.c
> index 1846a9a..84ded4e 100644
> --- a/lib/utils/timer/aclint_mtimer.c
> +++ b/lib/utils/timer/aclint_mtimer.c
> @@ -188,26 +188,34 @@ int aclint_mtimer_cold_init(struct aclint_mtimer_data *mt,
>                 rc = sbi_domain_root_add_memrange(mt->mtimecmp_addr,
>                                         mt->mtime_size + mt->mtimecmp_size,
>                                         MTIMER_REGION_ALIGN,
> -                                       SBI_DOMAIN_MEMREGION_MMIO);
> +                                       (SBI_DOMAIN_MEMREGION_MMIO |
> +                                        SBI_DOMAIN_MEMREGION_M_READABLE |
> +                                        SBI_DOMAIN_MEMREGION_M_WRITABLE));
>                 if (rc)
>                         return rc;
>         } else if (mt->mtimecmp_addr == (mt->mtime_addr + mt->mtime_size)) {
>                 rc = sbi_domain_root_add_memrange(mt->mtime_addr,
>                                         mt->mtime_size + mt->mtimecmp_size,
>                                         MTIMER_REGION_ALIGN,
> -                                       SBI_DOMAIN_MEMREGION_MMIO);
> +                                       (SBI_DOMAIN_MEMREGION_MMIO |
> +                                        SBI_DOMAIN_MEMREGION_M_READABLE |
> +                                        SBI_DOMAIN_MEMREGION_M_WRITABLE));
>                 if (rc)
>                         return rc;
>         } else {
>                 rc = sbi_domain_root_add_memrange(mt->mtime_addr,
>                                                 mt->mtime_size, MTIMER_REGION_ALIGN,
> -                                               SBI_DOMAIN_MEMREGION_MMIO);
> +                                               (SBI_DOMAIN_MEMREGION_MMIO |
> +                                                SBI_DOMAIN_MEMREGION_M_READABLE |
> +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE));
>                 if (rc)
>                         return rc;
>
>                 rc = sbi_domain_root_add_memrange(mt->mtimecmp_addr,
>                                                 mt->mtimecmp_size, MTIMER_REGION_ALIGN,
> -                                               SBI_DOMAIN_MEMREGION_MMIO);
> +                                               (SBI_DOMAIN_MEMREGION_MMIO |
> +                                                SBI_DOMAIN_MEMREGION_M_READABLE |
> +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE));
>                 if (rc)
>                         return rc;
>         }
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 9/9] docs: Update domain's region permissions and requirements

[Anup Patel]

On Tue, Dec 20, 2022 at 4:17 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> Updated the various permissions bits available for domains
> defined in DT node and restrictions on them.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  docs/domain_support.md | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/docs/domain_support.md b/docs/domain_support.md
> index 8963b57..e533bf7 100644
> --- a/docs/domain_support.md
> +++ b/docs/domain_support.md
> @@ -160,8 +160,11 @@ The DT properties of a domain instance DT node are as follows:
>  * **regions** (Optional) - The list of domain memory region DT node phandle
>    and access permissions for the domain instance. Each list entry is a pair
>    of DT node phandle and access permissions. The access permissions are
> -  represented as a 32bit bitmask having bits: **readable** (BIT[0]),
> -  **writeable** (BIT[1]), **executable** (BIT[2]), and **m-mode** (BIT[3]).
> +  represented as a 32bit bitmask having bits: **M readable** (BIT[0]),
> +  **M writeable** (BIT[1]), **M executable** (BIT[2]), **SU readable**
> +  (BIT[3]), **SU writable** (BIT[4]), and **SU executable** (BIT[5]).

I think you missed documenting BIT[6] which is for enforcing.

> +  Any region of a domain defined in DT node cannot have only M-bits set
> +  in access permissions i.e. it cannot be an m-mode only accessible region.
>  * **boot-hart** (Optional) - The DT node phandle of the HART booting the
>    domain instance. If coldboot HART is assigned to the domain instance then
>    this DT property is ignored and the coldboot HART is assumed to be the
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Regards,
Anup

[PATCH 1/9] include: sbi: Fine grain the permissions for M and SU modes

[hchauhan]

-----Original Message-----
From: Anup Patel <anup@brainfault.org> 
Sent: 06 January 2023 23:06
To: Himanshu Chauhan <hchauhan@ventanamicro.com>
Cc: opensbi at lists.infradead.org
Subject: Re: [PATCH 1/9] include: sbi: Fine grain the permissions for M and SU modes

On Tue, Dec 20, 2022 at 4:16 PM Himanshu Chauhan <hchauhan@ventanamicro.com> wrote:
>
> Split the permissions for M-mode and SU-mode. This would help if 
> different sections of OpenSBI need to be given different permissions 
> and if M-mode has different permisssions than the SU-mode over a 
> region.
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
>  include/sbi/sbi_domain.h | 40 
> +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h index 
> 5553d21..955ffa3 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -36,11 +36,41 @@ struct sbi_domain_memregion {
>          */
>         unsigned long base;
>         /** Flags representing memory region attributes */
> -#define SBI_DOMAIN_MEMREGION_READABLE          (1UL << 0)
> -#define SBI_DOMAIN_MEMREGION_WRITEABLE         (1UL << 1)
> -#define SBI_DOMAIN_MEMREGION_EXECUTABLE                (1UL << 2)
> -#define SBI_DOMAIN_MEMREGION_MMODE             (1UL << 3)
> -#define SBI_DOMAIN_MEMREGION_ACCESS_MASK       (0xfUL)
> +#define SBI_DOMAIN_MEMREGION_M_READABLE                (1UL << 0)
> +#define SBI_DOMAIN_MEMREGION_M_WRITABLE                (1UL << 1)
> +#define SBI_DOMAIN_MEMREGION_M_EXECUTABLE      (1UL << 2)
> +#define SBI_DOMAIN_MEMREGION_SU_READABLE       (1UL << 3)
> +#define SBI_DOMAIN_MEMREGION_SU_WRITABLE       (1UL << 4)
> +#define SBI_DOMAIN_MEMREGION_SU_EXECUTABLE     (1UL << 5)
> +
> +       /** Bit to control if permissions are enforced on all modes */
> +#define SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS   (1UL << 6)
> +
> +#define SBI_DOMAIN_MEMREGION_M_RWX             (SBI_DOMAIN_MEMREGION_M_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE | \
> +                                                
> +SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
> +
> +       /* Unrestricted M-mode accesses but enfoced on SU-mode */
> +#define SBI_DOMAIN_MEMREGION_READABLE          (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> +#define SBI_DOMAIN_MEMREGION_WRITEABLE         (SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> +#define SBI_DOMAIN_MEMREGION_EXECUTABLE                (SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
> +                                                
> +SBI_DOMAIN_MEMREGION_M_RWX)
> +
> +       /* Enforced accesses across all modes */
> +#define SBI_DOMAIN_MEMREGION_ENF_READABLE      (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_READABLE)
> +#define SBI_DOMAIN_MEMREGION_ENF_WRITABLE      (SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE)
> +#define SBI_DOMAIN_MEMREGION_ENF_EXECUTABLE    (SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
> +                                                
> +SBI_DOMAIN_MEMREGION_M_EXECUTABLE)

Small nit: Please try to keep each line within 80 characters

It becomes difficult to build upon existing macros having large names and keep the column restrictions. Only way to honour the column restriction is  to shorten the new and exiting names. I believe it would be too much change. Moreover, 80 column limit comes from very old terminal types. So it may be relaxed to a more sane value like 100 or so. Just my thoughts.

I can make the changes if you think restricting them to 80 column makes more sense. But that would take change to existing macro names.

Regards
Himanshu

> +
> +#define SBI_DOMAIN_MEMREGION_ACCESS_MASK       (0x3fUL)
> +#define SBI_DOMAIN_MEMREGION_M_ACCESS_MASK     (0x7UL)
> +#define SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK    (0x38UL)
> +
> +#define SBI_DOMAIN_MEMREGION_SU_ACCESS_SHIFT   (3)
>
>  #define SBI_DOMAIN_MEMREGION_MMIO              (1UL << 31)
>         unsigned long flags;
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Otherwise, it looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

[PATCH 1/9] include: sbi: Fine grain the permissions for M and SU modes

[Anup Patel]

On Mon, Jan 9, 2023 at 10:13 AM <hchauhan@ventanamicro.com> wrote:
>
>
>
> -----Original Message-----
> From: Anup Patel <anup@brainfault.org>
> Sent: 06 January 2023 23:06
> To: Himanshu Chauhan <hchauhan@ventanamicro.com>
> Cc: opensbi at lists.infradead.org
> Subject: Re: [PATCH 1/9] include: sbi: Fine grain the permissions for M and SU modes
>
> On Tue, Dec 20, 2022 at 4:16 PM Himanshu Chauhan <hchauhan@ventanamicro.com> wrote:
> >
> > Split the permissions for M-mode and SU-mode. This would help if
> > different sections of OpenSBI need to be given different permissions
> > and if M-mode has different permisssions than the SU-mode over a
> > region.
> >
> > Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> > ---
> >  include/sbi/sbi_domain.h | 40
> > +++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 35 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h index
> > 5553d21..955ffa3 100644
> > --- a/include/sbi/sbi_domain.h
> > +++ b/include/sbi/sbi_domain.h
> > @@ -36,11 +36,41 @@ struct sbi_domain_memregion {
> >          */
> >         unsigned long base;
> >         /** Flags representing memory region attributes */
> > -#define SBI_DOMAIN_MEMREGION_READABLE          (1UL << 0)
> > -#define SBI_DOMAIN_MEMREGION_WRITEABLE         (1UL << 1)
> > -#define SBI_DOMAIN_MEMREGION_EXECUTABLE                (1UL << 2)
> > -#define SBI_DOMAIN_MEMREGION_MMODE             (1UL << 3)
> > -#define SBI_DOMAIN_MEMREGION_ACCESS_MASK       (0xfUL)
> > +#define SBI_DOMAIN_MEMREGION_M_READABLE                (1UL << 0)
> > +#define SBI_DOMAIN_MEMREGION_M_WRITABLE                (1UL << 1)
> > +#define SBI_DOMAIN_MEMREGION_M_EXECUTABLE      (1UL << 2)
> > +#define SBI_DOMAIN_MEMREGION_SU_READABLE       (1UL << 3)
> > +#define SBI_DOMAIN_MEMREGION_SU_WRITABLE       (1UL << 4)
> > +#define SBI_DOMAIN_MEMREGION_SU_EXECUTABLE     (1UL << 5)
> > +
> > +       /** Bit to control if permissions are enforced on all modes */
> > +#define SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS   (1UL << 6)
> > +
> > +#define SBI_DOMAIN_MEMREGION_M_RWX             (SBI_DOMAIN_MEMREGION_M_READABLE | \
> > +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE | \
> > +
> > +SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
> > +
> > +       /* Unrestricted M-mode accesses but enfoced on SU-mode */
> > +#define SBI_DOMAIN_MEMREGION_READABLE          (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> > +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> > +#define SBI_DOMAIN_MEMREGION_WRITEABLE         (SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> > +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> > +#define SBI_DOMAIN_MEMREGION_EXECUTABLE                (SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
> > +
> > +SBI_DOMAIN_MEMREGION_M_RWX)
> > +
> > +       /* Enforced accesses across all modes */
> > +#define SBI_DOMAIN_MEMREGION_ENF_READABLE      (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> > +                                                SBI_DOMAIN_MEMREGION_M_READABLE)
> > +#define SBI_DOMAIN_MEMREGION_ENF_WRITABLE      (SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> > +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE)
> > +#define SBI_DOMAIN_MEMREGION_ENF_EXECUTABLE    (SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
> > +
> > +SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
>
> Small nit: Please try to keep each line within 80 characters
>
> It becomes difficult to build upon existing macros having large names and keep the column restrictions. Only way to honour the column restriction is  to shorten the new and exiting names. I believe it would be too much change. Moreover, 80 column limit comes from very old terminal types. So it may be relaxed to a more sane value like 100 or so. Just my thoughts.
>
> I can make the changes if you think restricting them to 80 column makes more sense. But that would take change to existing macro names.

I am not strict about the 80 column requirement. I will take care of
it at the time of merging this patch.

Regards,
Anup

>
> Regards
> Himanshu
>
> > +
> > +#define SBI_DOMAIN_MEMREGION_ACCESS_MASK       (0x3fUL)
> > +#define SBI_DOMAIN_MEMREGION_M_ACCESS_MASK     (0x7UL)
> > +#define SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK    (0x38UL)
> > +
> > +#define SBI_DOMAIN_MEMREGION_SU_ACCESS_SHIFT   (3)
> >
> >  #define SBI_DOMAIN_MEMREGION_MMIO              (1UL << 31)
> >         unsigned long flags;
> > --
> > 2.39.0
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
>
> Otherwise, it looks good to me.
>
> Reviewed-by: Anup Patel <anup@brainfault.org>
>
> Regards,
> Anup
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi