* [PATCH v2 01/15] x86/boot: introduce boot domain
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-30 13:45 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 02/15] x86/boot: introduce domid field to struct boot_domain Daniel P. Smith
` (13 subsequent siblings)
14 siblings, 1 reply; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
To begin moving toward allowing the hypervisor to construct more than one
domain at boot, a container is needed for a domain's build information.
Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial
struct boot_domain that encapsulate the build information for a domain.
Add a kernel and ramdisk boot module reference along with a struct domain
reference to the new struct boot_domain. This allows a struct boot_domain
reference to be the only parameter necessary to pass down through the domain
construction call chain.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since dom0 device tree v1:
- dropped unnecessary forward declarations
- moved pvh_load_kernel() changes forward to this commit
Changes since boot modules v9
- dropped unlikely
Changes since v8:
- code style correction
---
xen/arch/x86/dom0_build.c | 8 +++++---
xen/arch/x86/hvm/dom0_build.c | 23 ++++++++--------------
xen/arch/x86/include/asm/bootdomain.h | 28 +++++++++++++++++++++++++++
xen/arch/x86/include/asm/bootinfo.h | 5 +++++
xen/arch/x86/include/asm/dom0_build.h | 6 +++---
xen/arch/x86/include/asm/setup.h | 4 ++--
xen/arch/x86/pv/dom0_build.c | 24 ++++++++---------------
xen/arch/x86/setup.c | 24 ++++++++++-------------
8 files changed, 69 insertions(+), 53 deletions(-)
create mode 100644 xen/arch/x86/include/asm/bootdomain.h
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index e8f5bf5447bc..c231191faec7 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -13,6 +13,7 @@
#include <xen/softirq.h>
#include <asm/amd.h>
+#include <asm/bootinfo.h>
#include <asm/dom0_build.h>
#include <asm/guest.h>
#include <asm/hpet.h>
@@ -596,9 +597,10 @@ int __init dom0_setup_permissions(struct domain *d)
return rc;
}
-int __init construct_dom0(struct boot_info *bi, struct domain *d)
+int __init construct_dom0(struct boot_domain *bd)
{
int rc;
+ const struct domain *d = bd->d;
/* Sanity! */
BUG_ON(!pv_shim && d->domain_id != 0);
@@ -608,9 +610,9 @@ int __init construct_dom0(struct boot_info *bi, struct domain *d)
process_pending_softirqs();
if ( is_hvm_domain(d) )
- rc = dom0_construct_pvh(bi, d);
+ rc = dom0_construct_pvh(bd);
else if ( is_pv_domain(d) )
- rc = dom0_construct_pv(bi, d);
+ rc = dom0_construct_pv(bd);
else
panic("Cannot construct Dom0. No guest interface available\n");
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index ce5b5c31b318..cbc365d678d2 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address(
}
static int __init pvh_load_kernel(
- struct domain *d, struct boot_module *image, struct boot_module *initrd,
- paddr_t *entry, paddr_t *start_info_addr)
+ struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr)
{
+ struct domain *d = bd->d;
+ struct boot_module *image = bd->kernel;
+ struct boot_module *initrd = bd->ramdisk;
void *image_base = bootstrap_map_bm(image);
void *image_start = image_base + image->headroom;
unsigned long image_len = image->size;
@@ -1301,26 +1303,17 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
}
}
-int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
+int __init dom0_construct_pvh(struct boot_domain *bd)
{
paddr_t entry, start_info;
- struct boot_module *image;
- struct boot_module *initrd = NULL;
- unsigned int idx;
+ struct domain *d = bd->d;
int rc;
printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
- idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
- if ( idx >= bi->nr_modules )
+ if ( bd->kernel == NULL )
panic("Missing kernel boot module for %pd construction\n", d);
- image = &bi->mods[idx];
-
- idx = first_boot_module_index(bi, BOOTMOD_RAMDISK);
- if ( idx < bi->nr_modules )
- initrd = &bi->mods[idx];
-
if ( is_hardware_domain(d) )
{
/*
@@ -1358,7 +1351,7 @@ int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
return rc;
}
- rc = pvh_load_kernel(d, image, initrd, &entry, &start_info);
+ rc = pvh_load_kernel(bd, &entry, &start_info);
if ( rc )
{
printk("Failed to load Dom0 kernel\n");
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
new file mode 100644
index 000000000000..8d0e5c78d426
--- /dev/null
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2024 Apertus Solutions, LLC
+ * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
+ * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
+ */
+
+#ifndef __XEN_X86_BOOTDOMAIN_H__
+#define __XEN_X86_BOOTDOMAIN_H__
+
+struct boot_domain {
+ struct boot_module *kernel;
+ struct boot_module *ramdisk;
+
+ struct domain *d;
+};
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index f8b422913063..9f65e2c8f62d 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -11,10 +11,14 @@
#include <xen/init.h>
#include <xen/multiboot.h>
#include <xen/types.h>
+#include <asm/bootdomain.h>
/* Max number of boot modules a bootloader can provide in addition to Xen */
#define MAX_NR_BOOTMODS 63
+/* Max number of boot domains that Xen can construct */
+#define MAX_NR_BOOTDOMS 1
+
/* Boot module binary type / purpose */
enum bootmod_type {
BOOTMOD_UNKNOWN,
@@ -78,6 +82,7 @@ struct boot_info {
unsigned int nr_modules;
struct boot_module mods[MAX_NR_BOOTMODS + 1];
+ struct boot_domain domains[MAX_NR_BOOTDOMS];
};
/*
diff --git a/xen/arch/x86/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h
index 2d67b17213dc..8c94e87dc576 100644
--- a/xen/arch/x86/include/asm/dom0_build.h
+++ b/xen/arch/x86/include/asm/dom0_build.h
@@ -13,9 +13,9 @@ unsigned long dom0_compute_nr_pages(struct domain *d,
unsigned long initrd_len);
int dom0_setup_permissions(struct domain *d);
-struct boot_info;
-int dom0_construct_pv(struct boot_info *bi, struct domain *d);
-int dom0_construct_pvh(struct boot_info *bi, struct domain *d);
+struct boot_domain;
+int dom0_construct_pv(struct boot_domain *bd);
+int dom0_construct_pvh(struct boot_domain *bd);
unsigned long dom0_paging_pages(const struct domain *d,
unsigned long nr_pages);
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 5c2391a8684b..b517da6144de 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -26,8 +26,8 @@ void subarch_init_memory(void);
void init_IRQ(void);
-struct boot_info;
-int construct_dom0(struct boot_info *bi, struct domain *d);
+struct boot_domain;
+int construct_dom0(struct boot_domain *bd);
void setup_io_bitmap(struct domain *d);
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index f54d1da5c6f4..e0709a1c1a7a 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -355,7 +355,7 @@ static struct page_info * __init alloc_chunk(struct domain *d,
return page;
}
-static int __init dom0_construct(struct boot_info *bi, struct domain *d)
+static int __init dom0_construct(struct boot_domain *bd)
{
unsigned int i;
int rc, order, machine;
@@ -371,14 +371,15 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
struct page_info *page = NULL;
unsigned int flush_flags = 0;
start_info_t *si;
+ struct domain *d = bd->d;
struct vcpu *v = d->vcpu[0];
- struct boot_module *image;
- struct boot_module *initrd = NULL;
+ struct boot_module *image = bd->kernel;
+ struct boot_module *initrd = bd->ramdisk;
void *image_base;
unsigned long image_len;
void *image_start;
- unsigned long initrd_len = 0;
+ unsigned long initrd_len = initrd ? initrd->size : 0;
l4_pgentry_t *l4tab = NULL, *l4start = NULL;
l3_pgentry_t *l3tab = NULL, *l3start = NULL;
@@ -416,22 +417,13 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
printk(XENLOG_INFO "*** Building a PV Dom%d ***\n", d->domain_id);
- i = first_boot_module_index(bi, BOOTMOD_KERNEL);
- if ( i >= bi->nr_modules )
+ if ( !image )
panic("Missing kernel boot module for %pd construction\n", d);
- image = &bi->mods[i];
image_base = bootstrap_map_bm(image);
image_len = image->size;
image_start = image_base + image->headroom;
- i = first_boot_module_index(bi, BOOTMOD_RAMDISK);
- if ( i < bi->nr_modules )
- {
- initrd = &bi->mods[i];
- initrd_len = initrd->size;
- }
-
d->max_pages = ~0U;
if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
@@ -1078,7 +1070,7 @@ out:
return rc;
}
-int __init dom0_construct_pv(struct boot_info *bi, struct domain *d)
+int __init dom0_construct_pv(struct boot_domain *bd)
{
unsigned long cr4 = read_cr4();
int rc;
@@ -1096,7 +1088,7 @@ int __init dom0_construct_pv(struct boot_info *bi, struct domain *d)
write_cr4(cr4 & ~X86_CR4_SMAP);
}
- rc = dom0_construct(bi, d);
+ rc = dom0_construct(bd);
if ( cr4 & X86_CR4_SMAP )
{
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8ebe5a9443f3..7e4529d6bfb2 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -989,16 +989,9 @@ static struct domain *__init create_dom0(struct boot_info *bi)
.misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
},
};
+ struct boot_domain *bd = &bi->domains[0];
struct domain *d;
domid_t domid;
- struct boot_module *image;
- unsigned int idx;
-
- idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
- if ( idx >= bi->nr_modules )
- panic("Missing kernel boot module for building domain\n");
-
- image = &bi->mods[idx];
if ( opt_dom0_pvh )
{
@@ -1025,11 +1018,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
panic("Error creating d%uv0\n", domid);
/* Grab the DOM0 command line. */
- if ( image->cmdline_pa || bi->kextra )
+ if ( bd->kernel->cmdline_pa || bi->kextra )
{
- if ( image->cmdline_pa )
- safe_strcpy(
- cmdline, cmdline_cook(__va(image->cmdline_pa), bi->loader));
+ if ( bd->kernel->cmdline_pa )
+ safe_strcpy(cmdline,
+ cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
if ( bi->kextra )
/* kextra always includes exactly one leading space. */
@@ -1051,10 +1044,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
safe_strcat(cmdline, acpi_param);
}
- image->cmdline_pa = __pa(cmdline);
+ bd->kernel->cmdline_pa = __pa(cmdline);
}
- if ( construct_dom0(bi, d) != 0 )
+ bd->d = d;
+ if ( construct_dom0(bd) != 0 )
panic("Could not construct domain 0\n");
return d;
@@ -1261,6 +1255,7 @@ void asmlinkage __init noreturn __start_xen(void)
/* Dom0 kernel is always first */
bi->mods[0].type = BOOTMOD_KERNEL;
+ bi->domains[0].kernel = &bi->mods[0];
if ( pvh_boot )
{
@@ -2118,6 +2113,7 @@ void asmlinkage __init noreturn __start_xen(void)
if ( initrdidx < MAX_NR_BOOTMODS )
{
bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
+ bi->domains[0].ramdisk = &bi->mods[initrdidx];
if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
printk(XENLOG_WARNING
"Multiple initrd candidates, picking module #%u\n",
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 01/15] x86/boot: introduce boot domain
2024-12-26 16:57 ` [PATCH v2 01/15] x86/boot: introduce boot domain Daniel P. Smith
@ 2025-01-30 13:45 ` Jan Beulich
2025-04-05 0:04 ` Daniel P. Smith
0 siblings, 1 reply; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 13:45 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> @@ -596,9 +597,10 @@ int __init dom0_setup_permissions(struct domain *d)
> return rc;
> }
>
> -int __init construct_dom0(struct boot_info *bi, struct domain *d)
> +int __init construct_dom0(struct boot_domain *bd)
Pointer-to-const? Domain construction should only be consuming data
supplied, I expect.
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/bootdomain.h
Maybe boot-domain.h? Or was that suggested before and discarded for
whatever reason?
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2024 Apertus Solutions, LLC
> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
> + */
> +
> +#ifndef __XEN_X86_BOOTDOMAIN_H__
> +#define __XEN_X86_BOOTDOMAIN_H__
> +
> +struct boot_domain {
> + struct boot_module *kernel;
> + struct boot_module *ramdisk;
"ramdisk" is Linux-centric, I think. Can we name this more generically?
"module" perhaps, despite it then being the same name as we use for the
modules Xen is passed?
Also, are consumers of this struct supposed to be able to modify what
the pointers point to? I'd expect they aren't, in which case const will
want adding here, too.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 01/15] x86/boot: introduce boot domain
2025-01-30 13:45 ` Jan Beulich
@ 2025-04-05 0:04 ` Daniel P. Smith
2025-04-07 7:10 ` Jan Beulich
0 siblings, 1 reply; 56+ messages in thread
From: Daniel P. Smith @ 2025-04-05 0:04 UTC (permalink / raw)
To: Jan Beulich
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 1/30/25 08:45, Jan Beulich wrote:
> On 26.12.2024 17:57, Daniel P. Smith wrote:
>> @@ -596,9 +597,10 @@ int __init dom0_setup_permissions(struct domain *d)
>> return rc;
>> }
>>
>> -int __init construct_dom0(struct boot_info *bi, struct domain *d)
>> +int __init construct_dom0(struct boot_domain *bd)
>
> Pointer-to-const? Domain construction should only be consuming data
> supplied, I expect.
>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>
> Maybe boot-domain.h? Or was that suggested before and discarded for
> whatever reason?
>
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (c) 2024 Apertus Solutions, LLC
>> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
>> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
>> + */
>> +
>> +#ifndef __XEN_X86_BOOTDOMAIN_H__
>> +#define __XEN_X86_BOOTDOMAIN_H__
>> +
>> +struct boot_domain {
>> + struct boot_module *kernel;
>> + struct boot_module *ramdisk;
>
> "ramdisk" is Linux-centric, I think. Can we name this more generically?
> "module" perhaps, despite it then being the same name as we use for the
> modules Xen is passed?
Ramdisk is not a linux-centric, take OpenBSD for example [1]. Calling
the field "module" is a recipe for confusion. Especially considering
that we are more or less providing a lightweight version of the
toolstack interface which use the name ramdisk.
[1] https://openbsd.fandom.com/wiki/Creating_a_custom_OpenBSD_RAM_disk
> Also, are consumers of this struct supposed to be able to modify what
> the pointers point to? I'd expect they aren't, in which case const will
> want adding here, too.
>
> Jan
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 01/15] x86/boot: introduce boot domain
2025-04-05 0:04 ` Daniel P. Smith
@ 2025-04-07 7:10 ` Jan Beulich
2025-04-09 23:55 ` Daniel P. Smith
0 siblings, 1 reply; 56+ messages in thread
From: Jan Beulich @ 2025-04-07 7:10 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 05.04.2025 02:04, Daniel P. Smith wrote:
> On 1/30/25 08:45, Jan Beulich wrote:
>> On 26.12.2024 17:57, Daniel P. Smith wrote:
>>> @@ -596,9 +597,10 @@ int __init dom0_setup_permissions(struct domain *d)
>>> return rc;
>>> }
>>>
>>> -int __init construct_dom0(struct boot_info *bi, struct domain *d)
>>> +int __init construct_dom0(struct boot_domain *bd)
>>
>> Pointer-to-const? Domain construction should only be consuming data
>> supplied, I expect.
>>
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>>
>> Maybe boot-domain.h? Or was that suggested before and discarded for
>> whatever reason?
>>
>>> @@ -0,0 +1,28 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright (c) 2024 Apertus Solutions, LLC
>>> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
>>> + */
>>> +
>>> +#ifndef __XEN_X86_BOOTDOMAIN_H__
>>> +#define __XEN_X86_BOOTDOMAIN_H__
>>> +
>>> +struct boot_domain {
>>> + struct boot_module *kernel;
>>> + struct boot_module *ramdisk;
>>
>> "ramdisk" is Linux-centric, I think. Can we name this more generically?
>> "module" perhaps, despite it then being the same name as we use for the
>> modules Xen is passed?
>
> Ramdisk is not a linux-centric, take OpenBSD for example [1]. Calling
> the field "module" is a recipe for confusion. Especially considering
> that we are more or less providing a lightweight version of the
> toolstack interface which use the name ramdisk.
>
> [1] https://openbsd.fandom.com/wiki/Creating_a_custom_OpenBSD_RAM_disk
Just one other OS also using such a concept doesn't mean much. In fact, "ramdisk"
isn't quite appropriate a term for Linux nowadays anymore anyway. An initrd can
consist of multiple pieces now, not all of which end up taken as "ramdisk". I
wouldn't insist on "module" as a name, but I continue to think "ramdisk" is
inappropriate. The fact that the toolstack uses the term has historical reasons;
it doesn't mean new code in Xen needs to continue to use that term.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 01/15] x86/boot: introduce boot domain
2025-04-07 7:10 ` Jan Beulich
@ 2025-04-09 23:55 ` Daniel P. Smith
2025-04-10 6:37 ` Jan Beulich
0 siblings, 1 reply; 56+ messages in thread
From: Daniel P. Smith @ 2025-04-09 23:55 UTC (permalink / raw)
To: Jan Beulich
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 4/7/25 03:10, Jan Beulich wrote:
> On 05.04.2025 02:04, Daniel P. Smith wrote:
>> On 1/30/25 08:45, Jan Beulich wrote:
>>> On 26.12.2024 17:57, Daniel P. Smith wrote:
>>>> @@ -596,9 +597,10 @@ int __init dom0_setup_permissions(struct domain *d)
>>>> return rc;
>>>> }
>>>>
>>>> -int __init construct_dom0(struct boot_info *bi, struct domain *d)
>>>> +int __init construct_dom0(struct boot_domain *bd)
>>>
>>> Pointer-to-const? Domain construction should only be consuming data
>>> supplied, I expect.
>>>
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>>>
>>> Maybe boot-domain.h? Or was that suggested before and discarded for
>>> whatever reason?
>>>
>>>> @@ -0,0 +1,28 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * Copyright (c) 2024 Apertus Solutions, LLC
>>>> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
>>>> + */
>>>> +
>>>> +#ifndef __XEN_X86_BOOTDOMAIN_H__
>>>> +#define __XEN_X86_BOOTDOMAIN_H__
>>>> +
>>>> +struct boot_domain {
>>>> + struct boot_module *kernel;
>>>> + struct boot_module *ramdisk;
>>>
>>> "ramdisk" is Linux-centric, I think. Can we name this more generically?
>>> "module" perhaps, despite it then being the same name as we use for the
>>> modules Xen is passed?
>>
>> Ramdisk is not a linux-centric, take OpenBSD for example [1]. Calling
>> the field "module" is a recipe for confusion. Especially considering
>> that we are more or less providing a lightweight version of the
>> toolstack interface which use the name ramdisk.
>>
>> [1] https://openbsd.fandom.com/wiki/Creating_a_custom_OpenBSD_RAM_disk
>
> Just one other OS also using such a concept doesn't mean much. In fact, "ramdisk"
> isn't quite appropriate a term for Linux nowadays anymore anyway. An initrd can
> consist of multiple pieces now, not all of which end up taken as "ramdisk". I
> wouldn't insist on "module" as a name, but I continue to think "ramdisk" is
> inappropriate. The fact that the toolstack uses the term has historical reasons;
> it doesn't mean new code in Xen needs to continue to use that term.
That opening response is very disingenuous and goal post moving. Your
initial comment asserted that it is a Linux only concept, and when met
with another example, you now want to just brush it off.
The fact is that the concept of a ramdisk predates Linux by a
considerable amount, 1979 CP/M introduced the concept. Yes, initrd is a
variation of a ramdisk, which is why the field is not called initrd
(despite that term used elsewhere as a variable name). I would also
point out, as you very well know, Linux's multiple module ramdisk is not
supported by Xen today, nor is there any plan to add it.
The fact is that ramdisk **is** a general term for the specific
capability that the primary supported operating system uses, along with
other operating systems *BSD. As a result the concept is all over the
code base and so it is not at all unreasonable to have an explicit
reference reserved for it.
V/r,
DPS
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 01/15] x86/boot: introduce boot domain
2025-04-09 23:55 ` Daniel P. Smith
@ 2025-04-10 6:37 ` Jan Beulich
0 siblings, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-04-10 6:37 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 10.04.2025 01:55, Daniel P. Smith wrote:
> On 4/7/25 03:10, Jan Beulich wrote:
>> On 05.04.2025 02:04, Daniel P. Smith wrote:
>>> On 1/30/25 08:45, Jan Beulich wrote:
>>>> On 26.12.2024 17:57, Daniel P. Smith wrote:
>>>>> @@ -596,9 +597,10 @@ int __init dom0_setup_permissions(struct domain *d)
>>>>> return rc;
>>>>> }
>>>>>
>>>>> -int __init construct_dom0(struct boot_info *bi, struct domain *d)
>>>>> +int __init construct_dom0(struct boot_domain *bd)
>>>>
>>>> Pointer-to-const? Domain construction should only be consuming data
>>>> supplied, I expect.
>>>>
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>>>>
>>>> Maybe boot-domain.h? Or was that suggested before and discarded for
>>>> whatever reason?
>>>>
>>>>> @@ -0,0 +1,28 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + * Copyright (c) 2024 Apertus Solutions, LLC
>>>>> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
>>>>> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
>>>>> + */
>>>>> +
>>>>> +#ifndef __XEN_X86_BOOTDOMAIN_H__
>>>>> +#define __XEN_X86_BOOTDOMAIN_H__
>>>>> +
>>>>> +struct boot_domain {
>>>>> + struct boot_module *kernel;
>>>>> + struct boot_module *ramdisk;
>>>>
>>>> "ramdisk" is Linux-centric, I think. Can we name this more generically?
>>>> "module" perhaps, despite it then being the same name as we use for the
>>>> modules Xen is passed?
>>>
>>> Ramdisk is not a linux-centric, take OpenBSD for example [1]. Calling
>>> the field "module" is a recipe for confusion. Especially considering
>>> that we are more or less providing a lightweight version of the
>>> toolstack interface which use the name ramdisk.
>>>
>>> [1] https://openbsd.fandom.com/wiki/Creating_a_custom_OpenBSD_RAM_disk
>>
>> Just one other OS also using such a concept doesn't mean much. In fact, "ramdisk"
>> isn't quite appropriate a term for Linux nowadays anymore anyway. An initrd can
>> consist of multiple pieces now, not all of which end up taken as "ramdisk". I
>> wouldn't insist on "module" as a name, but I continue to think "ramdisk" is
>> inappropriate. The fact that the toolstack uses the term has historical reasons;
>> it doesn't mean new code in Xen needs to continue to use that term.
>
> That opening response is very disingenuous and goal post moving. Your
> initial comment asserted that it is a Linux only concept, and when met
> with another example, you now want to just brush it off.
Well, not quite. I deliberately said "..., I think" to indicate the my
horizon. For background, I've originally come from the DOS/Windows and
NetWare worlds, where no such concept ever existed (again, to my
necessarily limited knowledge).
> The fact is that the concept of a ramdisk predates Linux by a
> considerable amount, 1979 CP/M introduced the concept. Yes, initrd is a
> variation of a ramdisk, which is why the field is not called initrd
> (despite that term used elsewhere as a variable name). I would also
> point out, as you very well know, Linux's multiple module ramdisk is not
> supported by Xen today, nor is there any plan to add it.
I don't understand what you're alluding to here. Xen doesn't itself need
a ramdisk. Nevertheless to perhaps find microcode to load, it peeks into
the (sole) module provided to the Dom0 kernel.
> The fact is that ramdisk **is** a general term for the specific
> capability that the primary supported operating system uses, along with
> other operating systems *BSD. As a result the concept is all over the
> code base and so it is not at all unreasonable to have an explicit
> reference reserved for it.
Yet then, compared to multiboot, it being just a single module is a
perhaps severe (portability) limitation. And as soon as we talk about
multiple modules, I'm relatively sure you agree that we can't assume
them all to be RAM disk images. See how Xen itself has got Dom0 kernel,
Dom0 initrd, XSM policy, and CPU microcode.
Naming the thing as generically as possible at least clearly indicates
the route to go from 1 to N.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 02/15] x86/boot: introduce domid field to struct boot_domain
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
2024-12-26 16:57 ` [PATCH v2 01/15] x86/boot: introduce boot domain Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-30 13:51 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 03/15] x86/boot: add cmdline " Daniel P. Smith
` (12 subsequent siblings)
14 siblings, 1 reply; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Add a domid field to struct boot_domain to hold the assigned domain id for the
domain. During initialization, ensure all instances of struct boot_domain have
the invalid domid to ensure that the domid must be set either by convention or
configuration.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1 dom0 device tree:
- made .domid part of the static init of xen_boot_info
Changes since v9 boot modules
- missing include for domid_t def
---
xen/arch/x86/include/asm/bootdomain.h | 4 ++++
xen/arch/x86/setup.c | 10 +++++-----
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 8d0e5c78d426..67be575fe781 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -5,10 +5,14 @@
* Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
*/
+#include <public/xen.h>
+
#ifndef __XEN_X86_BOOTDOMAIN_H__
#define __XEN_X86_BOOTDOMAIN_H__
struct boot_domain {
+ domid_t domid;
+
struct boot_module *kernel;
struct boot_module *ramdisk;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 7e4529d6bfb2..27937a7f7aeb 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -292,6 +292,7 @@ static const char *cmdline_cook(const char *p, const char *loader_name);
struct boot_info __initdata xen_boot_info = {
.loader = "unknown",
.cmdline = "",
+ .domains = { {.domid = DOMID_INVALID} },
};
static struct boot_info *__init multiboot_fill_boot_info(
@@ -991,7 +992,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
};
struct boot_domain *bd = &bi->domains[0];
struct domain *d;
- domid_t domid;
if ( opt_dom0_pvh )
{
@@ -1007,15 +1007,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
/* Create initial domain. Not d0 for pvshim. */
- domid = get_initial_domain_id();
- d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
+ bd->domid = get_initial_domain_id();
+ d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
if ( IS_ERR(d) )
- panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
+ panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
init_dom0_cpuid_policy(d);
if ( alloc_dom0_vcpu0(d) == NULL )
- panic("Error creating d%uv0\n", domid);
+ panic("Error creating d%uv0\n", bd->domid);
/* Grab the DOM0 command line. */
if ( bd->kernel->cmdline_pa || bi->kextra )
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 02/15] x86/boot: introduce domid field to struct boot_domain
2024-12-26 16:57 ` [PATCH v2 02/15] x86/boot: introduce domid field to struct boot_domain Daniel P. Smith
@ 2025-01-30 13:51 ` Jan Beulich
0 siblings, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 13:51 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> Add a domid field to struct boot_domain to hold the assigned domain id for the
> domain. During initialization, ensure all instances of struct boot_domain have
> the invalid domid to ensure that the domid must be set either by convention or
> configuration.
I'm still missing justification for the duplication between the struct domain *
that's already in struct boot_domain and this new member. Iirc you responded to
this earlier question of mine, but nothing was put here.
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -292,6 +292,7 @@ static const char *cmdline_cook(const char *p, const char *loader_name);
> struct boot_info __initdata xen_boot_info = {
> .loader = "unknown",
> .cmdline = "",
> + .domains = { {.domid = DOMID_INVALID} },
Please can you fully use designated initializers here, thus also protecting
against MAX_NR_BOOTDOMS increasing without and update being done here (and
the compiler still being happy)?
.domains = { [0 ... MAX_NR_BOOTDOMS - 1] = { .domid = DOMID_INVALID } },
Nit: Note also the blanks I added.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 03/15] x86/boot: add cmdline to struct boot_domain
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
2024-12-26 16:57 ` [PATCH v2 01/15] x86/boot: introduce boot domain Daniel P. Smith
2024-12-26 16:57 ` [PATCH v2 02/15] x86/boot: introduce domid field to struct boot_domain Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-10 19:52 ` Jason Andryuk
2025-01-30 14:15 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 04/15] kconfig: introduce option to independently enable libfdt Daniel P. Smith
` (11 subsequent siblings)
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Add a container for the "cooked" command line for a domain. This provides for
the backing memory to be directly associated with the domain being constructed.
This is done in anticipation that the domain construction path may need to be
invoked multiple times, thus ensuring each instance had a distinct memory
allocation.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since dom0 device tree v1:
- switched over to bd->cmdline in pvh_load_kernel
- moved cmdline processing under if, eliminating goto
- zero-ed cmdline_pa for kernel module after cmdline processing
Changes since v9 boot modules:
- convert pvh_load_kernel to boot domain to directly use cmdline
- adjustments to domain_cmdline_size
- remove ASSERT and return 0 instead
- use strlen() of values instead of hardcoded sizes
- update cmdline parsing check to inspect multiboot string and not just pointer
- add goto to skip cmdline processing if domain_cmdline_size returns 0
- drop updating cmdline_pa with dynamic buffer with change of its last consumer
pvh_load_kernel
Changes since v8:
- switch to a dynamically allocated buffer
- dropped local cmdline var in pv dom0_construct()
Changes since v7:
- updated commit message to expand on intent and purpose
---
xen/arch/x86/hvm/dom0_build.c | 14 ++---
xen/arch/x86/include/asm/bootdomain.h | 2 +
xen/arch/x86/pv/dom0_build.c | 4 +-
xen/arch/x86/setup.c | 78 +++++++++++++++++++--------
4 files changed, 66 insertions(+), 32 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index cbc365d678d2..47bc3e9ce858 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -653,7 +653,6 @@ static int __init pvh_load_kernel(
void *image_start = image_base + image->headroom;
unsigned long image_len = image->size;
unsigned long initrd_len = initrd ? initrd->size : 0;
- const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL;
struct elf_binary elf;
struct elf_dom_parms parms;
paddr_t last_addr;
@@ -717,9 +716,9 @@ static int __init pvh_load_kernel(
(initrd ? ROUNDUP(initrd_len, PAGE_SIZE) +
sizeof(mod)
: 0) +
- (cmdline ? ROUNDUP(strlen(cmdline) + 1,
- elf_64bit(&elf) ? 8 : 4)
- : 0));
+ (bd->cmdline ? ROUNDUP(strlen(bd->cmdline) + 1,
+ elf_64bit(&elf) ? 8 : 4)
+ : 0));
if ( last_addr == INVALID_PADDR )
{
printk("Unable to find a memory region to load initrd and metadata\n");
@@ -759,9 +758,10 @@ static int __init pvh_load_kernel(
/* Free temporary buffers. */
free_boot_modules();
- if ( cmdline != NULL )
+ if ( bd->cmdline != NULL )
{
- rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
+ rc = hvm_copy_to_guest_phys(
+ last_addr, bd->cmdline, strlen(bd->cmdline) + 1, v);
if ( rc )
{
printk("Unable to copy guest command line\n");
@@ -772,7 +772,7 @@ static int __init pvh_load_kernel(
* Round up to 32/64 bits (depending on the guest kernel bitness) so
* the modlist/start_info is aligned.
*/
- last_addr += ROUNDUP(strlen(cmdline) + 1, elf_64bit(&elf) ? 8 : 4);
+ last_addr += ROUNDUP(strlen(bd->cmdline) + 1, elf_64bit(&elf) ? 8 : 4);
}
if ( initrd != NULL )
{
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 67be575fe781..101a0c643d74 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -11,6 +11,8 @@
#define __XEN_X86_BOOTDOMAIN_H__
struct boot_domain {
+ const char *cmdline;
+
domid_t domid;
struct boot_module *kernel;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index e0709a1c1a7a..580f2703a154 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -972,8 +972,8 @@ static int __init dom0_construct(struct boot_domain *bd)
}
memset(si->cmd_line, 0, sizeof(si->cmd_line));
- if ( image->cmdline_pa )
- strlcpy((char *)si->cmd_line, __va(image->cmdline_pa), sizeof(si->cmd_line));
+ if ( bd->cmdline )
+ strlcpy((char *)si->cmd_line, bd->cmdline, sizeof(si->cmd_line));
#ifdef CONFIG_VIDEO
if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 27937a7f7aeb..a61131365477 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -975,10 +975,29 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
return n;
}
-static struct domain *__init create_dom0(struct boot_info *bi)
+static size_t __init domain_cmdline_size(
+ struct boot_info *bi, struct boot_domain *bd)
{
- static char __initdata cmdline[MAX_GUEST_CMDLINE];
+ size_t s = bi->kextra ? strlen(bi->kextra) : 0;
+
+ s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
+
+ if ( s == 0 )
+ return s;
+
+ /*
+ * Certain parameters from the Xen command line may be added to the dom0
+ * command line. Add additional space for the possible cases along with one
+ * extra char to hold \0.
+ */
+ s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;
+ return s;
+}
+
+static struct domain *__init create_dom0(struct boot_info *bi)
+{
+ char *cmdline = NULL;
struct xen_domctl_createdomain dom0_cfg = {
.flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
.max_evtchn_port = -1,
@@ -1018,39 +1037,52 @@ static struct domain *__init create_dom0(struct boot_info *bi)
panic("Error creating d%uv0\n", bd->domid);
/* Grab the DOM0 command line. */
- if ( bd->kernel->cmdline_pa || bi->kextra )
+ if ( (bd->kernel->cmdline_pa &&
+ ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
+ bi->kextra )
{
- if ( bd->kernel->cmdline_pa )
- safe_strcpy(cmdline,
- cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
+ size_t cmdline_size = domain_cmdline_size(bi, bd);
+
+ if ( cmdline_size )
+ {
+ if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
+ panic("Error allocating cmdline buffer for %pd\n", d);
- if ( bi->kextra )
- /* kextra always includes exactly one leading space. */
- safe_strcat(cmdline, bi->kextra);
+ if ( bd->kernel->cmdline_pa )
+ strlcpy(cmdline,
+ cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
+ cmdline_size);
- /* Append any extra parameters. */
- if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
- safe_strcat(cmdline, " noapic");
+ if ( bi->kextra )
+ /* kextra always includes exactly one leading space. */
+ strlcat(cmdline, bi->kextra, cmdline_size);
- if ( (strlen(acpi_param) == 0) && acpi_disabled )
- {
- printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
- safe_strcpy(acpi_param, "off");
- }
+ /* Append any extra parameters. */
+ if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
+ strlcat(cmdline, " noapic", cmdline_size);
- if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
- {
- safe_strcat(cmdline, " acpi=");
- safe_strcat(cmdline, acpi_param);
- }
+ if ( (strlen(acpi_param) == 0) && acpi_disabled )
+ {
+ printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
+ safe_strcpy(acpi_param, "off");
+ }
- bd->kernel->cmdline_pa = __pa(cmdline);
+ if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
+ {
+ strlcat(cmdline, " acpi=", cmdline_size);
+ strlcat(cmdline, acpi_param, cmdline_size);
+ }
+ bd->kernel->cmdline_pa = 0;
+ bd->cmdline = cmdline;
+ }
}
bd->d = d;
if ( construct_dom0(bd) != 0 )
panic("Could not construct domain 0\n");
+ xfree(cmdline);
+
return d;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 03/15] x86/boot: add cmdline to struct boot_domain
2024-12-26 16:57 ` [PATCH v2 03/15] x86/boot: add cmdline " Daniel P. Smith
@ 2025-01-10 19:52 ` Jason Andryuk
2025-01-15 17:22 ` Daniel P. Smith
2025-01-30 14:15 ` Jan Beulich
1 sibling, 1 reply; 56+ messages in thread
From: Jason Andryuk @ 2025-01-10 19:52 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-12-26 11:57, Daniel P. Smith wrote:
> Add a container for the "cooked" command line for a domain. This provides for
> the backing memory to be directly associated with the domain being constructed.
> This is done in anticipation that the domain construction path may need to be
> invoked multiple times, thus ensuring each instance had a distinct memory
> allocation.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> @@ -1018,39 +1037,52 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> panic("Error creating d%uv0\n", bd->domid);
>
> /* Grab the DOM0 command line. */
> - if ( bd->kernel->cmdline_pa || bi->kextra )
> + if ( (bd->kernel->cmdline_pa &&
> + ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
> + bi->kextra )
> {
> - if ( bd->kernel->cmdline_pa )
> - safe_strcpy(cmdline,
> - cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
> + size_t cmdline_size = domain_cmdline_size(bi, bd);
> +
> + if ( cmdline_size )
> + {
> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
> + panic("Error allocating cmdline buffer for %pd\n", d);
I guess I wasn't clear last time. Instead of two levels of indent, I
was thinking at the top level:
/* Grab the DOM0 command line. */
cmdline_size = domain_cmdline_size(bi, bd);
if ( cmdline_size )
{
domain_cmdline_size() checks all the pointers, so this removes
duplication and indent.
The rest looks good.
Regards,
Jason
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 03/15] x86/boot: add cmdline to struct boot_domain
2025-01-10 19:52 ` Jason Andryuk
@ 2025-01-15 17:22 ` Daniel P. Smith
2025-01-15 19:50 ` Jason Andryuk
0 siblings, 1 reply; 56+ messages in thread
From: Daniel P. Smith @ 2025-01-15 17:22 UTC (permalink / raw)
To: Jason Andryuk, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 1/10/25 14:52, Jason Andryuk wrote:
> On 2024-12-26 11:57, Daniel P. Smith wrote:
>> Add a container for the "cooked" command line for a domain. This
>> provides for
>> the backing memory to be directly associated with the domain being
>> constructed.
>> This is done in anticipation that the domain construction path may
>> need to be
>> invoked multiple times, thus ensuring each instance had a distinct memory
>> allocation.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>
>
>> @@ -1018,39 +1037,52 @@ static struct domain *__init
>> create_dom0(struct boot_info *bi)
>> panic("Error creating d%uv0\n", bd->domid);
>> /* Grab the DOM0 command line. */
>> - if ( bd->kernel->cmdline_pa || bi->kextra )
>> + if ( (bd->kernel->cmdline_pa &&
>> + ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
>> + bi->kextra )
>> {
>> - if ( bd->kernel->cmdline_pa )
>> - safe_strcpy(cmdline,
>> - cmdline_cook(__va(bd->kernel->cmdline_pa),
>> bi->loader));
>> + size_t cmdline_size = domain_cmdline_size(bi, bd);
>> +
>> + if ( cmdline_size )
>> + {
>> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
>> + panic("Error allocating cmdline buffer for %pd\n", d);
>
> I guess I wasn't clear last time. Instead of two levels of indent, I
> was thinking at the top level:
>
> /* Grab the DOM0 command line. */
> cmdline_size = domain_cmdline_size(bi, bd);
> if ( cmdline_size )
> {
>
> domain_cmdline_size() checks all the pointers, so this removes
> duplication and indent.
But it is possible for there to be no command line, thus there is a
legitimate case where cmdline_size will be 0. If it is 0, there is no
reason to go through all of this logic.
v/r,
dps
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 03/15] x86/boot: add cmdline to struct boot_domain
2025-01-15 17:22 ` Daniel P. Smith
@ 2025-01-15 19:50 ` Jason Andryuk
0 siblings, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-01-15 19:50 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2025-01-15 12:22, Daniel P. Smith wrote:
> On 1/10/25 14:52, Jason Andryuk wrote:
>> On 2024-12-26 11:57, Daniel P. Smith wrote:
>>> Add a container for the "cooked" command line for a domain. This
>>> provides for
>>> the backing memory to be directly associated with the domain being
>>> constructed.
>>> This is done in anticipation that the domain construction path may
>>> need to be
>>> invoked multiple times, thus ensuring each instance had a distinct
>>> memory
>>> allocation.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>
>>
>>> @@ -1018,39 +1037,52 @@ static struct domain *__init
>>> create_dom0(struct boot_info *bi)
>>> panic("Error creating d%uv0\n", bd->domid);
>>> /* Grab the DOM0 command line. */
>>> - if ( bd->kernel->cmdline_pa || bi->kextra )
>>> + if ( (bd->kernel->cmdline_pa &&
>>> + ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
>>> + bi->kextra )
>>> {
>>> - if ( bd->kernel->cmdline_pa )
>>> - safe_strcpy(cmdline,
>>> - cmdline_cook(__va(bd->kernel->cmdline_pa),
>>> bi->loader));
>>> + size_t cmdline_size = domain_cmdline_size(bi, bd);
>>> +
>>> + if ( cmdline_size )
>>> + {
>>> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
>>> + panic("Error allocating cmdline buffer for %pd\n", d);
>>
>> I guess I wasn't clear last time. Instead of two levels of indent, I
>> was thinking at the top level:
>>
>> /* Grab the DOM0 command line. */
>> cmdline_size = domain_cmdline_size(bi, bd);
>> if ( cmdline_size )
>> {
>>
>> domain_cmdline_size() checks all the pointers, so this removes
>> duplication and indent.
>
> But it is possible for there to be no command line, thus there is a
> legitimate case where cmdline_size will be 0. If it is 0, there is no
> reason to go through all of this logic.
Sure, but domain_cmdline_size() already handles an empty command line
and returns 0 for that. So I think this logic is still skipped in that
case.
Regards,
Jason
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 03/15] x86/boot: add cmdline to struct boot_domain
2024-12-26 16:57 ` [PATCH v2 03/15] x86/boot: add cmdline " Daniel P. Smith
2025-01-10 19:52 ` Jason Andryuk
@ 2025-01-30 14:15 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 14:15 UTC (permalink / raw)
To: Daniel P. Smith, Roger Pau Monné
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> @@ -759,9 +758,10 @@ static int __init pvh_load_kernel(
> /* Free temporary buffers. */
> free_boot_modules();
>
> - if ( cmdline != NULL )
> + if ( bd->cmdline != NULL )
> {
> - rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
> + rc = hvm_copy_to_guest_phys(
> + last_addr, bd->cmdline, strlen(bd->cmdline) + 1, v);
Nit: Indentation. The anchor point for this kind of increased indentation
is the function name being called, so you want to add one more blank. (It
is not N times the usual indentation of 4, until "it looks okay".)
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -11,6 +11,8 @@
> #define __XEN_X86_BOOTDOMAIN_H__
>
> struct boot_domain {
> + const char *cmdline;
> +
> domid_t domid;
>
> struct boot_module *kernel;
I can see why in the earlier patch you added domid at the top. But cmdline?
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -975,10 +975,29 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
> return n;
> }
>
> -static struct domain *__init create_dom0(struct boot_info *bi)
> +static size_t __init domain_cmdline_size(
> + struct boot_info *bi, struct boot_domain *bd)
> {
> - static char __initdata cmdline[MAX_GUEST_CMDLINE];
> + size_t s = bi->kextra ? strlen(bi->kextra) : 0;
> +
> + s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
> +
> + if ( s == 0 )
> + return s;
> +
> + /*
> + * Certain parameters from the Xen command line may be added to the dom0
> + * command line. Add additional space for the possible cases along with one
> + * extra char to hold \0.
> + */
> + s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;
See below; I question this all being necessary for PVH Dom0.
> @@ -1018,39 +1037,52 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> panic("Error creating d%uv0\n", bd->domid);
>
> /* Grab the DOM0 command line. */
> - if ( bd->kernel->cmdline_pa || bi->kextra )
> + if ( (bd->kernel->cmdline_pa &&
> + ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
> + bi->kextra )
> {
> - if ( bd->kernel->cmdline_pa )
> - safe_strcpy(cmdline,
> - cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
> + size_t cmdline_size = domain_cmdline_size(bi, bd);
> +
> + if ( cmdline_size )
> + {
> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
> + panic("Error allocating cmdline buffer for %pd\n", d);
>
> - if ( bi->kextra )
> - /* kextra always includes exactly one leading space. */
> - safe_strcat(cmdline, bi->kextra);
> + if ( bd->kernel->cmdline_pa )
> + strlcpy(cmdline,
> + cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
> + cmdline_size);
>
> - /* Append any extra parameters. */
> - if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> - safe_strcat(cmdline, " noapic");
> + if ( bi->kextra )
> + /* kextra always includes exactly one leading space. */
> + strlcat(cmdline, bi->kextra, cmdline_size);
>
> - if ( (strlen(acpi_param) == 0) && acpi_disabled )
> - {
> - printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> - safe_strcpy(acpi_param, "off");
> - }
> + /* Append any extra parameters. */
> + if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> + strlcat(cmdline, " noapic", cmdline_size);
Roger - this isn't going to work very well with PVH Dom0, is it?
> - if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> - {
> - safe_strcat(cmdline, " acpi=");
> - safe_strcat(cmdline, acpi_param);
> - }
> + if ( (strlen(acpi_param) == 0) && acpi_disabled )
Not sure whether the compiler will do that transformation anyway, but this
check looks odd to me. Why not simply check whether the first char is the
nul one?
> + {
> + printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
> + safe_strcpy(acpi_param, "off");
> + }
Here I'm doubtful, too, when it comes to PVH Dom0. If Xen's even works in
this mode anymore at all, I don't think we can sensibly start a PVH Dom0
then.
(This is leaving aside that all of this is Linux-centric anyway.)
> - bd->kernel->cmdline_pa = __pa(cmdline);
> + if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> + {
> + strlcat(cmdline, " acpi=", cmdline_size);
> + strlcat(cmdline, acpi_param, cmdline_size);
> + }
(This, btw, won't work quite well when acpi= is specified more than once
on the command line.)
> + bd->kernel->cmdline_pa = 0;
> + bd->cmdline = cmdline;
> + }
> }
>
> bd->d = d;
> if ( construct_dom0(bd) != 0 )
> panic("Could not construct domain 0\n");
>
> + xfree(cmdline);
Leaving bd->cmdline dangling?
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 04/15] kconfig: introduce option to independently enable libfdt
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (2 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 03/15] x86/boot: add cmdline " Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-30 14:19 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 05/15] kconfig: introduce domain builder config option Daniel P. Smith
` (10 subsequent siblings)
14 siblings, 1 reply; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini
Currently, the inclusion of libfdt is controlled by the CONFIG_HAS_DEVICE_TREE
kconfig flag. This flag also changes behavior in a few places, such as boot
module processing for XSM. To support the ability to include libfdt without
changing these behaviors, introduce CONFIG_LIB_DEVICE_TREE. The inclusion of
libfdt is then moved under CONFIG_LIB_DEVICE_TREE.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1:
- grammar and spelling fixes to commit message
- corrected indentation to Kconfig format
- relocated LIB_DEVICE_TREE to alphabet ordered location
---
xen/common/Kconfig | 4 ++++
xen/common/Makefile | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6166327f4d14..028ed9c3631e 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -55,6 +55,7 @@ config HAS_COMPAT
config HAS_DEVICE_TREE
bool
+ select LIB_DEVICE_TREE
config HAS_DIT # Data Independent Timing
bool
@@ -89,6 +90,9 @@ config HAS_UBSAN
config HAS_VMAP
bool
+config LIB_DEVICE_TREE
+ bool
+
config MEM_ACCESS_ALWAYS_ON
bool
diff --git a/xen/common/Makefile b/xen/common/Makefile
index cba3b32733ba..3d29aef01155 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -79,7 +79,7 @@ obj-y += sched/
obj-$(CONFIG_UBSAN) += ubsan/
obj-$(CONFIG_NEEDS_LIBELF) += libelf/
-obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/
+obj-$(CONFIG_LIB_DEVICE_TREE) += libfdt/
CONF_FILE := $(if $(patsubst /%,,$(KCONFIG_CONFIG)),$(objtree)/)$(KCONFIG_CONFIG)
$(obj)/config.gz: $(CONF_FILE)
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 04/15] kconfig: introduce option to independently enable libfdt
2024-12-26 16:57 ` [PATCH v2 04/15] kconfig: introduce option to independently enable libfdt Daniel P. Smith
@ 2025-01-30 14:19 ` Jan Beulich
0 siblings, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 14:19 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Roger Pau Monné, Stefano Stabellini, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> Currently, the inclusion of libfdt is controlled by the CONFIG_HAS_DEVICE_TREE
> kconfig flag. This flag also changes behavior in a few places, such as boot
> module processing for XSM. To support the ability to include libfdt without
> changing these behaviors, introduce CONFIG_LIB_DEVICE_TREE. The inclusion of
> libfdt is then moved under CONFIG_LIB_DEVICE_TREE.
Hmm. I'm not a DT maintainer (imo approval here needs to come from one of
its maintainers, despite the files being touched not saying so; I notice
you have the larger Cc list already), but to me the 'f' in libfdt is lost
with CONFIG_LIB_DEVICE_TREE. What's wrong with CONFIG_LIBFDT when that's
the code that you want to cover?
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 05/15] kconfig: introduce domain builder config option
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (3 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 04/15] kconfig: introduce option to independently enable libfdt Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-10 19:55 ` Jason Andryuk
2025-01-30 14:30 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder Daniel P. Smith
` (9 subsequent siblings)
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Hyperlaunch domain builder will be the consolidated boot time domain building
logic framework. Introduces the config option to enable this domain builder to
and turn on the ability to load the domain configuration via a flattened device
tree.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v1:
- fixed Kconfig indentation
- change directory name to use - instead of _
---
xen/arch/x86/Kconfig | 2 ++
xen/arch/x86/domain-builder/Kconfig | 15 +++++++++++++++
2 files changed, 17 insertions(+)
create mode 100644 xen/arch/x86/domain-builder/Kconfig
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 9cdd04721afa..25b9b75423c5 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -383,6 +383,8 @@ config ALTP2M
If unsure, stay with defaults.
+source "arch/x86/domain_builder/Kconfig"
+
endmenu
source "common/Kconfig"
diff --git a/xen/arch/x86/domain-builder/Kconfig b/xen/arch/x86/domain-builder/Kconfig
new file mode 100644
index 000000000000..8ed493c3b545
--- /dev/null
+++ b/xen/arch/x86/domain-builder/Kconfig
@@ -0,0 +1,15 @@
+
+menu "Domain Builder Features"
+
+config DOMAIN_BUILDER
+ bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
+ select LIB_DEVICE_TREE
+ help
+ Enables the domain builder capability to configure boot domain
+ construction using a flattened device tree.
+
+ This feature is currently experimental.
+
+ If unsure, say N.
+
+endmenu
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 05/15] kconfig: introduce domain builder config option
2024-12-26 16:57 ` [PATCH v2 05/15] kconfig: introduce domain builder config option Daniel P. Smith
@ 2025-01-10 19:55 ` Jason Andryuk
2025-01-15 17:24 ` Daniel P. Smith
2025-01-30 14:30 ` Jan Beulich
1 sibling, 1 reply; 56+ messages in thread
From: Jason Andryuk @ 2025-01-10 19:55 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-12-26 11:57, Daniel P. Smith wrote:
> Hyperlaunch domain builder will be the consolidated boot time domain building
> logic framework. Introduces the config option to enable this domain builder to
> and turn on the ability to load the domain configuration via a flattened device
> tree.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 9cdd04721afa..25b9b75423c5 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -383,6 +383,8 @@ config ALTP2M
>
> If unsure, stay with defaults.
>
> +source "arch/x86/domain_builder/Kconfig"
s/_/-/ ?
With that,
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 05/15] kconfig: introduce domain builder config option
2025-01-10 19:55 ` Jason Andryuk
@ 2025-01-15 17:24 ` Daniel P. Smith
0 siblings, 0 replies; 56+ messages in thread
From: Daniel P. Smith @ 2025-01-15 17:24 UTC (permalink / raw)
To: Jason Andryuk, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 1/10/25 14:55, Jason Andryuk wrote:
> On 2024-12-26 11:57, Daniel P. Smith wrote:
>> Hyperlaunch domain builder will be the consolidated boot time domain
>> building
>> logic framework. Introduces the config option to enable this domain
>> builder to
>> and turn on the ability to load the domain configuration via a
>> flattened device
>> tree.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 9cdd04721afa..25b9b75423c5 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -383,6 +383,8 @@ config ALTP2M
>> If unsure, stay with defaults.
>> +source "arch/x86/domain_builder/Kconfig"
>
> s/_/-/ ?
Ack.
> With that,
>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Thanks!
v/r,
dps
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 05/15] kconfig: introduce domain builder config option
2024-12-26 16:57 ` [PATCH v2 05/15] kconfig: introduce domain builder config option Daniel P. Smith
2025-01-10 19:55 ` Jason Andryuk
@ 2025-01-30 14:30 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 14:30 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> Hyperlaunch domain builder will be the consolidated boot time domain building
> logic framework. Introduces the config option to enable this domain builder to
> and turn on the ability to load the domain configuration via a flattened device
> tree.
s/and/eventually/? Else I fear I'm not getting what is being said here. There's
no turning on of anything just yet, afaics.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (4 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 05/15] kconfig: introduce domain builder config option Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-08 21:54 ` Jason Andryuk
2025-01-30 14:52 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree Daniel P. Smith
` (8 subsequent siblings)
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Introduce the domain builder which is capable of consuming a device tree as the
first boot module. If it finds a device tree as the first boot module, it will
set its type to BOOTMOD_FDT. This change only detects the boot module and
continues to boot with slight change to the boot convention that the dom0
kernel is no longer first boot module but is the second.
No functional change intended.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1:
- coding style compliance
- removed unnecessary include of rangeset.h
- fixed a missed change from 0 to i
- add missing const
- remove unnecessary first_boot_module_index() usages
---
xen/arch/x86/Kconfig | 2 +-
xen/arch/x86/Makefile | 2 +
xen/arch/x86/domain-builder/Makefile | 3 ++
xen/arch/x86/domain-builder/core.c | 57 ++++++++++++++++++++++++
xen/arch/x86/domain-builder/fdt.c | 37 +++++++++++++++
xen/arch/x86/domain-builder/fdt.h | 21 +++++++++
xen/arch/x86/include/asm/bootinfo.h | 3 ++
xen/arch/x86/include/asm/domainbuilder.h | 8 ++++
xen/arch/x86/setup.c | 17 ++++---
9 files changed, 143 insertions(+), 7 deletions(-)
create mode 100644 xen/arch/x86/domain-builder/Makefile
create mode 100644 xen/arch/x86/domain-builder/core.c
create mode 100644 xen/arch/x86/domain-builder/fdt.c
create mode 100644 xen/arch/x86/domain-builder/fdt.h
create mode 100644 xen/arch/x86/include/asm/domainbuilder.h
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 25b9b75423c5..50b4697da565 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -383,7 +383,7 @@ config ALTP2M
If unsure, stay with defaults.
-source "arch/x86/domain_builder/Kconfig"
+source "arch/x86/domain-builder/Kconfig"
endmenu
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b35fd5196ce2..73b332dd471b 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -81,6 +81,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
obj-y += sysctl.o
endif
+obj-y += domain-builder/
+
extra-y += asm-macros.i
extra-y += xen.lds
diff --git a/xen/arch/x86/domain-builder/Makefile b/xen/arch/x86/domain-builder/Makefile
new file mode 100644
index 000000000000..309a0c4bdd9e
--- /dev/null
+++ b/xen/arch/x86/domain-builder/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
+obj-y += core.init.o
+
diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c
new file mode 100644
index 000000000000..d6ae94f45c72
--- /dev/null
+++ b/xen/arch/x86/domain-builder/core.c
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024, Apertus Solutions, LLC
+ */
+#include <xen/err.h>
+#include <xen/init.h>
+#include <xen/kconfig.h>
+#include <xen/lib.h>
+
+#include <asm/bootinfo.h>
+
+#include "fdt.h"
+
+void __init builder_init(struct boot_info *bi)
+{
+ if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
+ {
+ int ret;
+
+ switch ( ret = has_hyperlaunch_fdt(bi) )
+ {
+ case 0:
+ printk("Hyperlaunch device tree detected\n");
+ bi->hyperlaunch_enabled = true;
+ bi->mods[0].type = BOOTMOD_FDT;
+ break;
+
+ case -EINVAL:
+ printk("Hyperlaunch device tree was not detected\n");
+ bi->hyperlaunch_enabled = false;
+ break;
+
+ case -ENOENT:
+ case -ENODATA:
+ printk("Device tree found, but not hyperlaunch (%d)\n", ret);
+ bi->hyperlaunch_enabled = false;
+ bi->mods[0].type = BOOTMOD_FDT;
+ break;
+
+ default:
+ printk("Unknown error (%d) occured checking for hyperlaunch device tree\n",
+ ret);
+ bi->hyperlaunch_enabled = false;
+ break;
+ }
+ }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
new file mode 100644
index 000000000000..4a3f80648f86
--- /dev/null
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024, Apertus Solutions, LLC
+ */
+#include <xen/err.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/libfdt/libfdt.h>
+
+#include <asm/bootinfo.h>
+#include <asm/page.h>
+#include <asm/setup.h>
+
+#include "fdt.h"
+
+int __init has_hyperlaunch_fdt(struct boot_info *bi)
+{
+ int ret = 0;
+ const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+ if ( fdt_check_header(fdt) < 0 )
+ ret = -EINVAL;
+
+ bootstrap_unmap();
+
+ return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/domain-builder/fdt.h b/xen/arch/x86/domain-builder/fdt.h
new file mode 100644
index 000000000000..1c1569a9c633
--- /dev/null
+++ b/xen/arch/x86/domain-builder/fdt.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
+#ifndef __XEN_X86_FDT_H__
+#define __XEN_X86_FDT_H__
+
+#include <xen/init.h>
+
+#include <asm/bootinfo.h>
+
+/* hyperlaunch fdt is required to be module 0 */
+#define HYPERLAUNCH_MODULE_IDX 0
+
+#ifdef CONFIG_DOMAIN_BUILDER
+int has_hyperlaunch_fdt(struct boot_info *bi);
+#else
+static inline int __init has_hyperlaunch_fdt(struct boot_info *bi)
+{
+ return -EINVAL;
+}
+#endif
+
+#endif /* __XEN_X86_FDT_H__ */
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 9f65e2c8f62d..208bec90913d 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -27,6 +27,7 @@ enum bootmod_type {
BOOTMOD_RAMDISK,
BOOTMOD_MICROCODE,
BOOTMOD_XSM_POLICY,
+ BOOTMOD_FDT,
};
struct boot_module {
@@ -80,6 +81,8 @@ struct boot_info {
paddr_t memmap_addr;
size_t memmap_length;
+ bool hyperlaunch_enabled;
+
unsigned int nr_modules;
struct boot_module mods[MAX_NR_BOOTMODS + 1];
struct boot_domain domains[MAX_NR_BOOTDOMS];
diff --git a/xen/arch/x86/include/asm/domainbuilder.h b/xen/arch/x86/include/asm/domainbuilder.h
new file mode 100644
index 000000000000..aedc2b49f7c9
--- /dev/null
+++ b/xen/arch/x86/include/asm/domainbuilder.h
@@ -0,0 +1,8 @@
+#ifndef __XEN_X86_DOMBUILDER_H__
+#define __XEN_X86_DOMBUILDER_H__
+
+#include <asm/bootinfo.h>
+
+void builder_init(struct boot_info *bi);
+
+#endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a61131365477..e1aa9650d22e 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -33,6 +33,7 @@
#endif
#include <xen/bitops.h>
#include <asm/bootinfo.h>
+#include <asm/domainbuilder.h>
#include <asm/smp.h>
#include <asm/processor.h>
#include <asm/mpspec.h>
@@ -1285,9 +1286,12 @@ void asmlinkage __init noreturn __start_xen(void)
bi->nr_modules);
}
- /* Dom0 kernel is always first */
- bi->mods[0].type = BOOTMOD_KERNEL;
- bi->domains[0].kernel = &bi->mods[0];
+ 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 )
{
@@ -1470,8 +1474,9 @@ void asmlinkage __init noreturn __start_xen(void)
xen->size = __2M_rwdata_end - _stext;
}
- bi->mods[0].headroom =
- bzimage_headroom(bootstrap_map_bm(&bi->mods[0]), bi->mods[0].size);
+ bi->domains[0].kernel->headroom =
+ bzimage_headroom(bootstrap_map_bm(bi->domains[0].kernel),
+ bi->domains[0].kernel->size);
bootstrap_unmap();
#ifndef highmem_start
@@ -1595,7 +1600,7 @@ void asmlinkage __init noreturn __start_xen(void)
#endif
}
- if ( bi->mods[0].headroom && !bi->mods[0].relocated )
+ if ( bi->domains[0].kernel->headroom && !bi->domains[0].kernel->relocated )
panic("Not enough memory to relocate the dom0 kernel image\n");
for ( i = 0; i < bi->nr_modules; ++i )
{
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder
2024-12-26 16:57 ` [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder Daniel P. Smith
@ 2025-01-08 21:54 ` Jason Andryuk
2025-01-15 17:25 ` Daniel P. Smith
2025-01-30 14:52 ` Jan Beulich
1 sibling, 1 reply; 56+ messages in thread
From: Jason Andryuk @ 2025-01-08 21:54 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-12-26 11:57, Daniel P. Smith wrote:
> Introduce the domain builder which is capable of consuming a device tree as the
> first boot module. If it finds a device tree as the first boot module, it will
> set its type to BOOTMOD_FDT. This change only detects the boot module and
> continues to boot with slight change to the boot convention that the dom0
> kernel is no longer first boot module but is the second.
>
> No functional change intended.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> diff --git a/xen/arch/x86/domain-builder/Makefile b/xen/arch/x86/domain-builder/Makefile
> new file mode 100644
> index 000000000000..309a0c4bdd9e
> --- /dev/null
> +++ b/xen/arch/x86/domain-builder/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
> +obj-y += core.init.o
> +
When I git am-ed this series, git warned:
Applying: x86/hyperlaunch: introduce the domain builder
.git/rebase-apply/patch:59: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
I think that is here.
Regards,
Jason
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder
2025-01-08 21:54 ` Jason Andryuk
@ 2025-01-15 17:25 ` Daniel P. Smith
0 siblings, 0 replies; 56+ messages in thread
From: Daniel P. Smith @ 2025-01-15 17:25 UTC (permalink / raw)
To: Jason Andryuk, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 1/8/25 16:54, Jason Andryuk wrote:
> On 2024-12-26 11:57, Daniel P. Smith wrote:
>> Introduce the domain builder which is capable of consuming a device
>> tree as the
>> first boot module. If it finds a device tree as the first boot module,
>> it will
>> set its type to BOOTMOD_FDT. This change only detects the boot module and
>> continues to boot with slight change to the boot convention that the dom0
>> kernel is no longer first boot module but is the second.
>>
>> No functional change intended.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>
>> diff --git a/xen/arch/x86/domain-builder/Makefile b/xen/arch/x86/
>> domain-builder/Makefile
>> new file mode 100644
>> index 000000000000..309a0c4bdd9e
>> --- /dev/null
>> +++ b/xen/arch/x86/domain-builder/Makefile
>> @@ -0,0 +1,3 @@
>> +obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
>> +obj-y += core.init.o
>> +
>
> When I git am-ed this series, git warned:
> Applying: x86/hyperlaunch: introduce the domain builder
> .git/rebase-apply/patch:59: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.
>
> I think that is here.
Ack.
v/r,
dps
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder
2024-12-26 16:57 ` [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder Daniel P. Smith
2025-01-08 21:54 ` Jason Andryuk
@ 2025-01-30 14:52 ` Jan Beulich
2025-04-01 18:01 ` Jason Andryuk
1 sibling, 1 reply; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 14:52 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -81,6 +81,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
> obj-y += sysctl.o
> endif
>
> +obj-y += domain-builder/
The set of subdirs needed in $(obj-y) is specified at the top of the file.
Also shouldn't this be obj-$(CONFIG_DOMAIN_BUILDER)?
> --- /dev/null
> +++ b/xen/arch/x86/domain-builder/core.c
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024, Apertus Solutions, LLC
> + */
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/kconfig.h>
> +#include <xen/lib.h>
> +
> +#include <asm/bootinfo.h>
> +
> +#include "fdt.h"
> +
> +void __init builder_init(struct boot_info *bi)
> +{
> + if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> + {
> + int ret;
> +
> + switch ( ret = has_hyperlaunch_fdt(bi) )
> + {
> + case 0:
> + printk("Hyperlaunch device tree detected\n");
> + bi->hyperlaunch_enabled = true;
> + bi->mods[0].type = BOOTMOD_FDT;
> + break;
> +
> + case -EINVAL:
> + printk("Hyperlaunch device tree was not detected\n");
> + bi->hyperlaunch_enabled = false;
> + break;
> +
> + case -ENOENT:
> + case -ENODATA:
> + printk("Device tree found, but not hyperlaunch (%d)\n", ret);
> + bi->hyperlaunch_enabled = false;
> + bi->mods[0].type = BOOTMOD_FDT;
> + break;
> +
> + default:
> + printk("Unknown error (%d) occured checking for hyperlaunch device tree\n",
> + ret);
> + bi->hyperlaunch_enabled = false;
> + break;
> + }
> + }
> +}
What is it that's x86-specific in here?
> --- /dev/null
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024, Apertus Solutions, LLC
> + */
> +#include <xen/err.h>
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/libfdt/libfdt.h>
> +
> +#include <asm/bootinfo.h>
> +#include <asm/page.h>
> +#include <asm/setup.h>
> +
> +#include "fdt.h"
> +
> +int __init has_hyperlaunch_fdt(struct boot_info *bi)
Pointer-to-const?
> +{
> + int ret = 0;
> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> + if ( fdt_check_header(fdt) < 0 )
> + ret = -EINVAL;
> +
> + bootstrap_unmap();
> +
> + return ret;
> +}
Is this function intended to later be extended? Aiui anything fitting
the hyperlaunch-agnostic fdt_check_header() will do here, despite the
name of the function.
And again - what is it that's x86-specific in here?
> --- /dev/null
> +++ b/xen/arch/x86/domain-builder/fdt.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
> +#ifndef __XEN_X86_FDT_H__
> +#define __XEN_X86_FDT_H__
> +
> +#include <xen/init.h>
> +
> +#include <asm/bootinfo.h>
This isn't needed here, nor ...
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/domainbuilder.h
> @@ -0,0 +1,8 @@
> +#ifndef __XEN_X86_DOMBUILDER_H__
> +#define __XEN_X86_DOMBUILDER_H__
> +
> +#include <asm/bootinfo.h>
... here, is it? Forward decls of struct boot_info are going to do.
> @@ -1285,9 +1286,12 @@ void asmlinkage __init noreturn __start_xen(void)
> bi->nr_modules);
> }
>
> - /* Dom0 kernel is always first */
> - bi->mods[0].type = BOOTMOD_KERNEL;
> - bi->domains[0].kernel = &bi->mods[0];
> + 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];
This is going to change again later? Or else what about there already
being a module marked BOOTMOD_KERNEL?
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder
2025-01-30 14:52 ` Jan Beulich
@ 2025-04-01 18:01 ` Jason Andryuk
2025-04-02 9:25 ` Jan Beulich
0 siblings, 1 reply; 56+ messages in thread
From: Jason Andryuk @ 2025-04-01 18:01 UTC (permalink / raw)
To: Jan Beulich, Daniel P. Smith
Cc: christopher.w.clark, stefano.stabellini, Andrew Cooper,
Roger Pau Monné, xen-devel, Alejandro.GarciaVallejo
On 2025-01-30 09:52, Jan Beulich wrote:
> On 26.12.2024 17:57, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -81,6 +81,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
>> obj-y += sysctl.o
>> endif
>>
>> +obj-y += domain-builder/
>
> The set of subdirs needed in $(obj-y) is specified at the top of the file.
> Also shouldn't this be obj-$(CONFIG_DOMAIN_BUILDER)?
Later, all boot-time domain building is handled by
domain-builder/core.c. So some of domain-builder/ is always built, and
Kconfig disables multidomain support.
>> --- /dev/null
>> +++ b/xen/arch/x86/domain-builder/core.c
>> @@ -0,0 +1,57 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2024, Apertus Solutions, LLC
>> + */
>> +#include <xen/err.h>
>> +#include <xen/init.h>
>> +#include <xen/kconfig.h>
>> +#include <xen/lib.h>
>> +
>> +#include <asm/bootinfo.h>
>> +
>> +#include "fdt.h"
>> +
>> +void __init builder_init(struct boot_info *bi)
>> +{
>> + if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>> + {
>> + int ret;
>> +
>> + switch ( ret = has_hyperlaunch_fdt(bi) )
>> + {
>> + case 0:
>> + printk("Hyperlaunch device tree detected\n");
>> + bi->hyperlaunch_enabled = true;
>> + bi->mods[0].type = BOOTMOD_FDT;
>> + break;
>> +
>> + case -EINVAL:
>> + printk("Hyperlaunch device tree was not detected\n");
>> + bi->hyperlaunch_enabled = false;
>> + break;
>> +
>> + case -ENOENT:
>> + case -ENODATA:
>> + printk("Device tree found, but not hyperlaunch (%d)\n", ret);
>> + bi->hyperlaunch_enabled = false;
>> + bi->mods[0].type = BOOTMOD_FDT;
>> + break;
>> +
>> + default:
>> + printk("Unknown error (%d) occured checking for hyperlaunch device tree\n",
>> + ret);
>> + bi->hyperlaunch_enabled = false;
>> + break;
>> + }
>> + }
>> +}
>
> What is it that's x86-specific in here?
Would you prefer xen/common/domain-builder ?
>> --- /dev/null
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> +{
>> + int ret = 0;
>> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>> +
>> + if ( fdt_check_header(fdt) < 0 )
>> + ret = -EINVAL;
>> +
>> + bootstrap_unmap();
>> +
>> + return ret;
>> +}
>
> Is this function intended to later be extended? Aiui anything fitting
> the hyperlaunch-agnostic fdt_check_header() will do here, despite the
> name of the function.
Eventually, there will be some checking to ensure that the DT actually
contains hyperlaunch device nodes.
> And again - what is it that's x86-specific in here?
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/domainbuilder.h
>> @@ -0,0 +1,8 @@
>> +#ifndef __XEN_X86_DOMBUILDER_H__
>> +#define __XEN_X86_DOMBUILDER_H__
>> +
>> +#include <asm/bootinfo.h>
>
> ... here, is it? Forward decls of struct boot_info are going to do.
Generally, if you only need a type, just use a forward decl? Use an
include when you need function prototypes?
>> @@ -1285,9 +1286,12 @@ void asmlinkage __init noreturn __start_xen(void)
>> bi->nr_modules);
>> }
>>
>> - /* Dom0 kernel is always first */
>> - bi->mods[0].type = BOOTMOD_KERNEL;
>> - bi->domains[0].kernel = &bi->mods[0];
>> + 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];
>
> This is going to change again later? Or else what about there already
> being a module marked BOOTMOD_KERNEL?
Yes, it will change. There will be two paths, and this is part of the
non-Hyperlaunch path which needs to implicitly select kernel and initrd
from the module order, the same as today. For hyperlaunch, the device
tree explicitly assigns kernel and initrd.
Regards,
Jason
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder
2025-04-01 18:01 ` Jason Andryuk
@ 2025-04-02 9:25 ` Jan Beulich
2025-04-02 19:44 ` Jason Andryuk
0 siblings, 1 reply; 56+ messages in thread
From: Jan Beulich @ 2025-04-02 9:25 UTC (permalink / raw)
To: Jason Andryuk
Cc: christopher.w.clark, stefano.stabellini, Andrew Cooper,
Roger Pau Monné, xen-devel, Alejandro.GarciaVallejo,
Daniel P. Smith
On 01.04.2025 20:01, Jason Andryuk wrote:
> On 2025-01-30 09:52, Jan Beulich wrote:
>> On 26.12.2024 17:57, Daniel P. Smith wrote:
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -81,6 +81,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
>>> obj-y += sysctl.o
>>> endif
>>>
>>> +obj-y += domain-builder/
>>
>> The set of subdirs needed in $(obj-y) is specified at the top of the file.
>> Also shouldn't this be obj-$(CONFIG_DOMAIN_BUILDER)?
>
> Later, all boot-time domain building is handled by
> domain-builder/core.c. So some of domain-builder/ is always built, and
> Kconfig disables multidomain support.
Then CONFIG_DOMAIN_BUILDER is a misnomer?
>>> --- /dev/null
>>> +++ b/xen/arch/x86/domain-builder/core.c
>>> @@ -0,0 +1,57 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (C) 2024, Apertus Solutions, LLC
>>> + */
>>> +#include <xen/err.h>
>>> +#include <xen/init.h>
>>> +#include <xen/kconfig.h>
>>> +#include <xen/lib.h>
>>> +
>>> +#include <asm/bootinfo.h>
>>> +
>>> +#include "fdt.h"
>>> +
>>> +void __init builder_init(struct boot_info *bi)
>>> +{
>>> + if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>>> + {
>>> + int ret;
>>> +
>>> + switch ( ret = has_hyperlaunch_fdt(bi) )
>>> + {
>>> + case 0:
>>> + printk("Hyperlaunch device tree detected\n");
>>> + bi->hyperlaunch_enabled = true;
>>> + bi->mods[0].type = BOOTMOD_FDT;
>>> + break;
>>> +
>>> + case -EINVAL:
>>> + printk("Hyperlaunch device tree was not detected\n");
>>> + bi->hyperlaunch_enabled = false;
>>> + break;
>>> +
>>> + case -ENOENT:
>>> + case -ENODATA:
>>> + printk("Device tree found, but not hyperlaunch (%d)\n", ret);
>>> + bi->hyperlaunch_enabled = false;
>>> + bi->mods[0].type = BOOTMOD_FDT;
>>> + break;
>>> +
>>> + default:
>>> + printk("Unknown error (%d) occured checking for hyperlaunch device tree\n",
>>> + ret);
>>> + bi->hyperlaunch_enabled = false;
>>> + break;
>>> + }
>>> + }
>>> +}
>>
>> What is it that's x86-specific in here?
>
> Would you prefer xen/common/domain-builder ?
Whatever isn't arch-specific would better live somewhere under xen/common/, yes.
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/domainbuilder.h
>>> @@ -0,0 +1,8 @@
>>> +#ifndef __XEN_X86_DOMBUILDER_H__
>>> +#define __XEN_X86_DOMBUILDER_H__
>>> +
>>> +#include <asm/bootinfo.h>
>>
>> ... here, is it? Forward decls of struct boot_info are going to do.
>
> Generally, if you only need a type, just use a forward decl? Use an
> include when you need function prototypes?
Yes. The latter also when you need struct/union fields or sizes.
>>> @@ -1285,9 +1286,12 @@ void asmlinkage __init noreturn __start_xen(void)
>>> bi->nr_modules);
>>> }
>>>
>>> - /* Dom0 kernel is always first */
>>> - bi->mods[0].type = BOOTMOD_KERNEL;
>>> - bi->domains[0].kernel = &bi->mods[0];
>>> + 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];
>>
>> This is going to change again later? Or else what about there already
>> being a module marked BOOTMOD_KERNEL?
>
> Yes, it will change. There will be two paths, and this is part of the
> non-Hyperlaunch path which needs to implicitly select kernel and initrd
> from the module order, the same as today. For hyperlaunch, the device
> tree explicitly assigns kernel and initrd.
Here and elsewhere, for things that aren't quite right yet, mentioning
such aspects specially in the description (or in certain cases even in
FIXME comments) would help avoid questions like the one I had raised.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder
2025-04-02 9:25 ` Jan Beulich
@ 2025-04-02 19:44 ` Jason Andryuk
0 siblings, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-04-02 19:44 UTC (permalink / raw)
To: Jan Beulich
Cc: christopher.w.clark, stefano.stabellini, Andrew Cooper,
Roger Pau Monné, xen-devel, Alejandro.GarciaVallejo,
Daniel P. Smith
On 2025-04-02 05:25, Jan Beulich wrote:
> On 01.04.2025 20:01, Jason Andryuk wrote:
>> On 2025-01-30 09:52, Jan Beulich wrote:
>>> On 26.12.2024 17:57, Daniel P. Smith wrote:
>>>> --- a/xen/arch/x86/Makefile
>>>> +++ b/xen/arch/x86/Makefile
>>>> @@ -81,6 +81,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
>>>> obj-y += sysctl.o
>>>> endif
>>>>
>>>> +obj-y += domain-builder/
>>>
>>> The set of subdirs needed in $(obj-y) is specified at the top of the file.
>>> Also shouldn't this be obj-$(CONFIG_DOMAIN_BUILDER)?
>>
>> Later, all boot-time domain building is handled by
>> domain-builder/core.c. So some of domain-builder/ is always built, and
>> Kconfig disables multidomain support.
>
> Then CONFIG_DOMAIN_BUILDER is a misnomer?
Dan also adds CONFIG_MULTIDOMAIN_BUILDER later... which is used only for:
- BUG_ON(!pv_shim && d->domain_id != 0);
+ if ( ! IS_ENABLED(CONFIG_MULTIDOMAIN_BUILDER) )
+ BUG_ON(!pv_shim && d->domain_id != 0);
:(
I thought it at least restricted the size of the boot domains array.
-Jason
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (5 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 06/15] x86/hyperlaunch: introduce the domain builder Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-10 22:20 ` Jason Andryuk
2025-01-30 15:01 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch Daniel P. Smith
` (7 subsequent siblings)
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
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@apertussolutions.com>
---
Changes since v1:
- corrected typo in error message
- clarified commit message on "reporting"
- added missing inline decl
- code style
---
xen/arch/x86/domain-builder/core.c | 15 +++++++
xen/arch/x86/domain-builder/fdt.c | 65 ++++++++++++++++++++++++++++-
xen/arch/x86/domain-builder/fdt.h | 5 +++
xen/arch/x86/include/asm/bootinfo.h | 1 +
4 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c
index d6ae94f45c72..c50eff34fb68 100644
--- a/xen/arch/x86/domain-builder/core.c
+++ b/xen/arch/x86/domain-builder/core.c
@@ -44,6 +44,21 @@ void __init builder_init(struct boot_info *bi)
break;
}
}
+
+ if ( bi->hyperlaunch_enabled )
+ {
+ int ret;
+
+ printk(XENLOG_INFO "Hyperlaunch configuration:\n");
+ if ( (ret = walk_hyperlaunch_fdt(bi)) < 0 )
+ {
+ printk(XENLOG_INFO " walk of device tree failed (%d)\n", ret);
+ bi->hyperlaunch_enabled = false;
+ return;
+ }
+
+ printk(XENLOG_INFO " Number of domains: %d\n", bi->nr_domains);
+ }
}
/*
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index 4a3f80648f86..5793bdc9fd47 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -13,14 +13,77 @@
#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;
+ else
+ return hv_node;
+ }
+ else
+ {
+ /* Lood for dom0less config */
+ int node, chosen_node = fdt_path_offset(fdt, "/chosen");
+ if ( chosen_node < 0 )
+ return -ENOENT;
+
+ fdt_for_each_subnode(node, fdt, chosen_node)
+ {
+ if ( fdt_node_check_compatible(fdt, node, "xen,domain") == 0 )
+ return chosen_node;
+ }
+ }
+
+ return -ENODATA;
+}
+
int __init has_hyperlaunch_fdt(struct boot_info *bi)
{
int ret = 0;
const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
- if ( fdt_check_header(fdt) < 0 )
+ if ( !fdt || fdt_check_header(fdt) < 0 )
ret = -EINVAL;
+ else
+ ret = find_hyperlaunch_node(fdt);
+
+ bootstrap_unmap();
+
+ return ret < 0 ? ret : 0;
+}
+
+int __init walk_hyperlaunch_fdt(struct boot_info *bi)
+{
+ int ret = 0, hv_node, node;
+ void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+ if ( unlikely(!fdt) )
+ return -EINVAL;
+
+ hv_node = find_hyperlaunch_node(fdt);
+ if ( hv_node < 0 )
+ {
+ ret = hv_node;
+ goto err_out;
+ }
+
+ fdt_for_each_subnode(node, fdt, hv_node)
+ {
+ ret = fdt_node_check_compatible(fdt, node, "xen,domain");
+ if ( ret == 0 )
+ bi->nr_domains++;
+ }
+
+ /* Until multi-domain construction is added, throw an error */
+ if ( !bi->nr_domains || bi->nr_domains > 1 )
+ printk(XENLOG_ERR "Hyperlaunch only supports dom0 construction\n");
+ err_out:
bootstrap_unmap();
return ret;
diff --git a/xen/arch/x86/domain-builder/fdt.h b/xen/arch/x86/domain-builder/fdt.h
index 1c1569a9c633..f5b89cb54b29 100644
--- a/xen/arch/x86/domain-builder/fdt.h
+++ b/xen/arch/x86/domain-builder/fdt.h
@@ -11,11 +11,16 @@
#ifdef CONFIG_DOMAIN_BUILDER
int has_hyperlaunch_fdt(struct boot_info *bi);
+int walk_hyperlaunch_fdt(struct boot_info *bi);
#else
static inline int __init has_hyperlaunch_fdt(struct boot_info *bi)
{
return -EINVAL;
}
+static inline int __init walk_hyperlaunch_fdt(struct boot_info *bi)
+{
+ return -EINVAL;
+}
#endif
#endif /* __XEN_X86_FDT_H__ */
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 208bec90913d..683ca9dbe2e0 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -84,6 +84,7 @@ struct boot_info {
bool hyperlaunch_enabled;
unsigned int nr_modules;
+ unsigned int nr_domains;
struct boot_module mods[MAX_NR_BOOTMODS + 1];
struct boot_domain domains[MAX_NR_BOOTDOMS];
};
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree
2024-12-26 16:57 ` [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree Daniel P. Smith
@ 2025-01-10 22:20 ` Jason Andryuk
2025-01-15 17:47 ` Daniel P. Smith
2025-01-30 15:01 ` Jan Beulich
1 sibling, 1 reply; 56+ messages in thread
From: Jason Andryuk @ 2025-01-10 22:20 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-12-26 11:57, Daniel P. Smith wrote:
> 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@apertussolutions.com>
> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
> index 4a3f80648f86..5793bdc9fd47 100644
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -13,14 +13,77 @@
>
> #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;
> + else
> + return hv_node;
> + }
> + else
> + {
> + /* Lood for dom0less config */
Look
> + int node, chosen_node = fdt_path_offset(fdt, "/chosen");
> + if ( chosen_node < 0 )
> + return -ENOENT;
> +
> + fdt_for_each_subnode(node, fdt, chosen_node)
> + {
> + if ( fdt_node_check_compatible(fdt, node, "xen,domain") == 0 )
> + return chosen_node;
> + }
> + }
> +
> + return -ENODATA;
> +}
> +
> int __init has_hyperlaunch_fdt(struct boot_info *bi)
> {
> int ret = 0;
> const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>
> - if ( fdt_check_header(fdt) < 0 )
> + if ( !fdt || fdt_check_header(fdt) < 0 )
It seems to me the !fdt check should move into the earlier patch. What
do you think?
> ret = -EINVAL;
> + else
> + ret = find_hyperlaunch_node(fdt);
> +
> + bootstrap_unmap();
> +
> + return ret < 0 ? ret : 0;
> +}
> +
> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
> +{
> + int ret = 0, hv_node, node;
> + void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> + if ( unlikely(!fdt) )
> + return -EINVAL;
> +
> + hv_node = find_hyperlaunch_node(fdt);
You call find_hyperlaunch_node() twice. It seems like you can just have
has_hyperlaunch_fdt() return the node and pass it into this function.
Regards,
Jason
> + if ( hv_node < 0 )
> + {
> + ret = hv_node;
> + goto err_out;
> + }
> +
> + fdt_for_each_subnode(node, fdt, hv_node)
> + {
> + ret = fdt_node_check_compatible(fdt, node, "xen,domain");
> + if ( ret == 0 )
> + bi->nr_domains++;
> + }
> +
> + /* Until multi-domain construction is added, throw an error */
> + if ( !bi->nr_domains || bi->nr_domains > 1 )
> + printk(XENLOG_ERR "Hyperlaunch only supports dom0 construction\n");
>
> + err_out:
> bootstrap_unmap();
>
> return ret;
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree
2025-01-10 22:20 ` Jason Andryuk
@ 2025-01-15 17:47 ` Daniel P. Smith
0 siblings, 0 replies; 56+ messages in thread
From: Daniel P. Smith @ 2025-01-15 17:47 UTC (permalink / raw)
To: Jason Andryuk, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 1/10/25 17:20, Jason Andryuk wrote:
> On 2024-12-26 11:57, Daniel P. Smith wrote:
>> 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@apertussolutions.com>
>
>> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-
>> builder/fdt.c
>> index 4a3f80648f86..5793bdc9fd47 100644
>> --- a/xen/arch/x86/domain-builder/fdt.c
>> +++ b/xen/arch/x86/domain-builder/fdt.c
>> @@ -13,14 +13,77 @@
>> #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;
>> + else
>> + return hv_node;
>> + }
>> + else
>> + {
>> + /* Lood for dom0less config */
>
> Look
Ack.
>> + int node, chosen_node = fdt_path_offset(fdt, "/chosen");
>> + if ( chosen_node < 0 )
>> + return -ENOENT;
>> +
>> + fdt_for_each_subnode(node, fdt, chosen_node)
>> + {
>> + if ( fdt_node_check_compatible(fdt, node, "xen,domain")
>> == 0 )
>> + return chosen_node;
>> + }
>> + }
>> +
>> + return -ENODATA;
>> +}
>> +
>> int __init has_hyperlaunch_fdt(struct boot_info *bi)
>> {
>> int ret = 0;
>> const void *fdt = bootstrap_map_bm(&bi-
>> >mods[HYPERLAUNCH_MODULE_IDX]);
>> - if ( fdt_check_header(fdt) < 0 )
>> + if ( !fdt || fdt_check_header(fdt) < 0 )
>
> It seems to me the !fdt check should move into the earlier patch. What
> do you think?
You are correct, the two conditions should have been added together.
>> ret = -EINVAL;
>> + else
>> + ret = find_hyperlaunch_node(fdt);
>> +
>> + bootstrap_unmap();
>> +
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
>> +{
>> + int ret = 0, hv_node, node;
>> + void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>> +
>> + if ( unlikely(!fdt) )
>> + return -EINVAL;
>> +
>> + hv_node = find_hyperlaunch_node(fdt);
>
> You call find_hyperlaunch_node() twice. It seems like you can just have
> has_hyperlaunch_fdt() return the node and pass it into this function.
Can do.
v/r,
dps
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree
2024-12-26 16:57 ` [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree Daniel P. Smith
2025-01-10 22:20 ` Jason Andryuk
@ 2025-01-30 15:01 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 15:01 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -13,14 +13,77 @@
>
> #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;
> + else
> + return hv_node;
> + }
> + else
> + {
> + /* Lood for dom0less config */
> + int node, chosen_node = fdt_path_offset(fdt, "/chosen");
> + if ( chosen_node < 0 )
Nit: Blank line between declaration(s) and statement(s) please. I'd also
question "node" being plain int, if libfdt didn't have it like this.
> + return -ENOENT;
> +
> + fdt_for_each_subnode(node, fdt, chosen_node)
> + {
> + if ( fdt_node_check_compatible(fdt, node, "xen,domain") == 0 )
> + return chosen_node;
> + }
> + }
> +
> + return -ENODATA;
> +}
> +
> int __init has_hyperlaunch_fdt(struct boot_info *bi)
> {
> int ret = 0;
> const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>
> - if ( fdt_check_header(fdt) < 0 )
> + if ( !fdt || fdt_check_header(fdt) < 0 )
> ret = -EINVAL;
> + else
> + ret = find_hyperlaunch_node(fdt);
> +
> + bootstrap_unmap();
> +
> + return ret < 0 ? ret : 0;
> +}
> +
> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
const?
> +{
> + int ret = 0, hv_node, node;
> + void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
const?
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (6 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 07/15] x86/hyperlaunch: initial support for hyperlaunch device tree Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-10 23:06 ` Jason Andryuk
2025-01-30 15:42 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree Daniel P. Smith
` (6 subsequent siblings)
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch
2024-12-26 16:57 ` [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch Daniel P. Smith
@ 2025-01-10 23:06 ` Jason Andryuk
2025-01-30 15:42 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-01-10 23:06 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel
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
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch
2024-12-26 16:57 ` [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch Daniel P. Smith
2025-01-10 23:06 ` Jason Andryuk
@ 2025-01-30 15:42 ` Jan Beulich
2025-01-30 21:14 ` Stefano Stabellini
1 sibling, 1 reply; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 15:42 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, xen-devel
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
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch
2025-01-30 15:42 ` Jan Beulich
@ 2025-01-30 21:14 ` Stefano Stabellini
2025-01-31 6:36 ` Jan Beulich
0 siblings, 1 reply; 56+ messages in thread
From: Stefano Stabellini @ 2025-01-30 21:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Andrew Cooper, Roger Pau Monné,
Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
xen-devel
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.
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch
2025-01-30 21:14 ` Stefano Stabellini
@ 2025-01-31 6:36 ` Jan Beulich
0 siblings, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-31 6:36 UTC (permalink / raw)
To: Stefano Stabellini, Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, Julien Grall,
Bertrand Marquis, Michal Orzel, xen-devel
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (7 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 08/15] x86/hyperlaunch: locate dom0 kernel with hyperlaunch Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-15 16:38 ` Jason Andryuk
2025-01-30 15:58 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch Daniel P. Smith
` (5 subsequent siblings)
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel
If a command line is not provided through the bootloader's mechanism, e.g.
muiltboot module string field, then use one from the device tree if present.
The device tree command line is located in the bootargs property of the
`multiboot,kernel` node.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1:
- moved common fdt functions to libfdt
- rename prop_as_offset to more correct prop_by_offset
---
xen/arch/x86/domain-builder/core.c | 28 ++++++++++++++++++++++++
xen/arch/x86/domain-builder/fdt.c | 10 +++++++++
xen/arch/x86/domain-builder/fdt.h | 24 ++++++++++++++++++++
xen/arch/x86/include/asm/bootinfo.h | 6 +++--
xen/arch/x86/include/asm/domainbuilder.h | 4 ++++
xen/arch/x86/setup.c | 15 +++++++++----
xen/include/xen/libfdt/libfdt-xen.h | 24 ++++++++++++++++++++
7 files changed, 105 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c
index eda7fa7a8ffa..91d1b7367e76 100644
--- a/xen/arch/x86/domain-builder/core.c
+++ b/xen/arch/x86/domain-builder/core.c
@@ -8,9 +8,37 @@
#include <xen/lib.h>
#include <asm/bootinfo.h>
+#include <asm/setup.h>
#include "fdt.h"
+size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset)
+{
+#ifdef CONFIG_DOMAIN_BUILDER
+ const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+ int size = fdt_cmdline_prop_size(fdt, offset);
+
+ bootstrap_unmap();
+ return size < 0 ? 0 : (size_t) size;
+#else
+ return 0;
+#endif
+}
+
+int __init builder_get_cmdline(
+ struct boot_info *bi, int offset, char *cmdline, size_t size)
+{
+#ifdef CONFIG_DOMAIN_BUILDER
+ const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+ int ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
+
+ bootstrap_unmap();
+ return ret;
+#else
+ return 0;
+#endif
+}
+
void __init builder_init(struct boot_info *bi)
{
if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index bcaee50689a6..1094c8dc8838 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -109,6 +109,16 @@ static int __init process_domain_node(
printk(" kernel: boot module %d\n", idx);
bi->mods[idx].type = BOOTMOD_KERNEL;
bd->kernel = &bi->mods[idx];
+
+ /* If bootloader didn't set cmdline, see if FDT provides one. */
+ if ( bd->kernel->cmdline_pa &&
+ !((char *)__va(bd->kernel->cmdline_pa))[0] )
+ {
+ int ret = fdt_get_prop_by_offset(
+ fdt, node, "bootargs", &bd->kernel->cmdline_pa);
+ if ( ret > 0 )
+ bd->kernel->fdt_cmdline = true;
+ }
}
}
diff --git a/xen/arch/x86/domain-builder/fdt.h b/xen/arch/x86/domain-builder/fdt.h
index 0be4ac771bc4..3938b0d2619b 100644
--- a/xen/arch/x86/domain-builder/fdt.h
+++ b/xen/arch/x86/domain-builder/fdt.h
@@ -13,6 +13,30 @@
#ifdef CONFIG_DOMAIN_BUILDER
+static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
+{
+ int ret;
+
+ fdt_get_property_by_offset(fdt, offset, &ret);
+
+ return ret;
+}
+
+static inline int __init fdt_cmdline_prop_copy(
+ const void *fdt, int offset, char *cmdline, size_t size)
+{
+ int ret;
+ const struct fdt_property *prop =
+ fdt_get_property_by_offset(fdt, offset, &ret);
+
+ if ( ret < 0 )
+ return ret;
+
+ ASSERT(size > ret);
+
+ return strlcpy(cmdline, prop->data, ret);
+}
+
int has_hyperlaunch_fdt(struct boot_info *bi);
int walk_hyperlaunch_fdt(struct boot_info *bi);
#else
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 683ca9dbe2e0..433a0e66121b 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -35,11 +35,13 @@ struct boot_module {
/*
* Module State Flags:
- * relocated: indicates module has been relocated in memory.
- * released: indicates module's pages have been freed.
+ * relocated: indicates module has been relocated in memory.
+ * released: indicates module's pages have been freed.
+ * fdt_cmdline: indicates module's cmdline is in the FDT.
*/
bool relocated:1;
bool released:1;
+ bool fdt_cmdline:1;
/*
* A boot module may need decompressing by Xen. Headroom is an estimate of
diff --git a/xen/arch/x86/include/asm/domainbuilder.h b/xen/arch/x86/include/asm/domainbuilder.h
index aedc2b49f7c9..21221d8df8ec 100644
--- a/xen/arch/x86/include/asm/domainbuilder.h
+++ b/xen/arch/x86/include/asm/domainbuilder.h
@@ -3,6 +3,10 @@
#include <asm/bootinfo.h>
+size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset);
+int __init builder_get_cmdline(
+ struct boot_info *bi, int offset, char *cmdline, size_t size);
+
void builder_init(struct boot_info *bi);
#endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 71ce9315d3ac..3dd04b9afca7 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -981,7 +981,10 @@ static size_t __init domain_cmdline_size(
{
size_t s = bi->kextra ? strlen(bi->kextra) : 0;
- s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
+ if ( bd->kernel->fdt_cmdline )
+ s += builder_get_cmdline_size(bi, bd->kernel->cmdline_pa);
+ else
+ s += strlen(__va(bd->kernel->cmdline_pa));
if ( s == 0 )
return s;
@@ -1039,7 +1042,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
/* Grab the DOM0 command line. */
if ( (bd->kernel->cmdline_pa &&
- ((char *)__va(bd->kernel->cmdline_pa))[0]) ||
+ (bd->kernel->fdt_cmdline ||
+ ((char *)__va(bd->kernel->cmdline_pa))[0])) ||
bi->kextra )
{
size_t cmdline_size = domain_cmdline_size(bi, bd);
@@ -1049,9 +1053,12 @@ static struct domain *__init create_dom0(struct boot_info *bi)
if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
panic("Error allocating cmdline buffer for %pd\n", d);
- if ( bd->kernel->cmdline_pa )
+ if ( bd->kernel->fdt_cmdline )
+ builder_get_cmdline(
+ bi, bd->kernel->cmdline_pa, cmdline, cmdline_size);
+ else
strlcpy(cmdline,
- cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
+ cmdline_cook(__va(bd->kernel->cmdline_pa),bi->loader),
cmdline_size);
if ( bi->kextra )
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index 27d23df03af3..0e54aeeb6cc2 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -28,6 +28,30 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
return 0;
}
+static inline int __init fdt_get_prop_by_offset(
+ const void *fdt, int node, const char *name, unsigned long *offset)
+{
+ int ret, poffset;
+ const char *pname;
+ size_t nsize = strlen(name);
+
+ fdt_for_each_property_offset(poffset, fdt, node)
+ {
+ fdt_getprop_by_offset(fdt, poffset, &pname, &ret);
+
+ if ( ret < 0 || strlen(pname) != nsize )
+ continue;
+
+ if ( !strncmp(pname, name, nsize) )
+ {
+ *offset = poffset;
+ return nsize;
+ }
+ }
+
+ return -ENOENT;
+}
+
/*
* Property: reg
*
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree
2024-12-26 16:57 ` [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree Daniel P. Smith
@ 2025-01-15 16:38 ` Jason Andryuk
2025-01-30 15:58 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-01-15 16:38 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel
On 2024-12-26 11:57, Daniel P. Smith wrote:
> If a command line is not provided through the bootloader's mechanism, e.g.
> muiltboot module string field, then use one from the device tree if present.
> The device tree command line is located in the bootargs property of the
> `multiboot,kernel` node.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> Changes since v1:
> - moved common fdt functions to libfdt
> - rename prop_as_offset to more correct prop_by_offset
> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> index 27d23df03af3..0e54aeeb6cc2 100644
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -28,6 +28,30 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
> return 0;
> }
>
> +static inline int __init fdt_get_prop_by_offset(
I think fdt_get_prop_offset() is a better name. The point of this
function is to return the offset in the fdt of the named property. "by"
or "as" confuses the purpose, at least to me.
Compare the existing fdt_get_property_by_offset() which is performing a
property looking by consulting the offset.
Regards,
Jason
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree
2024-12-26 16:57 ` [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree Daniel P. Smith
2025-01-15 16:38 ` Jason Andryuk
@ 2025-01-30 15:58 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 15:58 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> If a command line is not provided through the bootloader's mechanism, e.g.
> muiltboot module string field, then use one from the device tree if present.
> The device tree command line is located in the bootargs property of the
> `multiboot,kernel` node.
This reads as if it can be mix-and-match (and the code looks to confirm
this), which doesn't sound quite right. If you deem it right, please add
justification here.
> --- a/xen/arch/x86/domain-builder/core.c
> +++ b/xen/arch/x86/domain-builder/core.c
> @@ -8,9 +8,37 @@
> #include <xen/lib.h>
>
> #include <asm/bootinfo.h>
> +#include <asm/setup.h>
>
> #include "fdt.h"
>
> +size_t __init builder_get_cmdline_size(struct boot_info *bi, int offset)
> +{
> +#ifdef CONFIG_DOMAIN_BUILDER
> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> + int size = fdt_cmdline_prop_size(fdt, offset);
> +
> + bootstrap_unmap();
> + return size < 0 ? 0 : (size_t) size;
Nit: While I wouldn't insist, we don't normally put blanks after casts.
(The cast also isn't really needed here anyway.)
> +#else
> + return 0;
> +#endif
> +}
> +
> +int __init builder_get_cmdline(
> + struct boot_info *bi, int offset, char *cmdline, size_t size)
> +{
> +#ifdef CONFIG_DOMAIN_BUILDER
> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> + int ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
> +
> + bootstrap_unmap();
> + return ret;
> +#else
> + return 0;
> +#endif
> +}
Such #ifdef-ary is better to be avoided by providing stubs in the header.
Which then also works towards not needing to build in domain-builder/ when
COMFIG_DOMAIN_BUILDER=n.
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -109,6 +109,16 @@ static int __init process_domain_node(
> printk(" kernel: boot module %d\n", idx);
> bi->mods[idx].type = BOOTMOD_KERNEL;
> bd->kernel = &bi->mods[idx];
> +
> + /* If bootloader didn't set cmdline, see if FDT provides one. */
> + if ( bd->kernel->cmdline_pa &&
> + !((char *)__va(bd->kernel->cmdline_pa))[0] )
> + {
> + int ret = fdt_get_prop_by_offset(
> + fdt, node, "bootargs", &bd->kernel->cmdline_pa);
> + if ( ret > 0 )
> + bd->kernel->fdt_cmdline = true;
Is there a guarantee that fdt_get_prop_by_offset() won't alter its output
(passed in by indirection) in case of failure? Otherwise ...
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -981,7 +981,10 @@ static size_t __init domain_cmdline_size(
> {
> size_t s = bi->kextra ? strlen(bi->kextra) : 0;
>
> - s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
> + if ( bd->kernel->fdt_cmdline )
> + s += builder_get_cmdline_size(bi, bd->kernel->cmdline_pa);
> + else
> + s += strlen(__va(bd->kernel->cmdline_pa));
... you'll be hosed here (and elsewhere).
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -28,6 +28,30 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
> return 0;
> }
>
> +static inline int __init fdt_get_prop_by_offset(
> + const void *fdt, int node, const char *name, unsigned long *offset)
> +{
> + int ret, poffset;
> + const char *pname;
> + size_t nsize = strlen(name);
> +
> + fdt_for_each_property_offset(poffset, fdt, node)
> + {
> + fdt_getprop_by_offset(fdt, poffset, &pname, &ret);
> +
> + if ( ret < 0 || strlen(pname) != nsize )
> + continue;
> +
> + if ( !strncmp(pname, name, nsize) )
I find this slightly odd: By now we know strlen(name) == strlen(pname) == nsize.
You could then use the usually more efficient memcmp() or the easier to invoke
strcmp().
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (8 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 09/15] x86/hyperlaunch: obtain cmdline from device tree Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-15 16:43 ` Jason Andryuk
2025-01-30 16:39 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 11/15] x86/hyperlaunch: add domain id parsing to domain config Daniel P. Smith
` (4 subsequent siblings)
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Look for a subnode of type `multiboot,ramdisk` within a domain node. If
found, process the reg property for the MB1 module index.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v1:
- switch to nested else/if
- dropped ternary name selection
---
xen/arch/x86/domain-builder/fdt.c | 26 +++++++++++++++++++++++
xen/arch/x86/setup.c | 35 +++++++++++++++++--------------
2 files changed, 45 insertions(+), 16 deletions(-)
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index 1094c8dc8838..27bc37ad45c9 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -119,6 +119,32 @@ static int __init process_domain_node(
if ( ret > 0 )
bd->kernel->fdt_cmdline = true;
}
+
+ continue;
+ }
+ else if (
+ fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
+ {
+ int idx = dom0less_module_node(fdt, node, size_size, address_size);
+ if ( idx < 0 )
+ {
+ printk(" failed processing ramdisk module for domain %s\n",
+ name);
+ return -EINVAL;
+ }
+
+ if ( idx > bi->nr_modules )
+ {
+ printk(" invalid ramdisk module index for domain node (%d)\n",
+ bi->nr_domains);
+ return -EINVAL;
+ }
+
+ printk(" ramdisk: boot module %d\n", idx);
+ bi->mods[idx].type = BOOTMOD_RAMDISK;
+ bd->ramdisk = &bi->mods[idx];
+
+ continue;
}
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 3dd04b9afca7..25ff029ecdda 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1103,7 +1103,7 @@ void asmlinkage __init noreturn __start_xen(void)
char *kextra;
void *bsp_stack;
struct cpu_info *info = get_cpu_info(), *bsp_info;
- unsigned int initrdidx, num_parked = 0;
+ unsigned int num_parked = 0;
struct boot_info *bi;
unsigned long nr_pages, raw_max_page;
int i, j, bytes = 0;
@@ -2141,22 +2141,25 @@ void asmlinkage __init noreturn __start_xen(void)
cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
cpu_has_nx ? "" : "not ");
- /*
- * At this point all capabilities that consume boot modules should have
- * claimed their boot modules. Find the first unclaimed boot module and
- * claim it as the initrd ramdisk. Do a second search to see if there are
- * any remaining unclaimed boot modules, and report them as unusued initrd
- * candidates.
- */
- initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
- if ( initrdidx < MAX_NR_BOOTMODS )
+ if ( !bi->hyperlaunch_enabled )
{
- bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
- bi->domains[0].ramdisk = &bi->mods[initrdidx];
- if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
- printk(XENLOG_WARNING
- "Multiple initrd candidates, picking module #%u\n",
- initrdidx);
+ /*
+ * At this point all capabilities that consume boot modules should have
+ * claimed their boot modules. Find the first unclaimed boot module and
+ * claim it as the initrd ramdisk. Do a second search to see if there are
+ * any remaining unclaimed boot modules, and report them as unusued initrd
+ * candidates.
+ */
+ unsigned int initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
+ if ( initrdidx < MAX_NR_BOOTMODS )
+ {
+ bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
+ bi->domains[0].ramdisk = &bi->mods[initrdidx];
+ if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
+ printk(XENLOG_WARNING
+ "Multiple initrd candidates, picking module #%u\n",
+ initrdidx);
+ }
}
/*
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch
2024-12-26 16:57 ` [PATCH v2 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch Daniel P. Smith
@ 2025-01-15 16:43 ` Jason Andryuk
2025-01-30 16:39 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-01-15 16:43 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-12-26 11:57, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,ramdisk` within a domain node. If
> found, process the reg property for the MB1 module index.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> Changes since v1:
> - switch to nested else/if
> - dropped ternary name selection
> ---
> xen/arch/x86/domain-builder/fdt.c | 26 +++++++++++++++++++++++
> xen/arch/x86/setup.c | 35 +++++++++++++++++--------------
> 2 files changed, 45 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
> index 1094c8dc8838..27bc37ad45c9 100644
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -119,6 +119,32 @@ static int __init process_domain_node(
> if ( ret > 0 )
> bd->kernel->fdt_cmdline = true;
> }
> +
> + continue;
> + }
> + else if (
> + fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
> + {
> + int idx = dom0less_module_node(fdt, node, size_size, address_size);
Your next patch has the hl_module_index() parsing you want moved into
this patch.
Regards,
Jason
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch
2024-12-26 16:57 ` [PATCH v2 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch Daniel P. Smith
2025-01-15 16:43 ` Jason Andryuk
@ 2025-01-30 16:39 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 16:39 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> Look for a subnode of type `multiboot,ramdisk` within a domain node. If
> found, process the reg property for the MB1 module index.
Unlike for cmdline it doesn't look to be mix-and-match here.
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -119,6 +119,32 @@ static int __init process_domain_node(
> if ( ret > 0 )
> bd->kernel->fdt_cmdline = true;
> }
> +
> + continue;
> + }
> + else if (
> + fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
I'm sorry, but this isn't style we use. Perhaps
else if ( fdt_node_check_compatible(
fdt, node, "multiboot,ramdisk") == 0 )
if you dislike
else if ( fdt_node_check_compatible(fdt, node,
"multiboot,ramdisk") == 0 )
> + {
> + int idx = dom0less_module_node(fdt, node, size_size, address_size);
> + if ( idx < 0 )
Nit: Blank line between declaration(s) and statement(s) please. (Again
at least once elsewhere in this patch.)
> + {
> + printk(" failed processing ramdisk module for domain %s\n",
> + name);
> + return -EINVAL;
> + }
> +
> + if ( idx > bi->nr_modules )
> + {
> + printk(" invalid ramdisk module index for domain node (%d)\n",
> + bi->nr_domains);
> + return -EINVAL;
> + }
See comments on similar printk()s in an earlier patch.
> @@ -2141,22 +2141,25 @@ void asmlinkage __init noreturn __start_xen(void)
> cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
> cpu_has_nx ? "" : "not ");
>
> - /*
> - * At this point all capabilities that consume boot modules should have
> - * claimed their boot modules. Find the first unclaimed boot module and
> - * claim it as the initrd ramdisk. Do a second search to see if there are
> - * any remaining unclaimed boot modules, and report them as unusued initrd
> - * candidates.
> - */
> - initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> - if ( initrdidx < MAX_NR_BOOTMODS )
> + if ( !bi->hyperlaunch_enabled )
Can't this be "if ( !bi->hyperlaunch_enabled && initrdidx < MAX_NR_BOOTMODS )"
and then all of the churn here can be avoided? An unnecessary call to
first_boot_module_index() is unlikely to be the end of the world. Otherwise ...
> {
> - bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
> - bi->domains[0].ramdisk = &bi->mods[initrdidx];
> - if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
> - printk(XENLOG_WARNING
> - "Multiple initrd candidates, picking module #%u\n",
> - initrdidx);
> + /*
> + * At this point all capabilities that consume boot modules should have
> + * claimed their boot modules. Find the first unclaimed boot module and
> + * claim it as the initrd ramdisk. Do a second search to see if there are
> + * any remaining unclaimed boot modules, and report them as unusued initrd
> + * candidates.
> + */
> + unsigned int initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> + if ( initrdidx < MAX_NR_BOOTMODS )
> + {
> + bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
> + bi->domains[0].ramdisk = &bi->mods[initrdidx];
> + if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
> + printk(XENLOG_WARNING
> + "Multiple initrd candidates, picking module #%u\n",
> + initrdidx);
> + }
... please pay attention to line length when re-indenting. (If you still need
to re-indent, perhaps also s/unusued/unused/ in the comment, while you touch
it.)
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 11/15] x86/hyperlaunch: add domain id parsing to domain config
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (9 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 10/15] x86/hyperlaunch: locate dom0 initrd with hyperlaunch Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-30 16:49 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 12/15] x86/hyperlaunch: specify dom0 mode with device tree Daniel P. Smith
` (3 subsequent siblings)
14 siblings, 1 reply; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel
Introduce the ability to specify the desired domain id for the domain
definition. The domain id will be populated in the domid property of the domain
node in the device tree configuration.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v1
- coding style changes
- moved comment with code movement
- updated warning message
- unrolled match_fdt_property()
---
xen/arch/x86/domain-builder/fdt.c | 63 ++++++++++++++++++++++++++++-
xen/arch/x86/setup.c | 5 ++-
xen/include/xen/libfdt/libfdt-xen.h | 9 +++++
3 files changed, 73 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index 27bc37ad45c9..efce0927c645 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -8,6 +8,7 @@
#include <xen/libfdt/libfdt.h>
#include <asm/bootinfo.h>
+#include <asm/guest.h>
#include <asm/page.h>
#include <asm/setup.h>
@@ -61,10 +62,40 @@ static int __init dom0less_module_index(
static int __init process_domain_node(
struct boot_info *bi, void *fdt, int dom_node)
{
- int node;
+ int node, property;
struct boot_domain *bd = &bi->domains[bi->nr_domains];
const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
+ fdt_for_each_property_offset(property, fdt, dom_node)
+ {
+ const struct fdt_property *prop;
+ const char *prop_name;
+ int name_len;
+
+ prop = fdt_get_property_by_offset(fdt, property, NULL);
+ if ( !prop )
+ continue; /* silently skip */
+
+ prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len);
+
+ if ( strncmp(prop_name, "domid", name_len) == 0 )
+ {
+ uint32_t val = DOMID_INVALID;
+ if ( fdt_prop_as_u32(prop, &val) != 0 )
+ {
+ printk(" failed processing domain id for domain %s\n", name);
+ return -EINVAL;
+ }
+ if ( val >= DOMID_FIRST_RESERVED )
+ {
+ printk(" invalid domain id for domain %s\n", name);
+ return -EINVAL;
+ }
+ bd->domid = (domid_t)val;
+ printk(" domid: %d\n", bd->domid);
+ }
+ }
+
fdt_for_each_subnode(node, fdt, dom_node)
{
if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 )
@@ -125,7 +156,29 @@ static int __init process_domain_node(
else if (
fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
{
- int idx = dom0less_module_node(fdt, node, size_size, address_size);
+ unsigned int idx;
+ int ret = 0;
+
+ if ( bd->ramdisk )
+ {
+ printk(XENLOG_ERR "Duplicate ramdisk 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 ( idx < 0 )
{
printk(" failed processing ramdisk module for domain %s\n",
@@ -154,6 +207,12 @@ static int __init process_domain_node(
return -EFAULT;
}
+ if ( bd->domid == DOMID_INVALID )
+ bd->domid = get_initial_domain_id();
+ else if ( bd->domid != get_initial_domain_id() )
+ printk(XENLOG_WARNING
+ "WARN: Booting without initial domid not supported.\n");
+
return 0;
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 25ff029ecdda..027b224151d1 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1029,8 +1029,9 @@ static struct domain *__init create_dom0(struct boot_info *bi)
if ( iommu_enabled )
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
- /* Create initial domain. Not d0 for pvshim. */
- bd->domid = get_initial_domain_id();
+ if ( bd->domid == DOMID_INVALID )
+ /* Create initial domain. Not d0 for pvshim. */
+ bd->domid = get_initial_domain_id();
d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
if ( IS_ERR(d) )
panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index 0e54aeeb6cc2..2057030dda45 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -28,6 +28,15 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
return 0;
}
+static inline int __init fdt_prop_as_u32(
+ const struct fdt_property *prop, uint32_t *val)
+{
+ if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u32) )
+ return -EINVAL;
+
+ return fdt_cell_as_u32((fdt32_t *)prop->data, val);
+}
+
static inline int __init fdt_get_prop_by_offset(
const void *fdt, int node, const char *name, unsigned long *offset)
{
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 11/15] x86/hyperlaunch: add domain id parsing to domain config
2024-12-26 16:57 ` [PATCH v2 11/15] x86/hyperlaunch: add domain id parsing to domain config Daniel P. Smith
@ 2025-01-30 16:49 ` Jan Beulich
0 siblings, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 16:49 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> Introduce the ability to specify the desired domain id for the domain
> definition. The domain id will be populated in the domid property of the domain
> node in the device tree configuration.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
(Not going to repeat style remarks already made on earlier patches. Please
apply throughout the series.)
> @@ -61,10 +62,40 @@ static int __init dom0less_module_index(
> static int __init process_domain_node(
> struct boot_info *bi, void *fdt, int dom_node)
> {
> - int node;
> + int node, property;
> struct boot_domain *bd = &bi->domains[bi->nr_domains];
> const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown";
>
> + fdt_for_each_property_offset(property, fdt, dom_node)
> + {
> + const struct fdt_property *prop;
> + const char *prop_name;
> + int name_len;
> +
> + prop = fdt_get_property_by_offset(fdt, property, NULL);
> + if ( !prop )
> + continue; /* silently skip */
> +
> + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len);
> +
> + if ( strncmp(prop_name, "domid", name_len) == 0 )
Isn't this going to (wrongly) match when e.g. the property has just "d" (and
hence name_len is 1).
> + {
> + uint32_t val = DOMID_INVALID;
> + if ( fdt_prop_as_u32(prop, &val) != 0 )
> + {
> + printk(" failed processing domain id for domain %s\n", name);
> + return -EINVAL;
> + }
> + if ( val >= DOMID_FIRST_RESERVED )
> + {
> + printk(" invalid domain id for domain %s\n", name);
> + return -EINVAL;
> + }
> + bd->domid = (domid_t)val;
> + printk(" domid: %d\n", bd->domid);
> + }
> + }
Perhaps the question comes too early (will be taken care of in later
patches), but still: What if multiple domains have the same ID specified?
> @@ -125,7 +156,29 @@ static int __init process_domain_node(
> else if (
> fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") == 0 )
> {
> - int idx = dom0less_module_node(fdt, node, size_size, address_size);
> + unsigned int idx;
> + int ret = 0;
> +
> + if ( bd->ramdisk )
> + {
> + printk(XENLOG_ERR "Duplicate ramdisk 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);
> + }
Doesn't this belong into the earlier patch?
> @@ -154,6 +207,12 @@ static int __init process_domain_node(
> return -EFAULT;
> }
>
> + if ( bd->domid == DOMID_INVALID )
> + bd->domid = get_initial_domain_id();
Isn't this redundant with ...
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1029,8 +1029,9 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> if ( iommu_enabled )
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> - /* Create initial domain. Not d0 for pvshim. */
> - bd->domid = get_initial_domain_id();
> + if ( bd->domid == DOMID_INVALID )
> + /* Create initial domain. Not d0 for pvshim. */
> + bd->domid = get_initial_domain_id();
... this?
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 12/15] x86/hyperlaunch: specify dom0 mode with device tree
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (10 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 11/15] x86/hyperlaunch: add domain id parsing to domain config Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-15 19:42 ` Jason Andryuk
2025-01-30 16:55 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 13/15] x86/hyperlaunch: add memory parsing to domain config Daniel P. Smith
` (2 subsequent siblings)
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Enable selecting the mode in which the domain will be built and ran. This
includes:
- whether it will be either a 32/64 bit domain
- if it will be run as a PV or HVM domain
- and if it will require a device model (not applicable for dom0)
In the device tree, this will be represented as a bit map that will be carried
through into struct boot_domain.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1:
- switched to nested else if
- dropped ternary op for reporting domain name
- switch to strncmp for the check for mode prop
- drop BUILD_MODE_LONG until PV construction series
---
xen/arch/x86/domain-builder/fdt.c | 19 +++++++++++++++++++
xen/arch/x86/include/asm/bootdomain.h | 5 +++++
xen/arch/x86/setup.c | 3 ++-
3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index efce0927c645..db584ba78e92 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -94,6 +94,25 @@ static int __init process_domain_node(
bd->domid = (domid_t)val;
printk(" domid: %d\n", bd->domid);
}
+ else if ( strncmp(prop_name, "mode", name_len) == 0 )
+ {
+ if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
+ {
+ printk(" failed processing mode for domain %s\n", name);
+ return -EINVAL;
+ }
+
+ printk(" mode: ");
+ if ( !(bd->mode & BUILD_MODE_PARAVIRT) )
+ {
+ if ( bd->mode & BUILD_MODE_ENABLE_DM )
+ printk("HVM\n");
+ else
+ printk("PVH\n");
+ }
+ else
+ printk("PV\n");
+ }
}
fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 101a0c643d74..5918aaf6bb63 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -15,6 +15,11 @@ struct boot_domain {
domid_t domid;
+ /* On | Off */
+#define BUILD_MODE_PARAVIRT (1 << 0) /* PV | PVH/HVM */
+#define BUILD_MODE_ENABLE_DM (1 << 1) /* HVM | PVH */
+ uint32_t mode;
+
struct boot_module *kernel;
struct boot_module *ramdisk;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 027b224151d1..a87e122b5a61 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1016,7 +1016,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
struct boot_domain *bd = &bi->domains[0];
struct domain *d;
- if ( opt_dom0_pvh )
+ if ( opt_dom0_pvh ||
+ (bi->hyperlaunch_enabled && !(bd->mode & BUILD_MODE_PARAVIRT)) )
{
dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
((hvm_hap_supported() && !opt_dom0_shadow) ?
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 12/15] x86/hyperlaunch: specify dom0 mode with device tree
2024-12-26 16:57 ` [PATCH v2 12/15] x86/hyperlaunch: specify dom0 mode with device tree Daniel P. Smith
@ 2025-01-15 19:42 ` Jason Andryuk
2025-01-30 16:55 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-01-15 19:42 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-12-26 11:57, Daniel P. Smith wrote:
> Enable selecting the mode in which the domain will be built and ran. This
s/built and ran/built and run/
Regards,
Jason
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH v2 12/15] x86/hyperlaunch: specify dom0 mode with device tree
2024-12-26 16:57 ` [PATCH v2 12/15] x86/hyperlaunch: specify dom0 mode with device tree Daniel P. Smith
2025-01-15 19:42 ` Jason Andryuk
@ 2025-01-30 16:55 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 16:55 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> Enable selecting the mode in which the domain will be built and ran. This
> includes:
>
> - whether it will be either a 32/64 bit domain
I can't spot anything like this in the code changes.
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -94,6 +94,25 @@ static int __init process_domain_node(
> bd->domid = (domid_t)val;
> printk(" domid: %d\n", bd->domid);
> }
> + else if ( strncmp(prop_name, "mode", name_len) == 0 )
> + {
> + if ( fdt_prop_as_u32(prop, &bd->mode) != 0 )
> + {
> + printk(" failed processing mode for domain %s\n", name);
> + return -EINVAL;
> + }
> +
> + printk(" mode: ");
> + if ( !(bd->mode & BUILD_MODE_PARAVIRT) )
> + {
> + if ( bd->mode & BUILD_MODE_ENABLE_DM )
> + printk("HVM\n");
> + else
> + printk("PVH\n");
> + }
> + else
> + printk("PV\n");
Shorter and less indentation as
if ( bd->mode & BUILD_MODE_PARAVIRT )
printk("PV\n");
else if ( bd->mode & BUILD_MODE_ENABLE_DM )
printk("HVM\n");
else
printk("PVH\n");
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1016,7 +1016,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> struct boot_domain *bd = &bi->domains[0];
> struct domain *d;
>
> - if ( opt_dom0_pvh )
> + if ( opt_dom0_pvh ||
> + (bi->hyperlaunch_enabled && !(bd->mode & BUILD_MODE_PARAVIRT)) )
And then dropping BUILD_MODE_ENABLE_DM on the floor?
Also shouldn't this be
if ( bi->hyperlaunch_enabled
? bd->mode & BUILD_MODE_PARAVIRT
: opt_dom0_pvh )
as it can't do any good to honor a conflicting command line option.
Command line doc then will want amending to clarify that the option
will be ignored in certain cases.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 13/15] x86/hyperlaunch: add memory parsing to domain config
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (11 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 12/15] x86/hyperlaunch: specify dom0 mode with device tree Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-15 16:55 ` Jason Andryuk
2025-01-30 17:01 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree Daniel P. Smith
2024-12-26 16:57 ` [PATCH v2 15/15] x86/hyperlaunch: add capabilities to boot domain Daniel P. Smith
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Michal Orzel
Add three properties, memory, mem-min, and mem-max, to the domain node device
tree parsing to define the memory allocation for a domain. All three fields are
expressed in kb and written as a u64 in the device tree entries.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v1
- moved common fdt parsing to libfdt
- dropped ternary for name selection
- swtich over from match_fdt to strncmp
- change mem prints to kb
---
xen/arch/x86/dom0_build.c | 8 +++++++
xen/arch/x86/domain-builder/fdt.c | 34 +++++++++++++++++++++++++++
xen/arch/x86/include/asm/bootdomain.h | 4 ++++
xen/include/xen/libfdt/libfdt-xen.h | 9 +++++++
4 files changed, 55 insertions(+)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index c231191faec7..1c3b7ff0e658 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -609,6 +609,14 @@ int __init construct_dom0(struct boot_domain *bd)
process_pending_softirqs();
+ /* If param dom0_size was not set and HL config provided memory size */
+ if ( !get_memsize(&dom0_size, LONG_MAX) && bd->mem_pages )
+ dom0_size.nr_pages = bd->mem_pages;
+ if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages )
+ dom0_size.nr_pages = bd->min_pages;
+ if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
+ dom0_size.nr_pages = bd->max_pages;
+
if ( is_hvm_domain(d) )
rc = dom0_construct_pvh(bd);
else if ( is_pv_domain(d) )
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index db584ba78e92..aff1b8c3235d 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -6,6 +6,7 @@
#include <xen/init.h>
#include <xen/lib.h>
#include <xen/libfdt/libfdt.h>
+#include <xen/sizes.h>
#include <asm/bootinfo.h>
#include <asm/guest.h>
@@ -113,6 +114,39 @@ static int __init process_domain_node(
else
printk("PV\n");
}
+ else if ( strncmp(prop_name, "memory", name_len) == 0 )
+ {
+ uint64_t kb;
+ if ( fdt_prop_as_u64(prop, &kb) != 0 )
+ {
+ printk(" failed processing memory for domain %s\n", name);
+ return -EINVAL;
+ }
+ bd->mem_pages = PFN_DOWN(kb * SZ_1K);
+ printk(" memory: %ld kb\n", kb);
+ }
+ else if ( strncmp(prop_name, "mem-min", name_len) == 0 )
+ {
+ uint64_t kb;
+ if ( fdt_prop_as_u64(prop, &kb) != 0 )
+ {
+ printk(" failed processing memory for domain %s\n", name);
+ return -EINVAL;
+ }
+ bd->min_pages = PFN_DOWN(kb * SZ_1K);
+ printk(" min memory: %ld kb\n", kb);
+ }
+ else if ( strncmp(prop_name, "mem-max", name_len) == 0 )
+ {
+ uint64_t kb;
+ if ( fdt_prop_as_u64(prop, &kb) != 0 )
+ {
+ printk(" failed processing memory for domain %s\n", name);
+ return -EINVAL;
+ }
+ bd->max_pages = PFN_DOWN(kb * SZ_1K);
+ printk(" max memory: %ld kb\n", kb);
+ }
}
fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 5918aaf6bb63..d7092bc32ad7 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -20,6 +20,10 @@ struct boot_domain {
#define BUILD_MODE_ENABLE_DM (1 << 1) /* HVM | PVH */
uint32_t mode;
+ unsigned long mem_pages;
+ unsigned long min_pages;
+ unsigned long max_pages;
+
struct boot_module *kernel;
struct boot_module *ramdisk;
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index 2057030dda45..3b653e626842 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -37,6 +37,15 @@ static inline int __init fdt_prop_as_u32(
return fdt_cell_as_u32((fdt32_t *)prop->data, val);
}
+static inline int __init fdt_prop_as_u64(
+ const struct fdt_property *prop, uint64_t *val)
+{
+ if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u64) )
+ return -EINVAL;
+
+ return fdt_cell_as_u64((fdt32_t *)prop->data, val);
+}
+
static inline int __init fdt_get_prop_by_offset(
const void *fdt, int node, const char *name, unsigned long *offset)
{
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 13/15] x86/hyperlaunch: add memory parsing to domain config
2024-12-26 16:57 ` [PATCH v2 13/15] x86/hyperlaunch: add memory parsing to domain config Daniel P. Smith
@ 2025-01-15 16:55 ` Jason Andryuk
2025-01-30 17:01 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-01-15 16:55 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel
On 2024-12-26 11:57, Daniel P. Smith wrote:
> Add three properties, memory, mem-min, and mem-max, to the domain node device
> tree parsing to define the memory allocation for a domain. All three fields are
> expressed in kb and written as a u64 in the device tree entries.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
> index db584ba78e92..aff1b8c3235d 100644
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -6,6 +6,7 @@
> #include <xen/init.h>
> #include <xen/lib.h>
> #include <xen/libfdt/libfdt.h>
> +#include <xen/sizes.h>
>
> #include <asm/bootinfo.h>
> #include <asm/guest.h>
> @@ -113,6 +114,39 @@ static int __init process_domain_node(
> else
> printk("PV\n");
> }
> + else if ( strncmp(prop_name, "memory", name_len) == 0 )
> + {
> + uint64_t kb;
> + if ( fdt_prop_as_u64(prop, &kb) != 0 )
> + {
> + printk(" failed processing memory for domain %s\n", name);
> + return -EINVAL;
> + }
> + bd->mem_pages = PFN_DOWN(kb * SZ_1K);
> + printk(" memory: %ld kb\n", kb);
> + }
> + else if ( strncmp(prop_name, "mem-min", name_len) == 0 )
> + {
> + uint64_t kb;
> + if ( fdt_prop_as_u64(prop, &kb) != 0 )
> + {
> + printk(" failed processing memory for domain %s\n", name);
s/memory/mem-min/
> + return -EINVAL;
> + }
> + bd->min_pages = PFN_DOWN(kb * SZ_1K);
> + printk(" min memory: %ld kb\n", kb);
> + }
> + else if ( strncmp(prop_name, "mem-max", name_len) == 0 )
> + {
> + uint64_t kb;
> + if ( fdt_prop_as_u64(prop, &kb) != 0 )
> + {
> + printk(" failed processing memory for domain %s\n", name);
s/memory/mem-max/
With that,
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> + return -EINVAL;
> + }
> + bd->max_pages = PFN_DOWN(kb * SZ_1K);
> + printk(" max memory: %ld kb\n", kb);
> + }
> }
>
> fdt_for_each_subnode(node, fdt, dom_node)
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 13/15] x86/hyperlaunch: add memory parsing to domain config
2024-12-26 16:57 ` [PATCH v2 13/15] x86/hyperlaunch: add memory parsing to domain config Daniel P. Smith
2025-01-15 16:55 ` Jason Andryuk
@ 2025-01-30 17:01 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 17:01 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Bertrand Marquis, Michal Orzel, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -609,6 +609,14 @@ int __init construct_dom0(struct boot_domain *bd)
>
> process_pending_softirqs();
>
> + /* If param dom0_size was not set and HL config provided memory size */
> + if ( !get_memsize(&dom0_size, LONG_MAX) && bd->mem_pages )
> + dom0_size.nr_pages = bd->mem_pages;
> + if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages )
> + dom0_size.nr_pages = bd->min_pages;
> + if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
> + dom0_size.nr_pages = bd->max_pages;
Again I don't think it should be mix-and-match: Either everything is
taken from DT, or everything is taken from the command line. Yet I'm
open to be convinced otherwise.
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -37,6 +37,15 @@ static inline int __init fdt_prop_as_u32(
> return fdt_cell_as_u32((fdt32_t *)prop->data, val);
> }
>
> +static inline int __init fdt_prop_as_u64(
> + const struct fdt_property *prop, uint64_t *val)
> +{
> + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u64) )
No new uses of u64 (and alike) please.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (12 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 13/15] x86/hyperlaunch: add memory parsing to domain config Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-01-15 19:40 ` Jason Andryuk
2025-01-30 17:04 ` Jan Beulich
2024-12-26 16:57 ` [PATCH v2 15/15] x86/hyperlaunch: add capabilities to boot domain Daniel P. Smith
14 siblings, 2 replies; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Introduce the `cpus` property, named as such for dom0less compatibility, that
represents the maximum number of vpcus to allocate for a domain. In the device
tree, it will be encoded as a u32 value.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1:
- switched from match_fdt to strncmp
- switched to nested else if
- dropped ternary for name selection
---
xen/arch/x86/dom0_build.c | 3 +++
xen/arch/x86/domain-builder/fdt.c | 11 +++++++++++
xen/arch/x86/include/asm/bootdomain.h | 2 ++
3 files changed, 16 insertions(+)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 1c3b7ff0e658..7ff052016bfd 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -617,6 +617,9 @@ int __init construct_dom0(struct boot_domain *bd)
if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages )
dom0_size.nr_pages = bd->max_pages;
+ if ( opt_dom0_max_vcpus_max == UINT_MAX && bd->max_vcpus )
+ opt_dom0_max_vcpus_max = bd->max_vcpus;
+
if ( is_hvm_domain(d) )
rc = dom0_construct_pvh(bd);
else if ( is_pv_domain(d) )
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index aff1b8c3235d..70a793db199b 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -147,6 +147,17 @@ static int __init process_domain_node(
bd->max_pages = PFN_DOWN(kb * SZ_1K);
printk(" max memory: %ld kb\n", kb);
}
+ else if ( strncmp(prop_name, "cpus", name_len) == 0 )
+ {
+ uint32_t val = UINT_MAX;
+ if ( fdt_prop_as_u32(prop, &val) != 0 )
+ {
+ printk(" failed processing max_vcpus for domain %s\n", name);
+ return -EINVAL;
+ }
+ bd->max_vcpus = val;
+ printk(" max vcpus: %d\n", bd->max_vcpus);
+ }
}
fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index d7092bc32ad7..1a15273043f5 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -24,6 +24,8 @@ struct boot_domain {
unsigned long min_pages;
unsigned long max_pages;
+ unsigned int max_vcpus;
+
struct boot_module *kernel;
struct boot_module *ramdisk;
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree
2024-12-26 16:57 ` [PATCH v2 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree Daniel P. Smith
@ 2025-01-15 19:40 ` Jason Andryuk
2025-01-30 17:04 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-01-15 19:40 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-12-26 11:57, Daniel P. Smith wrote:
> Introduce the `cpus` property, named as such for dom0less compatibility, that
> represents the maximum number of vpcus to allocate for a domain. In the device
> tree, it will be encoded as a u32 value.
s/vpcus/vcpus/
I would remove "maximum". Today, the DT only has `cpus`, and you get
all of them. So implicitly cpus=max_vcpus.
I could see a future max_vcpus property. In that case, you would get
`cpus` online and the rest offline.
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
> index aff1b8c3235d..70a793db199b 100644
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -147,6 +147,17 @@ static int __init process_domain_node(
> bd->max_pages = PFN_DOWN(kb * SZ_1K);
> printk(" max memory: %ld kb\n", kb);
> }
> + else if ( strncmp(prop_name, "cpus", name_len) == 0 )
> + {
> + uint32_t val = UINT_MAX;
> + if ( fdt_prop_as_u32(prop, &val) != 0 )
> + {
> + printk(" failed processing max_vcpus for domain %s\n", name);
s/max_vcpus/cpus/
Regards,
Jason
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree
2024-12-26 16:57 ` [PATCH v2 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree Daniel P. Smith
2025-01-15 19:40 ` Jason Andryuk
@ 2025-01-30 17:04 ` Jan Beulich
1 sibling, 0 replies; 56+ messages in thread
From: Jan Beulich @ 2025-01-30 17:04 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -147,6 +147,17 @@ static int __init process_domain_node(
> bd->max_pages = PFN_DOWN(kb * SZ_1K);
> printk(" max memory: %ld kb\n", kb);
> }
> + else if ( strncmp(prop_name, "cpus", name_len) == 0 )
> + {
> + uint32_t val = UINT_MAX;
It's not the first time I see such an initializer, yet it's even more
pronounced here, as the call ...
> + if ( fdt_prop_as_u32(prop, &val) != 0 )
... is coming right next. If that function succeeds, it surely should
set its output? And if it didn't, you're as hosed with initializer as
you're without.
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH v2 15/15] x86/hyperlaunch: add capabilities to boot domain
2024-12-26 16:57 [PATCH v2 00/15] Hyperlaunch device tree for dom0 Daniel P. Smith
` (13 preceding siblings ...)
2024-12-26 16:57 ` [PATCH v2 14/15] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree Daniel P. Smith
@ 2024-12-26 16:57 ` Daniel P. Smith
2025-02-04 11:13 ` Jan Beulich
14 siblings, 1 reply; 56+ messages in thread
From: Daniel P. Smith @ 2024-12-26 16:57 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Introduce the ability to assign capabilities to a domain via its definition in
device tree. The first capability enabled to select is the control domain
capability. The capability property is a bitfield in both the device tree and
`struct boot_domain`.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v1:
- switch to nested else if
- switch from match_fdt to strncmp
- drop ternary for name selection
- coding style changes
---
xen/arch/x86/domain-builder/core.c | 1 +
xen/arch/x86/domain-builder/fdt.c | 12 ++++++++++++
xen/arch/x86/include/asm/bootdomain.h | 4 ++++
xen/arch/x86/setup.c | 6 +++++-
4 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/domain-builder/core.c b/xen/arch/x86/domain-builder/core.c
index 91d1b7367e76..589496b6a3e1 100644
--- a/xen/arch/x86/domain-builder/core.c
+++ b/xen/arch/x86/domain-builder/core.c
@@ -96,6 +96,7 @@ void __init builder_init(struct boot_info *bi)
i = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
bi->mods[i].type = BOOTMOD_KERNEL;
bi->domains[0].kernel = &bi->mods[i];
+ bi->domains[0].capabilities |= BUILD_CAPS_CONTROL;
bi->nr_domains = 1;
}
}
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c
index 70a793db199b..e90b230eeffe 100644
--- a/xen/arch/x86/domain-builder/fdt.c
+++ b/xen/arch/x86/domain-builder/fdt.c
@@ -158,6 +158,18 @@ static int __init process_domain_node(
bd->max_vcpus = val;
printk(" max vcpus: %d\n", bd->max_vcpus);
}
+ else if ( strncmp(prop_name, "capabilities", name_len) == 0 )
+ {
+ if ( fdt_prop_as_u32(prop, &bd->capabilities) != 0 )
+ {
+ printk(" failed processing domain id for domain %s\n", name);
+ return -EINVAL;
+ }
+ printk(" caps: ");
+ if ( bd->capabilities & BUILD_CAPS_CONTROL )
+ printk("c");
+ printk("\n");
+ }
}
fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 1a15273043f5..67f43c13e905 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -15,6 +15,10 @@ struct boot_domain {
domid_t domid;
+#define BUILD_CAPS_NONE (0)
+#define BUILD_CAPS_CONTROL (1 << 0)
+ uint32_t capabilities;
+
/* On | Off */
#define BUILD_MODE_PARAVIRT (1 << 0) /* PV | PVH/HVM */
#define BUILD_MODE_ENABLE_DM (1 << 1) /* HVM | PVH */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a87e122b5a61..0fb8572b7145 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1002,6 +1002,7 @@ static size_t __init domain_cmdline_size(
static struct domain *__init create_dom0(struct boot_info *bi)
{
char *cmdline = NULL;
+ unsigned int create_flags = 0;
struct xen_domctl_createdomain dom0_cfg = {
.flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
.max_evtchn_port = -1,
@@ -1033,7 +1034,10 @@ static struct domain *__init create_dom0(struct boot_info *bi)
if ( bd->domid == DOMID_INVALID )
/* Create initial domain. Not d0 for pvshim. */
bd->domid = get_initial_domain_id();
- d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
+ if ( bd->capabilities & BUILD_CAPS_CONTROL )
+ create_flags |= CDF_privileged;
+ d = domain_create(bd->domid, &dom0_cfg,
+ pv_shim ? 0 : create_flags);
if ( IS_ERR(d) )
panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
--
2.30.2
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH v2 15/15] x86/hyperlaunch: add capabilities to boot domain
2024-12-26 16:57 ` [PATCH v2 15/15] x86/hyperlaunch: add capabilities to boot domain Daniel P. Smith
@ 2025-02-04 11:13 ` Jan Beulich
2025-02-04 15:40 ` Jason Andryuk
0 siblings, 1 reply; 56+ messages in thread
From: Jan Beulich @ 2025-02-04 11:13 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 26.12.2024 17:57, Daniel P. Smith wrote:
> Introduce the ability to assign capabilities to a domain via its definition in
> device tree. The first capability enabled to select is the control domain
> capability.
Hmm, and not at the same time another one to select "hardware domain"?
> --- a/xen/arch/x86/domain-builder/fdt.c
> +++ b/xen/arch/x86/domain-builder/fdt.c
> @@ -158,6 +158,18 @@ static int __init process_domain_node(
> bd->max_vcpus = val;
> printk(" max vcpus: %d\n", bd->max_vcpus);
> }
> + else if ( strncmp(prop_name, "capabilities", name_len) == 0 )
> + {
> + if ( fdt_prop_as_u32(prop, &bd->capabilities) != 0 )
> + {
> + printk(" failed processing domain id for domain %s\n", name);
"domain id"?
Jan
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH v2 15/15] x86/hyperlaunch: add capabilities to boot domain
2025-02-04 11:13 ` Jan Beulich
@ 2025-02-04 15:40 ` Jason Andryuk
0 siblings, 0 replies; 56+ messages in thread
From: Jason Andryuk @ 2025-02-04 15:40 UTC (permalink / raw)
To: Jan Beulich, Daniel P. Smith
Cc: christopher.w.clark, stefano.stabellini, Andrew Cooper,
Roger Pau Monné, xen-devel, Anthony PERARD, Michal Orzel,
Julien Grall
On 2025-02-04 06:13, Jan Beulich wrote:
> On 26.12.2024 17:57, Daniel P. Smith wrote:
>> Introduce the ability to assign capabilities to a domain via its definition in
>> device tree. The first capability enabled to select is the control domain
>> capability.
>
> Hmm, and not at the same time another one to select "hardware domain"?
Dan has an un-submitted patch that adds in hardware domain. Related, I
was preparing a dom0less patch that adds control, hardware, and xenstore
capabilities.
I've included it below. To keep them aligned, it creates a new common
public header with defines for the capabilities.
Regards,
Jason
commit 5d329e6ef7128a4999b28de6745810c595a7f9e8
Author: Jason Andryuk <jason.andryuk@amd.com>
Date: Fri Jan 31 14:50:53 2025 -0500
xen/arm: Add capabilities to dom0less
Add capabilities property to dom0less to allow building a
disaggregated system.
Introduce bootfdt.h to contain these constants.
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
---
There is overlap with hyperlaunch. The numeric values are the same.
Hyperlaunch doesn't expose the values in a public header as done here.
Is this to be expected for dom0less? It seems most of dom0less
isn't in
a header, but just in docs.
Hyperlaunch uses BUILD_CAPS_, but I chose DOMAIN_CAPS_ since there are
domain-level capabilities.
diff --git a/docs/misc/arm/device-tree/booting.txt
b/docs/misc/arm/device-tree/booting.txt
index 4346953a71..2cd99f9b79 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -167,6 +167,17 @@ with the following properties:
Refer to docs/misc/cache_coloring.rst for syntax. This option is
applicable
only to Arm64 guests.
+- capabilities
+ Optional. A bit field of domain capabilities for a disaggregated
+ system. A traditional dom0 has all all of these capabilities, and a
+ domU has none of them.
+
+ 0x1 DOMAIN_CAPS_CONTROL - A privileged, control domain
+ 0x2 DOMAIN_CAPS_HARDWARE - The hardware domain - there can be only 1
+ 0x4 DOMAIN_CAPS_XENSTORE - The xenstore domain - there can be only 1
+
+ The default is no capabilities.
+
- vpl011
An empty property to enable/disable a virtual pl011 for the guest to
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 9f24463ebd..bb49142d24 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -12,6 +12,7 @@
#include <xen/sizes.h>
#include <xen/vmap.h>
+#include <public/bootfdt.h>
#include <public/io/xs_wire.h>
#include <asm/arm64/sve.h>
@@ -1236,6 +1237,18 @@ void __init create_domUs(void)
d_cfg.max_maptrack_frames = val;
}
+ if ( dt_property_read_u32(node, "capabilities", &val) )
+ {
+ if ( val & ~DOMAIN_CAPS_MASK )
+ panic("invalid capabilities (%"PRIu32") overflow\n", val);
+ if ( val & DOMAIN_CAPS_CONTROL )
+ flags |= CDF_privileged;
+ if ( val & DOMAIN_CAPS_HARDWARE )
+ flags |= CDF_hardware;
+ if ( val & DOMAIN_CAPS_XENSTORE )
+ d_cfg.flags |= XEN_DOMCTL_CDF_xs_domain;
+ }
+
if ( dt_get_property(node, "sve", &val) )
{
#ifdef CONFIG_ARM64_SVE
diff --git a/xen/common/domain.c b/xen/common/domain.c
index c170597410..dbeda908be 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -701,6 +701,10 @@ struct domain *domain_create(domid_t domid,
/* Sort out our idea of is_hardware_domain(). */
if ( flags & CDF_hardware || domid == hardware_domid )
{
+ if ( hardware_domain )
+ panic("Can't set %pd - %pd is already hardware domain\n", d,
+ hardware_domain);
+
if ( hardware_domid < 0 || hardware_domid >=
DOMID_FIRST_RESERVED )
panic("The value of hardware_dom must be a valid domain
ID\n");
diff --git a/xen/include/public/bootfdt.h b/xen/include/public/bootfdt.h
new file mode 100644
index 0000000000..4e87aca8ac
--- /dev/null
+++ b/xen/include/public/bootfdt.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Xen Device Tree boot information
+ *
+ * Information for configuring Xen domains created at boot time.
+ */
+
+#ifndef __XEN_PUBLIC_BOOTFDT_H__
+#define __XEN_PUBLIC_BOOTFDT_H__
+
+/* Domain Capabilities specified in the "capabilities" property. Use of
+ * this property allows splitting up the monolithic dom0 into separate,
+ * less privileged components. A regular domU has no capabilities
+ * (which is the default if nothing is specified). A traditional dom0
+ * has all three capabilities.*/
+
+/* Control/Privileged domain capable of affecting other domains. */
+#define DOMAIN_CAPS_CONTROL (1 << 0)
+/* Hardware domain controlling physical hardware. Typically providing
+ * backends to other domains. */
+#define DOMAIN_CAPS_HARDWARE (1 << 1)
+/* Xenstore domain. */
+#define DOMAIN_CAPS_XENSTORE (1 << 2)
+#define DOMAIN_CAPS_MASK (DOMAIN_CAPS_CONTROL |
DOMAIN_CAPS_HARDWARE | \
+ DOMAIN_CAPS_XENSTORE)
+
+#endif /* __XEN_PUBLIC_BOOTFDT_H__ */
^ permalink raw reply related [flat|nested] 56+ messages in thread