Re: [Minios-devel] [UNIKRAFT PATCH 5/5] lib/vfscore: Add support to mount initramfs to root

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

I think, it would be less confusing for the user experience, when 
"initrd" is also in the list of the auto-mount options. I would even 
change it in a way so that users are able to change the root filesystem 
with the kernel command line options (in case uklibparam is enabled): 
`vfs.rootfs=initrd`. It should be possible that a different default 
rootfs option can be compiled in (e.g., 9pfs) while through the kernel 
command line your initrd implementation can be selected.
On 28.01.20 05:02, Robert Hrusecky wrote:
> Modify vfscore boot operation to run cpio extraction algorithm on initrd
> memory region and mount the resulting filesystem at root.
>
> Signed-off-by: Robert Hrusecky <roberth@xxxxxxxxxxxxx>
> Signed-off-by: Omar Jamil <omarj2898@xxxxxxxxx>
> Signed-off-by: Sachin Beldona <sachinbeldona@xxxxxxxxxx>
> ---
>   lib/vfscore/Config.uk | 12 ++++++++++++
>   lib/vfscore/rootfs.c  | 32 ++++++++++++++++++++++++++------
>   2 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/lib/vfscore/Config.uk b/lib/vfscore/Config.uk
> index f64810f..e3b305e 100644
> --- a/lib/vfscore/Config.uk
> +++ b/lib/vfscore/Config.uk
> @@ -9,6 +9,18 @@ config LIBVFSCORE
>   if LIBVFSCORE
>   menu "vfscore: Configuration"
>   
> +config LIBCPIO
> +    bool "cpio: general cpio archive extraction"
> +    default n
Don't let the library appear here, this is likely to be confusing. Put 
this to /lib/ukcpio/Config.uk (see my comments of patch 3/5).
> +
> +config LIBINITRAMFS
> +    bool "initramfs: extract the given cpio file to /"
> +    default n
> +    select LIBRAMFS
> +    select LIBUKLIBPARAM
> +    select LIBVFSCORE_AUTOMOUNT_ROOTFS
> +    select LIBCPIO
> +
I think it is better if you add `initrd` as a filesystem type to the 
"choice LIBVFSCORE_ROOTFS" list. Add the following new entry there:
        config LIBVFSCORE_ROOTFS_INITRD
        bool "InitRD"
        select LIBRAMFS
        select LIBUKCPIO

As soon as it got selected, it will enable RamFS and your ukcpio library 
due to the `select` lines.
You should then add this line:

        default "initrd" if LIBVFSCORE_ROOTFS_INITRD

to the hidden configuration `config LIBVFSCORE_ROOTFS` which sets the 
default for the compiled-in variable `static const char *rootfs`.
The fields LIBVFSCORE_ROOTDEV, LIBVFSCORE_ROOTFLAGS, and 
LIBVFSCORE_ROOTOPTS should not be visible when initrd was selected. This 
can be achieved by extending the `depends on` lines of each of the three 
items from
        depends on !LIBVFSCORE_ROOTFS_RAMFS

to

        depends on !LIBVFSCORE_ROOTFS_RAMFS && !LIBVFSCORE_ROOTFS_INITRD


>   config LIBVFSCORE_PIPE_SIZE_ORDER
>         int "Pipe size order"
>         default 16
> diff --git a/lib/vfscore/rootfs.c b/lib/vfscore/rootfs.c
> index 4b9512a..78dbe3a 100644
> --- a/lib/vfscore/rootfs.c
> +++ b/lib/vfscore/rootfs.c
> @@ -42,6 +42,11 @@
>   #include <sys/stat.h>
>   #include <sys/mount.h>
>   #include <uk/init.h>
> +#ifdef CONFIG_LIBINITRAMFS
> +#include <uk/plat/memory.h>
> +#include <uk/cpio.h>
> +#include <string.h>
> +#endif
>   
>   static const char *rootfs   = CONFIG_LIBVFSCORE_ROOTFS;
>   
> @@ -80,17 +85,32 @@ static int vfscore_rootfs(void)
>                 return -1;
>         }
>   
> +#ifdef CONFIG_LIBINITRAMFS
> +       struct ukplat_memregion_desc memregion_desc;
> +       int initrd;
> +       enum cpio_error error;
> +
> +       initrd = ukplat_memregion_find_initrd0(&memregion_desc);
> +       if (initrd != -1) {
> +               ukplat_memregion_get(initrd, &memregion_desc);
> +               if (mount("", "/", "ramfs", 0, NULL) < 0)
> +                       return -CPIO_MOUNT_FAILED;
> +
> +               error =
> +                   cpio_extract("/", memregion_desc.base, memregion_desc.len);
> +               if (error < 0)
> +                       uk_pr_err("Failed to mount initrd\n");
> +               return error;
> +       }
> +       uk_pr_err("Failed to mount initrd\n");
> +       return -CPIO_NO_MEMREGION;
> +#else
Instead of your #if-#else-#endif block you would introduce the special 
case of 'initrd' filesystem in a way that it only get activated when 
`rootfs` was set to "initrd". You also let compile-in this code as soon 
as RamFS and ukcpio is available. This enables your special mount 
feature when a user enables the necessary libraries; independent of what 
was configured as default with the auto mount option.
#if CONFIG_LIBUKCPIO && CONFIG_LIBRAMFS
        if (strncmp(rootfs, "initrd", 5) == 0) {
                struct ukplat_memregion_desc initrd;
                enum cpio_error error;

                if (ukplat_memregion_find_initrd0(&initrd) < 0){
                        uk_pr_crit("Could not find an initrd!\n");
                        return -1;
                }

                if (mount("", "/", "ramfs", 0, NULL) != 0) {
                        uk_pr_crit("Failed to mount ramfs to /: %d\n",
                                   errno);
                        return -1;
                }

                error = cpio_extract("/", initrd.base, initrd.len);
                if (error < 0) {
                        uk_pr_crit("Failed to extract cpio archive to /: %d\n",
                                   error);
                        return -1;
                }

                /* TODO: Hand-over memregion to default allocator */

                return 0;
        }
#endif

As soon as initrd is set with rootfs, this code (please check it) exits 
the function always. An else case is not needed, which avoids to many 
levels of if-case nesting. In the case initrd was not set, the previous 
mount code is run.
>         uk_pr_info("Mount %s to /...\n", rootfs); >        if (mount(rootdev, 
> "/", rootfs, rootflags, rootopts) != 0) {
>                 uk_pr_crit("Failed to mount /: %d\n", errno);
>                 return -1;
>         }
> -
> -       /*
> -        * TODO: Alternatively we could extract an archive found
> -        * as initrd to a ramfs '/' if we have got fsname 'initrd'
> -        */
> -
> +#endif
>         return 0;
>   }
>   
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel