Re: [PATCH 02/10] x86 setup: per-arch bootmodule structure, headroom field

[Jan Beulich <jbeulich@xxxxxxxx>]

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