[PATCH v2 0/5] Add Svadu extension support

[PATCH v2 0/5] Add Svadu extension support

[Yong-Xuan Wang]

This series enables Svadu extension support by configuring the menvcfg
CSR and, if available, displays the Svadu extension in the boot log.

Additionally, we've made some programming improvements in
lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.

---
v2:
- Rearrange the patches to do the code refactoring first before adding
  new features
- Suggested by Anup and Atish, detect extensions from DT instead of
  menvcfg CSR
- Enable access to some extensions through menvcfg CSR if they are
  present in the device tree.

Yong-Xuan Wang (5):
  lib: sbi: Improve the code of privilege mode and extensions detection
  lib: sbi: Refactor the code for enable extensions in menvfg CSR
  lib: sbi: Using one array to define the name of extensions
  lib: sbi: Detect extensions from the ISA string in DT
  lib: sbi: Add support for Svadu extension

 include/sbi/riscv_encoding.h |   6 +-
 include/sbi/sbi_hart.h       |  15 +++
 lib/sbi/sbi_hart.c           | 226 +++++++++++++----------------------
 lib/utils/fdt/fdt_helper.c   |   5 +-
 4 files changed, 104 insertions(+), 148 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] lib: sbi: Improve the code of privilege mode and extensions detection

[Yong-Xuan Wang]

We can enhance the code by creating 2 unified interface with macro  for
privilege mode and extensions detection, which relies on supported
privilege modes and CSRs.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_hart.c | 88 ++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 50 deletions(-)

diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index 29d6481..5c52b6c 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -802,6 +802,7 @@ static int hart_detect_features(struct sbi_scratch *scratch)
 	hfeatures->extensions = 0;
 	hfeatures->pmp_count = 0;
 	hfeatures->mhpm_mask = 0;
+	hfeatures->priv_version = SBI_HART_PRIV_VER_UNKNOWN;
 
 #define __check_hpm_csr(__csr, __mask) 					  \
 	oldval = csr_read_allowed(__csr, (ulong)&trap);			  \
@@ -894,67 +895,54 @@ __pmp_skip:
 #undef __check_csr_2
 #undef __check_csr
 
-	/* Detect if hart supports Priv v1.10 */
-	val = csr_read_allowed(CSR_MCOUNTEREN, (unsigned long)&trap);
-	if (!trap.cause)
-		hfeatures->priv_version = SBI_HART_PRIV_VER_1_10;
 
-	/* Detect if hart supports Priv v1.11 */
-	val = csr_read_allowed(CSR_MCOUNTINHIBIT, (unsigned long)&trap);
-	if (!trap.cause &&
-	    (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_10))
-		hfeatures->priv_version = SBI_HART_PRIV_VER_1_11;
+#define __check_priv(__csr, __base_priv, __priv)			\
+	val = csr_read_allowed(__csr, (ulong)&trap);			\
+	if (!trap.cause && (hfeatures->priv_version >= __base_priv)) {	\
+		hfeatures->priv_version = __priv;			\
+	}
 
+	/* Detect if hart supports Priv v1.10 */
+	__check_priv(CSR_MCOUNTEREN,
+		     SBI_HART_PRIV_VER_UNKNOWN, SBI_HART_PRIV_VER_1_10);
+	/* Detect if hart supports Priv v1.11 */
+	__check_priv(CSR_MCOUNTINHIBIT,
+		     SBI_HART_PRIV_VER_1_10, SBI_HART_PRIV_VER_1_11);
 	/* Detect if hart supports Priv v1.12 */
-	csr_read_allowed(CSR_MENVCFG, (unsigned long)&trap);
-	if (!trap.cause &&
-	    (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11))
-		hfeatures->priv_version = SBI_HART_PRIV_VER_1_12;
+	__check_priv(CSR_MENVCFG,
+		     SBI_HART_PRIV_VER_1_11, SBI_HART_PRIV_VER_1_12);
 
-	/* Counter overflow/filtering is not useful without mcounter/inhibit */
-	if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) {
-		/* Detect if hart supports sscofpmf */
-		csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap);
-		if (!trap.cause)
-			__sbi_hart_update_extension(hfeatures,
-					SBI_HART_EXT_SSCOFPMF, true);
+#undef __check_priv_csr
+
+#define __check_ext_csr(__base_priv, __csr, __ext)			\
+	if (hfeatures->priv_version >= __base_priv) {			\
+		csr_read_allowed(__csr, (ulong)&trap);			\
+		if (!trap.cause)					\
+			__sbi_hart_update_extension(hfeatures,		\
+						    __ext, true);	\
 	}
 
+	/* Counter overflow/filtering is not useful without mcounter/inhibit */
+	/* Detect if hart supports sscofpmf */
+	__check_ext_csr(SBI_HART_PRIV_VER_1_11,
+		        CSR_SCOUNTOVF, SBI_HART_EXT_SSCOFPMF);
 	/* Detect if hart supports time CSR */
-	csr_read_allowed(CSR_TIME, (unsigned long)&trap);
-	if (!trap.cause)
-		__sbi_hart_update_extension(hfeatures,
-					SBI_HART_EXT_ZICNTR, true);
-
+	__check_ext_csr(SBI_HART_PRIV_VER_UNKNOWN,
+			CSR_TIME, SBI_HART_EXT_ZICNTR);
 	/* Detect if hart has AIA local interrupt CSRs */
-	csr_read_allowed(CSR_MTOPI, (unsigned long)&trap);
-	if (!trap.cause)
-		__sbi_hart_update_extension(hfeatures,
-					SBI_HART_EXT_SMAIA, true);
-
+	__check_ext_csr(SBI_HART_PRIV_VER_UNKNOWN,
+			CSR_MTOPI, SBI_HART_EXT_SMAIA);
 	/* Detect if hart supports stimecmp CSR(Sstc extension) */
-	if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) {
-		csr_read_allowed(CSR_STIMECMP, (unsigned long)&trap);
-		if (!trap.cause)
-			__sbi_hart_update_extension(hfeatures,
-					SBI_HART_EXT_SSTC, true);
-	}
-
+	__check_ext_csr(SBI_HART_PRIV_VER_1_12,
+			CSR_STIMECMP, SBI_HART_EXT_SSTC);
 	/* Detect if hart supports mstateen CSRs */
-	if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) {
-		val = csr_read_allowed(CSR_MSTATEEN0, (unsigned long)&trap);
-		if (!trap.cause)
-			__sbi_hart_update_extension(hfeatures,
-					SBI_HART_EXT_SMSTATEEN, true);
-	}
-
+	__check_ext_csr(SBI_HART_PRIV_VER_1_12,
+			CSR_MSTATEEN0, SBI_HART_EXT_SMSTATEEN);
 	/* Detect if hart supports smcntrpmf */
