|
[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 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.
>
>> +
>> +#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.
>
>> +
>> + 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.
>>
>> - 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |