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

[Alejandro Vallejo <agarciav@xxxxxxx>]

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@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?

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