On 11.12.2024 13:36, Daniel P. Smith wrote:
> On 12/2/24 05:10, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> Introduce the domain builder which is capable of consuming a device tree as
>>> the
>>> first boot module. If it finds a device tree as the first boot module, it
>>> will
>>> set its type to BOOTMOD_FDT. This change only detects the boot module and
>>> continues to boot with slight change to the boot convention that the dom0
>>> kernel is no longer first boot module but is the second.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsm...@apertussolutions.com>
>>> ---
>>> xen/arch/x86/Makefile | 2 +
>>> xen/arch/x86/domain_builder/Makefile | 3 ++
>>> xen/arch/x86/domain_builder/core.c | 55 ++++++++++++++++++++++++
>>> xen/arch/x86/domain_builder/fdt.c | 38 ++++++++++++++++
>>> xen/arch/x86/domain_builder/fdt.h | 21 +++++++++
>>> xen/arch/x86/include/asm/bootinfo.h | 3 ++
>>> xen/arch/x86/include/asm/domainbuilder.h | 8 ++++
>>> xen/arch/x86/setup.c | 18 +++++---
>>> 8 files changed, 142 insertions(+), 6 deletions(-)
>>> create mode 100644 xen/arch/x86/domain_builder/Makefile
>>> create mode 100644 xen/arch/x86/domain_builder/core.c
>>> create mode 100644 xen/arch/x86/domain_builder/fdt.c
>>> create mode 100644 xen/arch/x86/domain_builder/fdt.h
>>
>> As I'm sure I indicated before: Dashes instead of underscores please in new
>> files' names.
>>
>>> create mode 100644 xen/arch/x86/include/asm/domainbuilder.h
>>
>> Why is there no separator in this file's name?
>
> Name was getting a bit long, but can add separator if desired.
Well, my desire is for the subdir and the header names to match up.
Personally I think that neater to achieve when both have a dash in the
middle.
>>> --- /dev/null
>>> +++ b/xen/arch/x86/domain_builder/core.c
>>> @@ -0,0 +1,55 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (C) 2024, Apertus Solutions, LLC
>>> + */
>>> +#include <xen/err.h>
>>> +#include <xen/init.h>
>>> +#include <xen/kconfig.h>
>>> +#include <xen/lib.h>
>>> +
>>> +#include <asm/bootinfo.h>
>>> +
>>> +#include "fdt.h"
>>> +
>>> +void __init builder_init(struct boot_info *bi)
>>> +{
>>> + if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>>> + {
>>> + int ret;
>>> +
>>> + switch ( ret = has_hyperlaunch_fdt(bi) )
>>> + {
>>> + case 0:
>>> + printk("Hyperlaunch device tree detected\n");
>>> + bi->hyperlaunch_enabled = true;
>>> + bi->mods[0].type = BOOTMOD_FDT;
>>> + break;
>>> + case -EINVAL:
>>> + printk("Hyperlaunch device tree was not detected\n");
>>> + bi->hyperlaunch_enabled = false;
>>> + break;
>>> + case -ENOENT:
>>> + fallthrough;
>>
>> No need for this.
>
> I thought MISRA called for explicit fallthrough?
Only when there are statements between two case labels. Which ...
>>> + case -ENODATA:
... isn't the case here.
>>> @@ -1277,9 +1278,12 @@ void asmlinkage __init noreturn __start_xen(void)
>>> bi->nr_modules);
>>> }
>>>
>>> - /* Dom0 kernel is always first */
>>> - bi->mods[0].type = BOOTMOD_KERNEL;
>>> - bi->domains[0].kernel = &bi->mods[0];
>>> + builder_init(bi);
>>> +
>>> + /* Find first unknown boot module to use as Dom0 kernel */
>>> + i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>>> + bi->mods[i].type = BOOTMOD_KERNEL;
>>> + bi->domains[0].kernel = &bi->mods[i];
>>
>> Better latch the result here into a separate local variable, for use ...
>>
>>> @@ -1466,8 +1470,9 @@ void asmlinkage __init noreturn __start_xen(void)
>>> xen->size = __2M_rwdata_end - _stext;
>>> }
>>>
>>> - bi->mods[0].headroom =
>>> - bzimage_headroom(bootstrap_map_bm(&bi->mods[0]), bi->mods[0].size);
>>> + i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>>> + bi->mods[i].headroom =
>>> + bzimage_headroom(bootstrap_map_bm(&bi->mods[i]), bi->mods[i].size);
>>> bootstrap_unmap();
>>>
>>> #ifndef highmem_start
>>> @@ -1591,7 +1596,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>> #endif
>>> }
>>>
>>> - if ( bi->mods[0].headroom && !bi->mods[0].relocated )
>>> + i = first_boot_module_index(bi, BOOTMOD_KERNEL);
>>> + if ( bi->mods[i].headroom && !bi->mods[0].relocated )
>>> panic("Not enough memory to relocate the dom0 kernel image\n");
>>> for ( i = 0; i < bi->nr_modules; ++i )
>>> {
>>
>> ... in these two places?
>
> I don't know if a local variable is need. I assume your suggestion is to
> drop the first_boot_module_index() call,
The latter two of the three, yes.
> but thinking about it, not sure
> why I kept the walk. A direct use of bi->domains[0].kernel could be used
> without the intermediate variable while removing the call.
If that's possible, the even better.
Jan
