OpenSBI Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] lib: sbi: Ensure SBI extension is available
@ 2023-05-11 16:11 Andrew Jones
  2023-05-11 16:11 ` [PATCH v4 1/3] lib: sbi: Don't register unavailable single ID extensions Andrew Jones
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Andrew Jones @ 2023-05-11 16:11 UTC (permalink / raw)
  To: opensbi

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 single extension ID extensions at
init time and even drop them from the extension list when their
probe fails. If the probe succeeds, we keep it, and then later
seeing that it's a single extension ID extension is enough to know
it's available. Extensions which have multiple IDs must have its probe
called for each, so rather than try probing at init time we provide
another callback allowing the extension to narrow its range or even
vanish.

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 (3):
  lib: sbi: Don't register unavailable single ID extensions
  lib: sbi: Introduce register_extensions extension callback
  lib: sbi: Apply register_extensions to vendor extensions

 include/sbi/sbi_ecall.h    |  1 +
 lib/sbi/sbi_ecall.c        | 17 ++++++++++++-----
 lib/sbi/sbi_ecall_vendor.c | 17 +++++++++++++++++
 3 files changed, 30 insertions(+), 5 deletions(-)

-- 
2.40.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 1/3] lib: sbi: Don't register unavailable single ID extensions
  2023-05-11 16:11 [PATCH v4 0/3] lib: sbi: Ensure SBI extension is available Andrew Jones
@ 2023-05-11 16:11 ` Andrew Jones
  2023-05-11 16:11 ` [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback Andrew Jones
  2023-05-11 16:11 ` [PATCH v4 3/3] lib: sbi: Apply register_extensions to vendor extensions Andrew Jones
  2 siblings, 0 replies; 8+ messages in thread
From: Andrew Jones @ 2023-05-11 16:11 UTC (permalink / raw)
  To: opensbi