-	if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) {
-		csr_read_allowed(CSR_MCYCLECFG, (unsigned long)&trap);
-		if (!trap.cause)
-			__sbi_hart_update_extension(hfeatures,
-					SBI_HART_EXT_SMCNTRPMF, true);
-	}
+	__check_ext_csr(SBI_HART_PRIV_VER_1_12,
+			CSR_MCYCLECFG, SBI_HART_EXT_SMCNTRPMF);
+
+#undef __check_ext_csr
 
 	/* Let platform populate extensions */
 	rc = sbi_platform_extensions_init(sbi_platform_thishart_ptr(),
-- 
2.17.1

[PATCH v2 2/5] lib: sbi: Refactor the code for enable extensions in menvfg CSR

[Yong-Xuan Wang]

Use 1 variable to store the value of menvcfg.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 include/sbi/riscv_encoding.h |  5 -----
 lib/sbi/sbi_hart.c           | 15 ++++++---------
 2 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
index a545242..92ee0a8 100644
--- a/include/sbi/riscv_encoding.h
+++ b/include/sbi/riscv_encoding.h
@@ -207,13 +207,8 @@
 
 #define MHPMEVENT_SSCOF_MASK		_ULL(0xFFFF000000000000)
 
-#if __riscv_xlen > 32
 #define ENVCFG_STCE			(_ULL(1) << 63)
 #define ENVCFG_PBMTE			(_ULL(1) << 62)
-#else
-#define ENVCFGH_STCE			(_UL(1) << 31)
-#define ENVCFGH_PBMTE			(_UL(1) << 30)
-#endif
 #define ENVCFG_CBZE			(_UL(1) << 7)
 #define ENVCFG_CBCFE			(_UL(1) << 6)
 #define ENVCFG_CBIE_SHIFT		4
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index 5c52b6c..1589111 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -108,6 +108,9 @@ static void mstatus_init(struct sbi_scratch *scratch)
 
 	if (sbi_hart_priv_version(scratch) >= SBI_HART_PRIV_VER_1_12) {
 		menvcfg_val = csr_read(CSR_MENVCFG);
+#if __riscv_xlen == 32
+		menvcfg_val |= ((uint64_t)csr_read(CSR_MENVCFGH)) << 32;
+#endif
 
 		/*
 		 * Set menvcfg.CBZE == 1
@@ -139,9 +142,7 @@ static void mstatus_init(struct sbi_scratch *scratch)
 		 * If Svpbmt extension is not available then menvcfg.PBMTE
 		 * will be read-only zero.
 		 */
-#if __riscv_xlen > 32
 		menvcfg_val |= ENVCFG_PBMTE;
-#endif
 
 		/*
 		 * The spec doesn't explicitly describe the reset value of menvcfg.
@@ -149,17 +150,13 @@ static void mstatus_init(struct sbi_scratch *scratch)
 		 * hardware.
 		 */
 		if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSTC)) {
-#if __riscv_xlen == 32
-			unsigned long menvcfgh_val;
-			menvcfgh_val = csr_read(CSR_MENVCFGH);
-			menvcfgh_val |= ENVCFGH_STCE;
-			csr_write(CSR_MENVCFGH, menvcfgh_val);
-#else
 			menvcfg_val |= ENVCFG_STCE;
-#endif
 		}
 
 		csr_write(CSR_MENVCFG, menvcfg_val);
+#if __riscv_xlen == 32
+		csr_write(CSR_MENVCFGH, menvcfg_val >> 32);
+#endif
 	}
 
 	/* Disable all interrupts */
-- 
2.17.1

[PATCH v2 3/5] lib: sbi: Using one array to define the name of extensions

[Yong-Xuan Wang]

Define an array sbi_hart_ext to map extension ID and name , and use it
for ISA parsing and printing out the supported extensions.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
---
 include/sbi/sbi_hart.h     |  7 ++++
 lib/sbi/sbi_hart.c         | 72 ++++++++++++--------------------------
 lib/utils/fdt/fdt_helper.c |  5 ++-
 3 files changed, 34 insertions(+), 50 deletions(-)

diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
index e60f415..33bf327 100644
--- a/include/sbi/sbi_hart.h
+++ b/include/sbi/sbi_hart.h
@@ -47,6 +47,13 @@ enum sbi_hart_extensions {
 	SBI_HART_EXT_MAX,
 };
 
+struct sbi_hart_ext_data {
+	const unsigned int id;
+	const char *name;
+};
+
+extern const struct sbi_hart_ext_data sbi_hart_ext[];
+
 /*
  * Smepmp enforces access boundaries between M-mode and
  * S/U-mode. When it is enabled, the PMPs are programmed
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index 1589111..add0155 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -652,42 +652,22 @@ bool sbi_hart_has_extension(struct sbi_scratch *scratch,
 		return false;
 }
 
-static inline char *sbi_hart_extension_id2string(int ext)
-{
-	char *estr = NULL;
-
-	switch (ext) {
-	case SBI_HART_EXT_SMAIA:
-		estr = "smaia";
-		break;
-	case SBI_HART_EXT_SMSTATEEN:
-		estr = "smstateen";
-		break;
-	case SBI_HART_EXT_SSCOFPMF:
-		estr = "sscofpmf";
-		break;
-	case SBI_HART_EXT_SSTC:
-		estr = "sstc";
-		break;
-	case SBI_HART_EXT_ZICNTR:
-		estr = "zicntr";
-		break;
-	case SBI_HART_EXT_ZIHPM:
-		estr = "zihpm";
-		break;
-	case SBI_HART_EXT_SMEPMP:
-		estr = "smepmp";
-		break;
-	case SBI_HART_EXT_SMCNTRPMF:
-		estr = "smcntrpmf";
-		break;
-	default:
-		break;
-	}
-
-	return estr;
+#define __SBI_HART_EXT_DATA(_name, _id) {	\
+	.name = #_name,				\
+	.id = _id,				\
 }
 
+const struct sbi_hart_ext_data sbi_hart_ext[] = {
+	__SBI_HART_EXT_DATA(smaia, SBI_HART_EXT_SMAIA),
+	__SBI_HART_EXT_DATA(smepmp, SBI_HART_EXT_SMEPMP),
+	__SBI_HART_EXT_DATA(smstateen, SBI_HART_EXT_SMSTATEEN),
+	__SBI_HART_EXT_DATA(sscofpmf, SBI_HART_EXT_SSCOFPMF),
+	__SBI_HART_EXT_DATA(sstc, SBI_HART_EXT_SSTC),
+	__SBI_HART_EXT_DATA(zicntr, SBI_HART_EXT_ZICNTR),
+	__SBI_HART_EXT_DATA(zihpm, SBI_HART_EXT_ZIHPM),
+	__SBI_HART_EXT_DATA(smcntrpmf, SBI_HART_EXT_SMCNTRPMF),
+};
+
 /**
  * Get the hart extensions in string format
  *
@@ -702,8 +682,8 @@ void sbi_hart_get_extensions_str(struct sbi_scratch *scratch,
 {
 	struct sbi_hart_features *hfeatures =
 			sbi_scratch_offset_ptr(scratch, hart_features_offset);
-	int offset = 0, ext = 0;
-	char *temp;
+	int offset = 0;
+	size_t i;
 
 	if (!extensions_str || nestr <= 0)
 		return;
@@ -712,20 +692,14 @@ void sbi_hart_get_extensions_str(struct sbi_scratch *scratch,
 	if (!hfeatures->extensions)
 		goto done;
 
-	do {
-		if (hfeatures->extensions & BIT(ext)) {
-			temp = sbi_hart_extension_id2string(ext);
-			if (temp) {
-				sbi_snprintf(extensions_str + offset,
-					     nestr - offset,
-					     "%s,", temp);
-				offset = offset + sbi_strlen(temp) + 1;
-			}
+	for (i = 0; i < SBI_HART_EXT_MAX; i++) {
+		if (hfeatures->extensions & BIT(sbi_hart_ext[i].id)) {
+			sbi_snprintf(extensions_str + offset,
+				     nestr - offset,
+				     "%s,", sbi_hart_ext[i].name);
+			offset = offset + sbi_strlen(sbi_hart_ext[i].name) + 1;
 		}
-
-		ext++;
-	} while (ext < SBI_HART_EXT_MAX);
-
+	}
 done:
 	if (offset)
 		extensions_str[offset - 1] = '\0';
diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index 9ae7f09..9cef68a 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -375,7 +375,10 @@ static int fdt_parse_isa_one_hart(const char *isa, unsigned long *extensions)
 				continue;			\
 			}
 
-		set_multi_letter_ext("smepmp", SBI_HART_EXT_SMEPMP);
+		for (j = 0; j < SBI_HART_EXT_MAX; j++) {
+			set_multi_letter_ext(sbi_hart_ext[j].name,
+					     sbi_hart_ext[j].id);
+		}
 #undef set_multi_letter_ext
 	}
 
-- 
2.17.1

[PATCH v2 4/5] lib: sbi: Detect extensions from the ISA string in DT

[Yong-Xuan Wang]

Enable access to some extensions through menvcfg and show them in "Boot
HART ISA Extensions" if they are present in the device tree.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
---
 include/sbi/sbi_hart.h |  6 +++++
 lib/sbi/sbi_hart.c     | 51 +++++++++++++-----------------------------
 2 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
index 33bf327..0424da8 100644
--- a/include/sbi/sbi_hart.h
+++ b/include/sbi/sbi_hart.h
@@ -42,6 +42,12 @@ enum sbi_hart_extensions {
 	SBI_HART_EXT_ZIHPM,
 	/** Hart has Smcntrpmf extension */
 	SBI_HART_EXT_SMCNTRPMF,
