On 01.07.2023 09:18, Christopher Clark wrote:
> @@ -105,11 +102,14 @@ unsigned long __init bzimage_headroom(void *image_start,
> }
>
> int __init bzimage_parse(void *image_base, void **image_start,
> + unsigned int headroom,
> unsigned long *image_len)
> {
> struct setup_header *hdr = (struct setup_header *)(*image_start);
> int err = bzimage_check(hdr, *image_len);
> - unsigned long output_len;
> + unsigned long output_len, orig_image_len;
> +
> + orig_image_len = *image_len - headroom;
>
> if ( err < 0 )
> return err;
> @@ -125,7 +125,7 @@ int __init bzimage_parse(void *image_base, void
> **image_start,
>
> BUG_ON(!(image_base < *image_start));
>
> - output_len = output_length(*image_start, orig_image_len);
> + output_len = output_length(*image_start, *image_len);
If this is correct, then I would imply that so far we passed too large a
value (a too small one pretty certainly wouldn't have worked). But I
wonder whether you aren't passing too large a value now. In any event
ideally such a functional change would be split out; otherwise it very
clearly needs mentioning (justifying) in the description.
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -0,0 +1,18 @@
> +#ifndef __ARCH_X86_BOOTINFO_H__
> +#define __ARCH_X86_BOOTINFO_H__
> +
> +struct arch_bootmodule {
> + unsigned headroom;
> +};
But this isn't a per-module property, is it?
> @@ -961,7 +967,8 @@ static struct domain *__init create_dom0(const module_t
> *image,
> write_cr4(read_cr4() & ~X86_CR4_SMAP);
> }
>
> - if ( construct_dom0(d, image, headroom, initrd, cmdline) != 0 )
> + if ( construct_dom0(d, image, boot_info->mods[0].arch->headroom, initrd,
> + cmdline) != 0 )
> panic("Could not construct domain 0\n");
This looks to render the function's "headroom" parameter unused.
> --- a/xen/include/xen/bootinfo.h
> +++ b/xen/include/xen/bootinfo.h
> @@ -3,8 +3,19 @@
>
> #include <xen/types.h>
>
> +#ifdef CONFIG_X86
> +#include <asm/bootinfo.h>
> +#else
> + struct arch_bootmodule { };
> +#endif
This wants making use of include/asm-generic/ now, provided the non-x86
header are going to remain empty. Otherwise arch headers will want
introducing right away; there shouldn't be a CONFIG_X86 use here.
> +struct boot_module {
> + struct arch_bootmodule *arch;
> +};
Why a pointer? By the names it's a 1:1 relationship, so ...
> struct boot_info {
> unsigned int nr_mods;
> + struct boot_module *mods;
... only the pointer here is what takes care of there being multiple
instances (likely as before represented as an array).
Jan
