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>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1:
- moved low-level fdt handlers to libfdt-xen.h
- coding style changes
- moved default to "unknown" up to a local declaration
- moved the general fdt parsing code out to libfdt
- reworked device tree property parsing for module index
  - reworked parsers to take index as parameter
  - parsers now return success or error value
- added check if kernel was already located, warn and continue
---
 xen/arch/x86/domain-builder/core.c  |  11 +++
 xen/arch/x86/domain-builder/fdt.c   | 118 ++++++++++++++++++++++++++++
 xen/arch/x86/domain-builder/fdt.h   |   3 +
 xen/arch/x86/setup.c                |   5 --
 xen/include/xen/libfdt/libfdt-xen.h |  76 ++++++++++++++++++
 5 files changed, 208 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c
index c50eff34fb68..eda7fa7a8ffa 100644
--- a/xen/arch/x86/domain-builder/core.c
+++ b/xen/arch/x86/domain-builder/core.c
@@ -59,6 +59,17 @@ void __init builder_init(struct boot_info *bi)
 
         printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
     }
+    else
+    {
+        unsigned 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 5793bdc9fd47..bcaee50689a6 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -13,6 +13,114 @@
 
 #include "fdt.h"
 
+static int __init hl_module_index(void *fdt, int node, uint32_t *idx)
+{
+    int ret = 0;
+    const struct fdt_property *prop =
+        fdt_get_property(fdt, node, "module-index", &ret);
+
+    /* FDT error or bad idx pointer, translate to -EINVAL */
+    if ( ret < 0 || idx == NULL )
+        return -EINVAL;
+
+    fdt_cell_as_u32((fdt32_t *)prop->data, idx);
+
+    if ( *idx > MAX_NR_BOOTMODS )
+        return -ERANGE;
+
+    return 0;
+}
+
+static int __init dom0less_module_index(
+    void *fdt, int node, int size_size, int address_size, uint32_t *idx)
+{
+    uint64_t size = ~0UL, addr = ~0UL;
+    int ret =
+        fdt_get_reg_prop(fdt, node, address_size, size_size, &addr, &size, 1);
+
+    /* FDT error or bad idx pointer, translate to -EINVAL */
+    if ( ret < 0 || idx == NULL )
+        return -EINVAL;
+
+    /* Convention is that zero size indicates address is an index */
+    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.
+     */
+    *idx = (uint32_t)addr;
+
+    return 0;
+}
+
+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) ?: "unknown";
+
+    fdt_for_each_subnode(node, fdt, dom_node)
+    {
+        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
+        {
+            unsigned int idx;
+            int ret = 0;
+
+            if ( bd->kernel )
+            {
+                printk(XENLOG_ERR "Duplicate kernel module for domain %s)\n",
+                       name);
+                continue;
+            }
+
+            /* Try hyperlaunch property, fall back to dom0less property. */
+            if ( hl_module_index(fdt, node, &idx) < 0 )
+            {
+                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 )
+                    ret = -EINVAL;
+                else
+                    ret = dom0less_module_index(
+                            fdt, node, size_size, address_size, &idx);
+            }
+
+            if ( ret < 0 )
+            {
+                printk("  failed processing kernel module for domain %s)\n",
+                       name);
+                return ret;
+            }
+
+            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(const void *fdt)
 {
     int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
@@ -74,9 +182,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 f5b89cb54b29..0be4ac771bc4 100644
--- a/xen/arch/x86/domain-builder/fdt.h
+++ b/xen/arch/x86/domain-builder/fdt.h
@@ -3,6 +3,8 @@
 #define __XEN_X86_FDT_H__
 
 #include <xen/init.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/libfdt/libfdt-xen.h>
 
 #include <asm/bootinfo.h>
 
@@ -10,6 +12,7 @@
 #define HYPERLAUNCH_MODULE_IDX 0
 
 #ifdef CONFIG_DOMAIN_BUILDER
+
 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 e1aa9650d22e..71ce9315d3ac 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1288,11 +1288,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 */
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index a5340bc9f4e1..27d23df03af3 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -13,6 +13,82 @@
 
 #include <xen/libfdt/libfdt.h>
 
+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;
+}
+
+/*
+ * Property: reg
+ *
+ * Defined in Section 2.3.6 of the Device Tree Specification is the "reg"
+ * standard property. The property is a prop-encoded-array that is encoded as
+ * an arbitrary number of (address, length) pairs.
+ */
+static inline int __init fdt_get_reg_prop(
+    const void *fdt, int node, unsigned int asize, unsigned int ssize,
+    uint64_t *addr, uint64_t *size, unsigned int pairs)
+{
+    int ret;
+    unsigned int i, count;
+    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, "reg", &ret);
+    if ( !prop || ret < sizeof(u32) )
+        return ret < 0 ? ret : -EINVAL;
+
+    /* Get the number of (addr, size) pairs and clamp down. */
+    count = fdt32_to_cpu(prop->len) / (ssize + asize);
+    count = count < pairs ? count : pairs;
+
+    cell = (fdt32_t *)prop->data;
+
+    for ( i = 0; i < count; i++ )
+    {
+        /* read address field */
+        if ( asize == 1 )
+        {
+            uint32_t val;
+            fdt_cell_as_u32(cell, &val);
+            addr[i] = val;
+        }
+        else
+            fdt_cell_as_u64(cell, &addr[i]);
+
+        /* read size field */
+        cell += asize;
+
+        if ( ssize == 1 )
+        {
+            uint32_t val;
+            fdt_cell_as_u32(cell, &val);
+            size[i] = val;
+        }
+        else
+            fdt_cell_as_u64(cell, &size[i]);
+
+        /* move to next pair */
+        cell += ssize;
+    }
+
+    return count;
+}
+
 static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
                                         paddr_t *address,
                                         paddr_t *size)
-- 
2.30.2

On 26.12.2024 17:57, 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.

While multiboot is apparently the first x86-specific part (as far as Xen goes)
to be put under domain-builder/, I wonder:
- Wouldn't looking for "multiboot,kernel" simply yield nothing on non-x86,
  so having the code under common/ would still be okay?
- What's "multiboot" describing here? The origin of the module? (What other
  origins would then be possible? How would MB1 and MB2 be distinguished?
  What about a native xen.efi boot?) A property of the kernel (when Linux
  doesn't use MB)?

> --- a/xen/arch/x86/domain-builder/core.c
> +++ b/xen/arch/x86/domain-builder/core.c
> @@ -59,6 +59,17 @@ void __init builder_init(struct boot_info *bi)
>  
>          printk(XENLOG_INFO "  Number of domains: %d\n", bi->nr_domains);
>      }
> +    else
> +    {
> +        unsigned int i;
> +
> +        /* Find first unknown boot module to use as Dom0 kernel */
> +        printk("Falling back to using first boot module as dom0\n");

Nit (personal taste?): Why Dom0 in the comment and dom0 in the log
message. I think the former is to be preferred, but at the very least
I see no reason to spell it differently on two adjacent lines.

> +        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;
> +    }

Relating to a question on an earlier patch: The assumption here is
that nothing could have marked another module as BOOTMOD_KERNEL?

> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -13,6 +13,114 @@
>  
>  #include "fdt.h"
>  
> +static int __init hl_module_index(void *fdt, int node, uint32_t *idx)

const void *?