+	/** Hart has Zicboz extension */
+	SBI_HART_EXT_ZICBOZ,
+	/** Hart has Zicbom extension */
+	SBI_HART_EXT_ZICBOM,
+	/** Hart has Svpbmt extension */
+	SBI_HART_EXT_SVPBMT,
 
 	/** Maximum index of Hart extension */
 	SBI_HART_EXT_MAX,
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index add0155..4fb5153 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -112,46 +112,24 @@ static void mstatus_init(struct sbi_scratch *scratch)
 		menvcfg_val |= ((uint64_t)csr_read(CSR_MENVCFGH)) << 32;
 #endif
 
-		/*
-		 * Set menvcfg.CBZE == 1
-		 *
-		 * If Zicboz extension is not available then writes to
-		 * menvcfg.CBZE will be ignored because it is a WARL field.
-		 */
-		menvcfg_val |= ENVCFG_CBZE;
+#define __set_menvcfg_ext(__ext, __bit)					\
+		if (sbi_hart_has_extension(scratch, __ext)) {		\
+			menvcfg_val |= __bit;				\
+		}
 
 		/*
-		 * Set menvcfg.CBCFE == 1
-		 *
-		 * If Zicbom extension is not available then writes to
-		 * menvcfg.CBCFE will be ignored because it is a WARL field.
+		* Enable access to extensions if they are present in the
+		 * hardware or in the device tree.
 		 */
-		menvcfg_val |= ENVCFG_CBCFE;
 
-		/*
-		 * Set menvcfg.CBIE == 3
-		 *
-		 * If Zicbom extension is not available then writes to
-		 * menvcfg.CBIE will be ignored because it is a WARL field.
-		 */
-		menvcfg_val |= ENVCFG_CBIE_INV << ENVCFG_CBIE_SHIFT;
+		__set_menvcfg_ext(SBI_HART_EXT_ZICBOZ, ENVCFG_CBZE);
+		__set_menvcfg_ext(SBI_HART_EXT_ZICBOM, ENVCFG_CBCFE);
+		__set_menvcfg_ext(SBI_HART_EXT_ZICBOM,
+				  ENVCFG_CBIE_INV << ENVCFG_CBIE_SHIFT);
+		__set_menvcfg_ext(SBI_HART_EXT_SVPBMT, ENVCFG_PBMTE);
+		__set_menvcfg_ext(SBI_HART_EXT_SSTC, ENVCFG_STCE);
 
-		/*
-		 * Set menvcfg.PBMTE == 1 for RV64 or RV128
-		 *
-		 * If Svpbmt extension is not available then menvcfg.PBMTE
-		 * will be read-only zero.
-		 */
-		menvcfg_val |= ENVCFG_PBMTE;
-
-		/*
-		 * The spec doesn't explicitly describe the reset value of menvcfg.
-		 * Enable access to stimecmp if sstc extension is present in the
-		 * hardware.
-		 */
-		if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSTC)) {
-			menvcfg_val |= ENVCFG_STCE;
-		}
+#undef __set_menvcfg_ext
 
 		csr_write(CSR_MENVCFG, menvcfg_val);
 #if __riscv_xlen == 32
@@ -666,6 +644,9 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
 	__SBI_HART_EXT_DATA(zicntr, SBI_HART_EXT_ZICNTR),
 	__SBI_HART_EXT_DATA(zihpm, SBI_HART_EXT_ZIHPM),
 	__SBI_HART_EXT_DATA(smcntrpmf, SBI_HART_EXT_SMCNTRPMF),
+	__SBI_HART_EXT_DATA(zicboz, SBI_HART_EXT_ZICBOZ),
+	__SBI_HART_EXT_DATA(zicbom, SBI_HART_EXT_ZICBOM),
+	__SBI_HART_EXT_DATA(svpbmt, SBI_HART_EXT_SVPBMT),
 };
 
 /**
-- 
2.17.1

[PATCH v2 5/5] lib: sbi: Add support for Svadu extension

[Yong-Xuan Wang]

This patch detects the Svadu extension from DT and enables the ADUE bit
in the menvcfg CSR for lower privilege modes.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
---
 include/sbi/riscv_encoding.h | 1 +
 include/sbi/sbi_hart.h       | 2 ++
 lib/sbi/sbi_hart.c           | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
index 92ee0a8..8a81787 100644
--- a/include/sbi/riscv_encoding.h
+++ b/include/sbi/riscv_encoding.h
@@ -209,6 +209,7 @@
 
 #define ENVCFG_STCE			(_ULL(1) << 63)
 #define ENVCFG_PBMTE			(_ULL(1) << 62)
+#define ENVCFG_ADUE			(_ULL(1) << 61)
 #define ENVCFG_CBZE			(_UL(1) << 7)
 #define ENVCFG_CBCFE			(_UL(1) << 6)
 #define ENVCFG_CBIE_SHIFT		4
diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
index 0424da8..83471c1 100644
--- a/include/sbi/sbi_hart.h
+++ b/include/sbi/sbi_hart.h
@@ -48,6 +48,8 @@ enum sbi_hart_extensions {
 	SBI_HART_EXT_ZICBOM,
 	/** Hart has Svpbmt extension */
 	SBI_HART_EXT_SVPBMT,
+	/** Hart has Svadu extension */
+	SBI_HART_EXT_SVADU,
 
 	/** Maximum index of Hart extension */
 	SBI_HART_EXT_MAX,
diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
index 4fb5153..9874e77 100644
--- a/lib/sbi/sbi_hart.c
+++ b/lib/sbi/sbi_hart.c
@@ -128,6 +128,7 @@ static void mstatus_init(struct sbi_scratch *scratch)
 				  ENVCFG_CBIE_INV << ENVCFG_CBIE_SHIFT);
 		__set_menvcfg_ext(SBI_HART_EXT_SVPBMT, ENVCFG_PBMTE);
 		__set_menvcfg_ext(SBI_HART_EXT_SSTC, ENVCFG_STCE);
