On 29.04.2025 14:36, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jason Andryuk <[email protected]>
> Signed-off-by: Alejandro Vallejo <[email protected]>
> Reviewed-by: Denis Mukhin <[email protected]>
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
