On Thu Apr 10, 2025 at 11:10 AM BST, Jan Beulich wrote:
> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/domain-builder/core.c
>> +++ b/xen/arch/x86/domain-builder/core.c
>> @@ -44,6 +44,21 @@ void __init builder_init(struct boot_info *bi)
>> break;
>> }
>> }
>> +
>> + if ( bi->hyperlaunch_enabled )
>> + {
>
> Not knowing what else if going to appear here and in what shape, could the
> if() here be avoided by making case blocks in the earlier switch setting
> the field to false (which is kind of redundant anyway with it starting out
> false) use "return" instead of "break"? The the setting of the field to
> true could also be centralized below the switch().
Looking ahead in the patchlist, not much. There's an else clause for
non-hyperlaunch with code picked from setup.c . It could very well stay
there and prevent core.c from being included at all, removing the if
guards here as well when the file is no longer included.
>
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -13,6 +13,36 @@
>>
>> #include "fdt.h"
>>
>> +static int __init find_hyperlaunch_node(const void *fdt)
>> +{
>> + int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
>> +
>> + if ( hv_node >= 0 )
>> + {
>> + /* Anything other than zero indicates no match */
>> + if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
>> + return -ENODATA;
>> + else
>> + return hv_node;
>
> Could I talk you into omitting such unnecessary "else"?
Yes, sure.
>
>> @@ -20,7 +50,41 @@ int __init has_hyperlaunch_fdt(const struct boot_info *bi)
>>
>> if ( !fdt || fdt_check_header(fdt) < 0 )
>> ret = -EINVAL;
>> + else
>> + ret = find_hyperlaunch_node(fdt);
>> +
>> + bootstrap_unmap();
>> +
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
>> +{
>> + int ret = 0, hv_node, node;
>> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>> +
>> + if ( unlikely(!fdt) )
>> + return -EINVAL;
>> +
>> + hv_node = find_hyperlaunch_node(fdt);
>> + if ( hv_node < 0 )
>> + {
>> + ret = hv_node;
>> + goto err_out;
>> + }
>> +
>> + fdt_for_each_subnode(node, fdt, hv_node)
>> + {
>> + ret = fdt_node_check_compatible(fdt, node, "xen,domain");
>> + if ( ret == 0 )
>> + bi->nr_domains++;
>> + }
>> +
>> + /* Until multi-domain construction is added, throw an error */
>> + if ( !bi->nr_domains || bi->nr_domains > 1 )
>
> Simply "!= 1"?
That would be simpler, yes :)
It's all temporary until the restriction is lifted later on, but will do.
>
>> --- a/xen/arch/x86/domain-builder/fdt.h
>> +++ b/xen/arch/x86/domain-builder/fdt.h
>> @@ -11,11 +11,16 @@ struct boot_info;
>>
>> #ifdef CONFIG_DOMAIN_BUILDER
>> int has_hyperlaunch_fdt(const struct boot_info *bi);
>> +int walk_hyperlaunch_fdt(struct boot_info *bi);
>> #else
>> static inline int __init has_hyperlaunch_fdt(const struct boot_info *bi)
>> {
>> return -EINVAL;
>> }
>> +static inline int __init walk_hyperlaunch_fdt(struct boot_info *bi)
>> +{
>> + return -EINVAL;
>> +}
>
> There's no need for this stub (nor the has_hyperlaunch_fdt() one, as I
> notice only now) - even with present arrangements calling code is guarded
> such that DCE will take care of eliminating the call, and hence having a
> declaration suffices.
>
> Jan
There's some refactoring to do for other helpers later on, but sure. It
should be fine.
Cheers,
Alejandro
