Look for a subnode of type `multiboot,kernel` within a domain node. If found,
process the reg property for the MB1 module index. If the bootargs property is
present and there was not an MB1 string, then use the command line from the
device tree definition.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/domain_builder/core.c |  12 +++
 xen/arch/x86/domain_builder/fdt.c  | 126 +++++++++++++++++++++++++++++
 xen/arch/x86/domain_builder/fdt.h  |  17 ++++
 xen/arch/x86/setup.c               |   5 --
 4 files changed, 155 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
index a80f3711c306..9335f3a9ebef 100644
--- a/xen/arch/x86/domain_builder/core.c
+++ b/xen/arch/x86/domain_builder/core.c
@@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
 
         printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
     }
+    else
+    {
+        int i;
+
+        /* Find first unknown boot module to use as Dom0 kernel */
+        printk("Falling back to using first boot module as dom0\n");
+        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
+        bi->mods[i].type = BOOTMOD_KERNEL;
+        bi->domains[0].kernel = &bi->mods[i];
+        bi->nr_domains = 1;
+    }
+
 }
 
 /*
diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index ff1ba58b6907..6bf1c4a297fe 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -14,6 +14,122 @@
 
 #include "fdt.h"
 
+static inline int __init fdt_get_prop_as_reg(
+    const void *fdt, int node, const char *name, unsigned int ssize,
+    unsigned int asize, uint64_t *size, uint64_t *addr)
+{
+    int ret;
+    const struct fdt_property *prop;
+    fdt32_t *cell;
+
+    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
+    if ( ssize > 2 || asize > 2 )
+        return -EINVAL;
+
+    prop = fdt_get_property(fdt, node, name, &ret);
+    if ( !prop || ret < sizeof(u32) )
+        return ret < 0 ? ret : -EINVAL;
+
+    /* read address field */
+    cell = (fdt32_t *)prop->data;
+
+    if ( asize == 1 )
+    {
+        uint32_t val;
+        fdt_cell_as_u32(cell, &val);
+        *addr = (uint64_t)val;
+    }
+    else
+        fdt_cell_as_u64(cell, addr);
+
+    /* read size field */
+    cell += asize;
+
+    if ( ssize == 1 )
+    {
+        uint32_t val;
+        fdt_cell_as_u32(cell, &val);
+        *size = (uint64_t)val;
+    }
+    else
+        fdt_cell_as_u64(cell, size);
+
+    return 0;
+}
+
+static int __init dom0less_module_node(
+    void *fdt, int node, int size_size, int address_size)
+{
+    uint64_t size, addr;
+    int ret = fdt_get_prop_as_reg(fdt, node, "reg", size_size, address_size,
+                                  &size, &addr);
+    /* An FDT error value may have been returned, translate to -EINVAL */
+    if ( ret < 0 )
+        return -EINVAL;
+
+    if ( size != 0 )
+        return -EOPNOTSUPP;
+
+    if ( addr > MAX_NR_BOOTMODS )
+        return -ERANGE;
+
+    /*
+     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
+     * thus the cast down to a u32 will be safe due to the prior check.
+     */
+    return (int)addr;
+}
+
+static int __init process_domain_node(
+    struct boot_info *bi, void *fdt, int dom_node)
+{
+    int node;
+    struct boot_domain *bd = &bi->domains[bi->nr_domains];
+    const char *name = fdt_get_name(fdt, dom_node, NULL);
+    int address_size = fdt_address_cells(fdt, dom_node);
+    int size_size = fdt_size_cells(fdt, dom_node);
+
+    if ( address_size < 0 || size_size < 0 )
+    {
+        printk("  failed processing #address or #size for domain %s)\n",
+               name == NULL ? "unknown" : name);
+        return -EINVAL;
+    }
+
+    fdt_for_each_subnode(node, fdt, dom_node)
+    {
+        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
+        {
+            int idx = dom0less_module_node(fdt, node, size_size, address_size);
+            if ( idx < 0 )
+            {
+                printk("  failed processing kernel module for domain %s)\n",
+                       name == NULL ? "unknown" : name);
+                return idx;
+            }
+
+            if ( idx > bi->nr_modules )
+            {
+                printk("  invalid kernel module index for domain node (%d)\n",
+                       bi->nr_domains);
+                return -EINVAL;
+            }
+
+            printk("  kernel: boot module %d\n", idx);
+            bi->mods[idx].type = BOOTMOD_KERNEL;
+            bd->kernel = &bi->mods[idx];
+        }
+    }
+
+    if ( !bd->kernel )
+    {
+        printk(XENLOG_ERR "ERR: no kernel assigned to domain\n");
+        return -EFAULT;
+    }
+
+    return 0;
+}
+
 static int __init find_hyperlaunch_node(void *fdt)
 {
     int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
@@ -74,9 +190,19 @@ int __init walk_hyperlaunch_fdt(struct boot_info *bi)
 
     fdt_for_each_subnode(node, fdt, hv_node)
     {
+        if ( bi->nr_domains >= MAX_NR_BOOTDOMS )
+        {
+            printk(XENLOG_WARNING "WARN: more domains defined than max allowed");
+            break;
+        }
+
         ret = fdt_node_check_compatible(fdt, node, "xen,domain");
         if ( ret == 0 )
+        {
+            if ( (ret = process_domain_node(bi, fdt, node)) < 0 )
+                break;
             bi->nr_domains++;
+        }
     }
 
     /* Until multi-domain construction is added, throw an error */
diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
index 84126db208ea..558d00a994fa 100644
--- a/xen/arch/x86/domain_builder/fdt.h
+++ b/xen/arch/x86/domain_builder/fdt.h
@@ -3,6 +3,7 @@
 #define __XEN_X86_FDT_H__
 
 #include <xen/init.h>
+#include <xen/libfdt/libfdt.h>
 
 #include <asm/bootinfo.h>
 
@@ -10,6 +11,22 @@
 #define HYPERLAUNCH_MODULE_IDX 0
 
 #ifdef CONFIG_DOMAIN_BUILDER
+
+static inline int __init fdt_cell_as_u32(const fdt32_t *cell, uint32_t *val)
+{
+    *val = fdt32_to_cpu(*cell);
+
+    return 0;
+}
+
+static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
+{
+    *val = ((uint64_t)fdt32_to_cpu(cell[0]) << 32) |
+           (uint64_t)fdt32_to_cpu(cell[1]);
+
+    return 0;
+}
+
 int has_hyperlaunch_fdt(struct boot_info *bi);
 int walk_hyperlaunch_fdt(struct boot_info *bi);
 #else
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8041aeb3dcfd..d6e9d4c1769c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1280,11 +1280,6 @@ void asmlinkage __init noreturn __start_xen(void)
 
     builder_init(bi);
 
-    /* Find first unknown boot module to use as Dom0 kernel */
-    i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
-    bi->mods[i].type = BOOTMOD_KERNEL;
-    bi->domains[0].kernel = &bi->mods[i];
-
     if ( pvh_boot )
     {
         /* pvh_init() already filled in e820_raw */
-- 
2.30.2

On 23.11.2024 19:20, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
> process the reg property for the MB1 module index. If the bootargs property is
> present and there was not an MB1 string, then use the command line from the
> device tree definition.

Why specifically MB1?

> --- a/xen/arch/x86/domain_builder/core.c
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>  
>          printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>      }
> +    else
> +    {
> +        int i;

Plain int when ...

> +        /* Find first unknown boot module to use as Dom0 kernel */
> +        printk("Falling back to using first boot module as dom0\n");
> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> +        bi->mods[i].type = BOOTMOD_KERNEL;
> +        bi->domains[0].kernel = &bi->mods[i];
> +        bi->nr_domains = 1;
> +    }

... it's used as array index (and there's no check for the function return
value being negative)?

> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -14,6 +14,122 @@
>  
>  #include "fdt.h"
>  
> +static inline int __init fdt_get_prop_as_reg(

What does "reg" stand for here?

> +    const void *fdt, int node, const char *name, unsigned int ssize,
> +    unsigned int asize, uint64_t *size, uint64_t *addr)
> +{
> +    int ret;
> +    const struct fdt_property *prop;
> +    fdt32_t *cell;
> +
> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
> +    if ( ssize > 2 || asize > 2 )
> +        return -EINVAL;
> +
> +    prop = fdt_get_property(fdt, node, name, &ret);
> +    if ( !prop || ret < sizeof(u32) )
> +        return ret < 0 ? ret : -EINVAL;
> +
> +    /* read address field */
> +    cell = (fdt32_t *)prop->data;
> +
> +    if ( asize == 1 )
> +    {
> +        uint32_t val;
> +        fdt_cell_as_u32(cell, &val);
> +        *addr = (uint64_t)val;

No need for a cast here nor ...

> +    }
> +    else
> +        fdt_cell_as_u64(cell, addr);
> +
> +    /* read size field */
> +    cell += asize;
> +
> +    if ( ssize == 1 )
> +    {
> +        uint32_t val;
> +        fdt_cell_as_u32(cell, &val);
> +        *size = (uint64_t)val;

... here?

> +    }
> +    else
> +        fdt_cell_as_u64(cell, size);
> +
> +    return 0;
> +}

