Re: [PATCH v1 10/18] x86: introduce the domain builder

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2022 23:04, Daniel P. Smith wrote:
> This commit introduces the domain builder configuration FDT parser along with
> the domain builder core for domain creation. To enable domain builder to be a
> cross architecture internal API, a new arch domain creation call is introduced
> for use by the domain builder.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Christopher Clark <christopher.clark@xxxxxxxxxx>
> ---
>  xen/arch/x86/setup.c               |   9 +
>  xen/common/Makefile                |   1 +
>  xen/common/domain-builder/Makefile |   2 +
>  xen/common/domain-builder/core.c   |  96 ++++++++++
>  xen/common/domain-builder/fdt.c    | 295 +++++++++++++++++++++++++++++
>  xen/common/domain-builder/fdt.h    |   7 +
>  xen/include/xen/bootinfo.h         |  16 ++
>  xen/include/xen/domain_builder.h   |   1 +
>  8 files changed, 427 insertions(+)

With this diffstat - why the x86: prefix in the subject?

Also note the naming inconsistency: domain-builder/ (preferred) vs
domain_builder.h (adjustment would require touching earlier patches).

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1,4 +1,6 @@
> +#include <xen/bootdomain.h>
>  #include <xen/bootinfo.h>
> +#include <xen/domain_builder.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/err.h>
> @@ -826,6 +828,13 @@ static struct domain *__init create_dom0(const struct 
> boot_info *bi)
>      return d;
>  }
>  
> +void __init arch_create_dom(
> +    const struct boot_info *bi, struct boot_domain *bd)
> +{
> +    if ( builder_is_initdom(bd) )
> +        create_dom0(bi);
> +}

You're not removing any code in exchange - is Dom0 now being built twice?
Or is the function above effectively dead code?

> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -72,6 +72,7 @@ extra-y := symbols-dummy.o
>  obj-$(CONFIG_COVERAGE) += coverage/
>  obj-y += sched/
>  obj-$(CONFIG_UBSAN) += ubsan/
> +obj-y += domain-builder/

At least as long as all of this is still experimental I would really like
to see a way to disable all of it via Kconfig.