> +{
> +    int ret = 0;
> +    const struct fdt_property *prop =
> +        fdt_get_property(fdt, node, "module-index", &ret);
> +
> +    /* FDT error or bad idx pointer, translate to -EINVAL */
> +    if ( ret < 0 || idx == NULL )

This is a static helper - why check the parameter for being NULL?

> +        return -EINVAL;
> +
> +    fdt_cell_as_u32((fdt32_t *)prop->data, idx);

While I'm aware libfdt has quite a few of such casts, they're problematic.
First and foremost this is a Misra violation, for casting away const-ness.
And then how do you know there are 4 bytes of data to legitimately access?
Hence why such casts would better be avoided altogether (or at least be
suitably abstracted away).

(There's at least one other instance further down.)

> +    if ( *idx > MAX_NR_BOOTMODS )

>= ?

> +        return -ERANGE;
> +
> +    return 0;
> +}
> +
> +static int __init dom0less_module_index(
> +    void *fdt, int node, int size_size, int address_size, uint32_t *idx)
> +{
> +    uint64_t size = ~0UL, addr = ~0UL;
> +    int ret =
> +        fdt_get_reg_prop(fdt, node, address_size, size_size, &addr, &size, 1);

    int ret = fdt_get_reg_prop(
                  fdt, node, address_size, size_size, &addr, &size, 1);

> +    /* FDT error or bad idx pointer, translate to -EINVAL */
> +    if ( ret < 0 || idx == NULL )

See above as to the NULL check.

> +        return -EINVAL;
> +
> +    /* Convention is that zero size indicates address is an index */
> +    if ( size != 0 )
> +        return -EOPNOTSUPP;
> +
> +    if ( addr > MAX_NR_BOOTMODS )

>= again?

> +        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.
> +     */

Instead of (or in addition to) the comment, put in a BUILD_BUG_ON()?

Also please can you avoid using u32 even in comments? It'll only yield
needless grep matches once we go about fully purging it.

> +    *idx = (uint32_t)addr;
> +
> +    return 0;
> +}
> +
> +static int __init process_domain_node(
> +    struct boot_info *bi, void *fdt, int dom_node)

const twice? (I guess I won't mention such any further. I think I
previously asked that you make things as const-correct as possible.)

> +{
> +    int node;
> +    struct boot_domain *bd = &bi->domains[bi->nr_domains];
> +    const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
> +
> +    fdt_for_each_subnode(node, fdt, dom_node)
> +    {
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> +        {
> +            unsigned int idx;
> +            int ret = 0;
> +
> +            if ( bd->kernel )
> +            {
> +                printk(XENLOG_ERR "Duplicate kernel module for domain %s)\n",
> +                       name);

It's XENLOG_ERR here (but a seemingly stray closing parenthesis at the end),
yet ...

> +                continue;
> +            }
> +
> +            /* Try hyperlaunch property, fall back to dom0less property. */
> +            if ( hl_module_index(fdt, node, &idx) < 0 )
> +            {
> +                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 )
> +                    ret = -EINVAL;
> +                else
> +                    ret = dom0less_module_index(
> +                            fdt, node, size_size, address_size, &idx);
> +            }
> +
> +            if ( ret < 0 )
> +            {
> +                printk("  failed processing kernel module for domain %s)\n",

... two blanks (and the same odd parenthesis) here and ...

> +                       name);
> +                return ret;
> +            }
> +
> +            if ( idx > bi->nr_modules )

>= again?

> +            {
> +                printk("  invalid kernel module index for domain node (%d)\n",

... again two blanks here. What's the deal?

> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }
> +
> +            printk("  kernel: boot module %d\n", idx);

This I expect has two leading blanks to somehow align (normal) output.

> +            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;

EFAULT? Maybe ENODATA or some such?

> +    }
> +
> +    return 0;
> +}
> +
>  static int __init find_hyperlaunch_node(const void *fdt)
>  {
>      int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> @@ -74,9 +182,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");

Missing \n. Also would all HL-related diagnostics perhaps better have a
respective prefix (for disambiguation and grep-ability)?

> --- a/xen/arch/x86/domain-builder/fdt.h
> +++ b/xen/arch/x86/domain-builder/fdt.h
> @@ -3,6 +3,8 @@
>  #define __XEN_X86_FDT_H__
>  
>  #include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/libfdt/libfdt-xen.h>
>  
>  #include <asm/bootinfo.h>
>  
> @@ -10,6 +12,7 @@
>  #define HYPERLAUNCH_MODULE_IDX 0
>  
>  #ifdef CONFIG_DOMAIN_BUILDER
> +
>  int has_hyperlaunch_fdt(struct boot_info *bi);
>  int walk_hyperlaunch_fdt(struct boot_info *bi);
>  #else

I can't explain the need for either of these two hunks.

> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -13,6 +13,82 @@
>  
>  #include <xen/libfdt/libfdt.h>
>  
> +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]);

As we try to conserve on the number of casts: There's no need for the
latter one, is there?

I'll leave it to DT folks to confirm (or otherwise) that the cell indexes
are invariant no matter what the endian-ness.

> +    return 0;

What's the point of this return value for both of the functions? Wouldn't
they better return the value if no error can occur anyway? Afaics none of
the callers checks the return value right now.

> +}
> +
> +/*
> + * Property: reg
> + *
> + * Defined in Section 2.3.6 of the Device Tree Specification is the "reg"
> + * standard property. The property is a prop-encoded-array that is encoded as
> + * an arbitrary number of (address, length) pairs.
> + */
> +static inline int __init fdt_get_reg_prop(
> +    const void *fdt, int node, unsigned int asize, unsigned int ssize,
> +    uint64_t *addr, uint64_t *size, unsigned int pairs)
> +{
> +    int ret;
> +    unsigned int i, count;
> +    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;

Hmm, so asize and ssize are already 32-bit granular. Slightly odd.

> +    prop = fdt_get_property(fdt, node, "reg", &ret);
> +    if ( !prop || ret < sizeof(u32) )
> +        return ret < 0 ? ret : -EINVAL;
> +
> +    /* Get the number of (addr, size) pairs and clamp down. */
> +    count = fdt32_to_cpu(prop->len) / (ssize + asize);

What if there's a remainder?

> +    count = count < pairs ? count : pairs;

Use min()?

Jan

On Thu, 30 Jan 2025, Jan Beulich wrote:
> On 26.12.2024 17:57, 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.
> 
> While multiboot is apparently the first x86-specific part (as far as Xen goes)
> to be put under domain-builder/, I wonder:
> - Wouldn't looking for "multiboot,kernel" simply yield nothing on non-x86,
>   so having the code under common/ would still be okay?

One small clarification: multiboot,kernel is actually common between
both ARM and x86. It is "module-index" which is x86-specific and would
"simply yield nothing on non-x86", as you wrote.

I'll let Dan address your point that "having the code under common/
would still be okay".


> - What's "multiboot" describing here? The origin of the module? (What other
>   origins would then be possible? How would MB1 and MB2 be distinguished?
>   What about a native xen.efi boot?) A property of the kernel (when Linux
>   doesn't use MB)?

Each device tree node has a compatible string to qualify what kind of
information the node is describing. The compatible string for device
tree nodes describing a kernel binary or a ramdisk previously loaded
into memory by a bootloader have a "multiboot," prefix. See
docs/misc/arm/device-tree/booting.txt. This is unrelated to the binary
multiboot protocol Grub uses on x86 to boot Xen.

A distinction between MB1 and MB2 is not needed in device tree, that
information is retrieved via the Grub multiboot protocol as usual. The
only thing needed here in device tree is the location of the kernel,
either by RAM address, or by Grub multiboot module index. This last
option (Grub multiboot module index) is the "module-index" property I
mentioned above.

On 30.01.2025 22:14, Stefano Stabellini wrote:
> On Thu, 30 Jan 2025, Jan Beulich wrote:
>> On 26.12.2024 17:57, 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.
>>
>> While multiboot is apparently the first x86-specific part (as far as Xen goes)
>> to be put under domain-builder/, I wonder:
>> - Wouldn't looking for "multiboot,kernel" simply yield nothing on non-x86,
>>   so having the code under common/ would still be okay?
> 
> One small clarification: multiboot,kernel is actually common between
> both ARM and x86. It is "module-index" which is x86-specific and would
> "simply yield nothing on non-x86", as you wrote.
> 
> I'll let Dan address your point that "having the code under common/
> would still be okay".
> 
> 
>> - What's "multiboot" describing here? The origin of the module? (What other
>>   origins would then be possible? How would MB1 and MB2 be distinguished?
>>   What about a native xen.efi boot?) A property of the kernel (when Linux
>>   doesn't use MB)?
> 
> Each device tree node has a compatible string to qualify what kind of
> information the node is describing. The compatible string for device
> tree nodes describing a kernel binary or a ramdisk previously loaded
> into memory by a bootloader have a "multiboot," prefix. See
> docs/misc/arm/device-tree/booting.txt. This is unrelated to the binary
> multiboot protocol Grub uses on x86 to boot Xen.
> 
> A distinction between MB1 and MB2 is not needed in device tree, that
> information is retrieved via the Grub multiboot protocol as usual. The
> only thing needed here in device tree is the location of the kernel,
> either by RAM address, or by Grub multiboot module index. This last
> option (Grub multiboot module index) is the "module-index" property I
> mentioned above.

Hmm, then I'm afraid I can't make sense of the mentioning of MB1 in the
description. Yet that's a point more towards Daniel than you.

Jan

On 2024-12-26 11:57, 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>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> Changes since v1:
> - moved low-level fdt handlers to libfdt-xen.h
> - coding style changes
> - moved default to "unknown" up to a local declaration
> - moved the general fdt parsing code out to libfdt
> - reworked device tree property parsing for module index
>    - reworked parsers to take index as parameter
>    - parsers now return success or error value
> - added check if kernel was already located, warn and continue
> ---

> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
> index 5793bdc9fd47..bcaee50689a6 100644
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -13,6 +13,114 @@
>   
>   #include "fdt.h"
>   
> +static int __init hl_module_index(void *fdt, int node, uint32_t *idx)
> +{
> +    int ret = 0;
> +    const struct fdt_property *prop =
> +        fdt_get_property(fdt, node, "module-index", &ret);
> +
> +    /* FDT error or bad idx pointer, translate to -EINVAL */
> +    if ( ret < 0 || idx == NULL )
> +        return -EINVAL;
> +
> +    fdt_cell_as_u32((fdt32_t *)prop->data, idx);

fdt_prop_as_u32() provides a few more checks and would not require the 
cast here.

> +
> +    if ( *idx > MAX_NR_BOOTMODS )
> +        return -ERANGE;
> +
> +    return 0;
> +}
> +
> +static int __init dom0less_module_index(
> +    void *fdt, int node, int size_size, int address_size, uint32_t *idx)
> +{
> +    uint64_t size = ~0UL, addr = ~0UL;
> +    int ret =
> +        fdt_get_reg_prop(fdt, node, address_size, size_size, &addr, &size, 1);
> +
> +    /* FDT error or bad idx pointer, translate to -EINVAL */
> +    if ( ret < 0 || idx == NULL )
> +        return -EINVAL;
> +
> +    /* Convention is that zero size indicates address is an index */
> +    if ( size != 0 )
> +        return -EOPNOTSUPP;

We wanted reg = <addr size> to be converted into a new boot module.

i.e. if the user is using grub - they should use `module-index` to 
specify binaries.  If the binaries are loaded in memory, they must use 
`reg`.

> +
> +    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.
> +     */
> +    *idx = (uint32_t)addr;
> +
> +    return 0;
> +}
> +
> +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) ?: "unknown";
> +
> +    fdt_for_each_subnode(node, fdt, dom_node)
> +    {
> +        if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
> +        {
> +            unsigned int idx;
> +            int ret = 0;
> +
> +            if ( bd->kernel )
> +            {
> +                printk(XENLOG_ERR "Duplicate kernel module for domain %s)\n",
> +                       name);
> +                continue;
> +            }
> +
> +            /* Try hyperlaunch property, fall back to dom0less property. */
> +            if ( hl_module_index(fdt, node, &idx) < 0 )
> +            {
> +                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 )
> +                    ret = -EINVAL;
> +                else
> +                    ret = dom0less_module_index(
> +                            fdt, node, size_size, address_size, &idx);
> +            }
> +
> +            if ( ret < 0 )
> +            {
> +                printk("  failed processing kernel module for domain %s)\n",
> +                       name);
> +                return ret;
> +            }
> +
> +            if ( idx > bi->nr_modules )
> +            {
> +                printk("  invalid kernel module index for domain node (%d)\n",
> +                       bi->nr_domains);
> +                return -EINVAL;
> +            }

The earlier MAX_NR_BOOTMODS seem superfluous since this is the one that 
matters.

> +
> +            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(const void *fdt)
>   {
>       int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");

> diff --git a/xen/arch/x86/domain-builder/fdt.h b/xen/arch/x86/domain-builder/fdt.h
> index f5b89cb54b29..0be4ac771bc4 100644
> --- a/xen/arch/x86/domain-builder/fdt.h
> +++ b/xen/arch/x86/domain-builder/fdt.h

> @@ -10,6 +12,7 @@
>   #define HYPERLAUNCH_MODULE_IDX 0
>   
>   #ifdef CONFIG_DOMAIN_BUILDER
> +

This newline should move to a more appropriate patch.

>   int has_hyperlaunch_fdt(struct boot_info *bi);
>   int walk_hyperlaunch_fdt(struct boot_info *bi);
>   #else

> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> index a5340bc9f4e1..27d23df03af3 100644
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -13,6 +13,82 @@
>   
>   #include <xen/libfdt/libfdt.h>
>   
> +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]);