+		__set_menvcfg_ext(SBI_HART_EXT_SVADU, ENVCFG_ADUE);
 
 #undef __set_menvcfg_ext
 
@@ -647,6 +648,7 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
 	__SBI_HART_EXT_DATA(zicboz, SBI_HART_EXT_ZICBOZ),
 	__SBI_HART_EXT_DATA(zicbom, SBI_HART_EXT_ZICBOM),
 	__SBI_HART_EXT_DATA(svpbmt, SBI_HART_EXT_SVPBMT),
+	__SBI_HART_EXT_DATA(svadu, SBI_HART_EXT_SVADU),
 };
 
 /**
-- 
2.17.1

[PATCH v2 0/5] Add Svadu extension support

[Anup Patel]

On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
>
> This series enables Svadu extension support by configuring the menvcfg
> CSR and, if available, displays the Svadu extension in the boot log.
>
> Additionally, we've made some programming improvements in
> lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
>
> ---
> v2:
> - Rearrange the patches to do the code refactoring first before adding
>   new features
> - Suggested by Anup and Atish, detect extensions from DT instead of
>   menvcfg CSR
> - Enable access to some extensions through menvcfg CSR if they are
>   present in the device tree.
>
> Yong-Xuan Wang (5):
>   lib: sbi: Improve the code of privilege mode and extensions detection
>   lib: sbi: Refactor the code for enable extensions in menvfg CSR
>   lib: sbi: Using one array to define the name of extensions
>   lib: sbi: Detect extensions from the ISA string in DT
>   lib: sbi: Add support for Svadu extension

For backward compatibility with existing OSes, it is better to have
supervisor OS explicitly enable Svadu using the upcoming SBI
FWFT extension instead of enabling it unconditionally whenever
Svadu extension is available.

Regards,
Anup

>
>  include/sbi/riscv_encoding.h |   6 +-
>  include/sbi/sbi_hart.h       |  15 +++
>  lib/sbi/sbi_hart.c           | 226 +++++++++++++----------------------
>  lib/utils/fdt/fdt_helper.c   |   5 +-
>  4 files changed, 104 insertions(+), 148 deletions(-)
>
> --
> 2.17.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 0/5] Add Svadu extension support

[Yong-Xuan Wang]

On Wed, Nov 15, 2023 at 12:46?AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> >
> > This series enables Svadu extension support by configuring the menvcfg
> > CSR and, if available, displays the Svadu extension in the boot log.
> >
> > Additionally, we've made some programming improvements in
> > lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
> >
> > ---
> > v2:
> > - Rearrange the patches to do the code refactoring first before adding
> >   new features
> > - Suggested by Anup and Atish, detect extensions from DT instead of
> >   menvcfg CSR
> > - Enable access to some extensions through menvcfg CSR if they are
> >   present in the device tree.
> >
> > Yong-Xuan Wang (5):
> >   lib: sbi: Improve the code of privilege mode and extensions detection
> >   lib: sbi: Refactor the code for enable extensions in menvfg CSR
> >   lib: sbi: Using one array to define the name of extensions
> >   lib: sbi: Detect extensions from the ISA string in DT
> >   lib: sbi: Add support for Svadu extension
>
> For backward compatibility with existing OSes, it is better to have
> supervisor OS explicitly enable Svadu using the upcoming SBI
> FWFT extension instead of enabling it unconditionally whenever
> Svadu extension is available.
>
> Regards,
> Anup
>

Hi Anup,

Thank you! I'll remove the enablement of Svadu extension in the next version.

Regards,
Yong-Xuan

> >
> >  include/sbi/riscv_encoding.h |   6 +-
> >  include/sbi/sbi_hart.h       |  15 +++
> >  lib/sbi/sbi_hart.c           | 226 +++++++++++++----------------------
> >  lib/utils/fdt/fdt_helper.c   |   5 +-
> >  4 files changed, 104 insertions(+), 148 deletions(-)
> >
> > --
> > 2.17.1
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 0/5] Add Svadu extension support

[Alexandre Ghiti]

Hi Anup,

On 14/11/2023 17:45, Anup Patel wrote:
> On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
>> This series enables Svadu extension support by configuring the menvcfg
>> CSR and, if available, displays the Svadu extension in the boot log.
>>
>> Additionally, we've made some programming improvements in
>> lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
>>
>> ---
>> v2:
>> - Rearrange the patches to do the code refactoring first before adding
>>    new features
>> - Suggested by Anup and Atish, detect extensions from DT instead of
>>    menvcfg CSR
>> - Enable access to some extensions through menvcfg CSR if they are
>>    present in the device tree.
>>
>> Yong-Xuan Wang (5):
>>    lib: sbi: Improve the code of privilege mode and extensions detection
>>    lib: sbi: Refactor the code for enable extensions in menvfg CSR
>>    lib: sbi: Using one array to define the name of extensions
>>    lib: sbi: Detect extensions from the ISA string in DT
>>    lib: sbi: Add support for Svadu extension
> For backward compatibility with existing OSes, it is better to have
> supervisor OS explicitly enable Svadu using the upcoming SBI
> FWFT extension instead of enabling it unconditionally whenever
> Svadu extension is available.


I find this weird because maintaining backward compatibility here means 
"continue ignoring svadu present in the device tree".

Why should we treat svadu differently than svpbmt? I understand FWFT 
will fix this, but will that be available?

To me, enabling svadu when present in the device tree is more a fix than 
an issue: if enabling it breaks something, that means svadu is broken on 
your platform so just remove that from your dt or enable the support in 
your kernel.

In a nutshell, if asked by the dt, that means the support is present in 
the kernel and then it expects it and should be enabled.

Let me know what you think,

Thanks,

Alex


>
> Regards,
> Anup
>
>>   include/sbi/riscv_encoding.h |   6 +-
>>   include/sbi/sbi_hart.h       |  15 +++
>>   lib/sbi/sbi_hart.c           | 226 +++++++++++++----------------------
>>   lib/utils/fdt/fdt_helper.c   |   5 +-
>>   4 files changed, 104 insertions(+), 148 deletions(-)
>>
>> --
>> 2.17.1
>>
>>
>> --
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 0/5] Add Svadu extension support

[Conor Dooley]

On Fri, May 24, 2024 at 01:31:29PM +0200, Alexandre Ghiti wrote:
> Hi Anup,
> 
> On 14/11/2023 17:45, Anup Patel wrote:
> > On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> > > This series enables Svadu extension support by configuring the menvcfg
> > > CSR and, if available, displays the Svadu extension in the boot log.
> > > 
> > > Additionally, we've made some programming improvements in
> > > lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
> > > 
> > > ---
> > > v2:
> > > - Rearrange the patches to do the code refactoring first before adding
> > >    new features
> > > - Suggested by Anup and Atish, detect extensions from DT instead of
> > >    menvcfg CSR
> > > - Enable access to some extensions through menvcfg CSR if they are
> > >    present in the device tree.
> > > 
> > > Yong-Xuan Wang (5):
> > >    lib: sbi: Improve the code of privilege mode and extensions detection
> > >    lib: sbi: Refactor the code for enable extensions in menvfg CSR
> > >    lib: sbi: Using one array to define the name of extensions
> > >    lib: sbi: Detect extensions from the ISA string in DT
> > >    lib: sbi: Add support for Svadu extension
> > For backward compatibility with existing OSes, it is better to have
> > supervisor OS explicitly enable Svadu using the upcoming SBI
> > FWFT extension instead of enabling it unconditionally whenever
> > Svadu extension is available.
> 
> 
> I find this weird because maintaining backward compatibility here means
> "continue ignoring svadu present in the device tree".
> 
> Why should we treat svadu differently than svpbmt? I understand FWFT will
> fix this, but will that be available?
> 
> To me, enabling svadu when present in the device tree is more a fix than an
> issue: if enabling it breaks something, that means svadu is broken on your
> platform so just remove that from your dt or enable the support in your
> kernel.
> 
> In a nutshell, if asked by the dt, that means the support is present in the
> kernel and then it expects it and should be enabled.

I think I agree with Anup here. OpenSBI is not aware of what is going to
come along later in the boot chain and should try not to enable extensions
that would cause an OS unaware of them to fall over. Say Linux has
support for Svadu and FreeBSD does not. Do you expect that people would
have to change the firmware on their devices because they want to run
another operating system?

Unfortunately I don't think we can apply the Zkr treatment here and skip
something like FWFT. I guess we should document in the binding that
Svadu only means that the hardware supports it and that an additional
mechanism may be required to flip it on?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20240524/04c70898/attachment.sig>

[PATCH v2 0/5] Add Svadu extension support

[Alexandre Ghiti]

Hi Conor,

On 24/05/2024 14:06, Conor Dooley wrote:
> On Fri, May 24, 2024 at 01:31:29PM +0200, Alexandre Ghiti wrote:
>> Hi Anup,
>>
>> On 14/11/2023 17:45, Anup Patel wrote:
>>> On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
>>>> This series enables Svadu extension support by configuring the menvcfg
>>>> CSR and, if available, displays the Svadu extension in the boot log.
>>>>
>>>> Additionally, we've made some programming improvements in
>>>> lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
>>>>
>>>> ---
>>>> v2:
>>>> - Rearrange the patches to do the code refactoring first before adding
>>>>     new features
>>>> - Suggested by Anup and Atish, detect extensions from DT instead of
>>>>     menvcfg CSR
>>>> - Enable access to some extensions through menvcfg CSR if they are
>>>>     present in the device tree.
>>>>
>>>> Yong-Xuan Wang (5):
>>>>     lib: sbi: Improve the code of privilege mode and extensions detection
>>>>     lib: sbi: Refactor the code for enable extensions in menvfg CSR
>>>>     lib: sbi: Using one array to define the name of extensions
>>>>     lib: sbi: Detect extensions from the ISA string in DT
>>>>     lib: sbi: Add support for Svadu extension
>>> For backward compatibility with existing OSes, it is better to have
>>> supervisor OS explicitly enable Svadu using the upcoming SBI
>>> FWFT extension instead of enabling it unconditionally whenever
>>> Svadu extension is available.
>>
>> I find this weird because maintaining backward compatibility here means
>> "continue ignoring svadu present in the device tree".
>>
>> Why should we treat svadu differently than svpbmt? I understand FWFT will
>> fix this, but will that be available?
>>
>> To me, enabling svadu when present in the device tree is more a fix than an
>> issue: if enabling it breaks something, that means svadu is broken on your
>> platform so just remove that from your dt or enable the support in your
>> kernel.
>>
>> In a nutshell, if asked by the dt, that means the support is present in the
>> kernel and then it expects it and should be enabled.
> I think I agree with Anup here. OpenSBI is not aware of what is going to
> come along later in the boot chain and should try not to enable extensions
> that would cause an OS unaware of them to fall over. Say Linux has
> support for Svadu and FreeBSD does not. Do you expect that people would
> have to change the firmware on their devices because they want to run
> another operating system?


On the other hand, an operating system can think the extension is 
enabled and rely on it: so should we favour the OS that does not 
implement the feature over the one that does?


>
> Unfortunately I don't think we can apply the Zkr treatment here and skip
> something like FWFT. I guess we should document in the binding that
> Svadu only means that the hardware supports it and that an additional
> mechanism may be required to flip it on?


In this case, Svadu can not break anything, so if we do this case by 
case without general rules, I'd be in favor of enabling it (like Zicbom, 
Zicboz, Svpbmt...).

Thanks,

Alex


>

[PATCH v2 0/5] Add Svadu extension support

[Conor Dooley]

On Fri, May 24, 2024 at 02:47:05PM +0200, Alexandre Ghiti wrote:
> Hi Conor,
> 
> On 24/05/2024 14:06, Conor Dooley wrote:
> > On Fri, May 24, 2024 at 01:31:29PM +0200, Alexandre Ghiti wrote:
> > > Hi Anup,
> > > 
> > > On 14/11/2023 17:45, Anup Patel wrote:
> > > > On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> > > > > This series enables Svadu extension support by configuring the menvcfg
> > > > > CSR and, if available, displays the Svadu extension in the boot log.
> > > > > 
> > > > > Additionally, we've made some programming improvements in
> > > > > lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
> > > > > 
> > > > > ---
> > > > > v2:
> > > > > - Rearrange the patches to do the code refactoring first before adding
> > > > >     new features
> > > > > - Suggested by Anup and Atish, detect extensions from DT instead of
> > > > >     menvcfg CSR
> > > > > - Enable access to some extensions through menvcfg CSR if they are
> > > > >     present in the device tree.
> > > > > 
> > > > > Yong-Xuan Wang (5):
> > > > >     lib: sbi: Improve the code of privilege mode and extensions detection
> > > > >     lib: sbi: Refactor the code for enable extensions in menvfg CSR
> > > > >     lib: sbi: Using one array to define the name of extensions
> > > > >     lib: sbi: Detect extensions from the ISA string in DT
> > > > >     lib: sbi: Add support for Svadu extension
> > > > For backward compatibility with existing OSes, it is better to have
> > > > supervisor OS explicitly enable Svadu using the upcoming SBI
> > > > FWFT extension instead of enabling it unconditionally whenever
> > > > Svadu extension is available.
> > > 
> > > I find this weird because maintaining backward compatibility here means
> > > "continue ignoring svadu present in the device tree".
> > > 
> > > Why should we treat svadu differently than svpbmt? I understand FWFT will
> > > fix this, but will that be available?
> > > 
> > > To me, enabling svadu when present in the device tree is more a fix than an
> > > issue: if enabling it breaks something, that means svadu is broken on your
> > > platform so just remove that from your dt or enable the support in your
> > > kernel.
> > > 
> > > In a nutshell, if asked by the dt, that means the support is present in the
> > > kernel and then it expects it and should be enabled.
> > I think I agree with Anup here. OpenSBI is not aware of what is going to
> > come along later in the boot chain and should try not to enable extensions
> > that would cause an OS unaware of them to fall over. Say Linux has
> > support for Svadu and FreeBSD does not. Do you expect that people would
> > have to change the firmware on their devices because they want to run
> > another operating system?
> 
> 
> On the other hand, an operating system can think the extension is enabled
> and rely on it: so should we favour the OS that does not implement the
> feature over the one that does?

I think you've missed my point. I was saying that if there are
extensions that the SBI firmware can optionally enable that would cause
an OS without support for them to fall over, then we should document
that the property for the extension means they're present in the
hardware and must be enabled before use.

In fact the binding says nothing about whether or not the extension is
enabled, just that the extension is supported by the hart. I'd argue
that passing a dtb containing an extension to a privilege level means
that an extension should available at that level - but compatibility is
a more important factor and we shouldn't define any properties that mean
non-implementers fall over.

Given there'd be a FWFT mechanism for enabling the extension, I don't
think either implementers or non-implementers are being favoured here.
The latter carries on like nothing has changed and the former does an
ecall during boot and gets the fancy new toy.

> > Unfortunately I don't think we can apply the Zkr treatment here and skip
> > something like FWFT. I guess we should document in the binding that
> > Svadu only means that the hardware supports it and that an additional
> > mechanism may be required to flip it on?
> 
> 
> In this case, Svadu can not break anything, so if we do this case by case
> without general rules, I'd be in favor of enabling it (like Zicbom, Zicboz,
> Svpbmt...).

I must have misunderstood then based Anup's original objection. If an OS
that's unaware of what Svadu is will run just fine with it enabled by
m-mode then sure.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20240524/35f6c6fa/attachment.sig>

[PATCH v2 0/5] Add Svadu extension support

[Conor Dooley]

On Fri, May 24, 2024 at 02:11:57PM +0100, Conor Dooley wrote:
> On Fri, May 24, 2024 at 02:47:05PM +0200, Alexandre Ghiti wrote:
> > Hi Conor,
> > 
> > On 24/05/2024 14:06, Conor Dooley wrote:
> > > On Fri, May 24, 2024 at 01:31:29PM +0200, Alexandre Ghiti wrote:
> > > > Hi Anup,
> > > > 
> > > > On 14/11/2023 17:45, Anup Patel wrote:
> > > > > On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> > > > > > This series enables Svadu extension support by configuring the menvcfg
> > > > > > CSR and, if available, displays the Svadu extension in the boot log.
> > > > > > 
> > > > > > Additionally, we've made some programming improvements in
> > > > > > lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
> > > > > > 
> > > > > > ---
> > > > > > v2:
> > > > > > - Rearrange the patches to do the code refactoring first before adding
> > > > > >     new features
> > > > > > - Suggested by Anup and Atish, detect extensions from DT instead of
> > > > > >     menvcfg CSR
> > > > > > - Enable access to some extensions through menvcfg CSR if they are
> > > > > >     present in the device tree.
> > > > > > 
> > > > > > Yong-Xuan Wang (5):
> > > > > >     lib: sbi: Improve the code of privilege mode and extensions detection
> > > > > >     lib: sbi: Refactor the code for enable extensions in menvfg CSR
> > > > > >     lib: sbi: Using one array to define the name of extensions
> > > > > >     lib: sbi: Detect extensions from the ISA string in DT
> > > > > >     lib: sbi: Add support for Svadu extension
> > > > > For backward compatibility with existing OSes, it is better to have
> > > > > supervisor OS explicitly enable Svadu using the upcoming SBI
> > > > > FWFT extension instead of enabling it unconditionally whenever
> > > > > Svadu extension is available.
> > > > 
> > > > I find this weird because maintaining backward compatibility here means
> > > > "continue ignoring svadu present in the device tree".
> > > > 
> > > > Why should we treat svadu differently than svpbmt? I understand FWFT will
> > > > fix this, but will that be available?
> > > > 
> > > > To me, enabling svadu when present in the device tree is more a fix than an
> > > > issue: if enabling it breaks something, that means svadu is broken on your
> > > > platform so just remove that from your dt or enable the support in your
> > > > kernel.
> > > > 
> > > > In a nutshell, if asked by the dt, that means the support is present in the
> > > > kernel and then it expects it and should be enabled.
> > > I think I agree with Anup here. OpenSBI is not aware of what is going to
> > > come along later in the boot chain and should try not to enable extensions
> > > that would cause an OS unaware of them to fall over. Say Linux has
> > > support for Svadu and FreeBSD does not. Do you expect that people would
> > > have to change the firmware on their devices because they want to run
> > > another operating system?
> > 
> > 
> > On the other hand, an operating system can think the extension is enabled
> > and rely on it: so should we favour the OS that does not implement the
> > feature over the one that does?
> 
> I think you've missed my point. I was saying that if there are
> extensions that the SBI firmware can optionally enable that would cause
> an OS without support for them to fall over, then we should document
> that the property for the extension means they're present in the
> hardware and must be enabled before use.
> 
> In fact the binding says nothing about whether or not the extension is
> enabled, just that the extension is supported by the hart. I'd argue
> that passing a dtb containing an extension to a privilege level means
> that an extension should available at that level - but compatibility is
> a more important factor and we shouldn't define any properties that mean
> non-implementers fall over.
> 
> Given there'd be a FWFT mechanism for enabling the extension, I don't
> think either implementers or non-implementers are being favoured here.
> The latter carries on like nothing has changed and the former does an
> ecall during boot and gets the fancy new toy.
> 
> > > Unfortunately I don't think we can apply the Zkr treatment here and skip
> > > something like FWFT. I guess we should document in the binding that
> > > Svadu only means that the hardware supports it and that an additional
> > > mechanism may be required to flip it on?
> > 
> > 
> > In this case, Svadu can not break anything, so if we do this case by case
> > without general rules, I'd be in favor of enabling it (like Zicbom, Zicboz,
> > Svpbmt...).
> 
> I must have misunderstood then based Anup's original objection. If an OS
> that's unaware of what Svadu is will run just fine with it enabled by
> m-mode then sure.

I think this might have been the compatibility in question?
https://lore.kernel.org/lkml/d141062b-e3e0-45ce-bc61-3404417c7d7c at app.fastmail.com/T/#m5c8417be951447568b119bec3c148a5f0a49c5ed
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/opensbi/attachments/20240524/8f69135c/attachment-0001.sig>

[PATCH v2 0/5] Add Svadu extension support

[Anup Patel]

Hi Alex,

On Fri, May 24, 2024 at 5:01?PM Alexandre Ghiti <alex@ghiti.fr> wrote:
>
> Hi Anup,
>
> On 14/11/2023 17:45, Anup Patel wrote:
> > On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> >> This series enables Svadu extension support by configuring the menvcfg
> >> CSR and, if available, displays the Svadu extension in the boot log.
> >>
> >> Additionally, we've made some programming improvements in
> >> lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
> >>
> >> ---
> >> v2:
> >> - Rearrange the patches to do the code refactoring first before adding
> >>    new features
> >> - Suggested by Anup and Atish, detect extensions from DT instead of
> >>    menvcfg CSR
> >> - Enable access to some extensions through menvcfg CSR if they are
> >>    present in the device tree.
> >>
> >> Yong-Xuan Wang (5):
> >>    lib: sbi: Improve the code of privilege mode and extensions detection
> >>    lib: sbi: Refactor the code for enable extensions in menvfg CSR
> >>    lib: sbi: Using one array to define the name of extensions
> >>    lib: sbi: Detect extensions from the ISA string in DT
> >>    lib: sbi: Add support for Svadu extension
> > For backward compatibility with existing OSes, it is better to have
> > supervisor OS explicitly enable Svadu using the upcoming SBI
> > FWFT extension instead of enabling it unconditionally whenever
> > Svadu extension is available.
>
>
> I find this weird because maintaining backward compatibility here means
> "continue ignoring svadu present in the device tree".
>
> Why should we treat svadu differently than svpbmt? I understand FWFT
> will fix this, but will that be available?

Svpbmt defines a backward compatible PTE encoding so OSes
unaware of Svpbmt work fine. Unfortunately, this is not true for
Svadu because it changes trapping behaviour of PTE A/D bits
in a non-backward compatible way.

>
> To me, enabling svadu when present in the device tree is more a fix than
> an issue: if enabling it breaks something, that means svadu is broken on
> your platform so just remove that from your dt or enable the support in
> your kernel.

It's not broken; rather it has backward compatibility issues
when left enabled at boot time.

Unfortunately, the switch to enable Svadu for a privilege mode
(S/VS) lies with higher privilege mode (M/HS) as-per RISC-V
privileged spec design philosophy so we need an SBI call.

We do have such switches for other ISA extensions as well and
the SBI implementation (M-mode or HS-modes) enables such
switches whenever backward compatibility is not an issue.

>
> In a nutshell, if asked by the dt, that means the support is present in
> the kernel and then it expects it and should be enabled.
>
> Let me know what you think,

At the moment, the presence of a XYZ extension in device tree
or ACPI only implies that XYZ extension is present and it says
nothing about XYZ extension being enabled.

I think we should improve Linux documentation to explicitly say
the default/boot-time enable status of each extension.

Regards,
Anup


>
> Thanks,
>
> Alex
>
>
> >
> > Regards,
> > Anup
> >
> >>   include/sbi/riscv_encoding.h |   6 +-
> >>   include/sbi/sbi_hart.h       |  15 +++
> >>   lib/sbi/sbi_hart.c           | 226 +++++++++++++----------------------
> >>   lib/utils/fdt/fdt_helper.c   |   5 +-
> >>   4 files changed, 104 insertions(+), 148 deletions(-)
> >>
> >> --
> >> 2.17.1
> >>
> >>
> >> --
> >> opensbi mailing list
> >> opensbi at lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 0/5] Add Svadu extension support

[Alexandre Ghiti]

Hi Anup,

On 24/05/2024 18:05, Anup Patel wrote:
> Hi Alex,
>
> On Fri, May 24, 2024 at 5:01?PM Alexandre Ghiti <alex@ghiti.fr> wrote:
>> Hi Anup,
>>
>> On 14/11/2023 17:45, Anup Patel wrote:
>>> On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
>>>> This series enables Svadu extension support by configuring the menvcfg
>>>> CSR and, if available, displays the Svadu extension in the boot log.
>>>>
>>>> Additionally, we've made some programming improvements in
>>>> lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
>>>>
>>>> ---
>>>> v2:
>>>> - Rearrange the patches to do the code refactoring first before adding
>>>>     new features
>>>> - Suggested by Anup and Atish, detect extensions from DT instead of
>>>>     menvcfg CSR
>>>> - Enable access to some extensions through menvcfg CSR if they are
>>>>     present in the device tree.
>>>>
>>>> Yong-Xuan Wang (5):
>>>>     lib: sbi: Improve the code of privilege mode and extensions detection
>>>>     lib: sbi: Refactor the code for enable extensions in menvfg CSR
>>>>     lib: sbi: Using one array to define the name of extensions
>>>>     lib: sbi: Detect extensions from the ISA string in DT
>>>>     lib: sbi: Add support for Svadu extension
>>> For backward compatibility with existing OSes, it is better to have
>>> supervisor OS explicitly enable Svadu using the upcoming SBI
>>> FWFT extension instead of enabling it unconditionally whenever
>>> Svadu extension is available.
>>
>> I find this weird because maintaining backward compatibility here means
>> "continue ignoring svadu present in the device tree".
>>
>> Why should we treat svadu differently than svpbmt? I understand FWFT
>> will fix this, but will that be available?
> Svpbmt defines a backward compatible PTE encoding so OSes
> unaware of Svpbmt work fine. Unfortunately, this is not true for
> Svadu because it changes trapping behaviour of PTE A/D bits
> in a non-backward compatible way.


Svadu-unaware kernels will protect pages so that they can set A/D bits 
when taking the trap, but that's not incompatible with Svadu. The 
setting of A or D bit in HW does not prevent this to work, so that's not 
non-backward compatible, it is just redundant.


>
>> To me, enabling svadu when present in the device tree is more a fix than
>> an issue: if enabling it breaks something, that means svadu is broken on
>> your platform so just remove that from your dt or enable the support in
>> your kernel.
> It's not broken; rather it has backward compatibility issues
> when left enabled at boot time.
>
> Unfortunately, the switch to enable Svadu for a privilege mode
> (S/VS) lies with higher privilege mode (M/HS) as-per RISC-V
> privileged spec design philosophy so we need an SBI call.


FYI, I was wondering why it needed M-mode intervention and Ved gave me 
the rationale: https://lists.riscv.org/g/tech-privileged/message/1907


>
> We do have such switches for other ISA extensions as well and
> the SBI implementation (M-mode or HS-modes) enables such
> switches whenever backward compatibility is not an issue.
>
>> In a nutshell, if asked by the dt, that means the support is present in
>> the kernel and then it expects it and should be enabled.
>>
>> Let me know what you think,
> At the moment, the presence of a XYZ extension in device tree
> or ACPI only implies that XYZ extension is present and it says
> nothing about XYZ extension being enabled.
>
> I think we should improve Linux documentation to explicitly say
> the default/boot-time enable status of each extension.


Yes, definitely, something needs to be done, not all extensions have the 
semantics, which can be counter-intuitive.

But for Svadu, enabling it by default will not break svadu-unaware OS, 
so I still think we should enable it by default.

Thanks,

Alex


>
> Regards,
> Anup
>
>
>> Thanks,
>>
>> Alex
>>
>>
>>> Regards,
>>> Anup
>>>
>>>>    include/sbi/riscv_encoding.h |   6 +-
>>>>    include/sbi/sbi_hart.h       |  15 +++
>>>>    lib/sbi/sbi_hart.c           | 226 +++++++++++++----------------------
>>>>    lib/utils/fdt/fdt_helper.c   |   5 +-
>>>>    4 files changed, 104 insertions(+), 148 deletions(-)
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>>>
>>>> --
>>>> opensbi mailing list
>>>> opensbi at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 0/5] Add Svadu extension support

[Andrew Jones]

On Fri, May 24, 2024 at 09:38:30PM GMT, Alexandre Ghiti wrote:
> Hi Anup,
> 
> On 24/05/2024 18:05, Anup Patel wrote:
> > Hi Alex,
> > 
> > On Fri, May 24, 2024 at 5:01?PM Alexandre Ghiti <alex@ghiti.fr> wrote:
> > > Hi Anup,
> > > 
> > > On 14/11/2023 17:45, Anup Patel wrote:
> > > > On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> > > > > This series enables Svadu extension support by configuring the menvcfg
> > > > > CSR and, if available, displays the Svadu extension in the boot log.
> > > > > 
> > > > > Additionally, we've made some programming improvements in
> > > > > lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
> > > > > 
> > > > > ---
> > > > > v2:
> > > > > - Rearrange the patches to do the code refactoring first before adding
> > > > >     new features
> > > > > - Suggested by Anup and Atish, detect extensions from DT instead of
> > > > >     menvcfg CSR
> > > > > - Enable access to some extensions through menvcfg CSR if they are
> > > > >     present in the device tree.
> > > > > 
> > > > > Yong-Xuan Wang (5):
> > > > >     lib: sbi: Improve the code of privilege mode and extensions detection
> > > > >     lib: sbi: Refactor the code for enable extensions in menvfg CSR
> > > > >     lib: sbi: Using one array to define the name of extensions
> > > > >     lib: sbi: Detect extensions from the ISA string in DT
> > > > >     lib: sbi: Add support for Svadu extension
> > > > For backward compatibility with existing OSes, it is better to have
> > > > supervisor OS explicitly enable Svadu using the upcoming SBI
> > > > FWFT extension instead of enabling it unconditionally whenever
> > > > Svadu extension is available.
> > > 
> > > I find this weird because maintaining backward compatibility here means
> > > "continue ignoring svadu present in the device tree".
> > > 
> > > Why should we treat svadu differently than svpbmt? I understand FWFT
> > > will fix this, but will that be available?
> > Svpbmt defines a backward compatible PTE encoding so OSes
> > unaware of Svpbmt work fine. Unfortunately, this is not true for
> > Svadu because it changes trapping behaviour of PTE A/D bits
> > in a non-backward compatible way.
> 
> 
> Svadu-unaware kernels will protect pages so that they can set A/D bits when
> taking the trap, but that's not incompatible with Svadu. The setting of A or
> D bit in HW does not prevent this to work, so that's not non-backward
> compatible, it is just redundant.

If an OS only sets A/D bits when it gets the traps, then it's redundant.
If an OS sets A/D bits and does some other stuff, then it's not.

> 
> 
> > 
> > > To me, enabling svadu when present in the device tree is more a fix than
> > > an issue: if enabling it breaks something, that means svadu is broken on
> > > your platform so just remove that from your dt or enable the support in
> > > your kernel.
> > It's not broken; rather it has backward compatibility issues
> > when left enabled at boot time.
> > 
> > Unfortunately, the switch to enable Svadu for a privilege mode
> > (S/VS) lies with higher privilege mode (M/HS) as-per RISC-V
> > privileged spec design philosophy so we need an SBI call.
> 
> 
> FYI, I was wondering why it needed M-mode intervention and Ved gave me the
> rationale: https://lists.riscv.org/g/tech-privileged/message/1907
> 
> 
> > 
> > We do have such switches for other ISA extensions as well and
> > the SBI implementation (M-mode or HS-modes) enables such
> > switches whenever backward compatibility is not an issue.
> > 
> > > In a nutshell, if asked by the dt, that means the support is present in
> > > the kernel and then it expects it and should be enabled.
> > > 
> > > Let me know what you think,
> > At the moment, the presence of a XYZ extension in device tree
> > or ACPI only implies that XYZ extension is present and it says
> > nothing about XYZ extension being enabled.
> > 
> > I think we should improve Linux documentation to explicitly say
> > the default/boot-time enable status of each extension.
> 
> 
> Yes, definitely, something needs to be done, not all extensions have the
> semantics, which can be counter-intuitive.

We also have extensions which appear to just state what to expect at boot
time (afaict). For example, svade just says "A/D bits will cause
exceptions". So, when combined with svadu (to me) it implies one should
expect exceptions at boot but have the ability to flip on hardware A/D
updating if desired. Without svade also in the DT, then (to me) it would
imply svadu is on by default. Or, for completeness, here's the table

 svade  svadu
 0      0        -- who knows... let's assume svade
 1      0        -- svade on by default and the only option
 0      1        -- svadu on by default and the only option
 1      1        -- svade on by default, svadu is an option

But I could be inferring/assuming too much!

Thanks,
drew


> 
> But for Svadu, enabling it by default will not break svadu-unaware OS, so I
> still think we should enable it by default.
> 
> Thanks,
> 
> Alex
> 
> 
> > 
> > Regards,
> > Anup
> > 
> > 
> > > Thanks,
> > > 
> > > Alex
> > > 
> > > 
> > > > Regards,
> > > > Anup
> > > > 
> > > > >    include/sbi/riscv_encoding.h |   6 +-
> > > > >    include/sbi/sbi_hart.h       |  15 +++
> > > > >    lib/sbi/sbi_hart.c           | 226 +++++++++++++----------------------
> > > > >    lib/utils/fdt/fdt_helper.c   |   5 +-
> > > > >    4 files changed, 104 insertions(+), 148 deletions(-)
> > > > > 
> > > > > --
> > > > > 2.17.1
> > > > > 
> > > > > 
> > > > > --
> > > > > opensbi mailing list
> > > > > opensbi at lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/opensbi
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2 0/5] Add Svadu extension support

[Anup Patel]

On Sat, May 25, 2024 at 1:08?AM Alexandre Ghiti <alex@ghiti.fr> wrote:
>
> Hi Anup,
>
> On 24/05/2024 18:05, Anup Patel wrote:
> > Hi Alex,
> >
> > On Fri, May 24, 2024 at 5:01?PM Alexandre Ghiti <alex@ghiti.fr> wrote:
> >> Hi Anup,
> >>
> >> On 14/11/2023 17:45, Anup Patel wrote:
> >>> On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang <yongxuan.wang@sifive.com> wrote:
> >>>> This series enables Svadu extension support by configuring the menvcfg
> >>>> CSR and, if available, displays the Svadu extension in the boot log.
> >>>>
> >>>> Additionally, we've made some programming improvements in
> >>>> lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
> >>>>
> >>>> ---
> >>>> v2:
> >>>> - Rearrange the patches to do the code refactoring first before adding
> >>>>     new features
> >>>> - Suggested by Anup and Atish, detect extensions from DT instead of
> >>>>     menvcfg CSR
> >>>> - Enable access to some extensions through menvcfg CSR if they are
> >>>>     present in the device tree.
> >>>>
> >>>> Yong-Xuan Wang (5):
> >>>>     lib: sbi: Improve the code of privilege mode and extensions detection
> >>>>     lib: sbi: Refactor the code for enable extensions in menvfg CSR
> >>>>     lib: sbi: Using one array to define the name of extensions
> >>>>     lib: sbi: Detect extensions from the ISA string in DT
> >>>>     lib: sbi: Add support for Svadu extension
> >>> For backward compatibility with existing OSes, it is better to have
> >>> supervisor OS explicitly enable Svadu using the upcoming SBI
> >>> FWFT extension instead of enabling it unconditionally whenever
> >>> Svadu extension is available.
> >>
> >> I find this weird because maintaining backward compatibility here means
> >> "continue ignoring svadu present in the device tree".
> >>
> >> Why should we treat svadu differently than svpbmt? I understand FWFT
> >> will fix this, but will that be available?
> > Svpbmt defines a backward compatible PTE encoding so OSes
> > unaware of Svpbmt work fine. Unfortunately, this is not true for
> > Svadu because it changes trapping behaviour of PTE A/D bits
> > in a non-backward compatible way.
>
>
> Svadu-unaware kernels will protect pages so that they can set A/D bits
> when taking the trap, but that's not incompatible with Svadu. The
> setting of A or D bit in HW does not prevent this to work, so that's not
> non-backward compatible, it is just redundant.

I think you are assuming that all OSes have a separate
background thread for scanning page tables for accessed/dirty
pages hence it does not matter who updates the PTE A/D bits.

Alternately, it is also possible that an OS does not have background
thread for scanning page tables and does more work in traps apart
from updating PTE A/D bits. In such case, if OS stops receiving
traps then it would definitely break.

We had a discussion about this on PRS mailing list as well.
(https://lists.riscv.org/g/tech-prs/message/915)

<snip>

Regards,
Anup