diff mbox series

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

Message ID 20230701071835.41599-3-christopher.w.clark@gmail.com (mailing list archive)
State New, archived
Headers show
Series v3: Boot modules for Hyperlaunch | expand

Commit Message

Christopher Clark July 1, 2023, 7:18 a.m. UTC 
Next step in incremental work towards adding a non-multiboot internal
representation of boot modules, converting the fields being accessed for
the startup calculations.

Add a new array of structs for per-boot-module state, though only
allocate space for a single array entry in this change since that is all
that is required for functionality modified in this patch: moving the
headroom field for the image decompression calculation into a new
per-arch boot module struct and then using it in x86 dom0 construction.

Introduces a per-arch header for x86 for arch-specific boot module
structures, and add a member to the common boot module structure for
access to it.

No functional change intended.

Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com>
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

---
Changes since v1: patch is a subset of v1 series patches 2 and 3.

 xen/arch/x86/bzimage.c              | 10 +++++-----
 xen/arch/x86/hvm/dom0_build.c       |  3 ++-
 xen/arch/x86/include/asm/bootinfo.h | 18 ++++++++++++++++++
 xen/arch/x86/include/asm/bzimage.h  |  2 +-
 xen/arch/x86/pv/dom0_build.c        |  3 ++-
 xen/arch/x86/setup.c                | 23 ++++++++++++++++-------
 xen/include/xen/bootinfo.h          | 11 +++++++++++
 7 files changed, 55 insertions(+), 15 deletions(-)
 create mode 100644 xen/arch/x86/include/asm/bootinfo.h

Comments

Stefano Stabellini July 8, 2023, 7:15 p.m. UTC | #1 
On Sat, 1 Jul 2023, Christopher Clark wrote:
> Next step in incremental work towards adding a non-multiboot internal
> representation of boot modules, converting the fields being accessed for
> the startup calculations.
> 
> Add a new array of structs for per-boot-module state, though only
> allocate space for a single array entry in this change since that is all
> that is required for functionality modified in this patch: moving the
> headroom field for the image decompression calculation into a new
> per-arch boot module struct and then using it in x86 dom0 construction.
> 
> Introduces a per-arch header for x86 for arch-specific boot module
> structures, and add a member to the common boot module structure for
> access to it.
> 
> No functional change intended.
> 
> Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
 
[...]


> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> new file mode 100644
> index 0000000000..a25054f372
> --- /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;
> +};
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

[...]

> diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
> index 6a7d55d20e..b72ae31a66 100644
> --- 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
> +
> +struct boot_module {
> +    struct arch_bootmodule *arch;
> +};
> +
>  struct boot_info {
>      unsigned int nr_mods;
> +    struct boot_module *mods;

Also here you already made the effort of using the same data structures
we use on ARM, you might as well use the same names too. Otherwise when
we try to use them on ARM it will require a rename somewhere.


>  };
>  
>  #endif
> -- 
> 2.25.1
> 
>
Christopher Clark July 20, 2023, 10:05 p.m. UTC | #2 
On Sat, Jul 8, 2023 at 12:15 PM Stefano Stabellini <sstabellini@kernel.org>
wrote:

> On Sat, 1 Jul 2023, Christopher Clark wrote:
> > Next step in incremental work towards adding a non-multiboot internal
> > representation of boot modules, converting the fields being accessed for
> > the startup calculations.
> >
> > Add a new array of structs for per-boot-module state, though only
> > allocate space for a single array entry in this change since that is all
> > that is required for functionality modified in this patch: moving the
> > headroom field for the image decompression calculation into a new
> > per-arch boot module struct and then using it in x86 dom0 construction.
> >
> > Introduces a per-arch header for x86 for arch-specific boot module
> > structures, and add a member to the common boot module structure for
> > access to it.
> >
> > No functional change intended.
> >
> > Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com>
> > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>
> [...]
>
>
> > diff --git a/xen/arch/x86/include/asm/bootinfo.h
> b/xen/arch/x86/include/asm/bootinfo.h
> > new file mode 100644
> > index 0000000000..a25054f372
> > --- /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;
> > +};
> > +
> > +#endif
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
>
> [...]
>
> > diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
> > index 6a7d55d20e..b72ae31a66 100644
> > --- 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
> > +
> > +struct boot_module {
> > +    struct arch_bootmodule *arch;
> > +};
> > +
> >  struct boot_info {
> >      unsigned int nr_mods;
> > +    struct boot_module *mods;
>
> Also here you already made the effort of using the same data structures
> we use on ARM, you might as well use the same names too. Otherwise when
> we try to use them on ARM it will require a rename somewhere.
>

Thanks for the review. We consciously made an effort to derive from the Arm
data structures to ensure that we're able to support the logic that Arm
needs. Arm's bootmodules were a
good start as a means for abstraction, and the design for hyperlaunch was
striving to abstract even further to incorporate x86-ism and hopefully
enough foresight for PPC and Risc-V.

Christopher


>
> >  };
> >
> >  #endif
> > --
> > 2.25.1
> >
> >
>
Jan Beulich Sept. 19, 2023, 1:39 p.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c
index ac4fd428be..dac2399b89 100644
--- a/xen/arch/x86/bzimage.c
+++ b/xen/arch/x86/bzimage.c
@@ -69,8 +69,6 @@  static __init int bzimage_check(struct setup_header *hdr, unsigned long len)
     return 1;
 }
 