When an extension provides a probe callback we can check it at
init time to see if we should register the extension at all.
However, this is only possible for single ID extensions since
attempting to probe all IDs of a potentially very large range
doesn't make sense.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 lib/sbi/sbi_ecall.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c
index 76a1ae9ab733..5a301fb7d403 100644
--- a/lib/sbi/sbi_ecall.c
+++ b/lib/sbi/sbi_ecall.c
@@ -148,15 +148,18 @@ int sbi_ecall_handler(struct sbi_trap_regs *regs)
 
 int sbi_ecall_init(void)
 {
-	int ret;
 	struct sbi_ecall_extension *ext;
-	unsigned long i;
+	unsigned long out_val, i;
+	int ret;
 
 	for (i = 0; i < sbi_ecall_exts_size; i++) {
 		ext = sbi_ecall_exts[i];
-		ret = sbi_ecall_register_extension(ext);
-		if (ret)
-			return ret;
+		if (ext->extid_start != ext->extid_end || !ext->probe ||
+		    (!ext->probe(ext->extid_end, &out_val) && out_val)) {
+			ret = sbi_ecall_register_extension(ext);
+			if (ret)
+				return ret;
+		}
 	}
 
 	return 0;
-- 
2.40.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback
  2023-05-11 16:11 [PATCH v4 0/3] lib: sbi: Ensure SBI extension is available Andrew Jones
  2023-05-11 16:11 ` [PATCH v4 1/3] lib: sbi: Don't register unavailable single ID extensions Andrew Jones
@ 2023-05-11 16:11 ` Andrew Jones
  2023-05-12 14:25   ` Anup Patel
  2023-05-11 16:11 ` [PATCH v4 3/3] lib: sbi: Apply register_extensions to vendor extensions Andrew Jones
  2 siblings, 1 reply; 8+ messages in thread
From: Andrew Jones @ 2023-05-11 16:11 UTC (permalink / raw)
  To: opensbi

The register_extensions callback is called from sbi_ecall_init() for
any extension that provides it. It's purpose is to register the
extension with one or more sbi_ecall_register_extension() calls after
possibly narrowing the range of extension IDs that should be handled.
More than one call of sbi_ecall_register_extension() is necessary
when the supported extension ID ranges have gaps. Additionally, when
an extension may not be available and has a probe function, then the
probe function should be consulted as to whether or not the extension
should be registered. In summary, when register_extensions() returns,
only valid extension IDs from the initial range, which are also
available, have been registered.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 include/sbi/sbi_ecall.h | 1 +
 lib/sbi/sbi_ecall.c     | 8 ++++++--
 2 files changed, 7 insertions(+), 2 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 5a301fb7d403..1c4a26fde438 100644
--- a/lib/sbi/sbi_ecall.c
+++ b/lib/sbi/sbi_ecall.c
@@ -154,8 +154,12 @@ int sbi_ecall_init(void)
 
 	for (i = 0; i < sbi_ecall_exts_size; i++) {
 		ext = sbi_ecall_exts[i];
-		if (ext->extid_start != ext->extid_end || !ext->probe ||
-		    (!ext->probe(ext->extid_end, &out_val) && out_val)) {
+		if (ext->register_extensions) {
+			ret = ext->register_extensions();
+			if (ret)
+				return ret;
+		} else if (ext->extid_start != ext->extid_end || !ext->probe ||
+			   (!ext->probe(ext->extid_end, &out_val) && out_val)) {
 			ret = sbi_ecall_register_extension(ext);
 			if (ret)
 				return ret;
-- 
2.40.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 3/3] lib: sbi: Apply register_extensions to vendor extensions
  2023-05-11 16:11 [PATCH v4 0/3] lib: sbi: Ensure SBI extension is available Andrew Jones
  2023-05-11 16:11 ` [PATCH v4 1/3] lib: sbi: Don't register unavailable single ID extensions Andrew Jones
  2023-05-11 16:11 ` [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback Andrew Jones
@ 2023-05-11 16:11 ` Andrew Jones
  2 siblings, 0 replies; 8+ messages in thread
From: Andrew Jones @ 2023-05-11 16:11 UTC (permalink / raw)
  To: opensbi

The vendor extension ID range is large, but at runtime at most a
single ID will be available. Apply the register_extensions callback
to check if that single ID is available. When it isn't, don't
register the vendor extension at all. When it is, then narrow the
extension range to the single ID and register it.

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

diff --git a/lib/sbi/sbi_ecall_vendor.c b/lib/sbi/sbi_ecall_vendor.c
index 8b8dab00c92d..a64a82a8a3b1 100644
--- a/lib/sbi/sbi_ecall_vendor.c
+++ b/lib/sbi/sbi_ecall_vendor.c
@@ -47,9 +47,26 @@ 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)
+{
+	unsigned long extid = sbi_ecall_vendor_id(), out_val;
+
+	if (sbi_ecall_vendor_probe(extid, &out_val) || !out_val)
+		return 0;
+
+	ecall_vendor.extid_start = extid;
+	ecall_vendor.extid_end = extid;
+	ecall_vendor.register_extensions = NULL;
+
+	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,
+	.register_extensions = sbi_ecall_vendor_register_extensions,
 };
-- 
2.40.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback
  2023-05-11 16:11 ` [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback Andrew Jones
@ 2023-05-12 14:25   ` Anup Patel
  2023-05-12 15:06     ` Andrew Jones
  0 siblings, 1 reply; 8+ messages in thread
From: Anup Patel @ 2023-05-12 14:25 UTC (permalink / raw)
  To: opensbi

On Thu, May 11, 2023 at 9:41?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> The register_extensions callback is called from sbi_ecall_init() for
> any extension that provides it. It's purpose is to register the
> extension with one or more sbi_ecall_register_extension() calls after
> possibly narrowing the range of extension IDs that should be handled.
> More than one call of sbi_ecall_register_extension() is necessary
> when the supported extension ID ranges have gaps. Additionally, when
> an extension may not be available and has a probe function, then the
> probe function should be consulted as to whether or not the extension
> should be registered. In summary, when register_extensions() returns,
> only valid extension IDs from the initial range, which are also
> available, have been registered.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  include/sbi/sbi_ecall.h | 1 +
>  lib/sbi/sbi_ecall.c     | 8 ++++++--
>  2 files changed, 7 insertions(+), 2 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 5a301fb7d403..1c4a26fde438 100644
> --- a/lib/sbi/sbi_ecall.c
> +++ b/lib/sbi/sbi_ecall.c
> @@ -154,8 +154,12 @@ int sbi_ecall_init(void)
>
>         for (i = 0; i < sbi_ecall_exts_size; i++) {
>                 ext = sbi_ecall_exts[i];
> -               if (ext->extid_start != ext->extid_end || !ext->probe ||
> -                   (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> +               if (ext->register_extensions) {
> +                       ret = ext->register_extensions();
> +                       if (ret)
> +                               return ret;
> +               } else if (ext->extid_start != ext->extid_end || !ext->probe ||
> +                          (!ext->probe(ext->extid_end, &out_val) && out_val)) {

We should replace probe() callback with register_extensions()
and code over here can be something like below:

if (ext->register_extensions)
     ret = ext->register_extensions();
else if (ext->extid_start == ext->extid_end)
     ret = sbi_ecall_register_extension(ext);
else
     ret = SBI_ENODEV;
if (ret)
    return ret;

Basically, register_extensions() is optional for single extension ID
but mandatory for extension ID range.

>                         ret = sbi_ecall_register_extension(ext);
>                         if (ret)
>                                 return ret;
> --
> 2.40.0
>

Regards,
Anup


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback
  2023-05-12 14:25   ` Anup Patel
@ 2023-05-12 15:06     ` Andrew Jones
  2023-05-12 16:47       ` Anup Patel
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Jones @ 2023-05-12 15:06 UTC (permalink / raw)
  To: opensbi

On Fri, May 12, 2023 at 07:55:56PM +0530, Anup Patel wrote:
> On Thu, May 11, 2023 at 9:41?PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > The register_extensions callback is called from sbi_ecall_init() for
> > any extension that provides it. It's purpose is to register the
> > extension with one or more sbi_ecall_register_extension() calls after
> > possibly narrowing the range of extension IDs that should be handled.
> > More than one call of sbi_ecall_register_extension() is necessary
> > when the supported extension ID ranges have gaps. Additionally, when
> > an extension may not be available and has a probe function, then the
> > probe function should be consulted as to whether or not the extension
> > should be registered. In summary, when register_extensions() returns,
> > only valid extension IDs from the initial range, which are also
> > available, have been registered.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  include/sbi/sbi_ecall.h | 1 +
> >  lib/sbi/sbi_ecall.c     | 8 ++++++--
> >  2 files changed, 7 insertions(+), 2 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 5a301fb7d403..1c4a26fde438 100644
> > --- a/lib/sbi/sbi_ecall.c
> > +++ b/lib/sbi/sbi_ecall.c
> > @@ -154,8 +154,12 @@ int sbi_ecall_init(void)
> >
> >         for (i = 0; i < sbi_ecall_exts_size; i++) {
> >                 ext = sbi_ecall_exts[i];
> > -               if (ext->extid_start != ext->extid_end || !ext->probe ||
> > -                   (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> > +               if (ext->register_extensions) {
> > +                       ret = ext->register_extensions();
> > +                       if (ret)
> > +                               return ret;
> > +               } else if (ext->extid_start != ext->extid_end || !ext->probe ||
> > +                          (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> 
> We should replace probe() callback with register_extensions()
> and code over here can be something like below:

We need to keep probe in case an extension wants to implement a
nonzero out_val, which can be inspected by S-mode with the Base
extension probe function:

"""
Probe SBI extension (FID #3)

Returns 0 if the given SBI extension ID (EID) is not available, or
1 if it is available unless defined as any other non-zero value by
the implementation.
"""

> 
> if (ext->register_extensions)
>      ret = ext->register_extensions();
> else if (ext->extid_start == ext->extid_end)
>      ret = sbi_ecall_register_extension(ext);

Also here, if we don't have probe, then we can't know if the single
ID extension should be added or not. Not adding the extensions which
fail probe to the list, so sbi_ecall_find_extension() can't find
them when their functions are invoked, is the main motivation of the
series.

> else
>      ret = SBI_ENODEV;
> if (ret)
>     return ret;
> 
> Basically, register_extensions() is optional for single extension ID
> but mandatory for extension ID range.

Oh, right, register_extensions() needs to be mandatory for multi-ID
extensions, otherwise we would still need a change in
sbi_ecall_find_extension() to ensure we don't return the extension for
IDs which aren't available. However, if we make it mandatory for all
multi-ID extensions, then the legacy extensions will need to implement
it, even though it will just call sbi_ecall_register_extension() without
making any changes first. I guess that's fine.

I'll go ahead and make register_extensions() mandatory for multi-ID
extensions and add the callback to the legacy range. So, assuming we
keep probe for the reasons I stated above, then we'll have this

 if (ext->register_extensions) {
     ret = ext->register_extensions();
 else if (ext->extid_start == ext->extid_end)
     if (!ext->probe || (!ext->probe(ext->extid_end, &out_val) && out_val))
         ret = sbi_ecall_register_extension(ext);
 else
     ret = SBI_ENODEV;

 if (ret)
     return ret;


Thanks,
drew


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback
  2023-05-12 15:06     ` Andrew Jones
@ 2023-05-12 16:47       ` Anup Patel
  2023-05-15  6:01         ` Andrew Jones
  0 siblings, 1 reply; 8+ messages in thread
From: Anup Patel @ 2023-05-12 16:47 UTC (permalink / raw)
  To: opensbi

On Fri, May 12, 2023 at 8:36?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, May 12, 2023 at 07:55:56PM +0530, Anup Patel wrote:
> > On Thu, May 11, 2023 at 9:41?PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >
> > > The register_extensions callback is called from sbi_ecall_init() for
> > > any extension that provides it. It's purpose is to register the
> > > extension with one or more sbi_ecall_register_extension() calls after
> > > possibly narrowing the range of extension IDs that should be handled.
> > > More than one call of sbi_ecall_register_extension() is necessary
> > > when the supported extension ID ranges have gaps. Additionally, when
> > > an extension may not be available and has a probe function, then the
> > > probe function should be consulted as to whether or not the extension
> > > should be registered. In summary, when register_extensions() returns,
> > > only valid extension IDs from the initial range, which are also
> > > available, have been registered.
> > >
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  include/sbi/sbi_ecall.h | 1 +
> > >  lib/sbi/sbi_ecall.c     | 8 ++++++--
> > >  2 files changed, 7 insertions(+), 2 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 5a301fb7d403..1c4a26fde438 100644
> > > --- a/lib/sbi/sbi_ecall.c
> > > +++ b/lib/sbi/sbi_ecall.c
> > > @@ -154,8 +154,12 @@ int sbi_ecall_init(void)
> > >
> > >         for (i = 0; i < sbi_ecall_exts_size; i++) {
> > >                 ext = sbi_ecall_exts[i];
> > > -               if (ext->extid_start != ext->extid_end || !ext->probe ||
> > > -                   (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> > > +               if (ext->register_extensions) {
> > > +                       ret = ext->register_extensions();
> > > +                       if (ret)
> > > +                               return ret;
> > > +               } else if (ext->extid_start != ext->extid_end || !ext->probe ||
> > > +                          (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> >
> > We should replace probe() callback with register_extensions()
> > and code over here can be something like below:
>
> We need to keep probe in case an extension wants to implement a
> nonzero out_val, which can be inspected by S-mode with the Base
> extension probe function:
>
> """
> Probe SBI extension (FID #3)
>
> Returns 0 if the given SBI extension ID (EID) is not available, or
> 1 if it is available unless defined as any other non-zero value by
> the implementation.
> """

Fair enough, we can keep the probe() callback for custom
return values but none of the existing SBI extensions have
custom probe return values so don't need probe existing
SBI extensions.

>
> >
> > if (ext->register_extensions)
> >      ret = ext->register_extensions();
> > else if (ext->extid_start == ext->extid_end)
> >      ret = sbi_ecall_register_extension(ext);
>
> Also here, if we don't have probe, then we can't know if the single
> ID extension should be added or not. Not adding the extensions which
> fail probe to the list, so sbi_ecall_find_extension() can't find
> them when their functions are invoked, is the main motivation of the
> series.

We should just implemetn register_extensions() for all
existing SBI extensions and remove probe() implementation
so the code over here will be a simple if-else:

if (ext->register_extensions)
    ret = ext->register_extensions();
else
    ret = SBI_ENODEV;
if (ret)
    return ret;

>
> > else
> >      ret = SBI_ENODEV;
> > if (ret)
> >     return ret;
> >
> > Basically, register_extensions() is optional for single extension ID
> > but mandatory for extension ID range.
>
> Oh, right, register_extensions() needs to be mandatory for multi-ID
> extensions, otherwise we would still need a change in
> sbi_ecall_find_extension() to ensure we don't return the extension for
> IDs which aren't available. However, if we make it mandatory for all
> multi-ID extensions, then the legacy extensions will need to implement
> it, even though it will just call sbi_ecall_register_extension() without
> making any changes first. I guess that's fine.
>
> I'll go ahead and make register_extensions() mandatory for multi-ID
> extensions and add the callback to the legacy range. So, assuming we
> keep probe for the reasons I stated above, then we'll have this
>
>  if (ext->register_extensions) {
>      ret = ext->register_extensions();
>  else if (ext->extid_start == ext->extid_end)
>      if (!ext->probe || (!ext->probe(ext->extid_end, &out_val) && out_val))
>          ret = sbi_ecall_register_extension(ext);
>  else
>      ret = SBI_ENODEV;
>
>  if (ret)
>      return ret;
>
>
> Thanks,
> drew

Regards,
Anup


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback
  2023-05-12 16:47       ` Anup Patel
@ 2023-05-15  6:01         ` Andrew Jones
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Jones @ 2023-05-15  6:01 UTC (permalink / raw)
  To: opensbi

On Fri, May 12, 2023 at 10:17:35PM +0530, Anup Patel wrote:
> On Fri, May 12, 2023 at 8:36?PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Fri, May 12, 2023 at 07:55:56PM +0530, Anup Patel wrote:
> > > On Thu, May 11, 2023 at 9:41?PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > > >
> > > > The register_extensions callback is called from sbi_ecall_init() for
> > > > any extension that provides it. It's purpose is to register the
> > > > extension with one or more sbi_ecall_register_extension() calls after
> > > > possibly narrowing the range of extension IDs that should be handled.
> > > > More than one call of sbi_ecall_register_extension() is necessary
> > > > when the supported extension ID ranges have gaps. Additionally, when
> > > > an extension may not be available and has a probe function, then the
> > > > probe function should be consulted as to whether or not the extension
> > > > should be registered. In summary, when register_extensions() returns,
> > > > only valid extension IDs from the initial range, which are also
> > > > available, have been registered.
> > > >
> > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > ---
> > > >  include/sbi/sbi_ecall.h | 1 +
> > > >  lib/sbi/sbi_ecall.c     | 8 ++++++--
> > > >  2 files changed, 7 insertions(+), 2 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 5a301fb7d403..1c4a26fde438 100644
> > > > --- a/lib/sbi/sbi_ecall.c
> > > > +++ b/lib/sbi/sbi_ecall.c
> > > > @@ -154,8 +154,12 @@ int sbi_ecall_init(void)
> > > >
> > > >         for (i = 0; i < sbi_ecall_exts_size; i++) {
> > > >                 ext = sbi_ecall_exts[i];
> > > > -               if (ext->extid_start != ext->extid_end || !ext->probe ||
> > > > -                   (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> > > > +               if (ext->register_extensions) {
> > > > +                       ret = ext->register_extensions();
> > > > +                       if (ret)
> > > > +                               return ret;
> > > > +               } else if (ext->extid_start != ext->extid_end || !ext->probe ||
> > > > +                          (!ext->probe(ext->extid_end, &out_val) && out_val)) {
> > >
> > > We should replace probe() callback with register_extensions()
> > > and code over here can be something like below:
> >
> > We need to keep probe in case an extension wants to implement a
> > nonzero out_val, which can be inspected by S-mode with the Base
> > extension probe function:
> >
> > """
> > Probe SBI extension (FID #3)
> >
> > Returns 0 if the given SBI extension ID (EID) is not available, or
> > 1 if it is available unless defined as any other non-zero value by
> > the implementation.
> > """
> 
> Fair enough, we can keep the probe() callback for custom
> return values but none of the existing SBI extensions have
> custom probe return values so don't need probe existing
> SBI extensions.

Sounds good. I'll add a patch documenting the probe callback's
purpose.

> 
> >
> > >
> > > if (ext->register_extensions)
> > >      ret = ext->register_extensions();
> > > else if (ext->extid_start == ext->extid_end)
> > >      ret = sbi_ecall_register_extension(ext);
> >
> > Also here, if we don't have probe, then we can't know if the single
> > ID extension should be added or not. Not adding the extensions which
> > fail probe to the list, so sbi_ecall_find_extension() can't find
> > them when their functions are invoked, is the main motivation of the
> > series.
> 
> We should just implemetn register_extensions() for all
> existing SBI extensions and remove probe() implementation
> so the code over here will be a simple if-else:
> 
> if (ext->register_extensions)
>     ret = ext->register_extensions();
> else
>     ret = SBI_ENODEV;
> if (ret)
>     return ret;

Works for me.

Thanks,
drew


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-05-15  6:01 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-11 16:11 [PATCH v4 0/3] lib: sbi: Ensure SBI extension is available Andrew Jones
2023-05-11 16:11 ` [PATCH v4 1/3] lib: sbi: Don't register unavailable single ID extensions Andrew Jones
2023-05-11 16:11 ` [PATCH v4 2/3] lib: sbi: Introduce register_extensions extension callback Andrew Jones
2023-05-12 14:25   ` Anup Patel
2023-05-12 15:06     ` Andrew Jones
2023-05-12 16:47       ` Anup Patel
2023-05-15  6:01         ` Andrew Jones
2023-05-11 16:11 ` [PATCH v4 3/3] lib: sbi: Apply register_extensions to vendor extensions Andrew Jones

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