This whole function reads very much like a library one. Does it really need
adding here, rather than to the FDT library code we already have? In any
event there's nothing x86-specific about it, afaics.

> +static int __init dom0less_module_node(
> +    void *fdt, int node, int size_size, int address_size)

Three times plain int, when ...

> +{
> +    uint64_t size, addr;
> +    int ret = fdt_get_prop_as_reg(fdt, node, "reg", size_size, address_size,

... two get converted to unsigned int in the course of the function call
here?

> +                                  &size, &addr);
> +    /* An FDT error value may have been returned, translate to -EINVAL */
> +    if ( ret < 0 )
> +        return -EINVAL;
> +
> +    if ( size != 0 )
> +        return -EOPNOTSUPP;

Not knowing much about DT: What does 0 represent here?

> +    if ( addr > MAX_NR_BOOTMODS )
> +        return -ERANGE;
> +
> +    /*
> +     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
> +     * thus the cast down to a u32 will be safe due to the prior check.
> +     */
> +    return (int)addr;

Comment and cast contradict one another. DYM u32 (really: uint32_t), or plain
int? If you mean to return a plain int (for the sake of the -errno values
further up), MAX_NR_BOOTMODS needs to stay below 2**31.

> +static int __init process_domain_node(
> +    struct boot_info *bi, void *fdt, int dom_node)
> +{
> +    int node;
> +    struct boot_domain *bd = &bi->domains[bi->nr_domains];
> +    const char *name = fdt_get_name(fdt, dom_node, NULL);
> +    int address_size = fdt_address_cells(fdt, dom_node);
> +    int size_size = fdt_size_cells(fdt, dom_node);
> +
> +    if ( address_size < 0 || size_size < 0 )
> +    {
> +        printk("  failed processing #address or #size for domain %s)\n",
> +               name == NULL ? "unknown" : name);
> +        return -EINVAL;
> +    }
> +
> +    fdt_for_each_subnode(node, fdt, dom_node)
> +    {
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> +        {
> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
> +            if ( idx < 0 )
> +            {
> +                printk("  failed processing kernel module for domain %s)\n",
> +                       name == NULL ? "unknown" : name);
> +                return idx;
> +            }
> +
> +            if ( idx > bi->nr_modules )
> +            {
> +                printk("  invalid kernel module index for domain node (%d)\n",
> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }
> +
> +            printk("  kernel: boot module %d\n", idx);
> +            bi->mods[idx].type = BOOTMOD_KERNEL;
> +            bd->kernel = &bi->mods[idx];
> +        }
> +    }

What if you find two?

> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -3,6 +3,7 @@
>  #define __XEN_X86_FDT_H__
>  
>  #include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
>  
>  #include <asm/bootinfo.h>
>  
> @@ -10,6 +11,22 @@
>  #define HYPERLAUNCH_MODULE_IDX 0
>  
>  #ifdef CONFIG_DOMAIN_BUILDER
> +
> +static inline int __init fdt_cell_as_u32(const fdt32_t *cell, uint32_t *val)
> +{
> +    *val = fdt32_to_cpu(*cell);
> +
> +    return 0;
> +}
> +
> +static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
> +{
> +    *val = ((uint64_t)fdt32_to_cpu(cell[0]) << 32) |
> +           (uint64_t)fdt32_to_cpu(cell[1]);
> +
> +    return 0;
> +}

Basic library routines again?

Jan

On 12/2/24 06:53, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
>> process the reg property for the MB1 module index. If the bootargs property is
>> present and there was not an MB1 string, then use the command line from the
>> device tree definition.
> 
> Why specifically MB1?

Because Xen converts MB2 into an MB1 chain very early in the entry 
points that take MB2. By the time HL code is executed, it will only ever 
see a list of MB1 modules.

>> --- a/xen/arch/x86/domain_builder/core.c
>> +++ b/xen/arch/x86/domain_builder/core.c
>> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>>   
>>           printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>>       }
>> +    else
>> +    {
>> +        int i;
> 
> Plain int when ...
> 
>> +        /* Find first unknown boot module to use as Dom0 kernel */
>> +        printk("Falling back to using first boot module as dom0\n");
>> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>> +        bi->mods[i].type = BOOTMOD_KERNEL;
>> +        bi->domains[0].kernel = &bi->mods[i];
>> +        bi->nr_domains = 1;
>> +    }
> 
> ... it's used as array index (and there's no check for the function return
> value being negative)?

Nope, that is an artifact from early version of boot module series in 
which first_boot_module_index() returned a plain int. Will fix.

>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>> @@ -14,6 +14,122 @@
>>   
>>   #include "fdt.h"
>>   
>> +static inline int __init fdt_get_prop_as_reg(
> 
> What does "reg" stand for here?

Device Tree defines a two field "prop-encoded=array" property type 
called reg.

>> +    const void *fdt, int node, const char *name, unsigned int ssize,
>> +    unsigned int asize, uint64_t *size, uint64_t *addr)
>> +{
>> +    int ret;
>> +    const struct fdt_property *prop;
>> +    fdt32_t *cell;
>> +
>> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
>> +    if ( ssize > 2 || asize > 2 )
>> +        return -EINVAL;
>> +
>> +    prop = fdt_get_property(fdt, node, name, &ret);
>> +    if ( !prop || ret < sizeof(u32) )
>> +        return ret < 0 ? ret : -EINVAL;
>> +
>> +    /* read address field */
>> +    cell = (fdt32_t *)prop->data;
>> +
>> +    if ( asize == 1 )
>> +    {
>> +        uint32_t val;
>> +        fdt_cell_as_u32(cell, &val);
>> +        *addr = (uint64_t)val;
> 
> No need for a cast here nor ...
> 
>> +    }
>> +    else
>> +        fdt_cell_as_u64(cell, addr);
>> +
>> +    /* read size field */
>> +    cell += asize;
>> +
>> +    if ( ssize == 1 )
>> +    {
>> +        uint32_t val;
>> +        fdt_cell_as_u32(cell, &val);
>> +        *size = (uint64_t)val;
> 
> ... here?

No the compiler does not need the cast, placed to remind readers what 
was being done. Can/will drop.

>> +    }
>> +    else
>> +        fdt_cell_as_u64(cell, size);
>> +
>> +    return 0;
>> +}
> 
> This whole function reads very much like a library one. Does it really need
> adding here, rather than to the FDT library code we already have? In any
> event there's nothing x86-specific about it, afaics.

This is where it gets complicated. Most of the higher order functions 
exposed by xen/device_tree.h are written to work with FDT indexing 
structures, referred to as the unflattened tree. Deconflicting the mixed 
use of FDT and FDT index in device_tree.h is beyond the scope of this 
series.

>> +static int __init dom0less_module_node(
>> +    void *fdt, int node, int size_size, int address_size)
> 
> Three times plain int, when ...
> 
>> +{
>> +    uint64_t size, addr;
>> +    int ret = fdt_get_prop_as_reg(fdt, node, "reg", size_size, address_size,
> 
> ... two get converted to unsigned int in the course of the function call
> here?

The libfdt function returns signed int for size_size and address_size to 
allow returning an error code, which is checked for at the time of 
invocation. After that point, the value is guaranteed to be >= 0. They 
can be left as int in the function calls or a pair of temporary plain 
ints could be used for the libfdt call and if no error, store the value 
into unsigned.

>> +                                  &size, &addr);
>> +    /* An FDT error value may have been returned, translate to -EINVAL */
>> +    if ( ret < 0 )
>> +        return -EINVAL;
>> +
>> +    if ( size != 0 )
>> +        return -EOPNOTSUPP;
> 
> Not knowing much about DT: What does 0 represent here?

The libfdt code treats 0 as a valid value, whether zero is a valid value 
is driven by the contextual usage of the property.

>> +    if ( addr > MAX_NR_BOOTMODS )
>> +        return -ERANGE;
>> +
>> +    /*
>> +     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
>> +     * thus the cast down to a u32 will be safe due to the prior check.
>> +     */
>> +    return (int)addr;
> 
> Comment and cast contradict one another. DYM u32 (really: uint32_t), or plain
> int? If you mean to return a plain int (for the sake of the -errno values
> further up), MAX_NR_BOOTMODS needs to stay below 2**31.

Good point, we cannot artificially impose 2^31 limit when 2^32 is the 
legitimate upper bound supported by the MB1 protocol. Even if that value 
is impractical. At the same time, it is beneficial to be able to 
communicate failures along with some delineation of the failure. Let me 
think about this, and in the meantime suggestions are welcomed.

>> +static int __init process_domain_node(
>> +    struct boot_info *bi, void *fdt, int dom_node)
>> +{
>> +    int node;
>> +    struct boot_domain *bd = &bi->domains[bi->nr_domains];
>> +    const char *name = fdt_get_name(fdt, dom_node, NULL);
>> +    int address_size = fdt_address_cells(fdt, dom_node);
>> +    int size_size = fdt_size_cells(fdt, dom_node);
>> +
>> +    if ( address_size < 0 || size_size < 0 )
>> +    {
>> +        printk("  failed processing #address or #size for domain %s)\n",
>> +               name == NULL ? "unknown" : name);
>> +        return -EINVAL;
>> +    }
>> +
>> +    fdt_for_each_subnode(node, fdt, dom_node)
>> +    {
>> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
>> +        {
>> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
>> +            if ( idx < 0 )
>> +            {
>> +                printk("  failed processing kernel module for domain %s)\n",
>> +                       name == NULL ? "unknown" : name);
>> +                return idx;
>> +            }
>> +
>> +            if ( idx > bi->nr_modules )
>> +            {
>> +                printk("  invalid kernel module index for domain node (%d)\n",
>> +                       bi->nr_domains);
>> +                return -EINVAL;
>> +            }
>> +
>> +            printk("  kernel: boot module %d\n", idx);
>> +            bi->mods[idx].type = BOOTMOD_KERNEL;
>> +            bd->kernel = &bi->mods[idx];
>> +        }
>> +    }
> 
> What if you find two?

No different than if someone accidentally duplicated the module line for 
the kernel in grub.cfg. It's a violation of the boot convention with the 
resulting behavior being indeterminate, which may or may not result in 
failure/panic when the domain attempts to boot. It might be worth adding 
a warning if a duplicate kernel entry is detected. It is possible that 
such a configuration would boot if it was a duplicate paste situation. 
So I would not feel right panicking, when there is a possibility that 
the configuration could boot.

>> --- a/xen/arch/x86/domain_builder/fdt.h
>> +++ b/xen/arch/x86/domain_builder/fdt.h
>> @@ -3,6 +3,7 @@
>>   #define __XEN_X86_FDT_H__
>>   
>>   #include <xen/init.h>
>> +#include <xen/libfdt/libfdt.h>
>>   
>>   #include <asm/bootinfo.h>
>>   
>> @@ -10,6 +11,22 @@
>>   #define HYPERLAUNCH_MODULE_IDX 0
>>   
>>   #ifdef CONFIG_DOMAIN_BUILDER
>> +
>> +static inline int __init fdt_cell_as_u32(const fdt32_t *cell, uint32_t *val)
>> +{
>> +    *val = fdt32_to_cpu(*cell);
>> +
>> +    return 0;
>> +}
>> +
>> +static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
>> +{
>> +    *val = ((uint64_t)fdt32_to_cpu(cell[0]) << 32) |
>> +           (uint64_t)fdt32_to_cpu(cell[1]);
>> +
>> +    return 0;
>> +}
> 
> Basic library routines again?

Same as above.

v/r,
dps

On 11.12.2024 16:41, Daniel P. Smith wrote:
> On 12/2/24 06:53, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
>>> process the reg property for the MB1 module index. If the bootargs property is
>>> present and there was not an MB1 string, then use the command line from the
>>> device tree definition.
>>
>> Why specifically MB1?
> 
> Because Xen converts MB2 into an MB1 chain very early in the entry 
> points that take MB2. By the time HL code is executed, it will only ever 
> see a list of MB1 modules.

Yet that's all Xen's internal representation, which merely is kind of
originating from MB1. That origin is, afaict, irrelevant here, and is
instead, imo, confusing.

>>> --- a/xen/arch/x86/domain_builder/fdt.c
>>> +++ b/xen/arch/x86/domain_builder/fdt.c
>>> @@ -14,6 +14,122 @@
>>>   
>>>   #include "fdt.h"
>>>   
>>> +static inline int __init fdt_get_prop_as_reg(
>>
>> What does "reg" stand for here?
> 
> Device Tree defines a two field "prop-encoded=array" property type 
> called reg.

Okay, this explains where you took it from, yet I remain curious what it
actually stands for. Just from the letters I would derive "register", yet
that seems unlikely here.

>>> +    const void *fdt, int node, const char *name, unsigned int ssize,
>>> +    unsigned int asize, uint64_t *size, uint64_t *addr)
>>> +{
>>> +    int ret;
>>> +    const struct fdt_property *prop;
>>> +    fdt32_t *cell;
>>> +
>>> +    /* FDT spec max size is 4 (128bit int), but largest arch int size is 64 */
>>> +    if ( ssize > 2 || asize > 2 )
>>> +        return -EINVAL;
>>> +
>>> +    prop = fdt_get_property(fdt, node, name, &ret);
>>> +    if ( !prop || ret < sizeof(u32) )
>>> +        return ret < 0 ? ret : -EINVAL;
>>> +
>>> +    /* read address field */
>>> +    cell = (fdt32_t *)prop->data;
>>> +
>>> +    if ( asize == 1 )
>>> +    {
>>> +        uint32_t val;
>>> +        fdt_cell_as_u32(cell, &val);
>>> +        *addr = (uint64_t)val;
>>
>> No need for a cast here nor ...
>>
>>> +    }
>>> +    else
>>> +        fdt_cell_as_u64(cell, addr);
>>> +
>>> +    /* read size field */
>>> +    cell += asize;
>>> +
>>> +    if ( ssize == 1 )
>>> +    {
>>> +        uint32_t val;
>>> +        fdt_cell_as_u32(cell, &val);
>>> +        *size = (uint64_t)val;
>>
>> ... here?
> 
> No the compiler does not need the cast, placed to remind readers what 
> was being done. Can/will drop.
> 
>>> +    }
>>> +    else
>>> +        fdt_cell_as_u64(cell, size);
>>> +
>>> +    return 0;
>>> +}
>>
>> This whole function reads very much like a library one. Does it really need
>> adding here, rather than to the FDT library code we already have? In any
>> event there's nothing x86-specific about it, afaics.
> 
> This is where it gets complicated. Most of the higher order functions 
> exposed by xen/device_tree.h are written to work with FDT indexing 
> structures, referred to as the unflattened tree. Deconflicting the mixed 
> use of FDT and FDT index in device_tree.h is beyond the scope of this 
> series.

Going that far also wasn't my request. Yet it's still library-like code,
which seems odd to introduce as x86-specific when later it may very well
want using by other architectures as well. If the FDT library code isn't
a good place, then put it somewhere under xen/lib/?

>>> +                                  &size, &addr);
>>> +    /* An FDT error value may have been returned, translate to -EINVAL */
>>> +    if ( ret < 0 )
>>> +        return -EINVAL;
>>> +
>>> +    if ( size != 0 )
>>> +        return -EOPNOTSUPP;
>>
>> Not knowing much about DT: What does 0 represent here?
> 
> The libfdt code treats 0 as a valid value, whether zero is a valid value 
> is driven by the contextual usage of the property.

And 0 is the _only_ valid value here? Another comment may be on order, to
at least briefly indicate why this is.

>>> +    if ( addr > MAX_NR_BOOTMODS )
>>> +        return -ERANGE;
>>> +
>>> +    /*
>>> +     * MAX_NR_BOOTMODS cannot exceed the max for MB1, represented by a u32,
>>> +     * thus the cast down to a u32 will be safe due to the prior check.
>>> +     */
>>> +    return (int)addr;
>>
>> Comment and cast contradict one another. DYM u32 (really: uint32_t), or plain
>> int? If you mean to return a plain int (for the sake of the -errno values
>> further up), MAX_NR_BOOTMODS needs to stay below 2**31.
> 
> Good point, we cannot artificially impose 2^31 limit when 2^32 is the 
> legitimate upper bound supported by the MB1 protocol. Even if that value 
> is impractical. At the same time, it is beneficial to be able to 
> communicate failures along with some delineation of the failure. Let me 
> think about this, and in the meantime suggestions are welcomed.

    BUILD_BUG_ON(MAX_NR_BOOTMODS > INT_MAX);

deferring the thinking about the "bigger than this" aspect until a (perhaps
much) later time?

>>> +static int __init process_domain_node(
>>> +    struct boot_info *bi, void *fdt, int dom_node)
>>> +{
>>> +    int node;
>>> +    struct boot_domain *bd = &bi->domains[bi->nr_domains];
>>> +    const char *name = fdt_get_name(fdt, dom_node, NULL);
>>> +    int address_size = fdt_address_cells(fdt, dom_node);
>>> +    int size_size = fdt_size_cells(fdt, dom_node);
>>> +
>>> +    if ( address_size < 0 || size_size < 0 )
>>> +    {
>>> +        printk("  failed processing #address or #size for domain %s)\n",
>>> +               name == NULL ? "unknown" : name);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    fdt_for_each_subnode(node, fdt, dom_node)
>>> +    {
>>> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
>>> +        {
>>> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
>>> +            if ( idx < 0 )
>>> +            {
>>> +                printk("  failed processing kernel module for domain %s)\n",
>>> +                       name == NULL ? "unknown" : name);
>>> +                return idx;
>>> +            }
>>> +
>>> +            if ( idx > bi->nr_modules )
>>> +            {
>>> +                printk("  invalid kernel module index for domain node (%d)\n",
>>> +                       bi->nr_domains);
>>> +                return -EINVAL;
>>> +            }
>>> +
>>> +            printk("  kernel: boot module %d\n", idx);
>>> +            bi->mods[idx].type = BOOTMOD_KERNEL;
>>> +            bd->kernel = &bi->mods[idx];
>>> +        }
>>> +    }
>>
>> What if you find two?
> 
> No different than if someone accidentally duplicated the module line for 
> the kernel in grub.cfg. It's a violation of the boot convention with the 
> resulting behavior being indeterminate, which may or may not result in 
> failure/panic when the domain attempts to boot. It might be worth adding 
> a warning if a duplicate kernel entry is detected. It is possible that 
> such a configuration would boot if it was a duplicate paste situation. 
> So I would not feel right panicking, when there is a possibility that 
> the configuration could boot.

I agree with not panic()ing, and I didn't mean to ask that you do so. A
warning ought to be enough indeed.

Jan

On 2024-11-23 13:20, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,kernel` within a domain node. If found,
> process the reg property for the MB1 module index. If the bootargs property is
> present and there was not an MB1 string, then use the command line from the
> device tree definition.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/domain_builder/core.c
> index a80f3711c306..9335f3a9ebef 100644
> --- a/xen/arch/x86/domain_builder/core.c
> +++ b/xen/arch/x86/domain_builder/core.c
> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>   
>           printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>       }
> +    else
> +    {
> +        int i;
> +
> +        /* Find first unknown boot module to use as Dom0 kernel */
> +        printk("Falling back to using first boot module as dom0\n");
> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> +        bi->mods[i].type = BOOTMOD_KERNEL;
> +        bi->domains[0].kernel = &bi->mods[i];
> +        bi->nr_domains = 1;
> +    }
> +

extra newline.

>   }
>   
>   /*
> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
> index ff1ba58b6907..6bf1c4a297fe 100644
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c

> +static int __init process_domain_node(
> +    struct boot_info *bi, void *fdt, int dom_node)
> +{
> +    int node;
> +    struct boot_domain *bd = &bi->domains[bi->nr_domains];
> +    const char *name = fdt_get_name(fdt, dom_node, NULL);

const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";

to avoid...

> +    int address_size = fdt_address_cells(fdt, dom_node);
> +    int size_size = fdt_size_cells(fdt, dom_node);
> +
> +    if ( address_size < 0 || size_size < 0 )
> +    {
> +        printk("  failed processing #address or #size for domain %s)\n",
> +               name == NULL ? "unknown" : name);

...all this duplication in the following patches.

> +        return -EINVAL;
> +    }
> +
> +    fdt_for_each_subnode(node, fdt, dom_node)
> +    {
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )

I thought you were going to use "module,kernel" and "module,index" as 
u32s for multiboot2?

Regards,
Jason

> +        {
> +            int idx = dom0less_module_node(fdt, node, size_size, address_size);
> +            if ( idx < 0 )
> +            {
> +                printk("  failed processing kernel module for domain %s)\n",
> +                       name == NULL ? "unknown" : name);
> +                return idx;
> +            }
> +
> +            if ( idx > bi->nr_modules )
> +            {
> +                printk("  invalid kernel module index for domain node (%d)\n",
> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }
> +
> +            printk("  kernel: boot module %d\n", idx);
> +            bi->mods[idx].type = BOOTMOD_KERNEL;
> +            bd->kernel = &bi->mods[idx];
> +        }
> +    }
> +
> +    if ( !bd->kernel )
> +    {
> +        printk(XENLOG_ERR "ERR: no kernel assigned to domain\n");
> +        return -EFAULT;
> +    }
> +
> +    return 0;
> +}
> +
>   static int __init find_hyperlaunch_node(void *fdt)
>   {
>       int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");

On 11/25/24 17:54, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> Look for a subnode of type `multiboot,kernel` within a domain node. If 
>> found,
>> process the reg property for the MB1 module index. If the bootargs 
>> property is
>> present and there was not an MB1 string, then use the command line 
>> from the
>> device tree definition.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
>> diff --git a/xen/arch/x86/domain_builder/core.c b/xen/arch/x86/ 
>> domain_builder/core.c
>> index a80f3711c306..9335f3a9ebef 100644
>> --- a/xen/arch/x86/domain_builder/core.c
>> +++ b/xen/arch/x86/domain_builder/core.c
>> @@ -56,6 +56,18 @@ void __init builder_init(struct boot_info *bi)
>>           printk(XENLOG_INFO "  Number of domains: %d\n", bi- 
>> >nr_domains);
>>       }
>> +    else
>> +    {
>> +        int i;
>> +
>> +        /* Find first unknown boot module to use as Dom0 kernel */
>> +        printk("Falling back to using first boot module as dom0\n");
>> +        i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>> +        bi->mods[i].type = BOOTMOD_KERNEL;
>> +        bi->domains[0].kernel = &bi->mods[i];
>> +        bi->nr_domains = 1;
>> +    }
>> +
> 
> extra newline.

ack.

>>   }
>>   /*
>> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/ 
>> domain_builder/fdt.c
>> index ff1ba58b6907..6bf1c4a297fe 100644
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
> 
>> +static int __init process_domain_node(
>> +    struct boot_info *bi, void *fdt, int dom_node)
>> +{
>> +    int node;
>> +    struct boot_domain *bd = &bi->domains[bi->nr_domains];
>> +    const char *name = fdt_get_name(fdt, dom_node, NULL);
> 
> const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
> 
> to avoid...

Sure.

>> +    int address_size = fdt_address_cells(fdt, dom_node);
>> +    int size_size = fdt_size_cells(fdt, dom_node);
>> +
>> +    if ( address_size < 0 || size_size < 0 )
>> +    {
>> +        printk("  failed processing #address or #size for domain %s)\n",
>> +               name == NULL ? "unknown" : name);
> 
> ...all this duplication in the following patches.
> 
>> +        return -EINVAL;
>> +    }
>> +
>> +    fdt_for_each_subnode(node, fdt, dom_node)
>> +    {
>> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") 
>> == 0 )
> 
> I thought you were going to use "module,kernel" and "module,index" as 
> u32s for multiboot2?

Per our discussion, I will update appropriately.

v/r,
dps