On 15/11/2024 1:11 pm, Daniel P. Smith wrote:
> With all the components used to construct dom0 encapsulated in struct
> boot_info
> and struct boot_module, it is no longer necessary to pass all them as
> parameters down the domain construction call chain. Change the parameter list
> to pass the struct boot_info instance and the struct domain reference.
>
> Signed-off-by: Daniel P. Smith <[email protected]>
There are two minor things needing noting in the commit message.
1) dom0_construct() turns i from being signed to unsigned. This is
necessary for it's new use, and compatible with all pre-existing uses.
2) dom0_construct() also splits some 3-way assignments to placate MISRA,
on lines which are modified.
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 3dd913bdb029..d1bdf1b14601 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -642,15 +643,15 @@ static bool __init check_and_adjust_load_address(
> return true;
> }
>
> -static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> - unsigned long image_headroom,
> - module_t *initrd, void *image_base,
> - const char *cmdline, paddr_t *entry,
> - paddr_t *start_info_addr)
> +static int __init pvh_load_kernel(
> + struct domain *d, struct boot_module *image, struct boot_module *initrd,
> + paddr_t *entry, paddr_t *start_info_addr)
> {
> - void *image_start = image_base + image_headroom;
> - unsigned long image_len = image->mod_end;
> - unsigned long initrd_len = initrd ? initrd->mod_end : 0;
> + void *image_base = bootstrap_map_bm(image);
> + void *image_start = image_base + image->headroom;
> + unsigned long image_len = image->mod->mod_end;
> + unsigned long initrd_len = initrd ? initrd->mod->mod_end : 0;
> + const char *cmdline = __va(image->cmdline_pa);
This isn't safe. __va(0) != NULL, so later between ...
> struct elf_binary elf;
> struct elf_dom_parms parms;
> paddr_t last_addr;
> @@ -725,8 +726,8 @@ static int __init pvh_load_kernel(struct domain *d, const
> module_t *image,
... these two hunks in the calculation for last_addr, we have:
... cmdline ? ROUNDUP(strlen(cmdline) + 1, ...
which does the wrong thing. (And includes the 16bit IVT onto the
guest's cmdline.)
I'd suggest doing the same as we do with initrd_len/etc, and having:
const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) :
NULL;
to maintain the prior semantics.
>
> if ( initrd != NULL )
> {
> - rc = hvm_copy_to_guest_phys(last_addr,
> mfn_to_virt(initrd->mod_start),
> - initrd_len, v);
> + rc = hvm_copy_to_guest_phys(
> + last_addr, mfn_to_virt(initrd->mod->mod_start), initrd_len, v);
This is a temporary adjustment, ending up shorter than it starts by
patch 3. I've tweaked it to reduce the churn overall. I can live with
83 chars width for a commit or two...
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index cc882bee61c3..6be3d7745fab 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -354,13 +355,10 @@ static struct page_info * __init alloc_chunk(struct
> domain *d,
> return page;
> }
>
> -static int __init dom0_construct(struct domain *d,
> - const module_t *image,
> - unsigned long image_headroom,
> - module_t *initrd,
> - const char *cmdline)
> +static int __init dom0_construct(struct boot_info *bi, struct domain *d)
> {
> - int i, rc, order, machine;
> + unsigned int i;
> + int rc, order, machine;
> bool compatible, compat;
> struct cpu_user_regs *regs;
> unsigned long pfn, mfn;
> @@ -374,10 +372,13 @@ static int __init dom0_construct(struct domain *d,
> unsigned int flush_flags = 0;
> start_info_t *si;
> struct vcpu *v = d->vcpu[0];
> - void *image_base = bootstrap_map(image);
> - unsigned long image_len = image->mod_end;
> - void *image_start = image_base + image_headroom;
> - unsigned long initrd_len = initrd ? initrd->mod_end : 0;
> + struct boot_module *image;
> + struct boot_module *initrd = NULL;
> + void *image_base;
> + unsigned long image_len;
> + void *image_start;
> + unsigned long initrd_len = 0;
> + const char *cmdline;
I'm tempted to put in some newlines here, just to break up the giant
block of variables.
This use of cmdline in principle needs a similar adjustment to the pvh
case, but it's only used once, so I suggest this instead:
@@ -984,8 +982,8 @@ static int __init dom0_construct(struct boot_info
*bi, struct domain *d)
}
memset(si->cmd_line, 0, sizeof(si->cmd_line));
- if ( cmdline != NULL )
- strlcpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line));
+ if ( image->cmdline_pa )
+ strlcpy((char *)si->cmd_line, __va(image->cmdline_pa),
sizeof(si->cmd_line));
#ifdef CONFIG_VIDEO
if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
[edit] Turns out you do this in patch 6 anyway, so this way around will
reduce churn.
Happy to fix on commit.
~Andrew
