| Message ID | 20230426172753.70664-1-ajones@ventanamicro.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | lib: sbi: Ensure SBI extension is available | expand |
在 2023-04-26星期三的 19:27 +0200，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. By adding a new > status member to the extension which has three states > (uninitialized, available, unavailable), we ensure that the > probe function is only invoked once, lazily, upon first use. Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an unused ID. Searching for an ID in this extension will return NULL if an unused ID is searched first. Regards, Xiang W > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > include/sbi/sbi_ecall.h | 6 ++++++ > lib/sbi/sbi_ecall.c | 18 ++++++++++++++---- > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h > index ff9bf8e2b435..8e31d0d91620 100644 > --- a/include/sbi/sbi_ecall.h > +++ b/include/sbi/sbi_ecall.h > @@ -20,10 +20,16 @@ > struct sbi_trap_regs; > struct sbi_trap_info; > > +enum sbi_ext_status { > + SBI_EXT_AVAILABLE = 1, > + SBI_EXT_UNAVAILABLE, > +}; > + > struct sbi_ecall_extension { > struct sbi_dlist head; > unsigned long extid_start; > unsigned long extid_end; > + enum sbi_ext_status status; > 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..20ff06f32dad 100644 > --- a/lib/sbi/sbi_ecall.c > +++ b/lib/sbi/sbi_ecall.c > @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > { > - struct sbi_ecall_extension *t, *ret = NULL; > + struct sbi_ecall_extension *t; > + unsigned long out_val; > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > if (t->extid_start <= extid && extid <= t->extid_end) { > - ret = t; > - break; > + if (t->status == SBI_EXT_AVAILABLE) > + return t; > + if (t->status == SBI_EXT_UNAVAILABLE) > + return NULL; > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { > + t->status = SBI_EXT_UNAVAILABLE; > + return NULL; > + } > + > + t->status = SBI_EXT_AVAILABLE; > + return t; > } > } > > - return ret; > + return NULL; > } > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > -- > 2.39.2 > >
On Thu, Apr 27, 2023 at 09:02:14AM +0800, Xiang W wrote: > 在 2023-04-26星期三的 19:27 +0200，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. By adding a new > > status member to the extension which has three states > > (uninitialized, available, unavailable), we ensure that the > > probe function is only invoked once, lazily, upon first use. > > Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an > unused ID. Searching for an ID in this extension will return NULL > if an unused ID is searched first. Hi Xiang, You're right. Either sbi_ecall_extension::status needs to be a function of extid, like probe is, or we should just always call probe (when it's available) and leave any optimizations to the individual probe functions. How about this? diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c index 76a1ae9ab733..c86e4e8c9134 100644 --- a/lib/sbi/sbi_ecall.c +++ b/lib/sbi/sbi_ecall.c @@ -42,16 +42,18 @@ static SBI_LIST_HEAD(ecall_exts_list); struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) { - struct sbi_ecall_extension *t, *ret = NULL; + struct sbi_ecall_extension *t; + unsigned long out_val; sbi_list_for_each_entry(t, &ecall_exts_list, head) { if (t->extid_start <= extid && extid <= t->extid_end) { - ret = t; - break; + if (t->probe && (t->probe(extid, &out_val) || !out_val)) + return NULL; + return t; } } - return ret; + return NULL; } int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) Thanks, drew > > Regards, > Xiang W > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > include/sbi/sbi_ecall.h | 6 ++++++ > > lib/sbi/sbi_ecall.c | 18 ++++++++++++++---- > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h > > index ff9bf8e2b435..8e31d0d91620 100644 > > --- a/include/sbi/sbi_ecall.h > > +++ b/include/sbi/sbi_ecall.h > > @@ -20,10 +20,16 @@ > > struct sbi_trap_regs; > > struct sbi_trap_info; > > > > +enum sbi_ext_status { > > + SBI_EXT_AVAILABLE = 1, > > + SBI_EXT_UNAVAILABLE, > > +}; > > + > > struct sbi_ecall_extension { > > struct sbi_dlist head; > > unsigned long extid_start; > > unsigned long extid_end; > > + enum sbi_ext_status status; > > 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..20ff06f32dad 100644 > > --- a/lib/sbi/sbi_ecall.c > > +++ b/lib/sbi/sbi_ecall.c > > @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > { > > - struct sbi_ecall_extension *t, *ret = NULL; > > + struct sbi_ecall_extension *t; > > + unsigned long out_val; > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > if (t->extid_start <= extid && extid <= t->extid_end) { > > - ret = t; > > - break; > > + if (t->status == SBI_EXT_AVAILABLE) > > + return t; > > + if (t->status == SBI_EXT_UNAVAILABLE) > > + return NULL; > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { > > + t->status = SBI_EXT_UNAVAILABLE; > > + return NULL; > > + } > > + > > + t->status = SBI_EXT_AVAILABLE; > > + return t; > > } > > } > > > > - return ret; > > + return NULL; > > } > > > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > > -- > > 2.39.2 > > > > > >
On Thu, Apr 27, 2023 at 09:00:35AM +0200, Andrew Jones wrote: > z4xshfbykgvcgcozn@rqv44g76lo4w> > References: <20230426172753.70664-1-ajones@ventanamicro.com> > <f798c3f500849d71aba59a06d4c90ea0e4cb2071.camel@126.com> > MIME-Version: 1.0 > Content-Type: text/plain; charset=utf-8 > Content-Disposition: inline > Content-Transfer-Encoding: 8bit > In-Reply-To: <f798c3f500849d71aba59a06d4c90ea0e4cb2071.camel@126.com> > > On Thu, Apr 27, 2023 at 09:02:14AM +0800, Xiang W wrote: > > 在 2023-04-26星期三的 19:27 +0200，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. By adding a new > > > status member to the extension which has three states > > > (uninitialized, available, unavailable), we ensure that the > > > probe function is only invoked once, lazily, upon first use. > > > > Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an > > unused ID. Searching for an ID in this extension will return NULL > > if an unused ID is searched first. > > Hi Xiang, > > You're right. Either sbi_ecall_extension::status needs to be a function of > extid, like probe is, or we should just always call probe (when it's > available) and leave any optimizations to the individual probe functions. > How about this? > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c > index 76a1ae9ab733..c86e4e8c9134 100644 > --- a/lib/sbi/sbi_ecall.c > +++ b/lib/sbi/sbi_ecall.c > @@ -42,16 +42,18 @@ static SBI_LIST_HEAD(ecall_exts_list); > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > { > - struct sbi_ecall_extension *t, *ret = NULL; > + struct sbi_ecall_extension *t; > + unsigned long out_val; > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > if (t->extid_start <= extid && extid <= t->extid_end) { > - ret = t; > - break; > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) > + return NULL; > + return t; > } > } > > - return ret; > + return NULL; > } > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) And, as a separate patch, to optimize probing of srst, which has a single extid for its range, and therefore doesn't care about it in its probe, diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c index 93b012ce024c..f2690b80775a 100644 --- a/lib/sbi/sbi_ecall_srst.c +++ b/lib/sbi/sbi_ecall_srst.c @@ -50,8 +50,15 @@ 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) { + static bool probed = false; + static unsigned long val; u32 type, count = 0; + if (probed) { + *out_val = val; + return 0; + } + /* * At least one standard reset types should be supported by * the platform for SBI SRST extension to be usable. @@ -63,7 +70,8 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val) count++; } - *out_val = (count) ? 1 : 0; + *out_val = val = (count) ? 1 : 0; + probed = true; return 0; } And susp can be optimized in exactly the same way. Thanks, drew
在 2023-04-27星期四的 09:00 +0200，Andrew Jones写道： > On Thu, Apr 27, 2023 at 09:02:14AM +0800, Xiang W wrote: > > 在 2023-04-26星期三的 19:27 +0200，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. By adding a new > > > status member to the extension which has three states > > > (uninitialized, available, unavailable), we ensure that the > > > probe function is only invoked once, lazily, upon first use. > > > > Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an > > unused ID. Searching for an ID in this extension will return NULL > > if an unused ID is searched first. > > Hi Xiang, > > You're right. Either sbi_ecall_extension::status needs to be a function of > extid, like probe is, or we should just always call probe (when it's > available) and leave any optimizations to the individual probe functions. > How about this? > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c > index 76a1ae9ab733..c86e4e8c9134 100644 > --- a/lib/sbi/sbi_ecall.c > +++ b/lib/sbi/sbi_ecall.c > @@ -42,16 +42,18 @@ static SBI_LIST_HEAD(ecall_exts_list); > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > { > - struct sbi_ecall_extension *t, *ret = NULL; > + struct sbi_ecall_extension *t; > + unsigned long out_val; > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > if (t->extid_start <= extid && extid <= t->extid_end) { > - ret = t; > - break; > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) > + return NULL; > + return t; > } > } > > - return ret; > + return NULL; > } > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > > Thanks, > drew Good. If so modified we can modify sbi_ecall_base_probe as well. As follows: diff --git a/lib/sbi/sbi_ecall_base.c b/lib/sbi/sbi_ecall_base.c index 786d2ac..e3aa02b 100644 --- a/lib/sbi/sbi_ecall_base.c +++ b/lib/sbi/sbi_ecall_base.c @@ -20,15 +20,7 @@ static int sbi_ecall_base_probe(unsigned long extid, unsigned long *out_val) struct sbi_ecall_extension *ext; ext = sbi_ecall_find_extension(extid); - if (!ext) { - *out_val = 0; - return 0; - } - - if (ext->probe) - return ext->probe(extid, out_val); - - *out_val = 1; + *out_val = ext ? 1 : 0; return 0; } Regards, Xiang W > > > > > Regards, > > Xiang W > > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > --- > > > include/sbi/sbi_ecall.h | 6 ++++++ > > > lib/sbi/sbi_ecall.c | 18 ++++++++++++++---- > > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h > > > index ff9bf8e2b435..8e31d0d91620 100644 > > > --- a/include/sbi/sbi_ecall.h > > > +++ b/include/sbi/sbi_ecall.h > > > @@ -20,10 +20,16 @@ > > > struct sbi_trap_regs; > > > struct sbi_trap_info; > > > > > > +enum sbi_ext_status { > > > + SBI_EXT_AVAILABLE = 1, > > > + SBI_EXT_UNAVAILABLE, > > > +}; > > > + > > > struct sbi_ecall_extension { > > > struct sbi_dlist head; > > > unsigned long extid_start; > > > unsigned long extid_end; > > > + enum sbi_ext_status status; > > > 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..20ff06f32dad 100644 > > > --- a/lib/sbi/sbi_ecall.c > > > +++ b/lib/sbi/sbi_ecall.c > > > @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > > { > > > - struct sbi_ecall_extension *t, *ret = NULL; > > > + struct sbi_ecall_extension *t; > > > + unsigned long out_val; > > > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > > if (t->extid_start <= extid && extid <= t->extid_end) { > > > - ret = t; > > > - break; > > > + if (t->status == SBI_EXT_AVAILABLE) > > > + return t; > > > + if (t->status == SBI_EXT_UNAVAILABLE) > > > + return NULL; > > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { > > > + t->status = SBI_EXT_UNAVAILABLE; > > > + return NULL; > > > + } > > > + > > > + t->status = SBI_EXT_AVAILABLE; > > > + return t; > > > } > > > } > > > > > > - return ret; > > > + return NULL; > > > } > > > > > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > > > -- > > > 2.39.2 > > > > > > > > > >
在 2023-04-27星期四的 09:18 +0200，Andrew Jones写道： > On Thu, Apr 27, 2023 at 09:00:35AM +0200, Andrew Jones wrote: > > z4xshfbykgvcgcozn@rqv44g76lo4w> > > References: <20230426172753.70664-1-ajones@ventanamicro.com> > > <f798c3f500849d71aba59a06d4c90ea0e4cb2071.camel@126.com> > > MIME-Version: 1.0 > > Content-Type: text/plain; charset=utf-8 > > Content-Disposition: inline > > Content-Transfer-Encoding: 8bit > > In-Reply-To: <f798c3f500849d71aba59a06d4c90ea0e4cb2071.camel@126.com> > > > > On Thu, Apr 27, 2023 at 09:02:14AM +0800, Xiang W wrote: > > > 在 2023-04-26星期三的 19:27 +0200，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. By adding a new > > > > status member to the extension which has three states > > > > (uninitialized, available, unavailable), we ensure that the > > > > probe function is only invoked once, lazily, upon first use. > > > > > > Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an > > > unused ID. Searching for an ID in this extension will return NULL > > > if an unused ID is searched first. > > > > Hi Xiang, > > > > You're right. Either sbi_ecall_extension::status needs to be a function of > > extid, like probe is, or we should just always call probe (when it's > > available) and leave any optimizations to the individual probe functions. > > How about this? > > > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c > > index 76a1ae9ab733..c86e4e8c9134 100644 > > --- a/lib/sbi/sbi_ecall.c > > +++ b/lib/sbi/sbi_ecall.c > > @@ -42,16 +42,18 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > { > > - struct sbi_ecall_extension *t, *ret = NULL; > > + struct sbi_ecall_extension *t; > > + unsigned long out_val; > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > if (t->extid_start <= extid && extid <= t->extid_end) { > > - ret = t; > > - break; > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) > > + return NULL; > > + return t; > > } > > } > > > > - return ret; > > + return NULL; > > } > > > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > > And, as a separate patch, to optimize probing of srst, which has a single > extid for its range, and therefore doesn't care about it in its probe, > > diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c > index 93b012ce024c..f2690b80775a 100644 > --- a/lib/sbi/sbi_ecall_srst.c > +++ b/lib/sbi/sbi_ecall_srst.c > @@ -50,8 +50,15 @@ 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) > { > + static bool probed = false; > + static unsigned long val; > u32 type, count = 0; > > + if (probed) { > + *out_val = val; > + return 0; > + } > + > /* > * At least one standard reset types should be supported by > * the platform for SBI SRST extension to be usable. > @@ -63,7 +70,8 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val) > count++; > } > > - *out_val = (count) ? 1 : 0; > + *out_val = val = (count) ? 1 : 0; > + probed = true; > return 0; > } > > And susp can be optimized in exactly the same way. > > Thanks, > drew > This can be a generic optimization. Since most extensions have only one extension id, here is my suggestion diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c index 76a1ae9..900a4fa 100644 --- a/lib/sbi/sbi_ecall.c +++ b/lib/sbi/sbi_ecall.c @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) { - struct sbi_ecall_extension *t, *ret = NULL; + struct sbi_ecall_extension *t; sbi_list_for_each_entry(t, &ecall_exts_list, head) { if (t->extid_start <= extid && extid <= t->extid_end) { - ret = t; - break; + if (t->extid_start == t->extid_end) { + if (t->status == SBI_EXT_AVAILABLE) + return t; + if (t->status == SBI_EXT_UNAVAILABLE) + return NULL; + } + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { + t->status = SBI_EXT_UNAVAILABLE; + return NULL + } + t->status = SBI_EXT_AVAILABLE; + return t; } } - return ret; + return NULL; }
On Thu, Apr 27, 2023 at 03:26:20PM +0800, Xiang W wrote: > 在 2023-04-27星期四的 09:00 +0200，Andrew Jones写道： > > On Thu, Apr 27, 2023 at 09:02:14AM +0800, Xiang W wrote: > > > 在 2023-04-26星期三的 19:27 +0200，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. By adding a new > > > > status member to the extension which has three states > > > > (uninitialized, available, unavailable), we ensure that the > > > > probe function is only invoked once, lazily, upon first use. > > > > > > Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an > > > unused ID. Searching for an ID in this extension will return NULL > > > if an unused ID is searched first. > > > > Hi Xiang, > > > > You're right. Either sbi_ecall_extension::status needs to be a function of > > extid, like probe is, or we should just always call probe (when it's > > available) and leave any optimizations to the individual probe functions. > > How about this? > > > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c > > index 76a1ae9ab733..c86e4e8c9134 100644 > > --- a/lib/sbi/sbi_ecall.c > > +++ b/lib/sbi/sbi_ecall.c > > @@ -42,16 +42,18 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > { > > - struct sbi_ecall_extension *t, *ret = NULL; > > + struct sbi_ecall_extension *t; > > + unsigned long out_val; > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > if (t->extid_start <= extid && extid <= t->extid_end) { > > - ret = t; > > - break; > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) > > + return NULL; > > + return t; > > } > > } > > > > - return ret; > > + return NULL; > > } > > > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > > > > Thanks, > > drew > Good. If so modified we can modify sbi_ecall_base_probe as well. > As follows: > > diff --git a/lib/sbi/sbi_ecall_base.c b/lib/sbi/sbi_ecall_base.c > index 786d2ac..e3aa02b 100644 > --- a/lib/sbi/sbi_ecall_base.c > +++ b/lib/sbi/sbi_ecall_base.c > @@ -20,15 +20,7 @@ static int sbi_ecall_base_probe(unsigned long extid, unsigned long *out_val) > struct sbi_ecall_extension *ext; > > ext = sbi_ecall_find_extension(extid); > - if (!ext) { > - *out_val = 0; > - return 0; > - } > - > - if (ext->probe) > - return ext->probe(extid, out_val); > - > - *out_val = 1; > + *out_val = ext ? 1 : 0; > return 0; > } > If we do this, then we lose the return value of the extension's probe function, which is currently getting propagated all the way back to the SBI caller. sbi_ecall_base_probe() isn't in a fast path, so even though probe will get invoked twice, I guess we can afford it. Thanks, drew
On Thu, Apr 27, 2023 at 03:52:16PM +0800, Xiang W wrote: > 在 2023-04-27星期四的 09:18 +0200，Andrew Jones写道： > > On Thu, Apr 27, 2023 at 09:00:35AM +0200, Andrew Jones wrote: > > > z4xshfbykgvcgcozn@rqv44g76lo4w> > > > References: <20230426172753.70664-1-ajones@ventanamicro.com> > > > <f798c3f500849d71aba59a06d4c90ea0e4cb2071.camel@126.com> > > > MIME-Version: 1.0 > > > Content-Type: text/plain; charset=utf-8 > > > Content-Disposition: inline > > > Content-Transfer-Encoding: 8bit > > > In-Reply-To: <f798c3f500849d71aba59a06d4c90ea0e4cb2071.camel@126.com> > > > > > > On Thu, Apr 27, 2023 at 09:02:14AM +0800, Xiang W wrote: > > > > 在 2023-04-26星期三的 19:27 +0200，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. By adding a new > > > > > status member to the extension which has three states > > > > > (uninitialized, available, unavailable), we ensure that the > > > > > probe function is only invoked once, lazily, upon first use. > > > > > > > > Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an > > > > unused ID. Searching for an ID in this extension will return NULL > > > > if an unused ID is searched first. > > > > > > Hi Xiang, > > > > > > You're right. Either sbi_ecall_extension::status needs to be a function of > > > extid, like probe is, or we should just always call probe (when it's > > > available) and leave any optimizations to the individual probe functions. > > > How about this? > > > > > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c > > > index 76a1ae9ab733..c86e4e8c9134 100644 > > > --- a/lib/sbi/sbi_ecall.c > > > +++ b/lib/sbi/sbi_ecall.c > > > @@ -42,16 +42,18 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > > { > > > - struct sbi_ecall_extension *t, *ret = NULL; > > > + struct sbi_ecall_extension *t; > > > + unsigned long out_val; > > > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > > if (t->extid_start <= extid && extid <= t->extid_end) { > > > - ret = t; > > > - break; > > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) > > > + return NULL; > > > + return t; > > > } > > > } > > > > > > - return ret; > > > + return NULL; > > > } > > > > > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > > > > And, as a separate patch, to optimize probing of srst, which has a single > > extid for its range, and therefore doesn't care about it in its probe, > > > > diff --git a/lib/sbi/sbi_ecall_srst.c b/lib/sbi/sbi_ecall_srst.c > > index 93b012ce024c..f2690b80775a 100644 > > --- a/lib/sbi/sbi_ecall_srst.c > > +++ b/lib/sbi/sbi_ecall_srst.c > > @@ -50,8 +50,15 @@ 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) > > { > > + static bool probed = false; > > + static unsigned long val; > > u32 type, count = 0; > > > > + if (probed) { > > + *out_val = val; > > + return 0; > > + } > > + > > /* > > * At least one standard reset types should be supported by > > * the platform for SBI SRST extension to be usable. > > @@ -63,7 +70,8 @@ static int sbi_ecall_srst_probe(unsigned long extid, unsigned long *out_val) > > count++; > > } > > > > - *out_val = (count) ? 1 : 0; > > + *out_val = val = (count) ? 1 : 0; > > + probed = true; > > return 0; > > } > > > > And susp can be optimized in exactly the same way. > > > > Thanks, > > drew > > > > This can be a generic optimization. Since most extensions have only > one extension id, here is my suggestion > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c > index 76a1ae9..900a4fa 100644 > --- a/lib/sbi/sbi_ecall.c > +++ b/lib/sbi/sbi_ecall.c > @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > { > - struct sbi_ecall_extension *t, *ret = NULL; > + struct sbi_ecall_extension *t; > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > if (t->extid_start <= extid && extid <= t->extid_end) { > - ret = t; > - break; > + if (t->extid_start == t->extid_end) { > + if (t->status == SBI_EXT_AVAILABLE) > + return t; > + if (t->status == SBI_EXT_UNAVAILABLE) > + return NULL; > + } > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { > + t->status = SBI_EXT_UNAVAILABLE; > + return NULL > + } > + t->status = SBI_EXT_AVAILABLE; > + return t; > } > } > > - return ret; > + return NULL; > } > Yeah, LGTM. v2 coming up. Thanks! drew
在 2023-04-27星期四的 10:01 +0200，Andrew Jones写道： > On Thu, Apr 27, 2023 at 03:26:20PM +0800, Xiang W wrote: > > 在 2023-04-27星期四的 09:00 +0200，Andrew Jones写道： > > > On Thu, Apr 27, 2023 at 09:02:14AM +0800, Xiang W wrote: > > > > 在 2023-04-26星期三的 19:27 +0200，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. By adding a new > > > > > status member to the extension which has three states > > > > > (uninitialized, available, unavailable), we ensure that the > > > > > probe function is only invoked once, lazily, upon first use. > > > > > > > > Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an > > > > unused ID. Searching for an ID in this extension will return NULL > > > > if an unused ID is searched first. > > > > > > Hi Xiang, > > > > > > You're right. Either sbi_ecall_extension::status needs to be a function of > > > extid, like probe is, or we should just always call probe (when it's > > > available) and leave any optimizations to the individual probe functions. > > > How about this? > > > > > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c > > > index 76a1ae9ab733..c86e4e8c9134 100644 > > > --- a/lib/sbi/sbi_ecall.c > > > +++ b/lib/sbi/sbi_ecall.c > > > @@ -42,16 +42,18 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > > { > > > - struct sbi_ecall_extension *t, *ret = NULL; > > > + struct sbi_ecall_extension *t; > > > + unsigned long out_val; > > > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > > if (t->extid_start <= extid && extid <= t->extid_end) { > > > - ret = t; > > > - break; > > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) > > > + return NULL; > > > + return t; > > > } > > > } > > > > > > - return ret; > > > + return NULL; > > > } > > > > > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > > > > > > Thanks, > > > drew > > Good. If so modified we can modify sbi_ecall_base_probe as well. > > As follows: > > > > diff --git a/lib/sbi/sbi_ecall_base.c b/lib/sbi/sbi_ecall_base.c > > index 786d2ac..e3aa02b 100644 > > --- a/lib/sbi/sbi_ecall_base.c > > +++ b/lib/sbi/sbi_ecall_base.c > > @@ -20,15 +20,7 @@ static int sbi_ecall_base_probe(unsigned long extid, unsigned long *out_val) > > struct sbi_ecall_extension *ext; > > > > ext = sbi_ecall_find_extension(extid); > > - if (!ext) { > > - *out_val = 0; > > - return 0; > > - } > > - > > - if (ext->probe) > > - return ext->probe(extid, out_val); > > - > > - *out_val = 1; > > + *out_val = ext ? 1 : 0; > > return 0; > > } > > > > If we do this, then we lose the return value of the extension's probe > function, which is currently getting propagated all the way back to > the SBI caller. sbi_ecall_base_probe() isn't in a fast path, so even > though probe will get invoked twice, I guess we can afford it. > > Thanks, > drew If you don't modify sbi_ecall_base_probe, I don't think we need to modify sbi_ecall_find_extension. sbi_ecall_find_extension just find the structure of extension. If there is a gap in the id number, the handle and probe will definitely handle it. Regards, Xiang W
On Wed, Apr 26, 2023 at 10:58 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > 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. By adding a new > status member to the extension which has three states > (uninitialized, available, unavailable), we ensure that the > probe function is only invoked once, lazily, upon first use. > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > include/sbi/sbi_ecall.h | 6 ++++++ > lib/sbi/sbi_ecall.c | 18 ++++++++++++++---- > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h > index ff9bf8e2b435..8e31d0d91620 100644 > --- a/include/sbi/sbi_ecall.h > +++ b/include/sbi/sbi_ecall.h > @@ -20,10 +20,16 @@ > struct sbi_trap_regs; > struct sbi_trap_info; > > +enum sbi_ext_status { > + SBI_EXT_AVAILABLE = 1, > + SBI_EXT_UNAVAILABLE, > +}; > + > struct sbi_ecall_extension { > struct sbi_dlist head; > unsigned long extid_start; > unsigned long extid_end; > + enum sbi_ext_status status; > 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..20ff06f32dad 100644 > --- a/lib/sbi/sbi_ecall.c > +++ b/lib/sbi/sbi_ecall.c > @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > { > - struct sbi_ecall_extension *t, *ret = NULL; > + struct sbi_ecall_extension *t; > + unsigned long out_val; > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > if (t->extid_start <= extid && extid <= t->extid_end) { > - ret = t; > - break; > + if (t->status == SBI_EXT_AVAILABLE) > + return t; > + if (t->status == SBI_EXT_UNAVAILABLE) > + return NULL; > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { > + t->status = SBI_EXT_UNAVAILABLE; Setting this `status` flag is a bit racey because two HARTs can end-up in sbi_ecall_find_extension() at the same time. I suggest an alternate approach whereby we don't need the `status` flag and sbi_ecall_find_extension() also does not need to change: 1) Rename `probe()` callback to `init()` callback 2) Remove sbi_ecall_register_extension() call from sbi_ecall_init() and instead call `init()` for each extension 3) Implement `init()` for each extension such that each extension explicitly register itself using sbi_ecall_register_extension() only if the extension is actually available. In other words, the alternate approach treats SBI extensions are driver instances which need to be registered by some init() call. Regards, Anup > + return NULL; > + } > + > + t->status = SBI_EXT_AVAILABLE; > + return t; > } > } > > - return ret; > + return NULL; > } > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > -- > 2.39.2 > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
On Fri, Apr 28, 2023 at 09:52:00AM +0800, Xiang W wrote: > 在 2023-04-27星期四的 10:01 +0200，Andrew Jones写道： > > On Thu, Apr 27, 2023 at 03:26:20PM +0800, Xiang W wrote: > > > 在 2023-04-27星期四的 09:00 +0200，Andrew Jones写道： > > > > On Thu, Apr 27, 2023 at 09:02:14AM +0800, Xiang W wrote: > > > > > 在 2023-04-26星期三的 19:27 +0200，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. By adding a new > > > > > > status member to the extension which has three states > > > > > > (uninitialized, available, unavailable), we ensure that the > > > > > > probe function is only invoked once, lazily, upon first use. > > > > > > > > > > Adding this state may cause errors. For example: the id numberof an extension is from extid_start to extid_end, which has an > > > > > unused ID. Searching for an ID in this extension will return NULL > > > > > if an unused ID is searched first. > > > > > > > > Hi Xiang, > > > > > > > > You're right. Either sbi_ecall_extension::status needs to be a function of > > > > extid, like probe is, or we should just always call probe (when it's > > > > available) and leave any optimizations to the individual probe functions. > > > > How about this? > > > > > > > > diff --git a/lib/sbi/sbi_ecall.c b/lib/sbi/sbi_ecall.c > > > > index 76a1ae9ab733..c86e4e8c9134 100644 > > > > --- a/lib/sbi/sbi_ecall.c > > > > +++ b/lib/sbi/sbi_ecall.c > > > > @@ -42,16 +42,18 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > > > { > > > > - struct sbi_ecall_extension *t, *ret = NULL; > > > > + struct sbi_ecall_extension *t; > > > > + unsigned long out_val; > > > > > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > > > if (t->extid_start <= extid && extid <= t->extid_end) { > > > > - ret = t; > > > > - break; > > > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) > > > > + return NULL; > > > > + return t; > > > > } > > > > } > > > > > > > > - return ret; > > > > + return NULL; > > > > } > > > > > > > > int sbi_ecall_register_extension(struct sbi_ecall_extension *ext) > > > > > > > > Thanks, > > > > drew > > > Good. If so modified we can modify sbi_ecall_base_probe as well. > > > As follows: > > > > > > diff --git a/lib/sbi/sbi_ecall_base.c b/lib/sbi/sbi_ecall_base.c > > > index 786d2ac..e3aa02b 100644 > > > --- a/lib/sbi/sbi_ecall_base.c > > > +++ b/lib/sbi/sbi_ecall_base.c > > > @@ -20,15 +20,7 @@ static int sbi_ecall_base_probe(unsigned long extid, unsigned long *out_val) > > > struct sbi_ecall_extension *ext; > > > > > > ext = sbi_ecall_find_extension(extid); > > > - if (!ext) { > > > - *out_val = 0; > > > - return 0; > > > - } > > > - > > > - if (ext->probe) > > > - return ext->probe(extid, out_val); > > > - > > > - *out_val = 1; > > > + *out_val = ext ? 1 : 0; > > > return 0; > > > } > > > > > > > If we do this, then we lose the return value of the extension's probe > > function, which is currently getting propagated all the way back to > > the SBI caller. sbi_ecall_base_probe() isn't in a fast path, so even > > though probe will get invoked twice, I guess we can afford it. > > > > Thanks, > > drew > > If you don't modify sbi_ecall_base_probe, I don't think we need to modify > sbi_ecall_find_extension. sbi_ecall_find_extension just find the structure > of extension. If there is a gap in the id number, the handle and probe > will definitely handle it. I'm not sure I follow. The main goal of modifying sbi_ecall_find_extension is to ensure that we don't call handle unless probe says we can. Thanks, drew
On Fri, Apr 28, 2023 at 09:35:45AM +0530, Anup Patel wrote: > On Wed, Apr 26, 2023 at 10:58 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > 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. By adding a new > > status member to the extension which has three states > > (uninitialized, available, unavailable), we ensure that the > > probe function is only invoked once, lazily, upon first use. > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > include/sbi/sbi_ecall.h | 6 ++++++ > > lib/sbi/sbi_ecall.c | 18 ++++++++++++++---- > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h > > index ff9bf8e2b435..8e31d0d91620 100644 > > --- a/include/sbi/sbi_ecall.h > > +++ b/include/sbi/sbi_ecall.h > > @@ -20,10 +20,16 @@ > > struct sbi_trap_regs; > > struct sbi_trap_info; > > > > +enum sbi_ext_status { > > + SBI_EXT_AVAILABLE = 1, > > + SBI_EXT_UNAVAILABLE, > > +}; > > + > > struct sbi_ecall_extension { > > struct sbi_dlist head; > > unsigned long extid_start; > > unsigned long extid_end; > > + enum sbi_ext_status status; > > 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..20ff06f32dad 100644 > > --- a/lib/sbi/sbi_ecall.c > > +++ b/lib/sbi/sbi_ecall.c > > @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > { > > - struct sbi_ecall_extension *t, *ret = NULL; > > + struct sbi_ecall_extension *t; > > + unsigned long out_val; > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > if (t->extid_start <= extid && extid <= t->extid_end) { > > - ret = t; > > - break; > > + if (t->status == SBI_EXT_AVAILABLE) > > + return t; > > + if (t->status == SBI_EXT_UNAVAILABLE) > > + return NULL; > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { > > + t->status = SBI_EXT_UNAVAILABLE; > > Setting this `status` flag is a bit racey because two HARTs can end-up > in sbi_ecall_find_extension() at the same time. It's racy, but it shouldn't matter, since the result will be the same whether a second hart ends up calling probe itself or not. > > I suggest an alternate approach whereby we don't need the `status` > flag and sbi_ecall_find_extension() also does not need to change: > > 1) Rename `probe()` callback to `init()` callback > 2) Remove sbi_ecall_register_extension() call from sbi_ecall_init() > and instead call `init()` for each extension > 3) Implement `init()` for each extension such that each extension > explicitly register itself using sbi_ecall_register_extension() > only if the extension is actually available. > > In other words, the alternate approach treats SBI extensions are > driver instances which need to be registered by some init() call. I mostly like that, but probe is specified as "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.", which means a future extension may want to return some nonzero value from its probe function to inform the caller of something. So, maybe we keep probe, add init, and call probe from init to decide if we register the extension? Thanks, drew
On Fri, Apr 28, 2023 at 4:21 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Fri, Apr 28, 2023 at 09:35:45AM +0530, Anup Patel wrote: > > On Wed, Apr 26, 2023 at 10:58 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > 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. By adding a new > > > status member to the extension which has three states > > > (uninitialized, available, unavailable), we ensure that the > > > probe function is only invoked once, lazily, upon first use. > > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > --- > > > include/sbi/sbi_ecall.h | 6 ++++++ > > > lib/sbi/sbi_ecall.c | 18 ++++++++++++++---- > > > 2 files changed, 20 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h > > > index ff9bf8e2b435..8e31d0d91620 100644 > > > --- a/include/sbi/sbi_ecall.h > > > +++ b/include/sbi/sbi_ecall.h > > > @@ -20,10 +20,16 @@ > > > struct sbi_trap_regs; > > > struct sbi_trap_info; > > > > > > +enum sbi_ext_status { > > > + SBI_EXT_AVAILABLE = 1, > > > + SBI_EXT_UNAVAILABLE, > > > +}; > > > + > > > struct sbi_ecall_extension { > > > struct sbi_dlist head; > > > unsigned long extid_start; > > > unsigned long extid_end; > > > + enum sbi_ext_status status; > > > 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..20ff06f32dad 100644 > > > --- a/lib/sbi/sbi_ecall.c > > > +++ b/lib/sbi/sbi_ecall.c > > > @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); > > > > > > struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) > > > { > > > - struct sbi_ecall_extension *t, *ret = NULL; > > > + struct sbi_ecall_extension *t; > > > + unsigned long out_val; > > > > > > sbi_list_for_each_entry(t, &ecall_exts_list, head) { > > > if (t->extid_start <= extid && extid <= t->extid_end) { > > > - ret = t; > > > - break; > > > + if (t->status == SBI_EXT_AVAILABLE) > > > + return t; > > > + if (t->status == SBI_EXT_UNAVAILABLE) > > > + return NULL; > > > + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { > > > + t->status = SBI_EXT_UNAVAILABLE; > > > > Setting this `status` flag is a bit racey because two HARTs can end-up > > in sbi_ecall_find_extension() at the same time. > > It's racy, but it shouldn't matter, since the result will be the > same whether a second hart ends up calling probe itself or not. > > > > > I suggest an alternate approach whereby we don't need the `status` > > flag and sbi_ecall_find_extension() also does not need to change: > > > > 1) Rename `probe()` callback to `init()` callback > > 2) Remove sbi_ecall_register_extension() call from sbi_ecall_init() > > and instead call `init()` for each extension > > 3) Implement `init()` for each extension such that each extension > > explicitly register itself using sbi_ecall_register_extension() > > only if the extension is actually available. > > > > In other words, the alternate approach treats SBI extensions are > > driver instances which need to be registered by some init() call. > > I mostly like that, but probe is specified as "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.", > which means a future extension may want to return some nonzero value > from its probe function to inform the caller of something. So, maybe > we keep probe, add init, and call probe from init to decide if we > register the extension? I am okay to keep the probe() callback but currently none of the extensions have custom probe return value. Regards, Anup
diff --git a/include/sbi/sbi_ecall.h b/include/sbi/sbi_ecall.h index ff9bf8e2b435..8e31d0d91620 100644 --- a/include/sbi/sbi_ecall.h +++ b/include/sbi/sbi_ecall.h @@ -20,10 +20,16 @@ struct sbi_trap_regs; struct sbi_trap_info; +enum sbi_ext_status { + SBI_EXT_AVAILABLE = 1, + SBI_EXT_UNAVAILABLE, +}; + struct sbi_ecall_extension { struct sbi_dlist head; unsigned long extid_start; unsigned long extid_end; + enum sbi_ext_status status; 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..20ff06f32dad 100644 --- a/lib/sbi/sbi_ecall.c +++ b/lib/sbi/sbi_ecall.c @@ -42,16 +42,26 @@ static SBI_LIST_HEAD(ecall_exts_list); struct sbi_ecall_extension *sbi_ecall_find_extension(unsigned long extid) { - struct sbi_ecall_extension *t, *ret = NULL; + struct sbi_ecall_extension *t; + unsigned long out_val; sbi_list_for_each_entry(t, &ecall_exts_list, head) { if (t->extid_start <= extid && extid <= t->extid_end) { - ret = t; - break; + if (t->status == SBI_EXT_AVAILABLE) + return t; + if (t->status == SBI_EXT_UNAVAILABLE) + return NULL; + if (t->probe && (t->probe(extid, &out_val) || !out_val)) { + t->status = SBI_EXT_UNAVAILABLE; + return NULL; + } + + t->status = SBI_EXT_AVAILABLE; + return t; } } - return ret; + return NULL; } int sbi_ecall_register_extension(struct sbi_ecall_extension *ext)
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. By adding a new status member to the extension which has three states (uninitialized, available, unavailable), we ensure that the probe function is only invoked once, lazily, upon first use. Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- include/sbi/sbi_ecall.h | 6 ++++++ lib/sbi/sbi_ecall.c | 18 ++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-)