Re: [PATCH v3 07/16] x86/hyperlaunch: initial support for hyperlaunch device tree

[Alejandro Vallejo <agarciav@xxxxxxx>]

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