[PATCH v6 0/7] lib: sbi: Ensure SBI extension is available

[PATCH v6 0/7] lib: sbi: Ensure SBI extension is available

[Andrew Jones]

Ensure attempts to invoke SBI extension functions fail with
SBI_ERR_NOT_SUPPORTED when the extension's probe function has reported
that the extension is not available. To avoid invoking probe too
frequently, we check extension availability at init time and skip
registering them in the extension list when their probe fails. If the
probes succeed, we keep them, and then later seeing them is enough to
know they're available. Extensions which have multiple IDs may also need
to narrow their ID range or remove IDs from their range to ensure when
the IDs are seen they are valid.

To handle both the available and valid cases at init time we introduce a
new callback which gets invoked instead of just registering the
extensions. We then modify the callbacks for probing and narrowing as
necessary. Additionally, considering the Base extension probe function
already knows how to set out_val to 1 for available extensions, we simply
remove all the probe functions, as now they must all succeed and none of
them return anything other than 1 on success.

v6:
  - Remove the check in sbi_ecall_vendor_handler() when adding it to
sbi_ecall_vendor_register_extensions()
  - Better line wrapping of the comments added for documenting the
    extension struct members
  - Added Anup's r-b's

v5:
  Another rework where I finally take Anup's advice that he gave on
  the initial posting and more or less replace probe with a new
  init callback.

v4:
  Almost a total redesign. Patch1 of the v4 (now series) does keep
  hunk 2 of the v3 patch, but the changes to sbi_ecall_find_extension()
  of the v3 patch have been dropped. Instead multi-ID extensions can
  now make use of a new extension callback (Patch2) which gets invoked
  at init time. This new callback has been applied to vendor extensions
  with Patch3.

v3: http://lists.infradead.org/pipermail/opensbi/2023-May/004894.html

Andrew Jones (6):
  lib: sbi: Introduce register_extensions extension callback
  lib: sbi: Narrow vendor extension range
  lib: sbi: pmu: Remove unnecessary probe function
  lib: sbi: Only register available extensions
  lib: sbi: Remove 0/1 probe implementations
  lib: sbi: Document sbi_ecall_extension members

Xiang W (1):
  lib: sbi: Optimize probe of srst/susp

 include/sbi/sbi_ecall.h    | 36 ++++++++++++++++++++++++++++++++++++
 lib/sbi/sbi_ecall.c        |  5 ++++-
 lib/sbi/sbi_ecall_base.c   | 14 +++++++++++---
 lib/sbi/sbi_ecall_cppc.c   | 18 +++++++++++-------
 lib/sbi/sbi_ecall_dbcn.c   | 18 +++++++++++-------
 lib/sbi/sbi_ecall_hsm.c    | 14 +++++++++++---
 lib/sbi/sbi_ecall_ipi.c    | 14 +++++++++++---
 lib/sbi/sbi_ecall_legacy.c | 14 +++++++++++---
 lib/sbi/sbi_ecall_pmu.c    | 16 ++++++++--------
 lib/sbi/sbi_ecall_rfence.c | 14 +++++++++++---
 lib/sbi/sbi_ecall_srst.c   | 28 ++++++++++++++++++----------
 lib/sbi/sbi_ecall_susp.c   | 27 ++++++++++++++++++---------
 lib/sbi/sbi_ecall_time.c   | 14 +++++++++++---
 lib/sbi/sbi_ecall_vendor.c | 38 +++++++++++++++++++-------------------
 14 files changed, 191 insertions(+), 79 deletions(-)

-- 
2.40.0

[PATCH v6 1/7] lib: sbi: Introduce register_extensions extension callback

[Andrew Jones]

Rather than registering all extensions on their behalf in
sbi_ecall_init(), introduce another extension callback and
invoke that instead. For now, implement each callback by
simply registering the extension, which means this patch
has no intended functional change. In later patches, extension
callbacks will be modified to choose when to register and to
possibly narrow the extension ID range prior to registering.
When an extension range needs to remove IDs, leaving gaps, then
multiple invocations of sbi_ecall_register_extension() may be
used. In summary, later patches for current extensions and the
introductions of future extensions will use the new callback to
ensure that only valid extension IDs from the initial range,
which are also available, will be registered.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 include/sbi/sbi_ecall.h    |  1 +
 lib/sbi/sbi_ecall.c        |  5 ++++-
 lib/sbi/sbi_ecall_base.c   | 14 +++++++++++---
 lib/sbi/sbi_ecall_cppc.c   | 16 ++++++++++++----
 lib/sbi/sbi_ecall_dbcn.c   | 16 ++++++++++++----
 lib/sbi/sbi_ecall_hsm.c    | 14 +++++++++++---
 lib/sbi/sbi_ecall_ipi.c    | 14 +++++++++++---
 lib/sbi/sbi_ecall_legacy.c | 14 +++++++++++---
 lib/sbi/sbi_ecall_pmu.c    | 16 ++++++++++++----
 lib/sbi/sbi_ecall_rfence.c | 14 +++++++++++---
 lib/sbi/sbi_ecall_srst.c   | 16 ++++++++++++----
 lib/sbi/sbi_ecall_susp.c   | 16 ++++++++++++----
 lib/sbi/sbi_ecall_time.c   | 14 +++++++++++---
 lib/sbi/sbi_ecall_vendor.c | 16 ++++++++++++----
 14 files changed, 143 insertions(+), 43 deletions(-)

diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h
index ff9bf8e2b435..fac26429cf5d 100644
--- a/include/sbi/sbi_ecall.h
+++ b/include/sbi/sbi_ecall.h
@@ -24,6 +24,7 @@ struct sbi_ecall_extension {
 	struct sbi_dlist head;
 	unsigned long extid_start;
 	unsigned long extid_end;
+	int (* register_extensions)(void);
 	int (* probe)(unsigned long extid, unsigned long *out_val);
 	int (* handle)(unsigned long extid, unsigned long funcid,
 		       const struct sbi_trap_regs *regs,
diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c
index 76a1ae9ab733..3eb4f0addb02 100644
--- a/lib/sbi/sbi_ecall.c
+++ b/lib/sbi/sbi_ecall.c
@@ -154,7 +154,10 @@ int sbi_ecall_init(void)
 
 	for (i = 0; i < sbi_ecall_exts_size; i++) {
 		ext = sbi_ecall_exts[i];
-		ret = sbi_ecall_register_extension(ext);
+		ret = SBI_ENODEV;
+
+		if (ext->register_extensions)
+			ret = ext->register_extensions();
 		if (ret)
 			return ret;
 	}
diff --git a/lib/sbi/sbi_ecall_base.c b/lib/sbi/sbi_ecall_base.c
index 786d2ac67924..74f05eb26a35 100644
--- a/lib/sbi/sbi_ecall_base.c
+++ b/lib/sbi/sbi_ecall_base.c
@@ -72,8 +72,16 @@ static int sbi_ecall_base_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
+struct sbi_ecall_extension ecall_base;
+
+static int sbi_ecall_base_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_base);
+}
+
 struct sbi_ecall_extension ecall_base = {
-	.extid_start = SBI_EXT_BASE,
-	.extid_end = SBI_EXT_BASE,
-	.handle = sbi_ecall_base_handler,
+	.extid_start		= SBI_EXT_BASE,
+	.extid_end		= SBI_EXT_BASE,
+	.register_extensions	= sbi_ecall_base_register_extensions,
+	.handle			= sbi_ecall_base_handler,
 };
diff --git a/lib/sbi/sbi_ecall_cppc.c b/lib/sbi/sbi_ecall_cppc.c
index 91585f3b74e3..42ec744c22ba 100644
--- a/lib/sbi/sbi_ecall_cppc.c
+++ b/lib/sbi/sbi_ecall_cppc.c
@@ -55,9 +55,17 @@ static int sbi_ecall_cppc_probe(unsigned long extid, unsigned long *out_val)
 	return 0;
 }
 
+struct sbi_ecall_extension ecall_cppc;
+
+static int sbi_ecall_cppc_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_cppc);
+}
+
 struct sbi_ecall_extension ecall_cppc = {
-	.extid_start	= SBI_EXT_CPPC,
-	.extid_end	= SBI_EXT_CPPC,
-	.handle		= sbi_ecall_cppc_handler,
-	.probe		= sbi_ecall_cppc_probe,
+	.extid_start		= SBI_EXT_CPPC,
+	.extid_end		= SBI_EXT_CPPC,
+	.register_extensions	= sbi_ecall_cppc_register_extensions,
+	.probe			= sbi_ecall_cppc_probe,
+	.handle			= sbi_ecall_cppc_handler,
 };
diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
index fe7e175a64c1..58b19e4468ef 100644
--- a/lib/sbi/sbi_ecall_dbcn.c
+++ b/lib/sbi/sbi_ecall_dbcn.c
@@ -64,9 +64,17 @@ static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
 	return 0;
 }
 
+struct sbi_ecall_extension ecall_dbcn;
+
+static int sbi_ecall_dbcn_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_dbcn);
+}
+
 struct sbi_ecall_extension ecall_dbcn = {
-	.extid_start = SBI_EXT_DBCN,
-	.extid_end = SBI_EXT_DBCN,
-	.handle = sbi_ecall_dbcn_handler,
-	.probe = sbi_ecall_dbcn_probe,
+	.extid_start		= SBI_EXT_DBCN,
+	.extid_end		= SBI_EXT_DBCN,
+	.register_extensions	= sbi_ecall_dbcn_register_extensions,
+	.probe			= sbi_ecall_dbcn_probe,
+	.handle			= sbi_ecall_dbcn_handler,
 };
