Re: [PATCH 02/10] x86 setup: per-arch bootmodule structure, headroom field
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
>
> +#ifdef CONFIG_X86
> +#include
> +#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
Re: [PATCH 02/10] x86 setup: per-arch bootmodule structure, headroom field
On Sat, Jul 8, 2023 at 12:15 PM Stefano Stabellini
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
> > Signed-off-by: Daniel P. Smith
>
> [...]
>
>
> > diff --git a/xen/arch/x86/include/asm/bootinfo.h
> b/xen/arch/x86/include/asm/bootinfo.h
> > new file mode 100644
> > index 00..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
> >
> > +#ifdef CONFIG_X86
> > +#include
> > +#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
> >
> >
>
Re: [PATCH 02/10] x86 setup: per-arch bootmodule structure, headroom field
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
> Signed-off-by: Daniel P. Smith
[...]
> diff --git a/xen/arch/x86/include/asm/bootinfo.h
> b/xen/arch/x86/include/asm/bootinfo.h
> new file mode 100644
> index 00..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
>
> +#ifdef CONFIG_X86
> +#include
> +#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
>
>
[PATCH 02/10] x86 setup: per-arch bootmodule structure, headroom field
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
Signed-off-by: Daniel P. Smith
---
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
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 00..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 (