A cast isn't needed on cell[1] since it's not shifted.

> +
> +    return 0;
> +}
> +
> +/*
> + * Property: reg
> + *
> + * Defined in Section 2.3.6 of the Device Tree Specification is the "reg"
> + * standard property. The property is a prop-encoded-array that is encoded as
> + * an arbitrary number of (address, length) pairs.
> + */
> +static inline int __init fdt_get_reg_prop(
> +    const void *fdt, int node, unsigned int asize, unsigned int ssize,
> +    uint64_t *addr, uint64_t *size, unsigned int pairs)

Your other function uses address_size and size_size compared to asize 
and ssize here.  While size_size is fun to write, I think addr_cells and 
size_cells are more accurate names.  It's the number of cells to parse 
for the respective values.  device_tree_get_reg() already exists - is 
that not usable?

> +{
> +    int ret;
> +    unsigned int i, count;
> +    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, "reg", &ret);
> +    if ( !prop || ret < sizeof(u32) )
> +        return ret < 0 ? ret : -EINVAL;
> +
> +    /* Get the number of (addr, size) pairs and clamp down. */
> +    count = fdt32_to_cpu(prop->len) / (ssize + asize);

Rounding down seems okay-ish.

> +    count = count < pairs ? count : pairs;

If the caller is asking for "pairs", should it just be an error if there 
aren't enough?

Regards,
Jason