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>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
---
xen/arch/x86/include/asm/bootinfo.h | 1 +
xen/common/domain-builder/core.c | 15 +++++++
xen/common/domain-builder/fdt.c | 62 +++++++++++++++++++++++++++++
xen/common/domain-builder/fdt.h | 1 +
xen/include/xen/domain-builder.h | 2 +
5 files changed, 81 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 97c92db571..c6917532be 100644
--- a/xen/common/domain-builder/core.c
+++ b/xen/common/domain-builder/core.c
@@ -16,10 +16,17 @@
enum fdt_kind __init builder_init(struct boot_info *bi)
{
enum fdt_kind kind;
+ int ret;
bi->hyperlaunch_enabled = false;
switch ( (kind = fdt_detect_kind(bi)) )
{
+ case FDT_KIND_HYPERLAUNCH:
+ printk(XENLOG_DEBUG "DT found: hyperlaunch\n");
+ bi->hyperlaunch_enabled = true;
+ bi->mods[0].type = BOOTMOD_FDT;
+ break;
+
case FDT_KIND_NONE:
/* No DT found */
return kind;
@@ -32,6 +39,14 @@ enum fdt_kind __init builder_init(struct boot_info *bi)
default:
BUG();
}
+
+ printk(XENLOG_INFO "Hyperlaunch configuration:\n");
+ if ( (ret = fdt_walk_hyperlaunch(bi)) < 0 )
+ panic("Walk of device tree failed (%d)\n", ret);
+
+ printk(XENLOG_INFO " number of domains: %u\n", bi->nr_domains);
+
+ return FDT_KIND_HYPERLAUNCH;
}
/*
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 4b07bd22c8..94ccff61e2 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;
+}
+
enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
{
enum fdt_kind kind;
@@ -20,6 +50,8 @@ enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
if ( !fdt || fdt_check_header(fdt) < 0 )
kind = FDT_KIND_NONE;
+ else if ( find_hyperlaunch_node(fdt) >= 0 )
+ kind = FDT_KIND_HYPERLAUNCH;
else
kind = FDT_KIND_UNKNOWN;
@@ -28,6 +60,36 @@ enum fdt_kind __init fdt_detect_kind(const struct boot_info *bi)
return kind;
}
+int __init fdt_walk_hyperlaunch(struct boot_info *bi)
+{
+ int ret = 0, hv_node, node;
+ const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+ BUG_ON(!fdt);
+
+ hv_node = find_hyperlaunch_node(fdt);
+ if ( hv_node < 0 )
+ {
+ ret = hv_node;
+ goto err_out;
+ }
+
+ fdt_for_each_subnode(node, fdt, hv_node)
+ {
+ if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
+ bi->nr_domains++;
+ }
+
+ /* Until multi-domain construction is added, throw an error */
+ if ( bi->nr_domains != 1 )
+ printk(XENLOG_ERR "hyperlaunch only supports dom0 construction\n");
+
+ err_out:
+ bootstrap_unmap();
+
+ return ret;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
index ef897fc412..d1bcc23fa2 100644
--- a/xen/common/domain-builder/fdt.h
+++ b/xen/common/domain-builder/fdt.h
@@ -10,5 +10,6 @@ struct boot_info;
#define HYPERLAUNCH_MODULE_IDX 0
enum fdt_kind fdt_detect_kind(const struct boot_info *bi);
+int fdt_walk_hyperlaunch(struct boot_info *bi);
#endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
index b9702db735..cbb3cbea7c 100644
--- a/xen/include/xen/domain-builder.h
+++ b/xen/include/xen/domain-builder.h
@@ -9,6 +9,8 @@ enum fdt_kind
{
/* FDT not found. Skipped builder. */
FDT_KIND_NONE,
+ /* Found Hyperlaunch FDT */
+ FDT_KIND_HYPERLAUNCH,
/* Found an FDT that wasn't hyperlaunch. */
FDT_KIND_UNKNOWN,
};
--
2.43.0On 29.04.2025 14:36, 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>
> Reviewed-by: Denis Mukhin <dmukhin@ford.com>
First: With your code re-use proposal sent earlier today I wonder how
meaningful it is to further review this series. Much of it would change
if that proposal was followed, I expect?
Then: When you say "hyperlaunch or dom0less" - is it entirely benign
which of the two is found, as to further parsing? I ask because I can't
spot anywhere that you would record which of the two (if any) was found.
> --- 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
Please can such unnecessary (and potentially misleading) "else" be omitted?
As ...
> + {
> + /* Look for dom0less config */
> + int node, chosen_node = fdt_path_offset(fdt, "/chosen");
... these will need to move to function scope then, one of the two may want
folding with "hv_node" above.
Jan
On Wed May 21, 2025 at 5:00 PM CEST, Jan Beulich wrote:
> On 29.04.2025 14:36, 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>
>> Reviewed-by: Denis Mukhin <dmukhin@ford.com>
>
> First: With your code re-use proposal sent earlier today I wonder how
> meaningful it is to further review this series. Much of it would change
> if that proposal was followed, I expect?
Should I follow through with that proposal, that would indeed have large
knock-on effects here. Sorry, I took longer than I thought I would
evaluating.
I'll go over your reviews and answer them in case they stay relevant
afterwards. Thanks for that.
>
> Then: When you say "hyperlaunch or dom0less" - is it entirely benign
> which of the two is found, as to further parsing? I ask because I can't
> spot anywhere that you would record which of the two (if any) was found.
Under dom0less everything is /chosen, mixed with other nodes.
Hyperlaunch mandates the initial system configuration to reside in
/chosen/hypervisor, which is meant to be "xen,hypervisor"-compatible.
The function is effectively finding a suitable root for the tree that
contains the initial system config.
>
>> --- 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
>
> Please can such unnecessary (and potentially misleading) "else" be omitted?
Not sure how it could be misleading, but...
> As ...
>
>> + {
>> + /* Look for dom0less config */
>> + int node, chosen_node = fdt_path_offset(fdt, "/chosen");
>
> ... these will need to move to function scope then, one of the two may want
> folding with "hv_node" above.
... there is indeed a more compact form the function could take. Noted.
Cheers,
Alejandro
On 21.05.2025 19:24, Alejandro Vallejo wrote:
> On Wed May 21, 2025 at 5:00 PM CEST, Jan Beulich wrote:
>> On 29.04.2025 14:36, Alejandro Vallejo wrote:
>>> --- 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
>>
>> Please can such unnecessary (and potentially misleading) "else" be omitted?
>
> Not sure how it could be misleading,
For context, just briefly looking at such a construct may leave one with
the (wrong) impression that both branches of the conditional can fall through
to subsequent code. This may be less of an issue as long as both sides are
reasonably short, but then it is imo better to avoid the pattern altogether.
Jan
> but...
>
>> As ...
>>
>>> + {
>>> + /* Look for dom0less config */
>>> + int node, chosen_node = fdt_path_offset(fdt, "/chosen");
>>
>> ... these will need to move to function scope then, one of the two may want
>> folding with "hv_node" above.
>
> ... there is indeed a more compact form the function could take. Noted.
>
> Cheers,
> Alejandro
© 2016 - 2025 Red Hat, Inc.