-static unsigned long __initdata orig_image_len;
-
 unsigned long __init bzimage_headroom(void *image_start,
                                       unsigned long image_length)
 {
@@ -91,7 +89,6 @@  unsigned long __init bzimage_headroom(void *image_start,
     if ( elf_is_elfbinary(image_start, image_length) )
         return 0;
 
-    orig_image_len = image_length;
     headroom = output_length(image_start, image_length);
     if (gzip_check(image_start, image_length))
     {
@@ -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 ( (err = perform_gunzip(image_base, *image_start, orig_image_len)) > 0 )
         err = decompress(*image_start, orig_image_len, image_base);
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index fd2cbf68bc..bf08998862 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -545,7 +545,8 @@  static int __init pvh_load_kernel(struct domain *d, const module_t *image,
     struct vcpu *v = d->vcpu[0];
     int rc;
 
-    if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
+    if ( (rc = bzimage_parse(image_base, &image_start, image_headroom,
+                             &image_len)) != 0 )
     {
         printk("Error trying to detect bz compressed kernel\n");
         return rc;
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
new file mode 100644
index 0000000000..a25054f372
--- /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;
+};
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/include/asm/bzimage.h b/xen/arch/x86/include/asm/bzimage.h
index 7ed69d3910..dd784f32ef 100644
--- a/xen/arch/x86/include/asm/bzimage.h
+++ b/xen/arch/x86/include/asm/bzimage.h
@@ -5,7 +5,7 @@ 
 
 unsigned long bzimage_headroom(void *image_start, unsigned long image_length);
 
-int bzimage_parse(void *image_base, void **image_start,
+int bzimage_parse(void *image_base, void **image_start, unsigned int headroom,
                   unsigned long *image_len);
 
 #endif /* __X86_BZIMAGE_H__ */
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index c99135a552..6ed9f8bbed 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -416,7 +416,8 @@  int __init dom0_construct_pv(struct domain *d,
 
     d->max_pages = ~0U;
 
-    if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
+    if ( (rc = bzimage_parse(image_base, &image_start, image_headroom,
+                             &image_len)) != 0 )
         return rc;
 
     if ( (rc = elf_init(&elf, image_start, image_len)) != 0 )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 708639b236..c0e8fc6ab7 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -273,10 +273,16 @@  static struct boot_info __initdata *boot_info;
 
 static void __init multiboot_to_bootinfo(multiboot_info_t *mbi)
 {
-    static struct boot_info __initdata info;
+    static struct boot_info       __initdata info;
+    static struct boot_module     __initdata boot_mods[1];
+    static struct arch_bootmodule __initdata arch_boot_mods[1];
+
+    info.mods = boot_mods;
 
     info.nr_mods = mbi->mods_count;
 
+    boot_mods[0].arch = &arch_boot_mods[0];
+
     boot_info = &info;
 }
 
@@ -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");
 
     if ( cpu_has_smap )
@@ -985,7 +992,7 @@  void __init noreturn __start_xen(unsigned long mbi_p)
     unsigned int initrdidx, num_parked = 0;
     multiboot_info_t *mbi;
     module_t *mod;
-    unsigned long nr_pages, raw_max_page, modules_headroom, module_map[1];
+    unsigned long nr_pages, raw_max_page, module_map[1];
     int i, j, e820_warn = 0, bytes = 0;
     unsigned long eb_start, eb_end;
     bool acpi_boot_table_init_done = false, relocated = false;
@@ -1353,7 +1360,8 @@  void __init noreturn __start_xen(unsigned long mbi_p)
         mod[boot_info->nr_mods].mod_end = __2M_rwdata_end - _stext;
     }
 
-    modules_headroom = bzimage_headroom(bootstrap_map(mod), mod->mod_end);
+    boot_info->mods[0].arch->headroom = bzimage_headroom(bootstrap_map(mod),
+                                                         mod->mod_end);
     bootstrap_map(NULL);
 
 #ifndef highmem_start
@@ -1433,7 +1441,8 @@  void __init noreturn __start_xen(unsigned long mbi_p)
         /* Is the region suitable for relocating the multiboot modules? */
         for ( j = boot_info->nr_mods - 1; j >= 0; j-- )
         {
-            unsigned long headroom = j ? 0 : modules_headroom;
+            struct boot_module *boot_mods = boot_info->mods;
+            unsigned long headroom = j ? 0 : boot_mods[0].arch->headroom;
             unsigned long size = PAGE_ALIGN(headroom + mod[j].mod_end);
 
             if ( mod[j].reserved )
@@ -1481,7 +1490,7 @@  void __init noreturn __start_xen(unsigned long mbi_p)
 #endif
     }
 
-    if ( modules_headroom && !mod->reserved )
+    if ( boot_info->mods[0].arch->headroom && !mod->reserved )
         panic("Not enough memory to relocate the dom0 kernel image\n");
     for ( i = 0; i < boot_info->nr_mods; ++i )
     {
@@ -2021,7 +2030,7 @@  void __init noreturn __start_xen(unsigned long mbi_p)
      * 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.
      */
-    dom0 = create_dom0(mod, modules_headroom,
+    dom0 = create_dom0(mod, boot_info->mods[0].arch->headroom,
                        initrdidx < boot_info->nr_mods ? mod + initrdidx : NULL,
                        kextra, loader);
     if ( !dom0 )
diff --git a/xen/include/xen/bootinfo.h b/xen/include/xen/bootinfo.h
index 6a7d55d20e..b72ae31a66 100644
--- 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
+
+struct boot_module {
+    struct arch_bootmodule *arch;
+};
+
 struct boot_info {
     unsigned int nr_mods;
+    struct boot_module *mods;
 };
 
 #endif