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" <dpsm...@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 <dpsm...@apertussolutions.com>
>> Signed-off-by: Jason Andryuk <jason.andr...@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarc...@amd.com>
>> Reviewed-by: Denis Mukhin <dmuk...@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
