Re: [PATCH v6 03/12] x86/hyperlaunch: initial support for hyperlaunch device tree

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2025 14:36, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
> Reviewed-by: Denis Mukhin <dmukhin@xxxxxxxx>

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