[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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.