Re: [PATCH 08/12] x86/boot: convert domain construction to use boot info

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/11/2024 11:06 pm, Jason Andryuk wrote:
> On 2024-11-02 13:25, Daniel P. Smith wrote:
>> With all the components used to construct dom0 encapsulated in struct
>> boot_info
>> and struct boot_module, it is no longer necessary to pass all them as
>> parameters down the domain construction call chain. Change the
>> parameter list
>> to pass the struct boot_info instance and the struct domain reference.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> Changes since v5:
>> - renamed from "x86/boot: convert create_dom0 to use boot info"
>>
>> Changes since v5:
>> - change headroom back to unsigned long
>> - make mod_idx unsigned int
>> ---
>>   xen/arch/x86/dom0_build.c             |  9 ++--
>>   xen/arch/x86/hvm/dom0_build.c         | 49 +++++++++++++---------
>>   xen/arch/x86/include/asm/dom0_build.h | 13 ++----
>>   xen/arch/x86/include/asm/setup.h      |  7 ++--
>>   xen/arch/x86/pv/dom0_build.c          | 59 ++++++++++++++++-----------
>>   xen/arch/x86/setup.c                  | 33 ++++++++-------
>>   6 files changed, 95 insertions(+), 75 deletions(-)
>>
>
>> diff --git a/xen/arch/x86/hvm/dom0_build.c
>> b/xen/arch/x86/hvm/dom0_build.c
>> index a4ac262db463..cd97f94a168a 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>
>> @@ -1301,16 +1302,25 @@ static void __hwdom_init
>> pvh_setup_mmcfg(struct domain *d)
>>       }
>>   }
>>   -int __init dom0_construct_pvh(struct domain *d, const module_t
>> *image,
>> -                              unsigned long image_headroom,
>> -                              module_t *initrd,
>> -                              const char *cmdline)
>> +int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
>>   {
>>       paddr_t entry, start_info;
>> +    struct boot_module *image;
>> +    struct boot_module *initrd = NULL;
>>       int rc;
>>         printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n",
>> d->domain_id);
>>   +    rc = first_boot_module_index(bi, BOOTMOD_KERNEL);
>> +    if ( unlikely(rc < 0 || rc > bi->nr_modules) )
>
> Here and ...
>
>> +        panic("Missing kernel boot module for %pd construction\n", d);
>> +
>> +    image = &bi->mods[rc];
>> +
>> +    rc = first_boot_module_index(bi, BOOTMOD_RAMDISK);
>> +    if ( rc > 0 || rc < bi->nr_modules )
>
> ... here.  Can we just check rc < bi->nr_modules for validity?

You could, but eventually MISRA will say no and I suspect it will then
be made your problem to fix.

In this case, we ought to have an `unsigned int idx` and not (re)use rc.

Also, we panic far earlier in __start_xen() if there's no dom0 kernel,
so I think we can just assert that rather than having a logically dead
panic() path.


>   Valid modules are 0...nr_modules and not found is MAX_NR_BOOTMODS +
> 1.  It eliminates these unecessary double checks.  This would apply to
> 04/12 "x86/boot: introduce module release" as well.
>
>> +        initrd = &bi->mods[rc];
>> +
>>       if ( is_hardware_domain(d) )
>>       {
>>           /*
>
>
>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>> index c1f44502a1ac..594874cd8d85 100644
>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>
>> @@ -374,10 +371,13 @@ static int __init dom0_construct(struct domain *d,
>>       unsigned int flush_flags = 0;
>>       start_info_t *si;
>>       struct vcpu *v = d->vcpu[0];
>> -    void *image_base = bootstrap_map(image);
>> -    unsigned long image_len = image->mod_end;
>> -    void *image_start = image_base + image_headroom;
>> -    unsigned long initrd_len = initrd ? initrd->mod_end : 0;
>> +    struct boot_module *image;
>> +    struct boot_module *initrd = NULL;
>> +    void *image_base;
>> +    unsigned long image_len;
>> +    void *image_start;
>> +    unsigned long initrd_len = 0;
>> +    const char *cmdline;
>>       l4_pgentry_t *l4tab = NULL, *l4start = NULL;
>>       l3_pgentry_t *l3tab = NULL, *l3start = NULL;
>>       l2_pgentry_t *l2tab = NULL, *l2start = NULL;
>> @@ -414,6 +414,23 @@ static int __init dom0_construct(struct domain *d,
>>         printk(XENLOG_INFO "*** Building a PV Dom%d ***\n",
>> d->domain_id);
>>   +    i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>> +    if ( unlikely(i < 0 || i > bi->nr_modules) )
>
> Single check here.

Similar argument.  Except it turns out that i is used for precisely two
loops in dom0_construct() both of which are from 0 to either 4 or 32.

So it very much can be converted to being an unsigned variable, and then
this works nicely.

That said, drop the unlikely().   This is an init function run once, and
all it is doing is reducing legibility.

>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index aba9df8620ef..d9785acf89b6 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -977,10 +977,7 @@ static unsigned int __init copy_bios_e820(struct
>> e820entry *map, unsigned int li
>>       return n;
>>   }
>>   -static struct domain *__init create_dom0(const module_t *image,
>> -                                         unsigned long headroom,
>> -                                         module_t *initrd, const
>> char *kextra,
>> -                                         const char *loader)
>> +static struct domain *__init create_dom0(struct boot_info *bi)
>>   {
>>       static char __initdata cmdline[MAX_GUEST_CMDLINE];
>>   @@ -997,6 +994,14 @@ static struct domain *__init create_dom0(const
>> module_t *image,
>>       };
>>       struct domain *d;
>>       domid_t domid;
>> +    struct boot_module *image;
>> +    unsigned int idx;
>> +
>> +    idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
>> +    if ( unlikely(idx < 0 || idx > bi->nr_modules) )
>
> Single check here please.

I'm surprised that the compiler didn't complain about "idx < 0" being
tautological here.

~Andrew