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 <dpsm...@apertussolutions.com>
> Reviewed-by: Christopher Clark <christopher.cl...@starlab.io>
> ---
> 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