> --- /dev/null
> +++ b/xen/common/domain-builder/core.c
> @@ -0,0 +1,96 @@
> +#include <xen/bootdomain.h>
> +#include <xen/bootinfo.h>
> +#include <xen/domain_builder.h>
> +#include <xen/init.h>
> +#include <xen/types.h>
> +
> +#include <asm/bzimage.h>
> +#include <asm/setup.h>
> +
> +#include "fdt.h"
> +
> +static struct domain_builder __initdata builder;
> +
> +void __init builder_init(struct boot_info *info)
> +{
> +    struct boot_domain *d = NULL;
> +
> +    info->builder = &builder;
> +
> +    if ( IS_ENABLED(CONFIG_BUILDER_FDT) )
> +    {
> +        /* fdt is required to be module 0 */
> +        switch ( check_fdt(info, __va(info->mods[0].start)) )

Besides requiring fixed order looking inflexible to me, what guarantees
there is at least one module? (Perhaps there is, but once again -
without seeing where this function is being called from, how am I to
judge?)

> +        {
> +        case 0:
> +            printk("Domain Builder: initialized from config\n");
> +            info->builder->fdt_enabled = true;
> +            return;
> +        case -EINVAL:
> +            info->builder->fdt_enabled = false;
> +            break;

Aiui this is the case where no FDT is present. I'd strongly suggest
to use a less common / ambiguous error code to cover that case. Maybe
-ENODEV or -EOPNOTSUPP or ...

> +        case -ENODATA:

... -ENODATA, albeit you having that here suggests this has some
other specific meaning already.

> +        default:
> +            panic("%s: error occured processing DTB\n", __func__);
> +        }
> +    }
> +
> +    /*
> +     * No FDT config support or an FDT wasn't present, do an initial
> +     * domain construction
> +     */
> +    printk("Domain Builder: falling back to initial domain build\n");
> +    info->builder->nr_doms = 1;
> +    d = &info->builder->domains[0];
> +
> +    d->mode = opt_dom0_pvh ? 0 : BUILD_MODE_PARAVIRTUALIZED;
> +
> +    d->kernel = &info->mods[0];
> +    d->kernel->kind = BOOTMOD_KERNEL;
> +
> +    d->permissions = BUILD_PERMISSION_CONTROL | BUILD_PERMISSION_HARDWARE;
> +    d->functions = BUILD_FUNCTION_CONSOLE | BUILD_FUNCTION_XENSTORE |
> +                     BUILD_FUNCTION_INITIAL_DOM;

Nit: Indentation.

> +    d->kernel->arch->headroom = bzimage_headroom(bootstrap_map(d->kernel),
> +                                                   d->kernel->size);

bzimage isn't an arch-agnostic concept afaict, so I don't see this
function legitimately being called from here.

And nit again: Indentation. (And at least one more further down.)

> +    bootstrap_map(NULL);
> +
> +    if ( d->kernel->string.len )
> +        d->kernel->string.kind = BOOTSTR_CMDLINE;
> +}
> +
> +uint32_t __init builder_create_domains(struct boot_info *info)
> +{
> +    uint32_t build_count = 0, functions_built = 0;
> +    int i;
> +
> +    for ( i = 0; i < info->builder->nr_doms; i++ )
> +    {
> +        struct boot_domain *d = &info->builder->domains[i];

Can variables of this type please not be named "d", but e.g. "bd"?

> +        if ( ! IS_ENABLED(CONFIG_MULTIDOM_BUILDER) &&
> +             ! builder_is_initdom(d) &&

Nit: Stray blanks after ! .

> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.c
> @@ -0,0 +1,295 @@
> +#include <xen/bootdomain.h>
> +#include <xen/bootinfo.h>
> +#include <xen/domain_builder.h>
> +#include <xen/fdt.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/page-size.h>
> +#include <xen/pfn.h>
> +#include <xen/types.h>
> +
> +#include <asm/bzimage.h>
> +#include <asm/setup.h>
> +
> +#include "fdt.h"
> +
> +#define BUILDER_FDT_TARGET_UNK 0
> +#define BUILDER_FDT_TARGET_X86 1
> +#define BUILDER_FDT_TARGET_ARM 2
> +static int __initdata target_arch = BUILDER_FDT_TARGET_UNK;
> +
> +static struct boot_module *read_module(
> +    const void *fdt, int node, uint32_t address_cells, uint32_t size_cells,
> +    struct boot_info *info)
> +{
> +    const struct fdt_property *prop;
> +    const __be32 *cell;
> +    struct boot_module *bm;
> +    bootmodule_kind kind = BOOTMOD_UNKNOWN;
> +    int len;
> +
> +    if ( device_tree_node_compatible(fdt, node, "module,kernel") )
> +        kind = BOOTMOD_KERNEL;
> +
> +    if ( device_tree_node_compatible(fdt, node, "module,ramdisk") )
> +        kind = BOOTMOD_RAMDISK;
> +
> +    if ( device_tree_node_compatible(fdt, node, "module,microcode") )
> +        kind = BOOTMOD_UCODE;
> +
> +    if ( device_tree_node_compatible(fdt, node, "module,xsm-policy") )
> +        kind = BOOTMOD_XSM;
> +
> +    if ( device_tree_node_compatible(fdt, node, "module,config") )
> +        kind = BOOTMOD_GUEST_CONF;
> +
> +    if ( device_tree_node_compatible(fdt, node, "module,index") )
> +    {
> +        uint32_t idx;
> +
> +        idx = (uint32_t)device_tree_get_u32(fdt, node, "module-index", 0);

Why the cast?

> +static int process_domain_node(

__init?

> +    const void *fdt, int node, const char *name, int depth,
> +    uint32_t address_cells, uint32_t size_cells, void *data)
> +{
> +    struct boot_info *info = (struct boot_info *)data;
> +    const struct fdt_property *prop;
> +    struct boot_domain *domain;
> +    int node_next, i, plen;
> +
> +    if ( !info )
> +        return -1;
> +
> +    if ( info->builder->nr_doms >= BUILD_MAX_BOOT_DOMAINS )
> +        return -1;
> +
> +    domain = &info->builder->domains[info->builder->nr_doms];
> +
> +    domain->domid = (domid_t)device_tree_get_u32(fdt, node, "domid", 0);
> +    domain->permissions = device_tree_get_u32(fdt, node, "permissions", 0);
> +    domain->functions = device_tree_get_u32(fdt, node, "functions", 0);
> +    domain->mode = device_tree_get_u32(fdt, node, "mode", 0);
> +
> +    prop = fdt_get_property(fdt, node, "domain-uuid", &plen);
> +    if ( prop )
> +        for ( i=0; i < sizeof(domain->uuid) % sizeof(uint32_t); i++ )
> +            *(domain->uuid + i) = fdt32_to_cpu((uint32_t)prop->data[i]);
> +
> +    domain->ncpus = device_tree_get_u32(fdt, node, "cpus", 1);
> +
> +    if ( target_arch == BUILDER_FDT_TARGET_X86 )
> +    {
> +        prop = fdt_get_property(fdt, node, "memory", &plen);
> +        if ( prop )
> +        {
> +            int sz = fdt32_to_cpu(prop->len);
> +            char s[64];
> +            unsigned long val;
> +
> +            if ( sz >= 64 )
> +                panic("node %s invalid `memory' property\n", name);
> +
> +            memcpy(s, prop->data, sz);
> +            s[sz] = '\0';
> +            val = parse_size_and_unit(s, NULL);
> +
> +            domain->meminfo.mem_size.nr_pages = PFN_UP(val);
> +            domain->meminfo.mem_max.nr_pages = PFN_UP(val);
> +        }
> +        else
> +            panic("node %s missing `memory' property\n", name);
> +    }
> +    else
> +            panic("%s: only x86 memory parsing supported\n", __func__);
> +
> +    prop = fdt_get_property(fdt, node, "security-id",
> +                                &plen);
> +    if ( prop )
> +    {
> +        int sz = fdt32_to_cpu(prop->len);
> +        sz = sz > BUILD_MAX_SECID_LEN ?  BUILD_MAX_SECID_LEN : sz;
> +        memcpy(domain->secid, prop->data, sz);
> +    }
> +
> +    for ( node_next = fdt_first_subnode(fdt, node);
> +          node_next > 0;
> +          node_next = fdt_next_subnode(fdt, node_next))
> +    {
> +        struct boot_module *bm = read_module(fdt, node_next, address_cells,
> +                                             size_cells, info);
> +
> +        switch ( bm->kind )
> +        {
> +        case BOOTMOD_KERNEL:
> +            /* kernel was already found */
> +            if ( domain->kernel != NULL )
> +                continue;
> +
> +            bm->arch->headroom = bzimage_headroom(bootstrap_map(bm), 
> bm->size);
> +            bootstrap_map(NULL);
> +
> +            if ( bm->string.len )
> +                bm->string.kind = BOOTSTR_CMDLINE;
> +            else
> +            {
> +                prop = fdt_get_property(fdt, node_next, "bootargs", &plen);
> +                if ( prop )
> +                {
> +                    int size = fdt32_to_cpu(prop->len);
> +                    size = size > BOOTMOD_MAX_STRING ?
> +                           BOOTMOD_MAX_STRING : size;
> +                    memcpy(bm->string.bytes, prop->data, size);
> +                    bm->string.kind = BOOTSTR_CMDLINE;
> +                }
> +            }
> +
> +            domain->kernel = bm;
> +
> +            break;
> +        case BOOTMOD_RAMDISK:
> +            /* ramdisk was already found */
> +            if ( domain->ramdisk != NULL )
> +                continue;
> +
> +            domain->ramdisk = bm;
> +
> +            break;
> +        case BOOTMOD_GUEST_CONF:
> +            /* guest config was already found */
> +            if ( domain->configs[BUILD_DOM_CONF_IDX] != NULL )
> +                continue;
> +
> +            domain->configs[BUILD_DOM_CONF_IDX] = bm;
> +
> +            break;
> +        default:
> +            continue;
> +        }

For larger switch() statements please have blank lines between non-fall-
through case blocks.

> +/* check_fdt
> + *   Attempts to initialize hyperlaunch config
> + *
> + * Returns:
> + *    -EINVAL: Not a valid DTB
> + *   -ENODATA: Valid DTB but not a valid hyperlaunch device tree
> + *          0: Valid hyperlaunch device tree
> + */
> +int __init check_fdt(struct boot_info *info, void *fdt)
> +{
> +    int hv_node, ret;
> +
> +    ret = fdt_check_header(fdt);
> +    if ( ret < 0 )
> +        return -EINVAL;
> +
> +    hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> +    if ( hv_node < 0 )
> +        return -ENODATA;
> +
> +    if ( !device_tree_node_compatible(fdt, hv_node, "hypervisor,xen") )
> +        return -EINVAL;
> +
> +    if ( IS_ENABLED(CONFIG_X86) &&
> +         device_tree_node_compatible(fdt, hv_node, "xen,x86") )
> +        target_arch = BUILDER_FDT_TARGET_X86;
> +    else if ( IS_ENABLED(CONFIG_ARM) &&
> +              device_tree_node_compatible(fdt, hv_node, "xen,arm") )
> +        target_arch = BUILDER_FDT_TARGET_ARM;
> +
> +    if ( target_arch != BUILDER_FDT_TARGET_X86 &&
> +         target_arch != BUILDER_FDT_TARGET_ARM )
> +        return -EINVAL;

So you'd happily accept BUILDER_FDT_TARGET_ARM on x86 or
BUILDER_FDT_TARGET_X86 on Arm? And there's no distinction between
Arm32 and Arm64?

> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef COMMON_BUILDER_FDT_H
> +#define COMMON_BUILDER_FDT_H
> +
> +int __init check_fdt(struct boot_info *info, void *fdt);
> +#endif

Nit: Please put another blank line before #endif.

Jan