diff --git a/lib/sbi/sbi_ecall_hsm.c b/lib/sbi/sbi_ecall_hsm.c
index f1b41d073b22..20705c395131 100644
--- a/lib/sbi/sbi_ecall_hsm.c
+++ b/lib/sbi/sbi_ecall_hsm.c
@@ -54,8 +54,16 @@ static int sbi_ecall_hsm_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
+struct sbi_ecall_extension ecall_hsm;
+
+static int sbi_ecall_hsm_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_hsm);
+}
+
 struct sbi_ecall_extension ecall_hsm = {
-	.extid_start = SBI_EXT_HSM,
-	.extid_end = SBI_EXT_HSM,
-	.handle = sbi_ecall_hsm_handler,
+	.extid_start		= SBI_EXT_HSM,
+	.extid_end		= SBI_EXT_HSM,
+	.register_extensions	= sbi_ecall_hsm_register_extensions,
+	.handle			= sbi_ecall_hsm_handler,
 };
diff --git a/lib/sbi/sbi_ecall_ipi.c b/lib/sbi/sbi_ecall_ipi.c
index f4797e117a34..a40d6b8cc8a8 100644
--- a/lib/sbi/sbi_ecall_ipi.c
+++ b/lib/sbi/sbi_ecall_ipi.c
@@ -29,8 +29,16 @@ static int sbi_ecall_ipi_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
+struct sbi_ecall_extension ecall_ipi;
+
+static int sbi_ecall_ipi_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_ipi);
+}
+
 struct sbi_ecall_extension ecall_ipi = {
-	.extid_start = SBI_EXT_IPI,
-	.extid_end = SBI_EXT_IPI,
-	.handle = sbi_ecall_ipi_handler,
+	.extid_start		= SBI_EXT_IPI,
+	.extid_end		= SBI_EXT_IPI,
+	.register_extensions	= sbi_ecall_ipi_register_extensions,
+	.handle			= sbi_ecall_ipi_handler,
 };
diff --git a/lib/sbi/sbi_ecall_legacy.c b/lib/sbi/sbi_ecall_legacy.c
index 8237498b538f..99e862e0f19f 100644
--- a/lib/sbi/sbi_ecall_legacy.c
+++ b/lib/sbi/sbi_ecall_legacy.c
@@ -117,8 +117,16 @@ static int sbi_ecall_legacy_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
+struct sbi_ecall_extension ecall_legacy;
+
+static int sbi_ecall_legacy_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_legacy);
+}
+
 struct sbi_ecall_extension ecall_legacy = {
-	.extid_start = SBI_EXT_0_1_SET_TIMER,
-	.extid_end = SBI_EXT_0_1_SHUTDOWN,
-	.handle = sbi_ecall_legacy_handler,
+	.extid_start		= SBI_EXT_0_1_SET_TIMER,
+	.extid_end		= SBI_EXT_0_1_SHUTDOWN,
+	.register_extensions	= sbi_ecall_legacy_register_extensions,
+	.handle			= sbi_ecall_legacy_handler,
 };
diff --git a/lib/sbi/sbi_ecall_pmu.c b/lib/sbi/sbi_ecall_pmu.c
index 367e92774261..b0589d0ecfa1 100644
--- a/lib/sbi/sbi_ecall_pmu.c
+++ b/lib/sbi/sbi_ecall_pmu.c
@@ -88,9 +88,17 @@ static int sbi_ecall_pmu_probe(unsigned long extid, unsigned long *out_val)
 	return 0;
 }
 
+struct sbi_ecall_extension ecall_pmu;
+
+static int sbi_ecall_pmu_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_pmu);
+}
+
 struct sbi_ecall_extension ecall_pmu = {
-	.extid_start = SBI_EXT_PMU,
-	.extid_end = SBI_EXT_PMU,
-	.handle = sbi_ecall_pmu_handler,
-	.probe = sbi_ecall_pmu_probe,
+	.extid_start		= SBI_EXT_PMU,
+	.extid_end		= SBI_EXT_PMU,
+	.register_extensions	= sbi_ecall_pmu_register_extensions,
+	.probe			= sbi_ecall_pmu_probe,
+	.handle			= sbi_ecall_pmu_handler,
 };
diff --git a/lib/sbi/sbi_ecall_rfence.c b/lib/sbi/sbi_ecall_rfence.c
index 6334c001d450..22c665227826 100644
--- a/lib/sbi/sbi_ecall_rfence.c
+++ b/lib/sbi/sbi_ecall_rfence.c
@@ -79,8 +79,16 @@ static int sbi_ecall_rfence_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
+struct sbi_ecall_extension ecall_rfence;
+
+static int sbi_ecall_rfence_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_rfence);
+}
+
 struct sbi_ecall_extension ecall_rfence = {
-	.extid_start = SBI_EXT_RFENCE,
-	.extid_end = SBI_EXT_RFENCE,
-	.handle = sbi_ecall_rfence_handler,
+	.extid_start		= SBI_EXT_RFENCE,
+	.extid_end		= SBI_EXT_RFENCE,
+	.register_extensions	= sbi_ecall_rfence_register_extensions,
+	.handle			= sbi_ecall_rfence_handler,
 };
diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c
index 93b012ce024c..ad31537604a3 100644
--- a/lib/sbi/sbi_ecall_srst.c
+++ b/lib/sbi/sbi_ecall_srst.c
@@ -67,9 +67,17 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
 	return 0;
 }
 
+struct sbi_ecall_extension ecall_srst;
+
+static int sbi_ecall_srst_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_srst);
+}
+
 struct sbi_ecall_extension ecall_srst = {
-	.extid_start = SBI_EXT_SRST,
-	.extid_end = SBI_EXT_SRST,
-	.handle = sbi_ecall_srst_handler,
-	.probe = sbi_ecall_srst_probe,
+	.extid_start		= SBI_EXT_SRST,
+	.extid_end		= SBI_EXT_SRST,
+	.register_extensions	= sbi_ecall_srst_register_extensions,
+	.probe			= sbi_ecall_srst_probe,
+	.handle			= sbi_ecall_srst_handler,
 };
diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
index f20126c49a60..bfbdbe648625 100644
--- a/lib/sbi/sbi_ecall_susp.c
+++ b/lib/sbi/sbi_ecall_susp.c
@@ -40,9 +40,17 @@ static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
 	return 0;
 }
 
+struct sbi_ecall_extension ecall_susp;
+
+static int sbi_ecall_susp_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_susp);
+}
+
 struct sbi_ecall_extension ecall_susp = {
-	.extid_start = SBI_EXT_SUSP,
-	.extid_end = SBI_EXT_SUSP,
-	.handle = sbi_ecall_susp_handler,
-	.probe = sbi_ecall_susp_probe,
+	.extid_start		= SBI_EXT_SUSP,
+	.extid_end		= SBI_EXT_SUSP,
+	.register_extensions	= sbi_ecall_susp_register_extensions,
+	.probe			= sbi_ecall_susp_probe,
+	.handle			= sbi_ecall_susp_handler,
 };
diff --git a/lib/sbi/sbi_ecall_time.c b/lib/sbi/sbi_ecall_time.c
index 668cb17680c0..e79196f77adc 100644
--- a/lib/sbi/sbi_ecall_time.c
+++ b/lib/sbi/sbi_ecall_time.c
@@ -33,8 +33,16 @@ static int sbi_ecall_time_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
+struct sbi_ecall_extension ecall_time;
+
+static int sbi_ecall_time_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_time);
+}
+
 struct sbi_ecall_extension ecall_time = {
-	.extid_start = SBI_EXT_TIME,
-	.extid_end = SBI_EXT_TIME,
-	.handle = sbi_ecall_time_handler,
+	.extid_start		= SBI_EXT_TIME,
+	.extid_end		= SBI_EXT_TIME,
+	.register_extensions	= sbi_ecall_time_register_extensions,
+	.handle			= sbi_ecall_time_handler,
 };
diff --git a/lib/sbi/sbi_ecall_vendor.c b/lib/sbi/sbi_ecall_vendor.c
index 8b8dab00c92d..126156f79a12 100644
--- a/lib/sbi/sbi_ecall_vendor.c
+++ b/lib/sbi/sbi_ecall_vendor.c
@@ -47,9 +47,17 @@ static int sbi_ecall_vendor_handler(unsigned long extid, unsigned long funcid,
 						out_val, out_trap);
 }
 
+struct sbi_ecall_extension ecall_vendor;
+
+static int sbi_ecall_vendor_register_extensions(void)
+{
+	return sbi_ecall_register_extension(&ecall_vendor);
+}
+
 struct sbi_ecall_extension ecall_vendor = {
-	.extid_start = SBI_EXT_VENDOR_START,
-	.extid_end = SBI_EXT_VENDOR_END,
-	.probe = sbi_ecall_vendor_probe,
-	.handle = sbi_ecall_vendor_handler,
+	.extid_start		= SBI_EXT_VENDOR_START,
+	.extid_end		= SBI_EXT_VENDOR_END,
+	.register_extensions	= sbi_ecall_vendor_register_extensions,
+	.probe			= sbi_ecall_vendor_probe,
+	.handle			= sbi_ecall_vendor_handler,
 };
-- 
2.40.0

[PATCH v6 2/7] lib: sbi: Narrow vendor extension range

[Andrew Jones]

The vendor extension ID range is large, but at runtime at most
a single ID will be available. Narrow the range in the
register_extensions callback. After narrowing, we no longer
need to check that the extension ID is correct in the other
callbacks, as those callbacks will never be invoked with
anything other than the single ID.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 lib/sbi/sbi_ecall_vendor.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/sbi/sbi_ecall_vendor.c b/lib/sbi/sbi_ecall_vendor.c
index 126156f79a12..39c58c8b6fd5 100644
--- a/lib/sbi/sbi_ecall_vendor.c
+++ b/lib/sbi/sbi_ecall_vendor.c
@@ -25,8 +25,7 @@ static inline unsigned long sbi_ecall_vendor_id(void)
 static int sbi_ecall_vendor_probe(unsigned long extid,
 				  unsigned long *out_val)
 {
-	if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()) ||
-	    extid != sbi_ecall_vendor_id())
+	if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
 		*out_val = 0;
 	else
 		*out_val = 1;
@@ -38,8 +37,7 @@ static int sbi_ecall_vendor_handler(unsigned long extid, unsigned long funcid,
 				    unsigned long *out_val,
 				    struct sbi_trap_info *out_trap)
 {
-	if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()) ||
-	    extid != sbi_ecall_vendor_id())
+	if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
 		return SBI_ERR_NOT_SUPPORTED;
 
 	return sbi_platform_vendor_ext_provider(sbi_platform_thishart_ptr(),
@@ -51,6 +49,11 @@ struct sbi_ecall_extension ecall_vendor;
 
 static int sbi_ecall_vendor_register_extensions(void)
 {
+	unsigned long extid = sbi_ecall_vendor_id();
+
+	ecall_vendor.extid_start = extid;
+	ecall_vendor.extid_end = extid;
+
 	return sbi_ecall_register_extension(&ecall_vendor);
 }
 
-- 
2.40.0

[PATCH v6 3/7] lib: sbi: pmu: Remove unnecessary probe function

[Andrew Jones]

The absence of a probe implementation means that the extension is
always available. Remove the implementation for the PMU extension,
which does no checking, and indeed even has a comment saying it's
always available.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_ecall_pmu.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/lib/sbi/sbi_ecall_pmu.c b/lib/sbi/sbi_ecall_pmu.c
index b0589d0ecfa1..1d5d512eeed7 100644
--- a/lib/sbi/sbi_ecall_pmu.c
+++ b/lib/sbi/sbi_ecall_pmu.c
@@ -81,13 +81,6 @@ static int sbi_ecall_pmu_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
-static int sbi_ecall_pmu_probe(unsigned long extid, unsigned long *out_val)
-{
-	/* PMU extension is always enabled */
-	*out_val = 1;
-	return 0;
-}
-
 struct sbi_ecall_extension ecall_pmu;
 
 static int sbi_ecall_pmu_register_extensions(void)
@@ -99,6 +92,5 @@ struct sbi_ecall_extension ecall_pmu = {
 	.extid_start		= SBI_EXT_PMU,
 	.extid_end		= SBI_EXT_PMU,
 	.register_extensions	= sbi_ecall_pmu_register_extensions,
-	.probe			= sbi_ecall_pmu_probe,
 	.handle			= sbi_ecall_pmu_handler,
 };
-- 
2.40.0

[PATCH v6 4/7] lib: sbi: Only register available extensions

[Andrew Jones]

When an extension implements a probe function it means there's a
chance that the extension is not available. Use this function in the
register_extensions callback to determine if the extension should be
registered at all. Where the probe implementation is simple, just
open code the check.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_ecall_cppc.c   |  3 +++
 lib/sbi/sbi_ecall_dbcn.c   |  3 +++
 lib/sbi/sbi_ecall_srst.c   |  6 ++++++
 lib/sbi/sbi_ecall_susp.c   |  6 ++++++
 lib/sbi/sbi_ecall_vendor.c | 17 +++--------------
 5 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/lib/sbi/sbi_ecall_cppc.c b/lib/sbi/sbi_ecall_cppc.c
index 42ec744c22ba..a6398ac78226 100644
--- a/lib/sbi/sbi_ecall_cppc.c
+++ b/lib/sbi/sbi_ecall_cppc.c
@@ -59,6 +59,9 @@ struct sbi_ecall_extension ecall_cppc;
 
 static int sbi_ecall_cppc_register_extensions(void)
 {
+	if (!sbi_cppc_get_device())
+		return 0;
+
 	return sbi_ecall_register_extension(&ecall_cppc);
 }
 
diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
index 58b19e4468ef..cbb2e802e615 100644
--- a/lib/sbi/sbi_ecall_dbcn.c
+++ b/lib/sbi/sbi_ecall_dbcn.c
@@ -68,6 +68,9 @@ struct sbi_ecall_extension ecall_dbcn;
 
 static int sbi_ecall_dbcn_register_extensions(void)
 {
+	if (!sbi_console_get_device())
+		return 0;
+
 	return sbi_ecall_register_extension(&ecall_dbcn);
 }
 
diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c
index ad31537604a3..ea0dc73f010d 100644
--- a/lib/sbi/sbi_ecall_srst.c
+++ b/lib/sbi/sbi_ecall_srst.c
@@ -71,6 +71,12 @@ struct sbi_ecall_extension ecall_srst;
 
 static int sbi_ecall_srst_register_extensions(void)
 {
+	unsigned long out_val;
+
+	sbi_ecall_srst_probe(SBI_EXT_SRST, &out_val);
+	if (!out_val)
+		return 0;
+
 	return sbi_ecall_register_extension(&ecall_srst);
 }
 
diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
index bfbdbe648625..c4124046b929 100644
--- a/lib/sbi/sbi_ecall_susp.c
+++ b/lib/sbi/sbi_ecall_susp.c
@@ -44,6 +44,12 @@ struct sbi_ecall_extension ecall_susp;
 
 static int sbi_ecall_susp_register_extensions(void)
 {
+	unsigned long out_val;
+
+	sbi_ecall_susp_probe(SBI_EXT_SUSP, &out_val);
+	if (!out_val)
+		return 0;
+
 	return sbi_ecall_register_extension(&ecall_susp);
 }
 
diff --git a/lib/sbi/sbi_ecall_vendor.c b/lib/sbi/sbi_ecall_vendor.c
index 39c58c8b6fd5..700f475f0a86 100644
--- a/lib/sbi/sbi_ecall_vendor.c
+++ b/lib/sbi/sbi_ecall_vendor.c
@@ -22,24 +22,11 @@ static inline unsigned long sbi_ecall_vendor_id(void)
 		 (SBI_EXT_VENDOR_END - SBI_EXT_VENDOR_START));
 }
 
-static int sbi_ecall_vendor_probe(unsigned long extid,
-				  unsigned long *out_val)
-{
-	if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
-		*out_val = 0;
-	else
-		*out_val = 1;
-	return 0;
-}
-
 static int sbi_ecall_vendor_handler(unsigned long extid, unsigned long funcid,
 				    const struct sbi_trap_regs *regs,
 				    unsigned long *out_val,
 				    struct sbi_trap_info *out_trap)
 {
-	if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
-		return SBI_ERR_NOT_SUPPORTED;
-
 	return sbi_platform_vendor_ext_provider(sbi_platform_thishart_ptr(),
 						funcid, regs,
 						out_val, out_trap);
@@ -51,6 +38,9 @@ static int sbi_ecall_vendor_register_extensions(void)
 {
 	unsigned long extid = sbi_ecall_vendor_id();
 
+	if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
+		return 0;
+
 	ecall_vendor.extid_start = extid;
 	ecall_vendor.extid_end = extid;
 
@@ -61,6 +51,5 @@ struct sbi_ecall_extension ecall_vendor = {
 	.extid_start		= SBI_EXT_VENDOR_START,
 	.extid_end		= SBI_EXT_VENDOR_END,
 	.register_extensions	= sbi_ecall_vendor_register_extensions,
-	.probe			= sbi_ecall_vendor_probe,
 	.handle			= sbi_ecall_vendor_handler,
 };
-- 
2.40.0

[PATCH v6 5/7] lib: sbi: Optimize probe of srst/susp

[Andrew Jones]

From: Xiang W <wxjstz@126.com>

No need to do a fully comprehensive count, just find a supported reset
or suspend type

Signed-off-by: Xiang W <wxjstz@126.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_ecall_srst.c | 10 ++++++----
 lib/sbi/sbi_ecall_susp.c | 10 ++++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c
index ea0dc73f010d..fd2dc0d251f3 100644
--- a/lib/sbi/sbi_ecall_srst.c
+++ b/lib/sbi/sbi_ecall_srst.c
@@ -50,7 +50,7 @@ static int sbi_ecall_srst_handler(unsigned long extid, unsigned long funcid,
 
 static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
 {
-	u32 type, count = 0;
+	u32 type;
 
 	/*
 	 * At least one standard reset types should be supported by
@@ -59,11 +59,13 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
 
 	for (type = 0; type <= SBI_SRST_RESET_TYPE_LAST; type++) {
 		if (sbi_system_reset_supported(type,
-					SBI_SRST_RESET_REASON_NONE))
-			count++;
+					SBI_SRST_RESET_REASON_NONE)) {
+			*out_val = 1;
+			return 0;
+		}
 	}
 
-	*out_val = (count) ? 1 : 0;
+	*out_val = 0;
 	return 0;
 }
 
diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
index c4124046b929..716a6d585af7 100644
--- a/lib/sbi/sbi_ecall_susp.c
+++ b/lib/sbi/sbi_ecall_susp.c
@@ -25,18 +25,20 @@ static int sbi_ecall_susp_handler(unsigned long extid, unsigned long funcid,
 
 static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
 {
-	u32 type, count = 0;
+	u32 type;
 
 	/*
 	 * At least one suspend type should be supported by the
 	 * platform for the SBI SUSP extension to be usable.
 	 */
 	for (type = 0; type <= SBI_SUSP_SLEEP_TYPE_LAST; type++) {
-		if (sbi_system_suspend_supported(type))
-			count++;
+		if (sbi_system_suspend_supported(type)) {
+			*out_val = 1;
+			return 0;
+		}
 	}
 
-	*out_val = count ? 1 : 0;
+	*out_val = 0;
 	return 0;
 }
 
-- 
2.40.0

[PATCH v6 6/7] lib: sbi: Remove 0/1 probe implementations

[Andrew Jones]

When a probe implementation just returns zero for not available and
one for available then we don't need it, as the extension won't be
registered at all if it would return zero and the Base extension
probe function will already set out_val to 1 if not probe function
is implemented. Currently all probe functions only return zero or
one, so remove them all.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 lib/sbi/sbi_ecall_cppc.c |  7 -------
 lib/sbi/sbi_ecall_dbcn.c |  7 -------
 lib/sbi/sbi_ecall_srst.c | 18 +++++-------------
 lib/sbi/sbi_ecall_susp.c | 17 +++++------------
 4 files changed, 10 insertions(+), 39 deletions(-)

diff --git a/lib/sbi/sbi_ecall_cppc.c b/lib/sbi/sbi_ecall_cppc.c
index a6398ac78226..b54d54ec684c 100644
--- a/lib/sbi/sbi_ecall_cppc.c
+++ b/lib/sbi/sbi_ecall_cppc.c
@@ -49,12 +49,6 @@ static int sbi_ecall_cppc_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
-static int sbi_ecall_cppc_probe(unsigned long extid, unsigned long *out_val)
-{
-	*out_val = sbi_cppc_get_device() ? 1 : 0;
-	return 0;
-}
-
 struct sbi_ecall_extension ecall_cppc;
 
 static int sbi_ecall_cppc_register_extensions(void)
@@ -69,6 +63,5 @@ struct sbi_ecall_extension ecall_cppc = {
 	.extid_start		= SBI_EXT_CPPC,
 	.extid_end		= SBI_EXT_CPPC,
 	.register_extensions	= sbi_ecall_cppc_register_extensions,
-	.probe			= sbi_ecall_cppc_probe,
 	.handle			= sbi_ecall_cppc_handler,
 };
diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
index cbb2e802e615..e0b892c2ed6b 100644
--- a/lib/sbi/sbi_ecall_dbcn.c
+++ b/lib/sbi/sbi_ecall_dbcn.c
@@ -58,12 +58,6 @@ static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
 	return SBI_ENOTSUPP;
 }
 
-static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
-{
-	*out_val = sbi_console_get_device() ? 1 : 0;
-	return 0;
-}
-
 struct sbi_ecall_extension ecall_dbcn;
 
 static int sbi_ecall_dbcn_register_extensions(void)
@@ -78,6 +72,5 @@ struct sbi_ecall_extension ecall_dbcn = {
 	.extid_start		= SBI_EXT_DBCN,
 	.extid_end		= SBI_EXT_DBCN,
 	.register_extensions	= sbi_ecall_dbcn_register_extensions,
-	.probe			= sbi_ecall_dbcn_probe,
 	.handle			= sbi_ecall_dbcn_handler,
 };
diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c
index fd2dc0d251f3..dcd560d22f9d 100644
--- a/lib/sbi/sbi_ecall_srst.c
+++ b/lib/sbi/sbi_ecall_srst.c
@@ -48,7 +48,7 @@ static int sbi_ecall_srst_handler(unsigned long extid, unsigned long funcid,
 	return SBI_ENOTSUPP;
 }
 
-static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
+static bool srst_available(void)
 {
 	u32 type;
 
@@ -56,27 +56,20 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
 	 * At least one standard reset types should be supported by
 	 * the platform for SBI SRST extension to be usable.
 	 */
-
 	for (type = 0; type <= SBI_SRST_RESET_TYPE_LAST; type++) {
 		if (sbi_system_reset_supported(type,
-					SBI_SRST_RESET_REASON_NONE)) {
-			*out_val = 1;
-			return 0;
-		}
+					SBI_SRST_RESET_REASON_NONE))
+			return true;
 	}
 
-	*out_val = 0;
-	return 0;
+	return false;
 }
 
 struct sbi_ecall_extension ecall_srst;
 
 static int sbi_ecall_srst_register_extensions(void)
 {
-	unsigned long out_val;
-
-	sbi_ecall_srst_probe(SBI_EXT_SRST, &out_val);
-	if (!out_val)
+	if (!srst_available())
 		return 0;
 
 	return sbi_ecall_register_extension(&ecall_srst);
@@ -86,6 +79,5 @@ struct sbi_ecall_extension ecall_srst = {
 	.extid_start		= SBI_EXT_SRST,
 	.extid_end		= SBI_EXT_SRST,
 	.register_extensions	= sbi_ecall_srst_register_extensions,
-	.probe			= sbi_ecall_srst_probe,
 	.handle			= sbi_ecall_srst_handler,
 };
diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
index 716a6d585af7..2bfd99ae8720 100644
--- a/lib/sbi/sbi_ecall_susp.c
+++ b/lib/sbi/sbi_ecall_susp.c
@@ -23,7 +23,7 @@ static int sbi_ecall_susp_handler(unsigned long extid, unsigned long funcid,
 	return ret;
 }
 
-static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
+static bool susp_available(void)
 {
 	u32 type;
 
@@ -32,24 +32,18 @@ static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
 	 * platform for the SBI SUSP extension to be usable.
 	 */
 	for (type = 0; type <= SBI_SUSP_SLEEP_TYPE_LAST; type++) {
-		if (sbi_system_suspend_supported(type)) {
-			*out_val = 1;
-			return 0;
-		}
+		if (sbi_system_suspend_supported(type))
+			return true;
 	}
 
-	*out_val = 0;
-	return 0;
+	return false;
 }
 
 struct sbi_ecall_extension ecall_susp;
 
 static int sbi_ecall_susp_register_extensions(void)
 {
-	unsigned long out_val;
-
-	sbi_ecall_susp_probe(SBI_EXT_SUSP, &out_val);
-	if (!out_val)
+	if (!susp_available())
 		return 0;
 
 	return sbi_ecall_register_extension(&ecall_susp);
@@ -59,6 +53,5 @@ struct sbi_ecall_extension ecall_susp = {
 	.extid_start		= SBI_EXT_SUSP,
 	.extid_end		= SBI_EXT_SUSP,
 	.register_extensions	= sbi_ecall_susp_register_extensions,
-	.probe			= sbi_ecall_susp_probe,
 	.handle			= sbi_ecall_susp_handler,
 };
-- 
2.40.0

[PATCH v6 7/7] lib: sbi: Document sbi_ecall_extension members

[Andrew Jones]

With the introduction of the register_extensions callback the
range members (extid_start and extid_end) may now change and it
has become a bit subtle as to when a probe function should be
implemented. Document all the members and their relationship to
the register_extensions callback.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
---
 include/sbi/sbi_ecall.h | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h
index fac26429cf5d..90f33bac0bcc 100644
--- a/include/sbi/sbi_ecall.h
+++ b/include/sbi/sbi_ecall.h
@@ -21,11 +21,46 @@ struct sbi_trap_regs;
 struct sbi_trap_info;
 
 struct sbi_ecall_extension {
+	/* head is used by the extension list */
 	struct sbi_dlist head;
+	/*
+	 * extid_start and extid_end specify the range for this extension. As
+	 * the initial range may be wider than the valid runtime range, the
+	 * register_extensions callback is responsible for narrowing the range
+	 * before other callbacks may be invoked.
+	 */
 	unsigned long extid_start;
 	unsigned long extid_end;
+	/*
+	 * register_extensions
+	 *
+	 * Calls sbi_ecall_register_extension() one or more times to register
+	 * extension ID range(s) which should be handled by this extension.
+	 * More than one sbi_ecall_extension struct and
+	 * sbi_ecall_register_extension() call is necessary when the supported
+	 * extension ID ranges have gaps. Additionally, extension availability
+	 * must be checked before registering, which means, when this callback
+	 * returns, only valid extension IDs from the initial range, which are
+	 * also available, have been registered.
+	 */
 	int (* register_extensions)(void);
+	/*
+	 * probe
+	 *
+	 * Implements the Base extension's probe function for the extension. As
+	 * the register_extensions callback ensures that no other extension
+	 * callbacks will be invoked when the extension is not available, then
+	 * probe can never fail. However, an extension may choose to set
+	 * out_val to a nonzero value other than one. In those cases, it should
+	 * implement this callback.
+	 */
 	int (* probe)(unsigned long extid, unsigned long *out_val);
+	/*
+	 * handle
+	 *
+	 * This is the extension handler. register_extensions ensures it is
+	 * never invoked with an invalid or unavailable extension ID.
+	 */
 	int (* handle)(unsigned long extid, unsigned long funcid,
 		       const struct sbi_trap_regs *regs,
 		       unsigned long *out_val,
-- 
2.40.0

[PATCH v6 2/7] lib: sbi: Narrow vendor extension range

[Anup Patel]

On Mon, May 15, 2023 at 4:42?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> The vendor extension ID range is large, but at runtime at most
> a single ID will be available. Narrow the range in the
> register_extensions callback. After narrowing, we no longer
> need to check that the extension ID is correct in the other
> callbacks, as those callbacks will never be invoked with
> anything other than the single ID.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

Looks good to me.

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

Thanks,
Anup

> ---
>  lib/sbi/sbi_ecall_vendor.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/sbi/sbi_ecall_vendor.c b/lib/sbi/sbi_ecall_vendor.c
> index 126156f79a12..39c58c8b6fd5 100644
> --- a/lib/sbi/sbi_ecall_vendor.c
> +++ b/lib/sbi/sbi_ecall_vendor.c
> @@ -25,8 +25,7 @@ static inline unsigned long sbi_ecall_vendor_id(void)
>  static int sbi_ecall_vendor_probe(unsigned long extid,
>                                   unsigned long *out_val)
>  {
> -       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()) ||
> -           extid != sbi_ecall_vendor_id())
> +       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
>                 *out_val = 0;
>         else
>                 *out_val = 1;
> @@ -38,8 +37,7 @@ static int sbi_ecall_vendor_handler(unsigned long extid, unsigned long funcid,
>                                     unsigned long *out_val,
>                                     struct sbi_trap_info *out_trap)
>  {
> -       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()) ||
> -           extid != sbi_ecall_vendor_id())
> +       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
>                 return SBI_ERR_NOT_SUPPORTED;
>
>         return sbi_platform_vendor_ext_provider(sbi_platform_thishart_ptr(),
> @@ -51,6 +49,11 @@ struct sbi_ecall_extension ecall_vendor;
>
>  static int sbi_ecall_vendor_register_extensions(void)
>  {
> +       unsigned long extid = sbi_ecall_vendor_id();
> +
> +       ecall_vendor.extid_start = extid;
> +       ecall_vendor.extid_end = extid;
> +
>         return sbi_ecall_register_extension(&ecall_vendor);
>  }
>
> --
> 2.40.0
>

[PATCH v6 1/7] lib: sbi: Introduce register_extensions extension callback

[Anup Patel]

On Mon, May 15, 2023 at 4:42?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> Rather than registering all extensions on their behalf in
> sbi_ecall_init(), introduce another extension callback and
> invoke that instead. For now, implement each callback by
> simply registering the extension, which means this patch
> has no intended functional change. In later patches, extension
> callbacks will be modified to choose when to register and to
> possibly narrow the extension ID range prior to registering.
> When an extension range needs to remove IDs, leaving gaps, then
> multiple invocations of sbi_ecall_register_extension() may be
> used. In summary, later patches for current extensions and the
> introductions of future extensions will use the new callback to
> ensure that only valid extension IDs from the initial range,
> which are also available, will be registered.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  include/sbi/sbi_ecall.h    |  1 +
>  lib/sbi/sbi_ecall.c        |  5 ++++-
>  lib/sbi/sbi_ecall_base.c   | 14 +++++++++++---
>  lib/sbi/sbi_ecall_cppc.c   | 16 ++++++++++++----
>  lib/sbi/sbi_ecall_dbcn.c   | 16 ++++++++++++----
>  lib/sbi/sbi_ecall_hsm.c    | 14 +++++++++++---
>  lib/sbi/sbi_ecall_ipi.c    | 14 +++++++++++---
>  lib/sbi/sbi_ecall_legacy.c | 14 +++++++++++---
>  lib/sbi/sbi_ecall_pmu.c    | 16 ++++++++++++----
>  lib/sbi/sbi_ecall_rfence.c | 14 +++++++++++---
>  lib/sbi/sbi_ecall_srst.c   | 16 ++++++++++++----
>  lib/sbi/sbi_ecall_susp.c   | 16 ++++++++++++----
>  lib/sbi/sbi_ecall_time.c   | 14 +++++++++++---
>  lib/sbi/sbi_ecall_vendor.c | 16 ++++++++++++----
>  14 files changed, 143 insertions(+), 43 deletions(-)
>
> diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h
> index ff9bf8e2b435..fac26429cf5d 100644
> --- a/include/sbi/sbi_ecall.h
> +++ b/include/sbi/sbi_ecall.h
> @@ -24,6 +24,7 @@ struct sbi_ecall_extension {
>         struct sbi_dlist head;
>         unsigned long extid_start;
>         unsigned long extid_end;
> +       int (* register_extensions)(void);
>         int (* probe)(unsigned long extid, unsigned long *out_val);
>         int (* handle)(unsigned long extid, unsigned long funcid,
>                        const struct sbi_trap_regs *regs,
> diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c
> index 76a1ae9ab733..3eb4f0addb02 100644
> --- a/lib/sbi/sbi_ecall.c
> +++ b/lib/sbi/sbi_ecall.c
> @@ -154,7 +154,10 @@ int sbi_ecall_init(void)
>
>         for (i = 0; i < sbi_ecall_exts_size; i++) {
>                 ext = sbi_ecall_exts[i];
> -               ret = sbi_ecall_register_extension(ext);
> +               ret = SBI_ENODEV;
> +
> +               if (ext->register_extensions)
> +                       ret = ext->register_extensions();
>                 if (ret)
>                         return ret;
>         }
> diff --git a/lib/sbi/sbi_ecall_base.c b/lib/sbi/sbi_ecall_base.c
> index 786d2ac67924..74f05eb26a35 100644
> --- a/lib/sbi/sbi_ecall_base.c
> +++ b/lib/sbi/sbi_ecall_base.c
> @@ -72,8 +72,16 @@ static int sbi_ecall_base_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> +struct sbi_ecall_extension ecall_base;
> +
> +static int sbi_ecall_base_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_base);
> +}
> +
>  struct sbi_ecall_extension ecall_base = {
> -       .extid_start = SBI_EXT_BASE,
> -       .extid_end = SBI_EXT_BASE,
> -       .handle = sbi_ecall_base_handler,
> +       .extid_start            = SBI_EXT_BASE,
> +       .extid_end              = SBI_EXT_BASE,
> +       .register_extensions    = sbi_ecall_base_register_extensions,
> +       .handle                 = sbi_ecall_base_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_cppc.c b/lib/sbi/sbi_ecall_cppc.c
> index 91585f3b74e3..42ec744c22ba 100644
> --- a/lib/sbi/sbi_ecall_cppc.c
> +++ b/lib/sbi/sbi_ecall_cppc.c
> @@ -55,9 +55,17 @@ static int sbi_ecall_cppc_probe(unsigned long extid, unsigned long *out_val)
>         return 0;
>  }
>
> +struct sbi_ecall_extension ecall_cppc;
> +
> +static int sbi_ecall_cppc_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_cppc);
> +}
> +
>  struct sbi_ecall_extension ecall_cppc = {
> -       .extid_start    = SBI_EXT_CPPC,
> -       .extid_end      = SBI_EXT_CPPC,
> -       .handle         = sbi_ecall_cppc_handler,
> -       .probe          = sbi_ecall_cppc_probe,
> +       .extid_start            = SBI_EXT_CPPC,
> +       .extid_end              = SBI_EXT_CPPC,
> +       .register_extensions    = sbi_ecall_cppc_register_extensions,
> +       .probe                  = sbi_ecall_cppc_probe,
> +       .handle                 = sbi_ecall_cppc_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> index fe7e175a64c1..58b19e4468ef 100644
> --- a/lib/sbi/sbi_ecall_dbcn.c
> +++ b/lib/sbi/sbi_ecall_dbcn.c
> @@ -64,9 +64,17 @@ static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
>         return 0;
>  }
>
> +struct sbi_ecall_extension ecall_dbcn;
> +
> +static int sbi_ecall_dbcn_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_dbcn);
> +}
> +
>  struct sbi_ecall_extension ecall_dbcn = {
> -       .extid_start = SBI_EXT_DBCN,
> -       .extid_end = SBI_EXT_DBCN,
> -       .handle = sbi_ecall_dbcn_handler,
> -       .probe = sbi_ecall_dbcn_probe,
> +       .extid_start            = SBI_EXT_DBCN,
> +       .extid_end              = SBI_EXT_DBCN,
> +       .register_extensions    = sbi_ecall_dbcn_register_extensions,
> +       .probe                  = sbi_ecall_dbcn_probe,
> +       .handle                 = sbi_ecall_dbcn_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_hsm.c b/lib/sbi/sbi_ecall_hsm.c
> index f1b41d073b22..20705c395131 100644
> --- a/lib/sbi/sbi_ecall_hsm.c
> +++ b/lib/sbi/sbi_ecall_hsm.c
> @@ -54,8 +54,16 @@ static int sbi_ecall_hsm_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> +struct sbi_ecall_extension ecall_hsm;
> +
> +static int sbi_ecall_hsm_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_hsm);
> +}
> +
>  struct sbi_ecall_extension ecall_hsm = {
> -       .extid_start = SBI_EXT_HSM,
> -       .extid_end = SBI_EXT_HSM,
> -       .handle = sbi_ecall_hsm_handler,
> +       .extid_start            = SBI_EXT_HSM,
> +       .extid_end              = SBI_EXT_HSM,
> +       .register_extensions    = sbi_ecall_hsm_register_extensions,
> +       .handle                 = sbi_ecall_hsm_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_ipi.c b/lib/sbi/sbi_ecall_ipi.c
> index f4797e117a34..a40d6b8cc8a8 100644
> --- a/lib/sbi/sbi_ecall_ipi.c
> +++ b/lib/sbi/sbi_ecall_ipi.c
> @@ -29,8 +29,16 @@ static int sbi_ecall_ipi_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> +struct sbi_ecall_extension ecall_ipi;
> +
> +static int sbi_ecall_ipi_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_ipi);
> +}
> +
>  struct sbi_ecall_extension ecall_ipi = {
> -       .extid_start = SBI_EXT_IPI,
> -       .extid_end = SBI_EXT_IPI,
> -       .handle = sbi_ecall_ipi_handler,
> +       .extid_start            = SBI_EXT_IPI,
> +       .extid_end              = SBI_EXT_IPI,
> +       .register_extensions    = sbi_ecall_ipi_register_extensions,
> +       .handle                 = sbi_ecall_ipi_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_legacy.c b/lib/sbi/sbi_ecall_legacy.c
> index 8237498b538f..99e862e0f19f 100644
> --- a/lib/sbi/sbi_ecall_legacy.c
> +++ b/lib/sbi/sbi_ecall_legacy.c
> @@ -117,8 +117,16 @@ static int sbi_ecall_legacy_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> +struct sbi_ecall_extension ecall_legacy;
> +
> +static int sbi_ecall_legacy_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_legacy);
> +}
> +
>  struct sbi_ecall_extension ecall_legacy = {
> -       .extid_start = SBI_EXT_0_1_SET_TIMER,
> -       .extid_end = SBI_EXT_0_1_SHUTDOWN,
> -       .handle = sbi_ecall_legacy_handler,
> +       .extid_start            = SBI_EXT_0_1_SET_TIMER,
> +       .extid_end              = SBI_EXT_0_1_SHUTDOWN,
> +       .register_extensions    = sbi_ecall_legacy_register_extensions,
> +       .handle                 = sbi_ecall_legacy_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_pmu.c b/lib/sbi/sbi_ecall_pmu.c
> index 367e92774261..b0589d0ecfa1 100644
> --- a/lib/sbi/sbi_ecall_pmu.c
> +++ b/lib/sbi/sbi_ecall_pmu.c
> @@ -88,9 +88,17 @@ static int sbi_ecall_pmu_probe(unsigned long extid, unsigned long *out_val)
>         return 0;
>  }
>
> +struct sbi_ecall_extension ecall_pmu;
> +
> +static int sbi_ecall_pmu_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_pmu);
> +}
> +
>  struct sbi_ecall_extension ecall_pmu = {
> -       .extid_start = SBI_EXT_PMU,
> -       .extid_end = SBI_EXT_PMU,
> -       .handle = sbi_ecall_pmu_handler,
> -       .probe = sbi_ecall_pmu_probe,
> +       .extid_start            = SBI_EXT_PMU,
> +       .extid_end              = SBI_EXT_PMU,
> +       .register_extensions    = sbi_ecall_pmu_register_extensions,
> +       .probe                  = sbi_ecall_pmu_probe,
> +       .handle                 = sbi_ecall_pmu_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_rfence.c b/lib/sbi/sbi_ecall_rfence.c
> index 6334c001d450..22c665227826 100644
> --- a/lib/sbi/sbi_ecall_rfence.c
> +++ b/lib/sbi/sbi_ecall_rfence.c
> @@ -79,8 +79,16 @@ static int sbi_ecall_rfence_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> +struct sbi_ecall_extension ecall_rfence;
> +
> +static int sbi_ecall_rfence_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_rfence);
> +}
> +
>  struct sbi_ecall_extension ecall_rfence = {
> -       .extid_start = SBI_EXT_RFENCE,
> -       .extid_end = SBI_EXT_RFENCE,
> -       .handle = sbi_ecall_rfence_handler,
> +       .extid_start            = SBI_EXT_RFENCE,
> +       .extid_end              = SBI_EXT_RFENCE,
> +       .register_extensions    = sbi_ecall_rfence_register_extensions,
> +       .handle                 = sbi_ecall_rfence_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c
> index 93b012ce024c..ad31537604a3 100644
> --- a/lib/sbi/sbi_ecall_srst.c
> +++ b/lib/sbi/sbi_ecall_srst.c
> @@ -67,9 +67,17 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
>         return 0;
>  }
>
> +struct sbi_ecall_extension ecall_srst;
> +
> +static int sbi_ecall_srst_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_srst);
> +}
> +
>  struct sbi_ecall_extension ecall_srst = {
> -       .extid_start = SBI_EXT_SRST,
> -       .extid_end = SBI_EXT_SRST,
> -       .handle = sbi_ecall_srst_handler,
> -       .probe = sbi_ecall_srst_probe,
> +       .extid_start            = SBI_EXT_SRST,
> +       .extid_end              = SBI_EXT_SRST,
> +       .register_extensions    = sbi_ecall_srst_register_extensions,
> +       .probe                  = sbi_ecall_srst_probe,
> +       .handle                 = sbi_ecall_srst_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
> index f20126c49a60..bfbdbe648625 100644
> --- a/lib/sbi/sbi_ecall_susp.c
> +++ b/lib/sbi/sbi_ecall_susp.c
> @@ -40,9 +40,17 @@ static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
>         return 0;
>  }
>
> +struct sbi_ecall_extension ecall_susp;
> +
> +static int sbi_ecall_susp_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_susp);
> +}
> +
>  struct sbi_ecall_extension ecall_susp = {
> -       .extid_start = SBI_EXT_SUSP,
> -       .extid_end = SBI_EXT_SUSP,
> -       .handle = sbi_ecall_susp_handler,
> -       .probe = sbi_ecall_susp_probe,
> +       .extid_start            = SBI_EXT_SUSP,
> +       .extid_end              = SBI_EXT_SUSP,
> +       .register_extensions    = sbi_ecall_susp_register_extensions,
> +       .probe                  = sbi_ecall_susp_probe,
> +       .handle                 = sbi_ecall_susp_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_time.c b/lib/sbi/sbi_ecall_time.c
> index 668cb17680c0..e79196f77adc 100644
> --- a/lib/sbi/sbi_ecall_time.c
> +++ b/lib/sbi/sbi_ecall_time.c
> @@ -33,8 +33,16 @@ static int sbi_ecall_time_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> +struct sbi_ecall_extension ecall_time;
> +
> +static int sbi_ecall_time_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_time);
> +}
> +
>  struct sbi_ecall_extension ecall_time = {
> -       .extid_start = SBI_EXT_TIME,
> -       .extid_end = SBI_EXT_TIME,
> -       .handle = sbi_ecall_time_handler,
> +       .extid_start            = SBI_EXT_TIME,
> +       .extid_end              = SBI_EXT_TIME,
> +       .register_extensions    = sbi_ecall_time_register_extensions,
> +       .handle                 = sbi_ecall_time_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_vendor.c b/lib/sbi/sbi_ecall_vendor.c
> index 8b8dab00c92d..126156f79a12 100644
> --- a/lib/sbi/sbi_ecall_vendor.c
> +++ b/lib/sbi/sbi_ecall_vendor.c
> @@ -47,9 +47,17 @@ static int sbi_ecall_vendor_handler(unsigned long extid, unsigned long funcid,
>                                                 out_val, out_trap);
>  }
>
> +struct sbi_ecall_extension ecall_vendor;
> +
> +static int sbi_ecall_vendor_register_extensions(void)
> +{
> +       return sbi_ecall_register_extension(&ecall_vendor);
> +}
> +
>  struct sbi_ecall_extension ecall_vendor = {
> -       .extid_start = SBI_EXT_VENDOR_START,
> -       .extid_end = SBI_EXT_VENDOR_END,
> -       .probe = sbi_ecall_vendor_probe,
> -       .handle = sbi_ecall_vendor_handler,
> +       .extid_start            = SBI_EXT_VENDOR_START,
> +       .extid_end              = SBI_EXT_VENDOR_END,
> +       .register_extensions    = sbi_ecall_vendor_register_extensions,
> +       .probe                  = sbi_ecall_vendor_probe,
> +       .handle                 = sbi_ecall_vendor_handler,
>  };
> --
> 2.40.0
>

