Re: [PATCH v1 11/18] x86: initial conversion to domain builder

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Christopher Clark <christopher.clark@xxxxxxxxxx>
> ---
>  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