On 06.07.2022 23:04, Daniel P. Smith wrote:
> This commit is the first step in adopting domain builder. It goes through the
> dom0 creation and construction functions, converting them over to consume
> struct boot_domaain and changes the startup sequence to use the domain builder
> to create and construct dom0.
>
> Signed-off-by: Daniel P. Smith <dpsm...@apertussolutions.com>
> Reviewed-by: Christopher Clark <christopher.cl...@starlab.io>
> ---
> xen/arch/x86/dom0_build.c | 30 +++----
> xen/arch/x86/hvm/dom0_build.c | 10 +--
> xen/arch/x86/include/asm/dom0_build.h | 8 +-
> xen/arch/x86/include/asm/setup.h | 5 +-
> xen/arch/x86/pv/dom0_build.c | 39 ++++-----
> xen/arch/x86/setup.c | 114 +++++++++++++++-----------
> 6 files changed, 109 insertions(+), 97 deletions(-)
>
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index e44f7f3c43..216c9e3590 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -6,6 +6,7 @@
>
> #include <xen/bootdomain.h>
> #include <xen/bootinfo.h>
> +#include <xen/domain_builder.h>
> #include <xen/init.h>
> #include <xen/iocap.h>
> #include <xen/libelf.h>
> @@ -556,31 +557,32 @@ int __init dom0_setup_permissions(struct domain *d)
> return rc;
> }
>
> -int __init construct_dom0(
> - struct domain *d, const struct boot_module *image,
> - struct boot_module *initrd, char *cmdline)
> +int __init construct_domain(struct boot_domain *bd)
> {
> - int rc;
> + int rc = 0;
>
> /* Sanity! */
> - BUG_ON(!pv_shim && d->domain_id != 0);
> - BUG_ON(d->vcpu[0] == NULL);
> - BUG_ON(d->vcpu[0]->is_initialised);
> + BUG_ON(!pv_shim && bd->domid != 0);
> + BUG_ON(bd->domain->vcpu[0] == NULL);
> + BUG_ON(bd->domain->vcpu[0]->is_initialised);
>
> process_pending_softirqs();
>
> - if ( is_hvm_domain(d) )
> - rc = dom0_construct_pvh(d, image, initrd, cmdline);
> - else if ( is_pv_domain(d) )
> - rc = dom0_construct_pv(d, image, initrd, cmdline);
> - else
> - panic("Cannot construct Dom0. No guest interface available\n");
> + if ( builder_is_initdom(bd) )
> + {
> + if ( is_hvm_domain(bd->domain) )
> + rc = dom0_construct_pvh(bd);
> + else if ( is_pv_domain(bd->domain) )
> + rc = dom0_construct_pv(bd);
> + else
> + panic("Cannot construct Dom0. No guest interface available\n");
> + }
Isn't there an "else" missing here, even if just to ASSERT_UNREACHABLE()
and set rc to, say, -EOPNOTSUPP?
> @@ -311,11 +310,12 @@ int __init dom0_construct_pv(
> unsigned long count;
> struct page_info *page = NULL;
> start_info_t *si;
> + struct domain *d = bd->domain;
> struct vcpu *v = d->vcpu[0];
> - void *image_base = bootstrap_map(image);
> - unsigned long image_len = image->size;
> - void *image_start = image_base + image->arch->headroom;
> - unsigned long initrd_len = initrd ? initrd->size : 0;
> + void *image_base = bootstrap_map(bd->kernel);
> + unsigned long image_len = bd->kernel->size;
> + void *image_start = image_base + bd->kernel->arch->headroom;
> + unsigned long initrd_len = bd->ramdisk ? bd->ramdisk->size : 0;
> l4_pgentry_t *l4tab = NULL, *l4start = NULL;
> l3_pgentry_t *l3tab = NULL, *l3start = NULL;
> l2_pgentry_t *l2tab = NULL, *l2start = NULL;
> @@ -355,7 +355,7 @@ int __init dom0_construct_pv(
> d->max_pages = ~0U;
>
> if ( (rc =
> - bzimage_parse(image_base, &image_start, image->arch->headroom,
> + bzimage_parse(image_base, &image_start, bd->kernel->arch->headroom,
> &image_len)) != 0 )
> return rc;
>
> @@ -545,7 +545,7 @@ int __init dom0_construct_pv(
> initrd_pfn = vinitrd_start ?
> (vinitrd_start - v_start) >> PAGE_SHIFT :
> domain_tot_pages(d);
> - initrd_mfn = mfn = mfn_x(initrd->mfn);
> + initrd_mfn = mfn = mfn_x(bd->ramdisk->mfn);
> count = PFN_UP(initrd_len);
> if ( d->arch.physaddr_bitsize &&
> ((mfn + count - 1) >> (d->arch.physaddr_bitsize - PAGE_SHIFT)) )
> @@ -560,13 +560,13 @@ int __init dom0_construct_pv(
> free_domheap_pages(page, order);
> page += 1UL << order;
> }
> - memcpy(page_to_virt(page), maddr_to_virt(initrd->start),
> + memcpy(page_to_virt(page), maddr_to_virt(bd->ramdisk->start),
> initrd_len);
> - mpt_alloc = initrd->start;
> + mpt_alloc = bd->ramdisk->start;
> init_domheap_pages(mpt_alloc,
> mpt_alloc + PAGE_ALIGN(initrd_len));
> - bootmodule_update_mfn(initrd, page_to_mfn(page));
> - initrd_mfn = mfn_x(initrd->mfn);
> + bootmodule_update_mfn(bd->ramdisk, page_to_mfn(page));
> + initrd_mfn = mfn_x(bd->ramdisk->mfn);
> }
> else
> {
> @@ -574,7 +574,7 @@ int __init dom0_construct_pv(
> if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
> BUG();
> }
> - initrd->size = 0;
> + bd->ramdisk->size = 0;
>From an abstract pov: Is it legitimate to alter values under bd-> ? I
would have assumed bd and everything under it is r/o at this point
(and could/should be const-qualified).
> @@ -272,6 +271,24 @@ static int __init cf_check parse_acpi_param(const char
> *s)
> }
> custom_param("acpi", parse_acpi_param);
>
> +void __init arch_builder_apply_cmdline(
> + struct boot_info *info, struct boot_domain *bd)
> +{
> + if ( skip_ioapic_setup && !strstr(bd->kernel->string.bytes, "noapic") )
> + strlcat(bd->kernel->string.bytes, " noapic", MAX_GUEST_CMDLINE);
> + if ( (strlen(acpi_param) == 0) && acpi_disabled )
> + {
> + printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> + strlcpy(acpi_param, "off", sizeof(acpi_param));
> + }
> + if ( (strlen(acpi_param) != 0) &&
> + !strstr(bd->kernel->string.bytes, "acpi=") )
> + {
> + strlcat(bd->kernel->string.bytes, " acpi=", MAX_GUEST_CMDLINE);
> + strlcat(bd->kernel->string.bytes, acpi_param, MAX_GUEST_CMDLINE);
> + }
> +}
This duplicates existing code rather than replacing it. How do
you envision the two to remain in sync? Such things should live in
exactly one place imo.
> @@ -816,7 +831,7 @@ static struct domain *__init create_dom0(const struct
> boot_info *bi)
> write_cr4(read_cr4() & ~X86_CR4_SMAP);
> }
>
> - if ( construct_dom0(d, image, initrd, cmdline) != 0 )
> + if ( construct_domain(bd) != 0 )
> panic("Could not construct domain 0\n");
You leave the log message text in place here, but ...
> @@ -1905,22 +1912,29 @@ void __init noreturn __start_xen(unsigned long bi_p)
> cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
> cpu_has_nx ? "" : "not ");
>
> - initrdidx = bootmodule_next_idx_by_kind(boot_info, BOOTMOD_UNKNOWN, 0);
> - if ( initrdidx < boot_info->nr_mods )
> - boot_info->mods[initrdidx].kind = BOOTMOD_RAMDISK;
> -
> - if ( bootmodule_count_by_kind(boot_info, BOOTMOD_UNKNOWN) > 1 )
> - printk(XENLOG_WARNING
> - "Multiple initrd candidates, picking module #%u\n",
> - initrdidx);
> -
> /*
> - * We're going to setup domain0 using the module(s) that we stashed
> safely
> - * above our heap. The second module, if present, is an initrd ramdisk.
> + * Boot description not provided, check to see if there are any remaining
> + * boot modules, the first one found will be provided as the ramdisk.
> */
> - dom0 = create_dom0(boot_info);
> + if ( ! boot_info->builder->fdt_enabled )
> + {
> + initrdidx = bootmodule_next_idx_by_kind(boot_info, BOOTMOD_UNKNOWN,
> 0);
> + if ( initrdidx < boot_info->nr_mods )
> + {
> + boot_info->builder->domains[0].ramdisk =
> &boot_info->mods[initrdidx];
> + boot_info->mods[initrdidx].kind = BOOTMOD_RAMDISK;
> + }
> + if ( bootmodule_count_by_kind(boot_info, BOOTMOD_UNKNOWN) > 1 )
> + printk(XENLOG_WARNING
> + "Multiple initrd candidates, picking module #%u\n",
> + initrdidx);
> + }
> +
> + builder_create_domains(boot_info);
> +
> + dom0 = builder_get_hwdom(boot_info);
> if ( !dom0 )
> - panic("Could not set up DOM0 guest OS\n");
> + panic("No hardware domain was built\n");
... you change it here, neglecting that in the late-hwdom case what is
being built here is only Dom0, not hwdom. This may also affect the name
of the function that you call.
Jan
