[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH 2/9] include: Clean-ups to <uk/ctors.h>
Hey Vlad,
thanks for the review. I put my replies inline.
Thanks,
Simon
On 30.01.20 18:04, Vlad-Andrei BĂDOIU (78692) wrote:
Hey Simon,
Please see inline, one comment should be ignored.
On 30.01.2020 18:56, Vlad-Andrei BĂDOIU (78692) wrote:
Hi Simon,
Thanks for the patch! Please see my comments inline.
Thanks,
Vlad
On 29.01.2020 23:48, Simon Kuenzer wrote:
This commit brings the Unikraft constructor definitions closer to the
one of the Unikraft Init table.
UK_CTOR_FUNC() is basically replaced by UK_CTOR_PRIO(). Naming and
argument order is derived from Init table definition <uk/init.h>.
The table iterator is now similar as the one found in the Init
table. Instead of providing an integer as index, the foreach-macro is
directly iterating with a function pointer.
Because of compatibility reasons, the previous macro UK_CTOR_FUNC() is
still provided. The idea is that it gets removed as soon all depending
code was updated (internal and external).
Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
---
include/uk/ctors.h | 60 ++++++++++++-------
lib/ukboot/boot.c | 46 +++++++-------
lib/uklibparam/include/uk/libparam.h | 2 +-
.../include/uk/plat/common/common.lds.h | 2 +-
4 files changed, 64 insertions(+), 46 deletions(-)
diff --git a/include/uk/ctors.h b/include/uk/ctors.h
index e7d0dece..d8a1c838 100644
--- a/include/uk/ctors.h
+++ b/include/uk/ctors.h
@@ -53,41 +53,55 @@ extern const uk_ctor_func_t __preinit_array_start[];
extern const uk_ctor_func_t __preinit_array_end;
extern const uk_ctor_func_t __init_array_start[];
extern const uk_ctor_func_t __init_array_end;
-extern const uk_ctor_func_t uk_ctortab[];
+extern const uk_ctor_func_t uk_ctortab_start[];
extern const uk_ctor_func_t uk_ctortab_end;
/**
* Register a Unikraft constructor function that is
* called during bootstrap (uk_ctortab)
*
- * @param lvl
- * Priority level (0 (higher) to 9 (least))
- * Note: Any other value for level will be ignored
- * @param ctorf
+ * @param fn
* Constructor function to be called
+ * @param prio
+ * Priority level (0 (earliest) to 9 (latest))
+ * Note: Any other value for level will be ignored
+ */
+#define __UK_CTORTAB(fn, libname, prio) \
+ static const uk_ctor_func_t \
+ __used __section(".uk_ctortab" #prio) \
+ __uk_ctortab ## lvl ## _ ## libname ## _ ## fn = (fn)
Since we have renamed lvl, here we should have prio not lvl.
If libname has special characters such as "-" the macro won't compile and
will result in an error such as: error: expected ‘=’, ‘,’, ‘;’, ‘asm’
or ‘__attribute__’ before ‘-’ token.
Maybe we should not concatenate the library's name.
Sure, I can take it out again. I added it to make sure that we do not
get any defintion conflicts in case two libs use the same name for its
constructors; however they are actually declared as static... hum...
maybe it is fine. I will remove it.
+
+#define _UK_CTORTAB(fn, libname, prio) \
+ __UK_CTORTAB(fn, libname, prio)
+
+#define UK_CTOR_PRIO(fn, prio) \
+ _UK_CTORTAB(fn, __LIBNAME__, prio)
+
+/**
+ * Similar interface without priority.
*/
-#define __UK_CTOR_FUNC(lvl, ctorf) \
- static const uk_ctor_func_t \
- __used __section(".uk_ctortab" #lvl) \
- __uk_ctab ## lvl ## _ ## ctorf = (ctorf)
-#define UK_CTOR_FUNC(lvl, ctorf) __UK_CTOR_FUNC(lvl, ctorf)
+#define UK_CTOR(fn) UK_CTOR_PRIO(fn, 9)
+
+/* DELETEME: Compatibility wrapper for existing code, to be removed! */
+#define UK_CTOR_FUNC(lvl, ctorf) \
+ _UK_CTORTAB(ctorf, __LIBNAME__, lvl)
There are two __ missing here, it should be __UK_CTOR_FUNC.
Please ignore this comment.
/**
- * Helper macro for iterating over constructor pointer arrays
- * Please note that the array may contain NULL pointer entries
+ * Helper macro for iterating over constructor pointer tables
+ * Please note that the table may contain NULL pointer entries
*
- * @param arr_start
- * Start address of pointer array (type: const uk_ctor_func_t const [])
- * @param arr_end
- * End address of pointer array
- * @param i
- * Iterator variable (integer) which should be used to access the
- * individual fields
+ * @param itr
+ * Iterator variable (uk_ctor_func_t *) which points to the individual
+ * table entries during iteration
+ * @param ctortab_start
+ * Start address of table (type: const uk_ctor_func_t[])
+ * @param ctortab_end
+ * End address of table (type: const uk_ctor_func_t)
*/
-#define uk_ctor_foreach(arr_start, arr_end, i) \
- for ((i) = 0; \
- &((arr_start)[i]) < &(arr_end); \
- ++(i))
+#define uk_ctortab_foreach(itr, ctortab_start, ctortab_end) \
+ for ((itr) = DECONST(uk_ctor_func_t*, ctortab_start); \
+ (itr) < &(ctortab_end); \
+ (itr)++)
#ifdef __cplusplus
}
diff --git a/lib/ukboot/boot.c b/lib/ukboot/boot.c
index 3f5046ca..aa6999cc 100644
--- a/lib/ukboot/boot.c
+++ b/lib/ukboot/boot.c
@@ -77,6 +77,7 @@ static void main_thread_func(void *arg)
int ret;
struct thread_main_arg *tma = arg;
uk_init_t *itr;
+ uk_ctor_func_t *ctorfn;
/**
* Run init table
@@ -111,25 +112,24 @@ static void main_thread_func(void *arg)
* from its OS being initialized.
*/
uk_pr_info("Pre-init table at %p - %p\n",
- __preinit_array_start, &__preinit_array_end);
- uk_ctor_foreach(__preinit_array_start, __preinit_array_end, i) {
- if (__preinit_array_start[i]) {
- uk_pr_debug("Call pre-init constructor (entry %d (%p):
%p())...\n",
- i, &__preinit_array_start[i],
- __preinit_array_start[i]);
- __preinit_array_start[i]();
- }
+ &__preinit_array_start[0], &__preinit_array_end);
+ uk_ctortab_foreach(ctorfn,
+ __preinit_array_start, __preinit_array_end) {
+ if (!*ctorfn)
+ continue;
Normally, ctorfn should never be NULL, by passing over NULLs we might hide
bugs that cause this section to be malformed.
I vaguely remember that you should do this with the GCC built
__preinit_array, __init_array. I don't remember exactly the reasons why
sometimes NULL pointers could be there... Since we had this check in the
code also before, I tend to keep it for those arrays.
+
+ uk_pr_debug("Call pre-init constructor: %p()...\n", *ctorfn);
+ (*ctorfn)();
}
uk_pr_info("Constructor table at %p - %p\n",
- __init_array_start, &__init_array_end);
- uk_ctor_foreach(__init_array_start, __init_array_end, i) {
- if (__init_array_start[i]) {
- uk_pr_debug("Call constructor (entry %d (%p):
%p())...\n",
- i, &__init_array_start[i],
- __init_array_start[i]);
- __init_array_start[i]();
- }
+ &__init_array_start[0], &__init_array_end);
+ uk_ctortab_foreach(ctorfn, __init_array_start, __init_array_end) {
+ if (!*ctorfn)
+ continue;
Same comment as above.
+
+ uk_pr_debug("Call constructor: %p()...\n", *ctorfn);
+ (*ctorfn)();
}
uk_pr_info("Calling main(%d, [", tma->argc);
@@ -170,7 +170,6 @@ void ukplat_entry_argp(char *arg0, char *argb, __sz
argb_len)
void ukplat_entry(int argc, char *argv[])
{
struct thread_main_arg tma;
- int i;
int kern_args = 0;
int rc __maybe_unused = 0;
#if CONFIG_LIBUKALLOC
@@ -183,11 +182,16 @@ void ukplat_entry(int argc, char *argv[])
struct uk_sched *s = NULL;
struct uk_thread *main_thread = NULL;
#endif
+ uk_ctor_func_t *ctorfn;
+
+ uk_pr_info("Unikraft constructor table at %p - %p\n",
+ &uk_ctortab_start[0], &uk_ctortab_end);
+ uk_ctortab_foreach(ctorfn, uk_ctortab_start, uk_ctortab_end) {
+ if (!*ctorfn)
+ continue;
Same comment as above.
I can replace this check with an assertion. The Unikraft constructor
table is provided and built anyways by us. We know the format and NULL
pointers shouldn't be there in fact.
- uk_pr_info("Unikraft constructors table at %p\n", uk_ctortab);
- uk_ctor_foreach(uk_ctortab, uk_ctortab_end, i) {
- uk_pr_debug("Call constructor %p\n", uk_ctortab[i]);
- uk_ctortab[i]();
+ uk_pr_debug("Call constructor: %p())...\n", *ctorfn);
+ (*ctorfn)();
}
#ifdef CONFIG_LIBUKLIBPARAM
diff --git a/lib/uklibparam/include/uk/libparam.h
b/lib/uklibparam/include/uk/libparam.h
index 2a271ed3..f8f8ba4b 100644
--- a/lib/uklibparam/include/uk/libparam.h
+++ b/lib/uklibparam/include/uk/libparam.h
@@ -300,7 +300,7 @@ void _uk_libparam_lib_add(struct uk_lib_section *lib_sec);
#define UK_LIB_CTOR_PRIO 1
#define UK_LIB_CONSTRUCTOR_SETUP(prio, name) \
- __UK_CTOR_FUNC(prio, name)
+ UK_CTOR_PRIO(name, prio)
/**
* Create a constructor to initialize the parameters in the library.
diff --git a/plat/common/include/uk/plat/common/common.lds.h
b/plat/common/include/uk/plat/common/common.lds.h
index 593e6699..d31aa1dd 100644
--- a/plat/common/include/uk/plat/common/common.lds.h
+++ b/plat/common/include/uk/plat/common/common.lds.h
@@ -87,7 +87,7 @@
#define CTORTAB_SECTION \
. = ALIGN(__PAGE_SIZE); \
- uk_ctortab = .; \
+ uk_ctortab_start = .; \
.uk_ctortab : \
{ \
KEEP(*(SORT_BY_NAME(.uk_ctortab[0-9]))) \
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
