Re: [PATCH v4 04/13] x86/hyperlaunch: initial support for hyperlaunch device tree

[Alejandro Vallejo <agarciav@amd.com>]

On Fri Apr 18, 2025 at 11:11 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:26PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>> 
>> Add the ability to detect both a formal hyperlaunch device tree or a dom0less
>> device tree. If the hyperlaunch device tree is found, then count the number of
>> domain entries, reporting an error if more than one is found.
>> 
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> v4:
>>   * Panic if we're booting on hyperlaunch, but walking the DTB fails.
>>   * Remove inconsequential "else" clause in fdt.c
>>   * Remove stub, as it's not required due to DCE
>>   * Use min() rather than open-code it
>> ---
>>  xen/arch/x86/include/asm/bootinfo.h |  1 +
>>  xen/common/domain-builder/core.c    | 11 +++++
>>  xen/common/domain-builder/fdt.c     | 63 +++++++++++++++++++++++++++++
>>  xen/common/domain-builder/fdt.h     |  1 +
>>  4 files changed, 76 insertions(+)
>> 
>> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
>> index 82c2650fcf..1e3d582e45 100644
>> --- a/xen/arch/x86/include/asm/bootinfo.h
>> +++ b/xen/arch/x86/include/asm/bootinfo.h
>> @@ -84,6 +84,7 @@ struct boot_info {
>>      bool hyperlaunch_enabled;
>> 
>>      unsigned int nr_modules;
>> +    unsigned int nr_domains;
>>      struct boot_module mods[MAX_NR_BOOTMODS + 1];
>>      struct boot_domain domains[MAX_NR_BOOTDOMS];
>>  };
>> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
>> index a5b21fc179..3b062e85ec 100644
>> --- a/xen/common/domain-builder/core.c
>> +++ b/xen/common/domain-builder/core.c
>> @@ -43,6 +43,17 @@ void __init builder_init(struct boot_info *bi)
>>              break;
>>          }
>>      }
>> +
>> +    if ( bi->hyperlaunch_enabled )
>> +    {
>> +        int ret;
>> +
>> +        printk(XENLOG_INFO "Hyperlaunch configuration:\n");
>> +        if ( (ret = walk_hyperlaunch_fdt(bi)) < 0 )
>> +            panic("Walk of device tree failed (%d)\n", ret);
>> +
>> +        printk(XENLOG_INFO "  number of domains: %u\n", bi->nr_domains);
>> +    }
>>  }
>> 
>>  /*
>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
>> index aaf8c1cc16..b5ff8220da 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/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;
>> +
>> +        return hv_node;
>> +    }
>> +    else
>> +    {
>> +        /* Look for dom0less config */
>> +        int node, chosen_node = fdt_path_offset(fdt, "/chosen");
>> +
>> +        if ( chosen_node < 0 )
>> +            return -ENOENT;
>> +
>> +        fdt_for_each_subnode(node, fdt, chosen_node)
>> +        {
>> +            if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
>> +                return chosen_node;
>> +        }
>> +    }
>> +
>> +    return -ENODATA;
>> +}
>> +
>>  int __init has_hyperlaunch_fdt(const struct boot_info *bi)
>>  {
>>      int ret = 0;
>> @@ -20,7 +50,40 @@ 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 min(0, ret);
>> +}
>> +
>> +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;
>
> I think this check can be converted to ASSERT() since walk_hyperlaunch_fdt()
> will be called after has_hyperlaunch_fdt() where condition is checked.

True that.

Cheers,
Alejandro