[PATCH v6 2/7] lib: sbi: Narrow vendor extension range

[Anup Patel]

On Mon, May 15, 2023 at 4:42?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> The vendor extension ID range is large, but at runtime at most
> a single ID will be available. Narrow the range in the
> register_extensions callback. After narrowing, we no longer
> need to check that the extension ID is correct in the other
> callbacks, as those callbacks will never be invoked with
> anything other than the single ID.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  lib/sbi/sbi_ecall_vendor.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/sbi/sbi_ecall_vendor.c b/lib/sbi/sbi_ecall_vendor.c
> index 126156f79a12..39c58c8b6fd5 100644
> --- a/lib/sbi/sbi_ecall_vendor.c
> +++ b/lib/sbi/sbi_ecall_vendor.c
> @@ -25,8 +25,7 @@ static inline unsigned long sbi_ecall_vendor_id(void)
>  static int sbi_ecall_vendor_probe(unsigned long extid,
>                                   unsigned long *out_val)
>  {
> -       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()) ||
> -           extid != sbi_ecall_vendor_id())
> +       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
>                 *out_val = 0;
>         else
>                 *out_val = 1;
> @@ -38,8 +37,7 @@ static int sbi_ecall_vendor_handler(unsigned long extid, unsigned long funcid,
>                                     unsigned long *out_val,
>                                     struct sbi_trap_info *out_trap)
>  {
> -       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()) ||
> -           extid != sbi_ecall_vendor_id())
> +       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
>                 return SBI_ERR_NOT_SUPPORTED;
>
>         return sbi_platform_vendor_ext_provider(sbi_platform_thishart_ptr(),
> @@ -51,6 +49,11 @@ struct sbi_ecall_extension ecall_vendor;
>
>  static int sbi_ecall_vendor_register_extensions(void)
>  {
> +       unsigned long extid = sbi_ecall_vendor_id();
> +
> +       ecall_vendor.extid_start = extid;
> +       ecall_vendor.extid_end = extid;
> +
>         return sbi_ecall_register_extension(&ecall_vendor);
>  }
>
> --
> 2.40.0
>

[PATCH v6 3/7] lib: sbi: pmu: Remove unnecessary probe function

[Anup Patel]

On Mon, May 15, 2023 at 4:42?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> The absence of a probe implementation means that the extension is
> always available. Remove the implementation for the PMU extension,
> which does no checking, and indeed even has a comment saying it's
> always available.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  lib/sbi/sbi_ecall_pmu.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/lib/sbi/sbi_ecall_pmu.c b/lib/sbi/sbi_ecall_pmu.c
> index b0589d0ecfa1..1d5d512eeed7 100644
> --- a/lib/sbi/sbi_ecall_pmu.c
> +++ b/lib/sbi/sbi_ecall_pmu.c
> @@ -81,13 +81,6 @@ static int sbi_ecall_pmu_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> -static int sbi_ecall_pmu_probe(unsigned long extid, unsigned long *out_val)
> -{
> -       /* PMU extension is always enabled */
> -       *out_val = 1;
> -       return 0;
> -}
> -
>  struct sbi_ecall_extension ecall_pmu;
>
>  static int sbi_ecall_pmu_register_extensions(void)
> @@ -99,6 +92,5 @@ struct sbi_ecall_extension ecall_pmu = {
>         .extid_start            = SBI_EXT_PMU,
>         .extid_end              = SBI_EXT_PMU,
>         .register_extensions    = sbi_ecall_pmu_register_extensions,
> -       .probe                  = sbi_ecall_pmu_probe,
>         .handle                 = sbi_ecall_pmu_handler,
>  };
> --
> 2.40.0
>

[PATCH v6 4/7] lib: sbi: Only register available extensions

[Anup Patel]

On Mon, May 15, 2023 at 4:42?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> When an extension implements a probe function it means there's a
> chance that the extension is not available. Use this function in the
> register_extensions callback to determine if the extension should be
> registered at all. Where the probe implementation is simple, just
> open code the check.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Anup Patel <anup@brainfault.org>

The sbi_ecall_init() needs to be called after sbi_platform_final_init()
so that platform devices registered by sbi_platform_final_init() can
be considered at the time of registering SBI extensions.

Applied this patch to the riscv/opensbi repo with above change
included.

Thanks,
Anup

> ---
>  lib/sbi/sbi_ecall_cppc.c   |  3 +++
>  lib/sbi/sbi_ecall_dbcn.c   |  3 +++
>  lib/sbi/sbi_ecall_srst.c   |  6 ++++++
>  lib/sbi/sbi_ecall_susp.c   |  6 ++++++
>  lib/sbi/sbi_ecall_vendor.c | 17 +++--------------
>  5 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/lib/sbi/sbi_ecall_cppc.c b/lib/sbi/sbi_ecall_cppc.c
> index 42ec744c22ba..a6398ac78226 100644
> --- a/lib/sbi/sbi_ecall_cppc.c
> +++ b/lib/sbi/sbi_ecall_cppc.c
> @@ -59,6 +59,9 @@ struct sbi_ecall_extension ecall_cppc;
>
>  static int sbi_ecall_cppc_register_extensions(void)
>  {
> +       if (!sbi_cppc_get_device())
> +               return 0;
> +
>         return sbi_ecall_register_extension(&ecall_cppc);
>  }
>
> diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> index 58b19e4468ef..cbb2e802e615 100644
> --- a/lib/sbi/sbi_ecall_dbcn.c
> +++ b/lib/sbi/sbi_ecall_dbcn.c
> @@ -68,6 +68,9 @@ struct sbi_ecall_extension ecall_dbcn;
>
>  static int sbi_ecall_dbcn_register_extensions(void)
>  {
> +       if (!sbi_console_get_device())
> +               return 0;
> +
>         return sbi_ecall_register_extension(&ecall_dbcn);
>  }
>
> diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c
> index ad31537604a3..ea0dc73f010d 100644
> --- a/lib/sbi/sbi_ecall_srst.c
> +++ b/lib/sbi/sbi_ecall_srst.c
> @@ -71,6 +71,12 @@ struct sbi_ecall_extension ecall_srst;
>
>  static int sbi_ecall_srst_register_extensions(void)
>  {
> +       unsigned long out_val;
> +
> +       sbi_ecall_srst_probe(SBI_EXT_SRST, &out_val);
> +       if (!out_val)
> +               return 0;
> +
>         return sbi_ecall_register_extension(&ecall_srst);
>  }
>
> diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
> index bfbdbe648625..c4124046b929 100644
> --- a/lib/sbi/sbi_ecall_susp.c
> +++ b/lib/sbi/sbi_ecall_susp.c
> @@ -44,6 +44,12 @@ struct sbi_ecall_extension ecall_susp;
>
>  static int sbi_ecall_susp_register_extensions(void)
>  {
> +       unsigned long out_val;
> +
> +       sbi_ecall_susp_probe(SBI_EXT_SUSP, &out_val);
> +       if (!out_val)
> +               return 0;
> +
>         return sbi_ecall_register_extension(&ecall_susp);
>  }
>
> diff --git a/lib/sbi/sbi_ecall_vendor.c b/lib/sbi/sbi_ecall_vendor.c
> index 39c58c8b6fd5..700f475f0a86 100644
> --- a/lib/sbi/sbi_ecall_vendor.c
> +++ b/lib/sbi/sbi_ecall_vendor.c
> @@ -22,24 +22,11 @@ static inline unsigned long sbi_ecall_vendor_id(void)
>                  (SBI_EXT_VENDOR_END - SBI_EXT_VENDOR_START));
>  }
>
> -static int sbi_ecall_vendor_probe(unsigned long extid,
> -                                 unsigned long *out_val)
> -{
> -       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
> -               *out_val = 0;
> -       else
> -               *out_val = 1;
> -       return 0;
> -}
> -
>  static int sbi_ecall_vendor_handler(unsigned long extid, unsigned long funcid,
>                                     const struct sbi_trap_regs *regs,
>                                     unsigned long *out_val,
>                                     struct sbi_trap_info *out_trap)
>  {
> -       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
> -               return SBI_ERR_NOT_SUPPORTED;
> -
>         return sbi_platform_vendor_ext_provider(sbi_platform_thishart_ptr(),
>                                                 funcid, regs,
>                                                 out_val, out_trap);
> @@ -51,6 +38,9 @@ static int sbi_ecall_vendor_register_extensions(void)
>  {
>         unsigned long extid = sbi_ecall_vendor_id();
>
> +       if (!sbi_platform_vendor_ext_check(sbi_platform_thishart_ptr()))
> +               return 0;
> +
>         ecall_vendor.extid_start = extid;
>         ecall_vendor.extid_end = extid;
>
> @@ -61,6 +51,5 @@ struct sbi_ecall_extension ecall_vendor = {
>         .extid_start            = SBI_EXT_VENDOR_START,
>         .extid_end              = SBI_EXT_VENDOR_END,
>         .register_extensions    = sbi_ecall_vendor_register_extensions,
> -       .probe                  = sbi_ecall_vendor_probe,
>         .handle                 = sbi_ecall_vendor_handler,
>  };
> --
> 2.40.0
>

[PATCH v6 5/7] lib: sbi: Optimize probe of srst/susp

[Anup Patel]

On Mon, May 15, 2023 at 4:42?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> From: Xiang W <wxjstz@126.com>
>
> No need to do a fully comprehensive count, just find a supported reset
> or suspend type
>
> Signed-off-by: Xiang W <wxjstz@126.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  lib/sbi/sbi_ecall_srst.c | 10 ++++++----
>  lib/sbi/sbi_ecall_susp.c | 10 ++++++----
>  2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c
> index ea0dc73f010d..fd2dc0d251f3 100644
> --- a/lib/sbi/sbi_ecall_srst.c
> +++ b/lib/sbi/sbi_ecall_srst.c
> @@ -50,7 +50,7 @@ static int sbi_ecall_srst_handler(unsigned long extid, unsigned long funcid,
>
>  static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
>  {
> -       u32 type, count = 0;
> +       u32 type;
>
>         /*
>          * At least one standard reset types should be supported by
> @@ -59,11 +59,13 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
>
>         for (type = 0; type <= SBI_SRST_RESET_TYPE_LAST; type++) {
>                 if (sbi_system_reset_supported(type,
> -                                       SBI_SRST_RESET_REASON_NONE))
> -                       count++;
> +                                       SBI_SRST_RESET_REASON_NONE)) {
> +                       *out_val = 1;
> +                       return 0;
> +               }
>         }
>
> -       *out_val = (count) ? 1 : 0;
> +       *out_val = 0;
>         return 0;
>  }
>
> diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
> index c4124046b929..716a6d585af7 100644
> --- a/lib/sbi/sbi_ecall_susp.c
> +++ b/lib/sbi/sbi_ecall_susp.c
> @@ -25,18 +25,20 @@ static int sbi_ecall_susp_handler(unsigned long extid, unsigned long funcid,
>
>  static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
>  {
> -       u32 type, count = 0;
> +       u32 type;
>
>         /*
>          * At least one suspend type should be supported by the
>          * platform for the SBI SUSP extension to be usable.
>          */
>         for (type = 0; type <= SBI_SUSP_SLEEP_TYPE_LAST; type++) {
> -               if (sbi_system_suspend_supported(type))
> -                       count++;
> +               if (sbi_system_suspend_supported(type)) {
> +                       *out_val = 1;
> +                       return 0;
> +               }
>         }
>
> -       *out_val = count ? 1 : 0;
> +       *out_val = 0;
>         return 0;
>  }
>
> --
> 2.40.0
>

[PATCH v6 6/7] lib: sbi: Remove 0/1 probe implementations

[Anup Patel]

On Mon, May 15, 2023 at 4:42?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> When a probe implementation just returns zero for not available and
> one for available then we don't need it, as the extension won't be
> registered at all if it would return zero and the Base extension
> probe function will already set out_val to 1 if not probe function
> is implemented. Currently all probe functions only return zero or
> one, so remove them all.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  lib/sbi/sbi_ecall_cppc.c |  7 -------
>  lib/sbi/sbi_ecall_dbcn.c |  7 -------
>  lib/sbi/sbi_ecall_srst.c | 18 +++++-------------
>  lib/sbi/sbi_ecall_susp.c | 17 +++++------------
>  4 files changed, 10 insertions(+), 39 deletions(-)
>
> diff --git a/lib/sbi/sbi_ecall_cppc.c b/lib/sbi/sbi_ecall_cppc.c
> index a6398ac78226..b54d54ec684c 100644
> --- a/lib/sbi/sbi_ecall_cppc.c
> +++ b/lib/sbi/sbi_ecall_cppc.c
> @@ -49,12 +49,6 @@ static int sbi_ecall_cppc_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> -static int sbi_ecall_cppc_probe(unsigned long extid, unsigned long *out_val)
> -{
> -       *out_val = sbi_cppc_get_device() ? 1 : 0;
> -       return 0;
> -}
> -
>  struct sbi_ecall_extension ecall_cppc;
>
>  static int sbi_ecall_cppc_register_extensions(void)
> @@ -69,6 +63,5 @@ struct sbi_ecall_extension ecall_cppc = {
>         .extid_start            = SBI_EXT_CPPC,
>         .extid_end              = SBI_EXT_CPPC,
>         .register_extensions    = sbi_ecall_cppc_register_extensions,
> -       .probe                  = sbi_ecall_cppc_probe,
>         .handle                 = sbi_ecall_cppc_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> index cbb2e802e615..e0b892c2ed6b 100644
> --- a/lib/sbi/sbi_ecall_dbcn.c
> +++ b/lib/sbi/sbi_ecall_dbcn.c
> @@ -58,12 +58,6 @@ static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
>         return SBI_ENOTSUPP;
>  }
>
> -static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
> -{
> -       *out_val = sbi_console_get_device() ? 1 : 0;
> -       return 0;
> -}
> -
>  struct sbi_ecall_extension ecall_dbcn;
>
>  static int sbi_ecall_dbcn_register_extensions(void)
> @@ -78,6 +72,5 @@ struct sbi_ecall_extension ecall_dbcn = {
>         .extid_start            = SBI_EXT_DBCN,
>         .extid_end              = SBI_EXT_DBCN,
>         .register_extensions    = sbi_ecall_dbcn_register_extensions,
> -       .probe                  = sbi_ecall_dbcn_probe,
>         .handle                 = sbi_ecall_dbcn_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c
> index fd2dc0d251f3..dcd560d22f9d 100644
> --- a/lib/sbi/sbi_ecall_srst.c
> +++ b/lib/sbi/sbi_ecall_srst.c
> @@ -48,7 +48,7 @@ static int sbi_ecall_srst_handler(unsigned long extid, unsigned long funcid,
>         return SBI_ENOTSUPP;
>  }
>
> -static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
> +static bool srst_available(void)
>  {
>         u32 type;
>
> @@ -56,27 +56,20 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val)
>          * At least one standard reset types should be supported by
>          * the platform for SBI SRST extension to be usable.
>          */
> -
>         for (type = 0; type <= SBI_SRST_RESET_TYPE_LAST; type++) {
>                 if (sbi_system_reset_supported(type,
> -                                       SBI_SRST_RESET_REASON_NONE)) {
> -                       *out_val = 1;
> -                       return 0;
> -               }
> +                                       SBI_SRST_RESET_REASON_NONE))
> +                       return true;
>         }
>
> -       *out_val = 0;
> -       return 0;
> +       return false;
>  }
>
>  struct sbi_ecall_extension ecall_srst;
>
>  static int sbi_ecall_srst_register_extensions(void)
>  {
> -       unsigned long out_val;
> -
> -       sbi_ecall_srst_probe(SBI_EXT_SRST, &out_val);
> -       if (!out_val)
> +       if (!srst_available())
>                 return 0;
>
>         return sbi_ecall_register_extension(&ecall_srst);
> @@ -86,6 +79,5 @@ struct sbi_ecall_extension ecall_srst = {
>         .extid_start            = SBI_EXT_SRST,
>         .extid_end              = SBI_EXT_SRST,
>         .register_extensions    = sbi_ecall_srst_register_extensions,
> -       .probe                  = sbi_ecall_srst_probe,
>         .handle                 = sbi_ecall_srst_handler,
>  };
> diff --git a/lib/sbi/sbi_ecall_susp.c b/lib/sbi/sbi_ecall_susp.c
> index 716a6d585af7..2bfd99ae8720 100644
> --- a/lib/sbi/sbi_ecall_susp.c
> +++ b/lib/sbi/sbi_ecall_susp.c
> @@ -23,7 +23,7 @@ static int sbi_ecall_susp_handler(unsigned long extid, unsigned long funcid,
>         return ret;
>  }
>
> -static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
> +static bool susp_available(void)
>  {
>         u32 type;
>
> @@ -32,24 +32,18 @@ static int sbi_ecall_susp_probe(unsigned long extid, unsigned long *out_val)
>          * platform for the SBI SUSP extension to be usable.
>          */
>         for (type = 0; type <= SBI_SUSP_SLEEP_TYPE_LAST; type++) {
> -               if (sbi_system_suspend_supported(type)) {
> -                       *out_val = 1;
> -                       return 0;
> -               }
> +               if (sbi_system_suspend_supported(type))
> +                       return true;
>         }
>
> -       *out_val = 0;
> -       return 0;
> +       return false;
>  }
>
>  struct sbi_ecall_extension ecall_susp;
>
>  static int sbi_ecall_susp_register_extensions(void)
>  {
> -       unsigned long out_val;
> -
> -       sbi_ecall_susp_probe(SBI_EXT_SUSP, &out_val);
> -       if (!out_val)
> +       if (!susp_available())
>                 return 0;
>
>         return sbi_ecall_register_extension(&ecall_susp);
> @@ -59,6 +53,5 @@ struct sbi_ecall_extension ecall_susp = {
>         .extid_start            = SBI_EXT_SUSP,
>         .extid_end              = SBI_EXT_SUSP,
>         .register_extensions    = sbi_ecall_susp_register_extensions,
> -       .probe                  = sbi_ecall_susp_probe,
>         .handle                 = sbi_ecall_susp_handler,
>  };
> --
> 2.40.0
>

[PATCH v6 7/7] lib: sbi: Document sbi_ecall_extension members

[Anup Patel]

On Mon, May 15, 2023 at 4:42?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> With the introduction of the register_extensions callback the
> range members (extid_start and extid_end) may now change and it
> has become a bit subtle as to when a probe function should be
> implemented. Document all the members and their relationship to
> the register_extensions callback.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
>  include/sbi/sbi_ecall.h | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h
> index fac26429cf5d..90f33bac0bcc 100644
> --- a/include/sbi/sbi_ecall.h
> +++ b/include/sbi/sbi_ecall.h
> @@ -21,11 +21,46 @@ struct sbi_trap_regs;
>  struct sbi_trap_info;
>
>  struct sbi_ecall_extension {
> +       /* head is used by the extension list */
>         struct sbi_dlist head;
> +       /*
> +        * extid_start and extid_end specify the range for this extension. As
> +        * the initial range may be wider than the valid runtime range, the
> +        * register_extensions callback is responsible for narrowing the range
> +        * before other callbacks may be invoked.
> +        */
>         unsigned long extid_start;
>         unsigned long extid_end;
> +       /*
> +        * register_extensions
> +        *
> +        * Calls sbi_ecall_register_extension() one or more times to register
> +        * extension ID range(s) which should be handled by this extension.
> +        * More than one sbi_ecall_extension struct and
> +        * sbi_ecall_register_extension() call is necessary when the supported
> +        * extension ID ranges have gaps. Additionally, extension availability
> +        * must be checked before registering, which means, when this callback
> +        * returns, only valid extension IDs from the initial range, which are
> +        * also available, have been registered.
> +        */
>         int (* register_extensions)(void);
> +       /*
> +        * probe
> +        *
> +        * Implements the Base extension's probe function for the extension. As
> +        * the register_extensions callback ensures that no other extension
> +        * callbacks will be invoked when the extension is not available, then
> +        * probe can never fail. However, an extension may choose to set
> +        * out_val to a nonzero value other than one. In those cases, it should
> +        * implement this callback.
> +        */
>         int (* probe)(unsigned long extid, unsigned long *out_val);
> +       /*
> +        * handle
> +        *
> +        * This is the extension handler. register_extensions ensures it is
> +        * never invoked with an invalid or unavailable extension ID.
> +        */
>         int (* handle)(unsigned long extid, unsigned long funcid,
>                        const struct sbi_trap_regs *regs,
>                        unsigned long *out_val,
> --
> 2.40.0
>