[v6,10/44] x86/boot: introduce boot module flags

Commit Message

Daniel P. Smith Oct. 17, 2024, 5:02 p.m. UTC

The existing startup code employs various ad-hoc state tracking about certain
boot module types by each area of the code. A boot module flags is added to
enable tracking these different states.  The first state to be transition by
this commit is module relocation.

Signed-off-by: Daniel P. Smith <[e-mail]>
Reviewed-by: Jason Andryuk <[e-mail]>
Acked-by: Jan Beulich <[e-mail]>
---
Changes since v5:
- removed trailing blank line.
---
 xen/arch/x86/include/asm/bootinfo.h | 3 +++
 xen/arch/x86/setup.c                | 8 ++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

Comments

Andrew Cooper Oct. 17, 2024, 11:58 p.m. UTC | #1

On 17/10/2024 6:02 pm, Daniel P. Smith wrote:
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 18281d80fa97..e8ba9390a51f 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -39,6 +39,9 @@ struct boot_module {
>       */
>      unsigned long headroom;
>      enum bootmod_type type;
> +
> +    uint32_t flags;
> +#define BOOTMOD_FLAG_X86_RELOCATED     (1U << 0)

There are two parts to this request.  First, there's nothing X86
specific about any of the flags added (in this series at least), and the
FLAG infix doesn't feel as if it adds much.

But, looking over the code, I get the feeling it would be better as simply:

    /*
     * Misc flags  XXX docs
     */
    bool relocated:1;
    bool consumed:1;

(Possibly with an enclosing struct { ... } flags; but I don't think
that's necessary either.)

because flags is never operated on as a unit of multiple things, or
passed around separately from a bm-> pointer.  For misc independent
flags like this, bitfields lead to far more legible code because you're
not having to express everything as binary operators in the logic.

I know this will be a moderately invasive change, but I suspect the
improvement will speak for itself.  (Assuming there isn't a need to keep
it as a plain flags field later.)

~Andrew

Daniel P. Smith Oct. 18, 2024, 12:54 a.m. UTC | #2

On 10/17/24 19:58, Andrew Cooper wrote:
> On 17/10/2024 6:02 pm, Daniel P. Smith wrote:
>> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
>> index 18281d80fa97..e8ba9390a51f 100644
>> --- a/xen/arch/x86/include/asm/bootinfo.h
>> +++ b/xen/arch/x86/include/asm/bootinfo.h
>> @@ -39,6 +39,9 @@ struct boot_module {
>>        */
>>       unsigned long headroom;
>>       enum bootmod_type type;
>> +
>> +    uint32_t flags;
>> +#define BOOTMOD_FLAG_X86_RELOCATED     (1U << 0)
> 
> There are two parts to this request.  First, there's nothing X86
> specific about any of the flags added (in this series at least), and the
> FLAG infix doesn't feel as if it adds much.
> 
> But, looking over the code, I get the feeling it would be better as simply:
> 
>      /*
>       * Misc flags  XXX docs
>       */
>      bool relocated:1;
>      bool consumed:1;
> 
> (Possibly with an enclosing struct { ... } flags; but I don't think
> that's necessary either.)

I see no reason why not a bitfield, and as you state, will make code 
simpler (and possibly shorter) elsewhere.

> because flags is never operated on as a unit of multiple things, or
> passed around separately from a bm-> pointer.  For misc independent
> flags like this, bitfields lead to far more legible code because you're
> not having to express everything as binary operators in the logic.
> 
> I know this will be a moderately invasive change, but I suspect the
> improvement will speak for itself.  (Assuming there isn't a need to keep
> it as a plain flags field later.)

Yah it will be a bit painful, but I would rather have cleaner and easier 
to read code.

v/r,
dps

Patch

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 18281d80fa97..e8ba9390a51f 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -39,6 +39,9 @@  struct boot_module {
      */
     unsigned long headroom;
     enum bootmod_type type;
+
+    uint32_t flags;
+#define BOOTMOD_FLAG_X86_RELOCATED     (1U << 0)
 };
 
 /*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index fed9bef16305..f87faa31a2cf 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1357,7 +1357,6 @@  void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
             panic("Bootloader didn't honor module alignment request\n");
         bi->mods[i].mod->mod_end -= bi->mods[i].mod->mod_start;
         bi->mods[i].mod->mod_start >>= PAGE_SHIFT;
-        bi->mods[i].mod->reserved = 0;
     }
 
     /*
@@ -1471,7 +1470,7 @@  void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
             struct boot_module *bm = &bi->mods[j];
             unsigned long size = PAGE_ALIGN(bm->headroom + bm->mod->mod_end);
 
-            if ( bi->mods[j].mod->reserved )
+            if ( bi->mods[j].flags & BOOTMOD_FLAG_X86_RELOCATED )
                 continue;
 
             /* Don't overlap with other modules (or Xen itself). */
@@ -1490,7 +1489,7 @@  void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
                             bm->mod->mod_end);
                 bm->mod->mod_start = (end - size) >> PAGE_SHIFT;
                 bm->mod->mod_end += bm->headroom;
-                bm->mod->reserved = 1;
+                bm->flags |= BOOTMOD_FLAG_X86_RELOCATED;
             }
         }
 
@@ -1516,7 +1515,8 @@  void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
 #endif
     }
 
-    if ( bi->mods[0].headroom && !bi->mods[0].mod->reserved )
+    if ( bi->mods[0].headroom &&
+         !(bi->mods[0].flags & BOOTMOD_FLAG_X86_RELOCATED) )
         panic("Not enough memory to relocate the dom0 kernel image\n");
     for ( i = 0; i < bi->nr_modules; ++i )
     {