Re: [Minios-devel] [UNIKRAFT PATCH 3/9] include: Clean-ups to <uk/init.h>

[Vlad-Andrei BĂDOIU (78692) <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>]

Hey Simon,

Please see my comments inline.

Thank you,

Vlad

On 29.01.2020 23:48, Simon Kuenzer wrote:
> This commit brings the Unikraft init table definitions to the same
> style as the updated one of the Unikraft Constructor table.
>
> The new macro uk_initcall_class_prio() enables specifying the
> target initialization class with its corresponding number 1 to 6.
>
> The changes to this header should not break existing code.
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> ---
>   include/uk/init.h                             | 71 ++++++++++++++-----
>   lib/ukboot/boot.c                             | 16 +++--
>   .../include/uk/plat/common/common.lds.h       |  2 +-
>   3 files changed, 63 insertions(+), 26 deletions(-)
>
> diff --git a/include/uk/init.h b/include/uk/init.h
> index 8b7a8aa2..149b4926 100644
> --- a/include/uk/init.h
> +++ b/include/uk/init.h
> @@ -41,46 +41,65 @@
>   extern "C" {
>   #endif
>   
> -typedef int (*uk_init_t)(void);
> +typedef int (*uk_init_func_t)(void);
>   
> -#define INITTAB_STR_VAR(libname, fn, base, prio) libname ## fn ## base ## 
> prio
> -#define INITTAB_SECTION(base, prio)  .uk_inittab_ ## base ## prio
> -#define INITTAB_SECTION_NAME(name) STRINGIFY(name)
> +/**
> + * Register a Unikraft init function that is
> + * called during bootstrap (uk_inittab)
> + *
> + * @param fn
> + *   Initialization function to be called
> + * @param class
> + *   Initialization class (1 (earliest) to 6 (latest))
> + * @param prio
> + *   Priority level (0 (earliest) to 9 (latest)), must be a constant.
> + *   Note: Any other value for level will be ignored
> + */
> +#define __UK_INITTAB(libname, fn, base, prio)                                
> \
> +     static const uk_init_func_t                                     \
> +     __used __section(".uk_inittab" #base #prio)                     \
> +     __uk_inittab ## base ## prio ## _ ## libname ## _ ## fn = (fn)

As pointed out in the previous patch, concatenating the libname seems

to cause problems. Until this patch there was a bug hid this problem,

we were using LIBNAME in place of __LIBNAME__. Please see my error

when compiling pthreads-embedded below:

error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘-’ token

> +
> +#define _UK_INITTAB(libname, fn, base, prio)                         \
> +     __UK_INITTAB(libname, fn, base, prio)
>   
> -#define __inittab(libname, fn, base, prio)                           \
> -     static  const __used __section(INITTAB_SECTION_NAME(            \
> -                                     INITTAB_SECTION(base, prio))    \
> -                                   )                                 \
> -             uk_init_t INITTAB_STR_VAR(libname, fn, base, prio) = fn
> +#define uk_initcall_class_prio(fn, class, prio)                              
> \
> +     _UK_INITTAB(__LIBNAME__, fn, class, prio)
>   
>   /**
>    * Define a library initialization. At this point in time some platform
>    * component may not be initialized, so it wise to initializes those 
> component
>    * to initialized.
>    */
> -#define uk_early_initcall_prio(fn, prio)  __inittab(LIBNAME, fn, 1, prio)
> +#define uk_early_initcall_prio(fn, prio) \
> +     uk_initcall_class_prio(fn, 1, prio)
>   /**
>    * Define a stage for platform initialization. Platform at this point read
>    * all the device and device are initialized.
>    */
> -#define uk_plat_initcall_prio(fn, prio)  __inittab(LIBNAME, fn, 2, prio)
> +#define uk_plat_initcall_prio(fn, prio) \
> +     uk_initcall_class_prio(fn, 2, prio)
>   /**
>    * Define a stage for performing library initialization. This library
>    * initialization is performed after the platform is completely initialized.
>    */
> -#define uk_lib_initcall_prio(fn, prio)       __inittab(LIBNAME, fn, 3, prio)
> +#define uk_lib_initcall_prio(fn, prio) \
> +     uk_initcall_class_prio(fn, 3, prio)
>   /**
>    * Define a stage for filesystem initialization.
>    */
> -#define uk_rootfs_initcall_prio(fn, prio) __inittab(LIBNAME, fn, 4, prio)
> +#define uk_rootfs_initcall_prio(fn, prio) \
> +     uk_initcall_class_prio(fn, 4, prio)
>   /**
>    * Define a stage for device initialization
>    */
> -#define uk_sys_initcall_prio(fn, prio) __inittab(LIBNAME, fn, 5, prio)
> +#define uk_sys_initcall_prio(fn, prio) \
> +     uk_initcall_class_prio(fn, 5, prio)
>   /**
>    * Define a stage for application pre-initialization
>    */
> -#define uk_late_initcall_prio(fn, prio)  __inittab(LIBNAME, fn, 6, prio)
> +#define uk_late_initcall_prio(fn, prio) \
> +     uk_initcall_class_prio(fn, 6, prio)
>   
>   /**
>    * Similar interface without priority.
> @@ -92,11 +111,25 @@ typedef int (*uk_init_t)(void);
>   #define uk_sys_initcall(fn)       uk_sys_initcall_prio(fn, 9)
>   #define uk_late_initcall(fn)      uk_late_initcall_prio(fn, 9)
>   
> -extern const uk_init_t uk_inittab_start[];
> -extern const uk_init_t uk_inittab_end;
> +extern const uk_init_func_t uk_inittab_start[];
> +extern const uk_init_func_t uk_inittab_end;
>   
> -#define uk_inittab_foreach(init_start, init_end, itr)                \
> -     for (itr = DECONST(uk_init_t*, init_start); itr < &init_end; itr++)
> +/**
> + * Helper macro for iterating over init pointer tables
> + * Please note that the table may contain NULL pointer entries
> + *
> + * @param itr
> + *   Iterator variable (uk_init_func_t *) which points to the individual
> + *   table entries during iteration
> + * @param inittab_start
> + *   Start address of table (type: const uk_init_func_t[])
> + * @param inittab_end
> + *   End address of table (type: const uk_init_func_t)
> + */
> +#define uk_inittab_foreach(itr, inittab_start, inittab_end)  \
> +     for ((itr) = DECONST(uk_init_func_t*, inittab_start);   \
> +          (itr) < &(inittab_end);                            \
> +          (itr)++)
>   
>   #ifdef __cplusplus
>   }
> diff --git a/lib/ukboot/boot.c b/lib/ukboot/boot.c
> index aa6999cc..c6dac1a8 100644
> --- a/lib/ukboot/boot.c
> +++ b/lib/ukboot/boot.c
> @@ -76,19 +76,23 @@ static void main_thread_func(void *arg)
>       int i;
>       int ret;
>       struct thread_main_arg *tma = arg;
> -     uk_init_t *itr;
>       uk_ctor_func_t *ctorfn;
> +     uk_init_func_t *initfn;
>   
>       /**
>        * Run init table
>        */
> -     uk_pr_info("Init Table @ %p - %p\n", &uk_inittab_start[0],
> -                &uk_inittab_end);
> -     uk_inittab_foreach(uk_inittab_start, uk_inittab_end, itr) {
> -             ret = (*itr)();
> +     uk_pr_info("Init Table @ %p - %p\n",
> +                &uk_inittab_start[0], &uk_inittab_end);
> +     uk_inittab_foreach(initfn, uk_inittab_start, uk_inittab_end) {
> +             if (!*initfn)
> +                     continue;
This might hide possible bugs.
> +
> +             uk_pr_debug("Call init function: %p()...\n", *initfn);
> +             ret = (*initfn)();
>               if (ret < 0) {
>                       uk_pr_err("Init function at %p returned error %d\n",
> -                               itr, ret);
> +                               *initfn, ret);
>                       ret = UKPLAT_CRASH;
>                       goto exit;
>               }
> diff --git a/plat/common/include/uk/plat/common/common.lds.h 
> b/plat/common/include/uk/plat/common/common.lds.h
> index d31aa1dd..676c20b3 100644
> --- a/plat/common/include/uk/plat/common/common.lds.h
> +++ b/plat/common/include/uk/plat/common/common.lds.h
> @@ -98,7 +98,7 @@
>       uk_inittab_start = .;                                           \
>       .uk_inittab :                                                   \
>       {                                                               \
> -             KEEP(*(SORT_BY_NAME(.uk_inittab_[1-6][0-9])))           \
> +             KEEP(*(SORT_BY_NAME(.uk_inittab[1-6][0-9])))            \
>       }                                                               \
>       uk_inittab_end = .;
>   
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel