* [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-17 14:54 ` Jan Beulich
2025-04-18 21:16 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 02/13] kconfig: introduce domain builder config options Alejandro Vallejo
` (12 subsequent siblings)
13 siblings, 2 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
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>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Manually nullify bd->cmdline before xfree()ing cmdline.
* const-ify arguments of domain_cmdline_size()
* Add cmdline_len to pvh_load_kernel()
---
xen/arch/x86/hvm/dom0_build.c | 31 ++++++++--------
xen/arch/x86/include/asm/boot-domain.h | 1 +
xen/arch/x86/pv/dom0_build.c | 4 +-
xen/arch/x86/setup.c | 51 ++++++++++++++++++++------
4 files changed, 57 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 2a094b3145..49832f921c 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -653,7 +653,7 @@ 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;
+ unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0;
const char *initrd_cmdline = NULL;
struct elf_binary elf;
struct elf_dom_parms parms;
@@ -736,8 +736,7 @@ static int __init pvh_load_kernel(
initrd = NULL;
}
- if ( cmdline )
- extra_space += elf_round_up(&elf, strlen(cmdline) + 1);
+ extra_space += elf_round_up(&elf, cmdline_len);
last_addr = find_memory(d, &elf, extra_space);
if ( last_addr == INVALID_PADDR )
@@ -778,21 +777,21 @@ static int __init pvh_load_kernel(
/* Free temporary buffers. */
free_boot_modules();
- if ( cmdline != NULL )
+ rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline, cmdline_len, v);
+ if ( rc )
{
- rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
- if ( rc )
- {
- printk("Unable to copy guest command line\n");
- return rc;
- }
- start_info.cmdline_paddr = last_addr;
- /*
- * Round up to 32/64 bits (depending on the guest kernel bitness) so
- * the modlist/start_info is aligned.
- */
- last_addr += elf_round_up(&elf, strlen(cmdline) + 1);
+ printk("Unable to copy guest command line\n");
+ return rc;
}
+
+ start_info.cmdline_paddr = cmdline_len ? last_addr : 0;
+
+ /*
+ * Round up to 32/64 bits (depending on the guest kernel bitness) so
+ * the modlist/start_info is aligned.
+ */
+ last_addr += elf_round_up(&elf, cmdline_len);
+
if ( initrd != NULL )
{
rc = hvm_copy_to_guest_phys(last_addr, &mod, sizeof(mod), v);
diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index fcbedda0f0..d7c6042e25 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -15,6 +15,7 @@ struct boot_domain {
struct boot_module *kernel;
struct boot_module *module;
+ const char *cmdline;
struct domain *d;
};
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index b485eea05f..e1b78d47c2 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(const 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 3c257f0bad..4df012460d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -978,10 +978,30 @@ 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(const struct boot_info *bi,
+ const 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;
+ size_t cmdline_size;
struct xen_domctl_createdomain dom0_cfg = {
.flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
.max_evtchn_port = -1,
@@ -1020,20 +1040,24 @@ static struct domain *__init create_dom0(struct boot_info *bi)
if ( alloc_dom0_vcpu0(d) == NULL )
panic("Error creating %pdv0\n", d);
- /* Grab the DOM0 command line. */
- if ( bd->kernel->cmdline_pa || bi->kextra )
+ 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 ( bd->kernel->cmdline_pa )
- safe_strcpy(cmdline,
- cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
+ strlcpy(cmdline,
+ cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
+ cmdline_size);
if ( bi->kextra )
/* kextra always includes exactly one leading space. */
- safe_strcat(cmdline, bi->kextra);
+ strlcat(cmdline, bi->kextra, cmdline_size);
/* Append any extra parameters. */
if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
- safe_strcat(cmdline, " noapic");
+ strlcat(cmdline, " noapic", cmdline_size);
if ( (strlen(acpi_param) == 0) && acpi_disabled )
{
@@ -1043,17 +1067,20 @@ static struct domain *__init create_dom0(struct boot_info *bi)
if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
{
- safe_strcat(cmdline, " acpi=");
- safe_strcat(cmdline, acpi_param);
+ strlcat(cmdline, " acpi=", cmdline_size);
+ strlcat(cmdline, acpi_param, cmdline_size);
}
-
- bd->kernel->cmdline_pa = __pa(cmdline);
+ bd->kernel->cmdline_pa = 0;
+ bd->cmdline = cmdline;
}
bd->d = d;
if ( construct_dom0(bd) != 0 )
panic("Could not construct domain 0\n");
+ bd->cmdline = NULL;
+ xfree(cmdline);
+
return d;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain
2025-04-17 12:48 ` [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain Alejandro Vallejo
@ 2025-04-17 14:54 ` Jan Beulich
2025-04-17 16:06 ` Alejandro Vallejo
2025-04-18 21:16 ` dmkhn
1 sibling, 1 reply; 46+ messages in thread
From: Jan Beulich @ 2025-04-17 14:54 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Daniel P. Smith, Andrew Cooper, Roger Pau Monné,
Jason Andryuk, xen-devel
On 17.04.2025 14:48, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> 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>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
preferably with ...
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -653,7 +653,7 @@ 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;
> + unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0;
... this becoming either size_t (as you have it elsewhere) or unsigned int.
Happy to make the adjustment while committing, so long as you agree with
either of the suggested variants.
Jan
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain
2025-04-17 14:54 ` Jan Beulich
@ 2025-04-17 16:06 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 16:06 UTC (permalink / raw)
To: Jan Beulich
Cc: Daniel P. Smith, Andrew Cooper, Roger Pau Monné,
Jason Andryuk, xen-devel
On Thu Apr 17, 2025 at 3:54 PM BST, Jan Beulich wrote:
> On 17.04.2025 14:48, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> 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>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
Thanks,
> preferably with ...
>
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -653,7 +653,7 @@ 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;
>> + unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0;
>
> ... this becoming either size_t (as you have it elsewhere) or unsigned int.
> Happy to make the adjustment while committing, so long as you agree with
> either of the suggested variants.
>
> Jan
Yes, sounds good. I'd rather it be size_t seeing as it's the output type
of strlen()
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain
2025-04-17 12:48 ` [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain Alejandro Vallejo
2025-04-17 14:54 ` Jan Beulich
@ 2025-04-18 21:16 ` dmkhn
2025-04-23 11:40 ` Alejandro Vallejo
1 sibling, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 21:16 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:23PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> 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>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> v4:
> * Manually nullify bd->cmdline before xfree()ing cmdline.
> * const-ify arguments of domain_cmdline_size()
> * Add cmdline_len to pvh_load_kernel()
> ---
> xen/arch/x86/hvm/dom0_build.c | 31 ++++++++--------
> xen/arch/x86/include/asm/boot-domain.h | 1 +
> xen/arch/x86/pv/dom0_build.c | 4 +-
> xen/arch/x86/setup.c | 51 ++++++++++++++++++++------
> 4 files changed, 57 insertions(+), 30 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 2a094b3145..49832f921c 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -653,7 +653,7 @@ 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;
> + unsigned long cmdline_len = bd->cmdline ? strlen(bd->cmdline) + 1 : 0;
> const char *initrd_cmdline = NULL;
> struct elf_binary elf;
> struct elf_dom_parms parms;
> @@ -736,8 +736,7 @@ static int __init pvh_load_kernel(
> initrd = NULL;
> }
>
> - if ( cmdline )
> - extra_space += elf_round_up(&elf, strlen(cmdline) + 1);
> + extra_space += elf_round_up(&elf, cmdline_len);
>
> last_addr = find_memory(d, &elf, extra_space);
> if ( last_addr == INVALID_PADDR )
> @@ -778,21 +777,21 @@ static int __init pvh_load_kernel(
> /* Free temporary buffers. */
> free_boot_modules();
>
> - if ( cmdline != NULL )
> + rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline, cmdline_len, v);
> + if ( rc )
> {
> - rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
> - if ( rc )
> - {
> - printk("Unable to copy guest command line\n");
> - return rc;
> - }
> - start_info.cmdline_paddr = last_addr;
> - /*
> - * Round up to 32/64 bits (depending on the guest kernel bitness) so
> - * the modlist/start_info is aligned.
> - */
> - last_addr += elf_round_up(&elf, strlen(cmdline) + 1);
> + printk("Unable to copy guest command line\n");
Side note: I think it makes sense to add domain ID to all printouts in
pvh_load_kernel(). E.g. block under elf_xen_parse() logs domain ID.
> + return rc;
> }
> +
> + start_info.cmdline_paddr = cmdline_len ? last_addr : 0;
> +
> + /*
> + * Round up to 32/64 bits (depending on the guest kernel bitness) so
> + * the modlist/start_info is aligned.
> + */
> + last_addr += elf_round_up(&elf, cmdline_len);
> +
> if ( initrd != NULL )
> {
> rc = hvm_copy_to_guest_phys(last_addr, &mod, sizeof(mod), v);
> diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
> index fcbedda0f0..d7c6042e25 100644
> --- a/xen/arch/x86/include/asm/boot-domain.h
> +++ b/xen/arch/x86/include/asm/boot-domain.h
> @@ -15,6 +15,7 @@ struct boot_domain {
>
> struct boot_module *kernel;
> struct boot_module *module;
> + const char *cmdline;
>
> struct domain *d;
> };
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index b485eea05f..e1b78d47c2 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(const 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 3c257f0bad..4df012460d 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -978,10 +978,30 @@ 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(const struct boot_info *bi,
> + const 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;
> + size_t cmdline_size;
> struct xen_domctl_createdomain dom0_cfg = {
> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> .max_evtchn_port = -1,
> @@ -1020,20 +1040,24 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> if ( alloc_dom0_vcpu0(d) == NULL )
> panic("Error creating %pdv0\n", d);
>
> - /* Grab the DOM0 command line. */
> - if ( bd->kernel->cmdline_pa || bi->kextra )
> + 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 ( bd->kernel->cmdline_pa )
> - safe_strcpy(cmdline,
> - cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
> + strlcpy(cmdline,
> + cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
> + cmdline_size);
>
> if ( bi->kextra )
> /* kextra always includes exactly one leading space. */
> - safe_strcat(cmdline, bi->kextra);
> + strlcat(cmdline, bi->kextra, cmdline_size);
>
> /* Append any extra parameters. */
> if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> - safe_strcat(cmdline, " noapic");
> + strlcat(cmdline, " noapic", cmdline_size);
>
> if ( (strlen(acpi_param) == 0) && acpi_disabled )
> {
> @@ -1043,17 +1067,20 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>
> if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
> {
> - safe_strcat(cmdline, " acpi=");
> - safe_strcat(cmdline, acpi_param);
> + strlcat(cmdline, " acpi=", cmdline_size);
> + strlcat(cmdline, acpi_param, cmdline_size);
> }
> -
> - bd->kernel->cmdline_pa = __pa(cmdline);
> + bd->kernel->cmdline_pa = 0;
> + bd->cmdline = cmdline;
> }
>
> bd->d = d;
> if ( construct_dom0(bd) != 0 )
> panic("Could not construct domain 0\n");
>
> + bd->cmdline = NULL;
> + xfree(cmdline);
> +
> return d;
> }
>
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain
2025-04-18 21:16 ` dmkhn
@ 2025-04-23 11:40 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 11:40 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Jason Andryuk
On Fri Apr 18, 2025 at 10:16 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:23PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> 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>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>
>
> Reviewed-by: Denis Mukhin <dmukhin@ford.com>
Thanks
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
>> index 2a094b3145..49832f921c 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -778,21 +777,21 @@ static int __init pvh_load_kernel(
>> /* Free temporary buffers. */
>> free_boot_modules();
>>
>> - if ( cmdline != NULL )
>> + rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline, cmdline_len, v);
>> + if ( rc )
>> {
>> - rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) + 1, v);
>> - if ( rc )
>> - {
>> - printk("Unable to copy guest command line\n");
>> - return rc;
>> - }
>> - start_info.cmdline_paddr = last_addr;
>> - /*
>> - * Round up to 32/64 bits (depending on the guest kernel bitness) so
>> - * the modlist/start_info is aligned.
>> - */
>> - last_addr += elf_round_up(&elf, strlen(cmdline) + 1);
>> + printk("Unable to copy guest command line\n");
>
> Side note: I think it makes sense to add domain ID to all printouts in
> pvh_load_kernel(). E.g. block under elf_xen_parse() logs domain ID.
Sounds sensible and easy enough. Sure.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 02/13] kconfig: introduce domain builder config options
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
2025-04-17 12:48 ` [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-17 15:00 ` Jan Beulich
2025-04-17 12:48 ` [PATCH v4 03/13] common/hyperlaunch: introduce the domain builder Alejandro Vallejo
` (11 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Hyperlaunch domain builder will be the consolidated boot time domain
building logic framework. Introduces the config option to enable this
domain builder to eventually turn on the ability to load the domain
configuration via a flattened device tree.
This is common code, but it's tightly integrated with boot_info, so the
whole builder is gated on CONFIG_HAS_BOOT_INFO, autoselected on x86 only
for the time being.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Moved from arch/x86 to common/
* Present the domain builder submenu for X86 only.
* s/LIB_DEVICE_TREE/LIBFDT/
* Reworded Kconfig to be a bit more user-friendly.
* Dropped Jason's R-by, due to the new Kconfig option.
---
xen/arch/x86/Kconfig | 1 +
xen/common/Kconfig | 5 +++++
xen/common/domain-builder/Kconfig | 18 ++++++++++++++++++
3 files changed, 24 insertions(+)
create mode 100644 xen/common/domain-builder/Kconfig
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index de2fa37f08..67de2decc5 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -15,6 +15,7 @@ config X86
select FUNCTION_ALIGNMENT_16B
select GENERIC_BUG_FRAME
select HAS_ALTERNATIVE
+ select HAS_BOOT_INFO
select HAS_COMPAT
select HAS_CPUFREQ
select HAS_DIT
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index be28060716..5c9d4eb3ab 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -67,6 +67,9 @@ config GENERIC_BUG_FRAME
config HAS_ALTERNATIVE
bool
+config HAS_BOOT_INFO
+ bool
+
config HAS_COMPAT
bool
@@ -144,6 +147,8 @@ config STATIC_MEMORY
If unsure, say N.
+source "common/domain-builder/Kconfig"
+
menu "Speculative hardening"
config INDIRECT_THUNK
diff --git a/xen/common/domain-builder/Kconfig b/xen/common/domain-builder/Kconfig
new file mode 100644
index 0000000000..5b137e4c2b
--- /dev/null
+++ b/xen/common/domain-builder/Kconfig
@@ -0,0 +1,18 @@
+
+menu "Domain Builder Features"
+depends on HAS_BOOT_INFO
+
+config DOMAIN_BUILDER
+ bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
+ select LIBFDT
+ help
+ Xen has a built-in mechanisms to automatically construct domains
+ (like dom0) during the boot phase. The domain builder is an enhanced
+ form of that mechanism to enable constructing predefined domains
+ described on a flattened device tree.
+
+ This feature is currently experimental.
+
+ If unsure, say N.
+
+endmenu
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 02/13] kconfig: introduce domain builder config options
2025-04-17 12:48 ` [PATCH v4 02/13] kconfig: introduce domain builder config options Alejandro Vallejo
@ 2025-04-17 15:00 ` Jan Beulich
2025-04-17 15:39 ` Jason Andryuk
2025-04-17 16:18 ` Alejandro Vallejo
0 siblings, 2 replies; 46+ messages in thread
From: Jan Beulich @ 2025-04-17 15:00 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Daniel P. Smith, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
xen-devel
On 17.04.2025 14:48, Alejandro Vallejo wrote:
> --- /dev/null
> +++ b/xen/common/domain-builder/Kconfig
> @@ -0,0 +1,18 @@
> +
> +menu "Domain Builder Features"
> +depends on HAS_BOOT_INFO
That is, what's going to further be added here will not ...
> +config DOMAIN_BUILDER
...depend on this, but just on HAS_BOOT_INFO? Seems not very likely, but
I'll be looking forward to learn what the plans are.
Also, if the entire contents here is to depend on HAS_BOOT_INFO, can't
the "source" line pulling in this Kconfig be put inside "if HAS_BOOT_INFO"?
That would centralize definition and use of that symbol to a single file.
> + bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
> + select LIBFDT
> + help
> + Xen has a built-in mechanisms to automatically construct domains
> + (like dom0) during the boot phase. The domain builder is an enhanced
> + form of that mechanism to enable constructing predefined domains
> + described on a flattened device tree.
I'm not a native speaker, but (perhaps because of that) "on" here reads
odd. More logical to me would be "by" or "via".
Jan
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 02/13] kconfig: introduce domain builder config options
2025-04-17 15:00 ` Jan Beulich
@ 2025-04-17 15:39 ` Jason Andryuk
2025-04-17 16:18 ` Alejandro Vallejo
1 sibling, 0 replies; 46+ messages in thread
From: Jason Andryuk @ 2025-04-17 15:39 UTC (permalink / raw)
To: Jan Beulich, Alejandro Vallejo
Cc: Daniel P. Smith, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
xen-devel
On 2025-04-17 11:00, Jan Beulich wrote:
> On 17.04.2025 14:48, Alejandro Vallejo wrote:
>> --- /dev/null
>> +++ b/xen/common/domain-builder/Kconfig
>> + bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
>> + select LIBFDT
>> + help
>> + Xen has a built-in mechanisms to automatically construct domains
>> + (like dom0) during the boot phase. The domain builder is an enhanced
>> + form of that mechanism to enable constructing predefined domains
>> + described on a flattened device tree.
>
> I'm not a native speaker, but (perhaps because of that) "on" here reads
> odd. More logical to me would be "by" or "via".
Yes, "by" is better.
The description is a little backwards - it should state what it is
first. Maybe:
Support for constructing predefined domains described by a flattened
device tree. This allows constructing multiple domains at boot time
instead of being limited to a single dom0.
Regards,
Jason
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 02/13] kconfig: introduce domain builder config options
2025-04-17 15:00 ` Jan Beulich
2025-04-17 15:39 ` Jason Andryuk
@ 2025-04-17 16:18 ` Alejandro Vallejo
2025-04-22 6:57 ` Jan Beulich
1 sibling, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 16:18 UTC (permalink / raw)
To: Jan Beulich
Cc: Daniel P. Smith, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
xen-devel
On Thu Apr 17, 2025 at 4:00 PM BST, Jan Beulich wrote:
> On 17.04.2025 14:48, Alejandro Vallejo wrote:
>> --- /dev/null
>> +++ b/xen/common/domain-builder/Kconfig
>> @@ -0,0 +1,18 @@
>> +
>> +menu "Domain Builder Features"
>> +depends on HAS_BOOT_INFO
>
> That is, what's going to further be added here will not ...
>
>> +config DOMAIN_BUILDER
>
> ...depend on this, but just on HAS_BOOT_INFO? Seems not very likely, but
> I'll be looking forward to learn what the plans are.
CONFIG_HAS_BOOT_INFO has nothing to do with future plans. The domain
builder is tightly integrated with the boot_info infrastructure and
cannot be used (or linked) unless the arch-specific definitions are
present. It cannot function without it. And this movement from arch/ to
common/ forces this new Kconfig to gate core.c on boot_info existing
(because it's in asm/bootinfo.h atm). I _COULD_ also move the boot_info
elsewhere, but without a drive to actually use it, that seems a bit
pointless.
HAS_BOOT_INFO && !DOMAIN_BUILDER still links core.c, because that
contains the common initialiser for boot_info.
>
> Also, if the entire contents here is to depend on HAS_BOOT_INFO, can't
> the "source" line pulling in this Kconfig be put inside "if HAS_BOOT_INFO"?
> That would centralize definition and use of that symbol to a single file.
Sure.
>
>> + bool "Domain builder (UNSUPPORTED)" if UNSUPPORTED
>> + select LIBFDT
>> + help
>> + Xen has a built-in mechanisms to automatically construct domains
>> + (like dom0) during the boot phase. The domain builder is an enhanced
>> + form of that mechanism to enable constructing predefined domains
>> + described on a flattened device tree.
>
> I'm not a native speaker, but (perhaps because of that) "on" here reads
> odd. More logical to me would be "by" or "via".
>
> Jan
Yes, you're right. I just felt the previous help wasn't very helpful
unless you knew in advance what you were toggling. Jason's more concrete
suggestion in the following reply sounds reasonable too, so I'll just
copy that on resend.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 02/13] kconfig: introduce domain builder config options
2025-04-17 16:18 ` Alejandro Vallejo
@ 2025-04-22 6:57 ` Jan Beulich
0 siblings, 0 replies; 46+ messages in thread
From: Jan Beulich @ 2025-04-22 6:57 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: Daniel P. Smith, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
xen-devel
On 17.04.2025 18:18, Alejandro Vallejo wrote:
> On Thu Apr 17, 2025 at 4:00 PM BST, Jan Beulich wrote:
>> On 17.04.2025 14:48, Alejandro Vallejo wrote:
>>> --- /dev/null
>>> +++ b/xen/common/domain-builder/Kconfig
>>> @@ -0,0 +1,18 @@
>>> +
>>> +menu "Domain Builder Features"
>>> +depends on HAS_BOOT_INFO
>>
>> That is, what's going to further be added here will not ...
>>
>>> +config DOMAIN_BUILDER
>>
>> ...depend on this, but just on HAS_BOOT_INFO? Seems not very likely, but
>> I'll be looking forward to learn what the plans are.
>
> CONFIG_HAS_BOOT_INFO has nothing to do with future plans. The domain
> builder is tightly integrated with the boot_info infrastructure and
> cannot be used (or linked) unless the arch-specific definitions are
> present. It cannot function without it. And this movement from arch/ to
> common/ forces this new Kconfig to gate core.c on boot_info existing
> (because it's in asm/bootinfo.h atm). I _COULD_ also move the boot_info
> elsewhere, but without a drive to actually use it, that seems a bit
> pointless.
>
> HAS_BOOT_INFO && !DOMAIN_BUILDER still links core.c, because that
> contains the common initialiser for boot_info.
Which, as voiced earlier, I have reservations against. The entire
domain-builder/ sub-tree would imo better not be recursed into when
DOMAIN_BUILDER=n.
Jan
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 03/13] common/hyperlaunch: introduce the domain builder
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
2025-04-17 12:48 ` [PATCH v4 01/13] x86/boot: add cmdline to struct boot_domain Alejandro Vallejo
2025-04-17 12:48 ` [PATCH v4 02/13] kconfig: introduce domain builder config options Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 21:55 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 04/13] x86/hyperlaunch: initial support for hyperlaunch device tree Alejandro Vallejo
` (10 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
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>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Moved from arch/x86/ to common/
* gated all of domain-builder/ on CONFIG_BOOT_INFO
* Hide the domain builder submenu for !X86
* Factor out the "hyperlaunch_enabled = false" toggle core.c
* Removed stub inline, as DCE makes it unnecessary
* Adjusted printks.
---
xen/arch/x86/include/asm/bootinfo.h | 3 ++
xen/arch/x86/setup.c | 17 +++++----
xen/common/Makefile | 1 +
xen/common/domain-builder/Makefile | 2 ++
xen/common/domain-builder/core.c | 56 +++++++++++++++++++++++++++++
xen/common/domain-builder/fdt.c | 37 +++++++++++++++++++
xen/common/domain-builder/fdt.h | 12 +++++++
xen/include/xen/domain-builder.h | 9 +++++
8 files changed, 131 insertions(+), 6 deletions(-)
create mode 100644 xen/common/domain-builder/Makefile
create mode 100644 xen/common/domain-builder/core.c
create mode 100644 xen/common/domain-builder/fdt.c
create mode 100644 xen/common/domain-builder/fdt.h
create mode 100644 xen/include/xen/domain-builder.h
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 3afc214c17..82c2650fcf 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/setup.c b/xen/arch/x86/setup.c
index 4df012460d..ccc57cc70a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -5,6 +5,7 @@
#include <xen/cpuidle.h>
#include <xen/dmi.h>
#include <xen/domain.h>
+#include <xen/domain-builder.h>
#include <xen/domain_page.h>
#include <xen/efi.h>
#include <xen/err.h>
@@ -1282,9 +1283,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 )
{
@@ -1467,8 +1471,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
@@ -1592,7 +1597,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 )
{
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 98f0873056..565837bc71 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
obj-$(CONFIG_IOREQ_SERVER) += dm.o
obj-y += domain.o
+obj-$(CONFIG_HAS_BOOT_INFO) += domain-builder/
obj-y += event_2l.o
obj-y += event_channel.o
obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
diff --git a/xen/common/domain-builder/Makefile b/xen/common/domain-builder/Makefile
new file mode 100644
index 0000000000..b10cd56b28
--- /dev/null
+++ b/xen/common/domain-builder/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
+obj-y += core.init.o
diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
new file mode 100644
index 0000000000..a5b21fc179
--- /dev/null
+++ b/xen/common/domain-builder/core.c
@@ -0,0 +1,56 @@
+/* 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)
+{
+ bi->hyperlaunch_enabled = false;
+
+ if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
+ {
+ int ret;
+
+ switch ( ret = has_hyperlaunch_fdt(bi) )
+ {
+ case 0:
+ printk(XENLOG_DEBUG "DT found: hyperlaunch\n");
+ bi->hyperlaunch_enabled = true;
+ bi->mods[0].type = BOOTMOD_FDT;
+ break;
+
+ case -EINVAL:
+ /* No DT found */
+ break;
+
+ case -ENOENT:
+ case -ENODATA:
+ printk(XENLOG_DEBUG "DT found: non-hyperlaunch (%d)\n", ret);
+ bi->mods[0].type = BOOTMOD_FDT;
+ break;
+
+ default:
+ printk(XENLOG_ERR "unknown error (%d) checking hyperlaunch DT\n",
+ ret);
+ 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/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
new file mode 100644
index 0000000000..aaf8c1cc16
--- /dev/null
+++ b/xen/common/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(const struct boot_info *bi)
+{
+ int ret = 0;
+ const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+
+ if ( !fdt || 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/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
new file mode 100644
index 0000000000..97a45a6ec4
--- /dev/null
+++ b/xen/common/domain-builder/fdt.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __XEN_DOMAIN_BUILDER_FDT_H__
+#define __XEN_DOMAIN_BUILDER_FDT_H__
+
+struct boot_info;
+
+/* hyperlaunch fdt is required to be module 0 */
+#define HYPERLAUNCH_MODULE_IDX 0
+
+int has_hyperlaunch_fdt(const struct boot_info *bi);
+
+#endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
new file mode 100644
index 0000000000..ac2b84775d
--- /dev/null
+++ b/xen/include/xen/domain-builder.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __XEN_DOMAIN_BUILDER_H__
+#define __XEN_DOMAIN_BUILDER_H__
+
+struct boot_info;
+
+void builder_init(struct boot_info *bi);
+
+#endif /* __XEN_DOMAIN_BUILDER_H__ */
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 03/13] common/hyperlaunch: introduce the domain builder
2025-04-17 12:48 ` [PATCH v4 03/13] common/hyperlaunch: introduce the domain builder Alejandro Vallejo
@ 2025-04-18 21:55 ` dmkhn
2025-04-23 11:52 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 21:55 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:25PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> 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>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> v4:
> * Moved from arch/x86/ to common/
> * gated all of domain-builder/ on CONFIG_BOOT_INFO
> * Hide the domain builder submenu for !X86
> * Factor out the "hyperlaunch_enabled = false" toggle core.c
> * Removed stub inline, as DCE makes it unnecessary
> * Adjusted printks.
> ---
> xen/arch/x86/include/asm/bootinfo.h | 3 ++
> xen/arch/x86/setup.c | 17 +++++----
> xen/common/Makefile | 1 +
> xen/common/domain-builder/Makefile | 2 ++
> xen/common/domain-builder/core.c | 56 +++++++++++++++++++++++++++++
> xen/common/domain-builder/fdt.c | 37 +++++++++++++++++++
> xen/common/domain-builder/fdt.h | 12 +++++++
> xen/include/xen/domain-builder.h | 9 +++++
> 8 files changed, 131 insertions(+), 6 deletions(-)
> create mode 100644 xen/common/domain-builder/Makefile
> create mode 100644 xen/common/domain-builder/core.c
> create mode 100644 xen/common/domain-builder/fdt.c
> create mode 100644 xen/common/domain-builder/fdt.h
> create mode 100644 xen/include/xen/domain-builder.h
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 3afc214c17..82c2650fcf 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/setup.c b/xen/arch/x86/setup.c
> index 4df012460d..ccc57cc70a 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -5,6 +5,7 @@
> #include <xen/cpuidle.h>
> #include <xen/dmi.h>
> #include <xen/domain.h>
> +#include <xen/domain-builder.h>
> #include <xen/domain_page.h>
> #include <xen/efi.h>
> #include <xen/err.h>
> @@ -1282,9 +1283,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];
Nit: perhaps add convenience aliases for bi->domains[0] (e.g. dom0) and
for bi->mods[0] (e.g. mod)?
>
> if ( pvh_boot )
> {
> @@ -1467,8 +1471,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
> @@ -1592,7 +1597,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 )
> {
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 98f0873056..565837bc71 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
> obj-$(CONFIG_IOREQ_SERVER) += dm.o
> obj-y += domain.o
> +obj-$(CONFIG_HAS_BOOT_INFO) += domain-builder/
> obj-y += event_2l.o
> obj-y += event_channel.o
> obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
> diff --git a/xen/common/domain-builder/Makefile b/xen/common/domain-builder/Makefile
> new file mode 100644
> index 0000000000..b10cd56b28
> --- /dev/null
> +++ b/xen/common/domain-builder/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
> +obj-y += core.init.o
> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> new file mode 100644
> index 0000000000..a5b21fc179
> --- /dev/null
> +++ b/xen/common/domain-builder/core.c
> @@ -0,0 +1,56 @@
> +/* 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)
> +{
> + bi->hyperlaunch_enabled = false;
> +
> + if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
I would re-organize the code to remove one level of indentation, e.g.:
if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
return;
switch ( ret = has_hyperlaunch_fdt(bi) )
...
or even add #ifdef CONFIG_DOMAIN_BUILDER for builder_init() in the header file.
What do you think?
> + {
> + int ret;
> +
> + switch ( ret = has_hyperlaunch_fdt(bi) )
> + {
> + case 0:
> + printk(XENLOG_DEBUG "DT found: hyperlaunch\n");
> + bi->hyperlaunch_enabled = true;
> + bi->mods[0].type = BOOTMOD_FDT;
> + break;
> +
> + case -EINVAL:
> + /* No DT found */
> + break;
> +
> + case -ENOENT:
> + case -ENODATA:
Looks like this code accounts for the follow on change: current implementation
only returns -EINVAL or 0.
Is it possible to convert has_hyperlaunch_fdt() to a simple predicate?
> + printk(XENLOG_DEBUG "DT found: non-hyperlaunch (%d)\n", ret);
> + bi->mods[0].type = BOOTMOD_FDT;
> + break;
> +
> + default:
> + printk(XENLOG_ERR "unknown error (%d) checking hyperlaunch DT\n",
> + ret);
> + 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/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> new file mode 100644
> index 0000000000..aaf8c1cc16
> --- /dev/null
> +++ b/xen/common/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(const struct boot_info *bi)
I think the function can return `bool` in current patch.
> +{
> + int ret = 0;
> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> + if ( !fdt || 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/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
> new file mode 100644
> index 0000000000..97a45a6ec4
> --- /dev/null
> +++ b/xen/common/domain-builder/fdt.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_FDT_H__
> +#define __XEN_DOMAIN_BUILDER_FDT_H__
> +
> +struct boot_info;
> +
> +/* hyperlaunch fdt is required to be module 0 */
> +#define HYPERLAUNCH_MODULE_IDX 0
> +
> +int has_hyperlaunch_fdt(const struct boot_info *bi);
> +
> +#endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
> diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
> new file mode 100644
> index 0000000000..ac2b84775d
> --- /dev/null
> +++ b/xen/include/xen/domain-builder.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __XEN_DOMAIN_BUILDER_H__
> +#define __XEN_DOMAIN_BUILDER_H__
> +
> +struct boot_info;
> +
> +void builder_init(struct boot_info *bi);
> +
> +#endif /* __XEN_DOMAIN_BUILDER_H__ */
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 03/13] common/hyperlaunch: introduce the domain builder
2025-04-18 21:55 ` dmkhn
@ 2025-04-23 11:52 ` Alejandro Vallejo
2025-04-23 21:53 ` dmkhn
0 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 11:52 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Fri Apr 18, 2025 at 10:55 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:25PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> 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>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> v4:
>> * Moved from arch/x86/ to common/
>> * gated all of domain-builder/ on CONFIG_BOOT_INFO
>> * Hide the domain builder submenu for !X86
>> * Factor out the "hyperlaunch_enabled = false" toggle core.c
>> * Removed stub inline, as DCE makes it unnecessary
>> * Adjusted printks.
>> ---
>> xen/arch/x86/include/asm/bootinfo.h | 3 ++
>> xen/arch/x86/setup.c | 17 +++++----
>> xen/common/Makefile | 1 +
>> xen/common/domain-builder/Makefile | 2 ++
>> xen/common/domain-builder/core.c | 56 +++++++++++++++++++++++++++++
>> xen/common/domain-builder/fdt.c | 37 +++++++++++++++++++
>> xen/common/domain-builder/fdt.h | 12 +++++++
>> xen/include/xen/domain-builder.h | 9 +++++
>> 8 files changed, 131 insertions(+), 6 deletions(-)
>> create mode 100644 xen/common/domain-builder/Makefile
>> create mode 100644 xen/common/domain-builder/core.c
>> create mode 100644 xen/common/domain-builder/fdt.c
>> create mode 100644 xen/common/domain-builder/fdt.h
>> create mode 100644 xen/include/xen/domain-builder.h
>>
>> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
>> index 3afc214c17..82c2650fcf 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/setup.c b/xen/arch/x86/setup.c
>> index 4df012460d..ccc57cc70a 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -5,6 +5,7 @@
>> #include <xen/cpuidle.h>
>> #include <xen/dmi.h>
>> #include <xen/domain.h>
>> +#include <xen/domain-builder.h>
>> #include <xen/domain_page.h>
>> #include <xen/efi.h>
>> #include <xen/err.h>
>> @@ -1282,9 +1283,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];
>
> Nit: perhaps add convenience aliases for bi->domains[0] (e.g. dom0) and
> for bi->mods[0] (e.g. mod)?
Inside the boot_info? As in separate aliasing pointers into the arrays?
I'd rather not. It'd be dangerous on systems without an actual dom0.
The PV shim comes to mind, but other configurations might arise in the
future where no domain holds the id of 0.
>
>>
>> if ( pvh_boot )
>> {
>> @@ -1467,8 +1471,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
>> @@ -1592,7 +1597,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 )
>> {
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index 98f0873056..565837bc71 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
>> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
>> obj-$(CONFIG_IOREQ_SERVER) += dm.o
>> obj-y += domain.o
>> +obj-$(CONFIG_HAS_BOOT_INFO) += domain-builder/
>> obj-y += event_2l.o
>> obj-y += event_channel.o
>> obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
>> diff --git a/xen/common/domain-builder/Makefile b/xen/common/domain-builder/Makefile
>> new file mode 100644
>> index 0000000000..b10cd56b28
>> --- /dev/null
>> +++ b/xen/common/domain-builder/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
>> +obj-y += core.init.o
>> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
>> new file mode 100644
>> index 0000000000..a5b21fc179
>> --- /dev/null
>> +++ b/xen/common/domain-builder/core.c
>> @@ -0,0 +1,56 @@
>> +/* 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)
>> +{
>> + bi->hyperlaunch_enabled = false;
>> +
>> + if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>
> I would re-organize the code to remove one level of indentation, e.g.:
>
> if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> return;
>
> switch ( ret = has_hyperlaunch_fdt(bi) )
> ...
>
> or even add #ifdef CONFIG_DOMAIN_BUILDER for builder_init() in the header file.
>
> What do you think?
In this patch it sounds good, but a later patch adds more stuff at the
tail of the function that must not be skipped, so it wouldn't work
as-is.
Another matter is whether this function could be skipped in the "no-fdt"
case, and it probably could. But I do know the longer series (big RFC
from Daniel) adds more common logic present when !CONFIG_DOMAIN_BUILDER,
so I'm reticent to deviate too much from it to avoid rebasing headaches.
>
>> + {
>> + int ret;
>> +
>> + switch ( ret = has_hyperlaunch_fdt(bi) )
>> + {
>> + case 0:
>> + printk(XENLOG_DEBUG "DT found: hyperlaunch\n");
>> + bi->hyperlaunch_enabled = true;
>> + bi->mods[0].type = BOOTMOD_FDT;
>> + break;
>> +
>> + case -EINVAL:
>> + /* No DT found */
>> + break;
>> +
>> + case -ENOENT:
>> + case -ENODATA:
>
> Looks like this code accounts for the follow on change: current implementation
> only returns -EINVAL or 0.
>
> Is it possible to convert has_hyperlaunch_fdt() to a simple predicate?
The function is a misnomer and it ought to change to return an
enumerated type instead where it returns FDT_HYPERLAUNCH, FDT_DOM0LESS,
FDT_UNKNOWN or NO_FDT. Using error codes for identification is a tad too
hacky.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 03/13] common/hyperlaunch: introduce the domain builder
2025-04-23 11:52 ` Alejandro Vallejo
@ 2025-04-23 21:53 ` dmkhn
0 siblings, 0 replies; 46+ messages in thread
From: dmkhn @ 2025-04-23 21:53 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Wed, Apr 23, 2025 at 12:52:58PM +0100, Alejandro Vallejo wrote:
> On Fri Apr 18, 2025 at 10:55 PM BST, dmkhn wrote:
> > On Thu, Apr 17, 2025 at 01:48:25PM +0100, Alejandro Vallejo wrote:
> >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
> >>
> >> 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>
> >> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> >> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> >> ---
> >> v4:
> >> * Moved from arch/x86/ to common/
> >> * gated all of domain-builder/ on CONFIG_BOOT_INFO
> >> * Hide the domain builder submenu for !X86
> >> * Factor out the "hyperlaunch_enabled = false" toggle core.c
> >> * Removed stub inline, as DCE makes it unnecessary
> >> * Adjusted printks.
> >> ---
> >> xen/arch/x86/include/asm/bootinfo.h | 3 ++
> >> xen/arch/x86/setup.c | 17 +++++----
> >> xen/common/Makefile | 1 +
> >> xen/common/domain-builder/Makefile | 2 ++
> >> xen/common/domain-builder/core.c | 56 +++++++++++++++++++++++++++++
> >> xen/common/domain-builder/fdt.c | 37 +++++++++++++++++++
> >> xen/common/domain-builder/fdt.h | 12 +++++++
> >> xen/include/xen/domain-builder.h | 9 +++++
> >> 8 files changed, 131 insertions(+), 6 deletions(-)
> >> create mode 100644 xen/common/domain-builder/Makefile
> >> create mode 100644 xen/common/domain-builder/core.c
> >> create mode 100644 xen/common/domain-builder/fdt.c
> >> create mode 100644 xen/common/domain-builder/fdt.h
> >> create mode 100644 xen/include/xen/domain-builder.h
> >>
> >> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> >> index 3afc214c17..82c2650fcf 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/setup.c b/xen/arch/x86/setup.c
> >> index 4df012460d..ccc57cc70a 100644
> >> --- a/xen/arch/x86/setup.c
> >> +++ b/xen/arch/x86/setup.c
> >> @@ -5,6 +5,7 @@
> >> #include <xen/cpuidle.h>
> >> #include <xen/dmi.h>
> >> #include <xen/domain.h>
> >> +#include <xen/domain-builder.h>
> >> #include <xen/domain_page.h>
> >> #include <xen/efi.h>
> >> #include <xen/err.h>
> >> @@ -1282,9 +1283,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];
> >
> > Nit: perhaps add convenience aliases for bi->domains[0] (e.g. dom0) and
> > for bi->mods[0] (e.g. mod)?
>
> Inside the boot_info? As in separate aliasing pointers into the arrays?
I was thinking about local variables inside the function pointing to the
bi->mods[0] and bi->domains[0].
> I'd rather not. It'd be dangerous on systems without an actual dom0.
>
> The PV shim comes to mind, but other configurations might arise in the
> future where no domain holds the id of 0.
>
> >
> >>
> >> if ( pvh_boot )
> >> {
> >> @@ -1467,8 +1471,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
> >> @@ -1592,7 +1597,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 )
> >> {
> >> diff --git a/xen/common/Makefile b/xen/common/Makefile
> >> index 98f0873056..565837bc71 100644
> >> --- a/xen/common/Makefile
> >> +++ b/xen/common/Makefile
> >> @@ -11,6 +11,7 @@ obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
> >> obj-$(CONFIG_HAS_DEVICE_TREE) += device-tree/
> >> obj-$(CONFIG_IOREQ_SERVER) += dm.o
> >> obj-y += domain.o
> >> +obj-$(CONFIG_HAS_BOOT_INFO) += domain-builder/
> >> obj-y += event_2l.o
> >> obj-y += event_channel.o
> >> obj-$(CONFIG_EVTCHN_FIFO) += event_fifo.o
> >> diff --git a/xen/common/domain-builder/Makefile b/xen/common/domain-builder/Makefile
> >> new file mode 100644
> >> index 0000000000..b10cd56b28
> >> --- /dev/null
> >> +++ b/xen/common/domain-builder/Makefile
> >> @@ -0,0 +1,2 @@
> >> +obj-$(CONFIG_DOMAIN_BUILDER) += fdt.init.o
> >> +obj-y += core.init.o
> >> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> >> new file mode 100644
> >> index 0000000000..a5b21fc179
> >> --- /dev/null
> >> +++ b/xen/common/domain-builder/core.c
> >> @@ -0,0 +1,56 @@
> >> +/* 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)
> >> +{
> >> + bi->hyperlaunch_enabled = false;
> >> +
> >> + if ( IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> >
> > I would re-organize the code to remove one level of indentation, e.g.:
> >
> > if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> > return;
> >
> > switch ( ret = has_hyperlaunch_fdt(bi) )
> > ...
> >
> > or even add #ifdef CONFIG_DOMAIN_BUILDER for builder_init() in the header file.
> >
> > What do you think?
>
> In this patch it sounds good, but a later patch adds more stuff at the
> tail of the function that must not be skipped, so it wouldn't work
> as-is.
>
> Another matter is whether this function could be skipped in the "no-fdt"
> case, and it probably could. But I do know the longer series (big RFC
> from Daniel) adds more common logic present when !CONFIG_DOMAIN_BUILDER,
> so I'm reticent to deviate too much from it to avoid rebasing headaches.
I see, thanks for clarification.
>
> >
> >> + {
> >> + int ret;
> >> +
> >> + switch ( ret = has_hyperlaunch_fdt(bi) )
> >> + {
> >> + case 0:
> >> + printk(XENLOG_DEBUG "DT found: hyperlaunch\n");
> >> + bi->hyperlaunch_enabled = true;
> >> + bi->mods[0].type = BOOTMOD_FDT;
> >> + break;
> >> +
> >> + case -EINVAL:
> >> + /* No DT found */
> >> + break;
> >> +
> >> + case -ENOENT:
> >> + case -ENODATA:
> >
> > Looks like this code accounts for the follow on change: current implementation
> > only returns -EINVAL or 0.
> >
> > Is it possible to convert has_hyperlaunch_fdt() to a simple predicate?
>
> The function is a misnomer and it ought to change to return an
> enumerated type instead where it returns FDT_HYPERLAUNCH, FDT_DOM0LESS,
> FDT_UNKNOWN or NO_FDT. Using error codes for identification is a tad too
> hacky.
>
> Cheers,
> Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 04/13] x86/hyperlaunch: initial support for hyperlaunch device tree
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (2 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 03/13] common/hyperlaunch: introduce the domain builder Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 22:11 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 05/13] x86/hyperlaunch: Add helpers to locate multiboot modules Alejandro Vallejo
` (9 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Add the ability to detect both a formal hyperlaunch device tree or a dom0less
device tree. If the hyperlaunch device tree is found, then count the number of
domain entries, reporting an error if more than one is found.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Panic if we're booting on hyperlaunch, but walking the DTB fails.
* Remove inconsequential "else" clause in fdt.c
* Remove stub, as it's not required due to DCE
* Use min() rather than open-code it
---
xen/arch/x86/include/asm/bootinfo.h | 1 +
xen/common/domain-builder/core.c | 11 +++++
xen/common/domain-builder/fdt.c | 63 +++++++++++++++++++++++++++++
xen/common/domain-builder/fdt.h | 1 +
4 files changed, 76 insertions(+)
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 82c2650fcf..1e3d582e45 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];
};
diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
index a5b21fc179..3b062e85ec 100644
--- a/xen/common/domain-builder/core.c
+++ b/xen/common/domain-builder/core.c
@@ -43,6 +43,17 @@ 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 )
+ panic("Walk of device tree failed (%d)\n", ret);
+
+ printk(XENLOG_INFO " number of domains: %u\n", bi->nr_domains);
+ }
}
/*
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index aaf8c1cc16..b5ff8220da 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -13,6 +13,36 @@
#include "fdt.h"
+static int __init find_hyperlaunch_node(const void *fdt)
+{
+ int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
+
+ if ( hv_node >= 0 )
+ {
+ /* Anything other than zero indicates no match */
+ if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
+ return -ENODATA;
+
+ return hv_node;
+ }
+ else
+ {
+ /* Look 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") )
+ return chosen_node;
+ }
+ }
+
+ return -ENODATA;
+}
+
int __init has_hyperlaunch_fdt(const struct boot_info *bi)
{
int ret = 0;
@@ -20,7 +50,40 @@ int __init has_hyperlaunch_fdt(const struct boot_info *bi)
if ( !fdt || fdt_check_header(fdt) < 0 )
ret = -EINVAL;
+ else
+ ret = find_hyperlaunch_node(fdt);
+
+ bootstrap_unmap();
+
+ return min(0, ret);
+}
+
+int __init walk_hyperlaunch_fdt(struct boot_info *bi)
+{
+ int ret = 0, hv_node, node;
+ const 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)
+ {
+ if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
+ bi->nr_domains++;
+ }
+
+ /* Until multi-domain construction is added, throw an error */
+ if ( bi->nr_domains != 1 )
+ printk(XENLOG_ERR "hyperlaunch only supports dom0 construction\n");
+ err_out:
bootstrap_unmap();
return ret;
diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
index 97a45a6ec4..955aead497 100644
--- a/xen/common/domain-builder/fdt.h
+++ b/xen/common/domain-builder/fdt.h
@@ -8,5 +8,6 @@ struct boot_info;
#define HYPERLAUNCH_MODULE_IDX 0
int has_hyperlaunch_fdt(const struct boot_info *bi);
+int walk_hyperlaunch_fdt(struct boot_info *bi);
#endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 04/13] x86/hyperlaunch: initial support for hyperlaunch device tree
2025-04-17 12:48 ` [PATCH v4 04/13] x86/hyperlaunch: initial support for hyperlaunch device tree Alejandro Vallejo
@ 2025-04-18 22:11 ` dmkhn
2025-04-23 11:54 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 22:11 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:26PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> Add the ability to detect both a formal hyperlaunch device tree or a dom0less
> device tree. If the hyperlaunch device tree is found, then count the number of
> domain entries, reporting an error if more than one is found.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> v4:
> * Panic if we're booting on hyperlaunch, but walking the DTB fails.
> * Remove inconsequential "else" clause in fdt.c
> * Remove stub, as it's not required due to DCE
> * Use min() rather than open-code it
> ---
> xen/arch/x86/include/asm/bootinfo.h | 1 +
> xen/common/domain-builder/core.c | 11 +++++
> xen/common/domain-builder/fdt.c | 63 +++++++++++++++++++++++++++++
> xen/common/domain-builder/fdt.h | 1 +
> 4 files changed, 76 insertions(+)
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 82c2650fcf..1e3d582e45 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];
> };
> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> index a5b21fc179..3b062e85ec 100644
> --- a/xen/common/domain-builder/core.c
> +++ b/xen/common/domain-builder/core.c
> @@ -43,6 +43,17 @@ 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 )
> + panic("Walk of device tree failed (%d)\n", ret);
> +
> + printk(XENLOG_INFO " number of domains: %u\n", bi->nr_domains);
> + }
> }
>
> /*
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index aaf8c1cc16..b5ff8220da 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -13,6 +13,36 @@
>
> #include "fdt.h"
>
> +static int __init find_hyperlaunch_node(const void *fdt)
> +{
> + int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> +
> + if ( hv_node >= 0 )
> + {
> + /* Anything other than zero indicates no match */
> + if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
> + return -ENODATA;
> +
> + return hv_node;
> + }
> + else
> + {
> + /* Look 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") )
> + return chosen_node;
> + }
> + }
> +
> + return -ENODATA;
> +}
> +
> int __init has_hyperlaunch_fdt(const struct boot_info *bi)
> {
> int ret = 0;
> @@ -20,7 +50,40 @@ int __init has_hyperlaunch_fdt(const struct boot_info *bi)
>
> if ( !fdt || fdt_check_header(fdt) < 0 )
> ret = -EINVAL;
> + else
> + ret = find_hyperlaunch_node(fdt);
> +
> + bootstrap_unmap();
> +
> + return min(0, ret);
> +}
> +
> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
> +{
> + int ret = 0, hv_node, node;
> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> +
> + if ( unlikely(!fdt) )
> + return -EINVAL;
I think this check can be converted to ASSERT() since walk_hyperlaunch_fdt()
will be called after has_hyperlaunch_fdt() where condition is checked.
> +
> + hv_node = find_hyperlaunch_node(fdt);
> + if ( hv_node < 0 )
> + {
> + ret = hv_node;
> + goto err_out;
> + }
> +
> + fdt_for_each_subnode(node, fdt, hv_node)
> + {
> + if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
> + bi->nr_domains++;
> + }
> +
> + /* Until multi-domain construction is added, throw an error */
> + if ( bi->nr_domains != 1 )
> + printk(XENLOG_ERR "hyperlaunch only supports dom0 construction\n");
>
> + err_out:
> bootstrap_unmap();
>
> return ret;
> diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
> index 97a45a6ec4..955aead497 100644
> --- a/xen/common/domain-builder/fdt.h
> +++ b/xen/common/domain-builder/fdt.h
> @@ -8,5 +8,6 @@ struct boot_info;
> #define HYPERLAUNCH_MODULE_IDX 0
>
> int has_hyperlaunch_fdt(const struct boot_info *bi);
> +int walk_hyperlaunch_fdt(struct boot_info *bi);
>
> #endif /* __XEN_DOMAIN_BUILDER_FDT_H__ */
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 04/13] x86/hyperlaunch: initial support for hyperlaunch device tree
2025-04-18 22:11 ` dmkhn
@ 2025-04-23 11:54 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 11:54 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Fri Apr 18, 2025 at 11:11 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:26PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> Add the ability to detect both a formal hyperlaunch device tree or a dom0less
>> device tree. If the hyperlaunch device tree is found, then count the number of
>> domain entries, reporting an error if more than one is found.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> v4:
>> * Panic if we're booting on hyperlaunch, but walking the DTB fails.
>> * Remove inconsequential "else" clause in fdt.c
>> * Remove stub, as it's not required due to DCE
>> * Use min() rather than open-code it
>> ---
>> xen/arch/x86/include/asm/bootinfo.h | 1 +
>> xen/common/domain-builder/core.c | 11 +++++
>> xen/common/domain-builder/fdt.c | 63 +++++++++++++++++++++++++++++
>> xen/common/domain-builder/fdt.h | 1 +
>> 4 files changed, 76 insertions(+)
>>
>> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
>> index 82c2650fcf..1e3d582e45 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];
>> };
>> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
>> index a5b21fc179..3b062e85ec 100644
>> --- a/xen/common/domain-builder/core.c
>> +++ b/xen/common/domain-builder/core.c
>> @@ -43,6 +43,17 @@ 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 )
>> + panic("Walk of device tree failed (%d)\n", ret);
>> +
>> + printk(XENLOG_INFO " number of domains: %u\n", bi->nr_domains);
>> + }
>> }
>>
>> /*
>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
>> index aaf8c1cc16..b5ff8220da 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -13,6 +13,36 @@
>>
>> #include "fdt.h"
>>
>> +static int __init find_hyperlaunch_node(const void *fdt)
>> +{
>> + int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
>> +
>> + if ( hv_node >= 0 )
>> + {
>> + /* Anything other than zero indicates no match */
>> + if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
>> + return -ENODATA;
>> +
>> + return hv_node;
>> + }
>> + else
>> + {
>> + /* Look 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") )
>> + return chosen_node;
>> + }
>> + }
>> +
>> + return -ENODATA;
>> +}
>> +
>> int __init has_hyperlaunch_fdt(const struct boot_info *bi)
>> {
>> int ret = 0;
>> @@ -20,7 +50,40 @@ int __init has_hyperlaunch_fdt(const struct boot_info *bi)
>>
>> if ( !fdt || fdt_check_header(fdt) < 0 )
>> ret = -EINVAL;
>> + else
>> + ret = find_hyperlaunch_node(fdt);
>> +
>> + bootstrap_unmap();
>> +
>> + return min(0, ret);
>> +}
>> +
>> +int __init walk_hyperlaunch_fdt(struct boot_info *bi)
>> +{
>> + int ret = 0, hv_node, node;
>> + const void *fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>> +
>> + if ( unlikely(!fdt) )
>> + return -EINVAL;
>
> I think this check can be converted to ASSERT() since walk_hyperlaunch_fdt()
> will be called after has_hyperlaunch_fdt() where condition is checked.
True that.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 05/13] x86/hyperlaunch: Add helpers to locate multiboot modules
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (3 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 04/13] x86/hyperlaunch: initial support for hyperlaunch device tree Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 22:30 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 06/13] x86/hyperlaunch: locate dom0 kernel with hyperlaunch Alejandro Vallejo
` (8 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Alejandro Vallejo, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Bertrand Marquis, Jason Andryuk
Hyperlaunch mandates either a reg or module-index DT prop on nodes that
contain `multiboot,module" under their "compatible" prop. This patch
introduces a helper to generically find such index, appending the module
to the list of modules if it wasn't already (i.e: because it's given via
the "reg" prop).
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Remove stray reg prop parser in libfdt-xen.h.
* Remove fdt32_as_uX accessors.
* Brough fdt_prop_as_u32() accesor from later patches.
* So it can be used in place of a hardcoded fdt32_as_u32().
* Limited MAX_NR_BOOTMODS to INT_MAX.
* Preserved BOOTMOD_XEN on module append logic.
* Add missing bounds check to module-index parsed in multiboot module helper.
* Converted idx variable to uint32_t for better bounds checking.
* Braces from switch statement to conform to coding style.
* Added missing XENLOG_X.
* Print address_cells and size_cells on error parsing reg properties.
* Added (transient) missing declaration for extern helper.
* becomes static on the next patch.
---
xen/common/domain-builder/fdt.c | 162 ++++++++++++++++++++++++++++
xen/common/domain-builder/fdt.h | 2 +
xen/include/xen/domain-builder.h | 3 +
xen/include/xen/libfdt/libfdt-xen.h | 11 ++
4 files changed, 178 insertions(+)
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index b5ff8220da..d73536fed6 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -13,6 +13,168 @@
#include "fdt.h"
+/*
+ * Unpacks a "reg" property into its address and size constituents.
+ *
+ * @param prop Pointer to an FDT "reg" property.
+ * @param address_cells Number of 4-octet cells that make up an "address".
+ * @param size_cells Number of 4-octet cells that make up a "size".
+ * @param p_addr[out] Address encoded in the property.
+ * @param p_size[out] Size encoded in the property.
+ * @returns -EINVAL on malformed property, 0 otherwise.
+ */
+static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
+ int address_cells, int size_cells,
+ uint64_t *p_addr, uint64_t *p_size)
+{
+ const fdt32_t *cell = (const fdt32_t *)prop->data;
+ uint64_t addr, size;
+
+ if ( fdt32_to_cpu(prop->len) !=
+ (address_cells + size_cells) * sizeof(*cell) )
+ {
+ printk(XENLOG_ERR " cannot read reg %lu+%lu from prop len %u\n",
+ address_cells * sizeof(*cell), size_cells * sizeof(*cell),
+ fdt32_to_cpu(prop->len));
+ return -EINVAL;
+ }
+
+ switch ( address_cells )
+ {
+ case 1:
+ addr = fdt32_to_cpu(*cell);
+ break;
+ case 2:
+ addr = fdt64_to_cpu(*(const fdt64_t *)cell);
+ break;
+ default:
+ printk(XENLOG_ERR " unsupported address_cells=%d\n", address_cells);
+ return -EINVAL;
+ }
+
+ cell += address_cells;
+ switch ( size_cells )
+ {
+ case 1:
+ size = fdt32_to_cpu(*cell);
+ break;
+ case 2:
+ size = fdt64_to_cpu(*(const fdt64_t *)cell);
+ break;
+ default:
+ printk(XENLOG_ERR " unsupported size_cells=%d\n", size_cells);
+ return -EINVAL;
+ }
+
+ *p_addr = addr;
+ *p_size = size;
+
+ return 0;
+}
+
+/*
+ * Locate a multiboot module given its node offset in the FDT.
+ *
+ * The module location may be given via either FDT property:
+ * * reg = <address, size>
+ * * Mutates `bi` to append the module.
+ * * module-index = <idx>
+ * * Leaves `bi` unchanged.
+ *
+ * @param fdt Pointer to the full FDT.
+ * @param node Offset for the module node.
+ * @param address_cells Number of 4-octet cells that make up an "address".
+ * @param size_cells Number of 4-octet cells that make up a "size".
+ * @param bi[inout] Xen's representation of the boot parameters.
+ * @return -EINVAL on malformed nodes, otherwise
+ * index inside `bi->mods`
+ */
+int __init fdt_read_multiboot_module(const void *fdt, int node,
+ int address_cells, int size_cells,
+ struct boot_info *bi)
+{
+ const struct fdt_property *prop;
+ uint64_t addr, size;
+ int ret;
+ uint32_t idx;
+
+ if ( fdt_node_check_compatible(fdt, node, "multiboot,module") )
+ {
+ printk(XENLOG_ERR " bad module. multiboot,module not found");
+ return -ENODATA;
+ }
+
+ /* Location given as a `module-index` property. */
+ if ( (prop = fdt_get_property(fdt, node, "module-index", NULL)) )
+ {
+ if ( fdt_get_property(fdt, node, "reg", NULL) )
+ {
+ printk(XENLOG_ERR " found both reg and module-index for module\n");
+ return -EINVAL;
+ }
+ if ( (ret = fdt_prop_as_u32(prop, &idx)) )
+ {
+ printk(XENLOG_ERR " bad module-index prop\n");
+ return ret;
+ }
+ if ( idx >= MAX_NR_BOOTMODS )
+ {
+ printk(XENLOG_ERR " module-index overflow. %s=%u\n",
+ STR(MAX_NR_BOOTMODS), MAX_NR_BOOTMODS);
+ return -EINVAL;
+ }
+
+ return idx;
+ }
+
+ /* Otherwise location given as a `reg` property. */
+ if ( !(prop = fdt_get_property(fdt, node, "reg", NULL)) )
+ {
+ printk(XENLOG_ERR " no location for multiboot,module\n");
+ return -EINVAL;
+ }
+ if ( fdt_get_property(fdt, node, "module-index", NULL) )
+ {
+ printk(XENLOG_ERR " found both reg and module-index for module\n");
+ return -EINVAL;
+ }
+
+ ret = read_fdt_prop_as_reg(prop, address_cells, size_cells, &addr, &size);
+ if ( ret < 0 )
+ {
+ printk(XENLOG_ERR " failed reading reg for multiboot,module\n");
+ return -EINVAL;
+ }
+
+ idx = bi->nr_modules;
+ if ( idx > MAX_NR_BOOTMODS )
+ {
+ /*
+ * MAX_NR_BOOTMODS must fit in 31 bits so it's representable in the
+ * positive side of an int; for the return value.
+ */
+ BUILD_BUG_ON(MAX_NR_BOOTMODS > (uint64_t)INT_MAX);
+ printk(XENLOG_ERR " idx=%u exceeds len=%u\n", idx, MAX_NR_BOOTMODS);
+ return -EINVAL;
+ }
+
+ /*
+ * Append new module to the existing list
+ *
+ * Note that bi->nr_modules points to Xen itself, so we must shift it first
+ */
+ bi->nr_modules++;
+ bi->mods[bi->nr_modules] = bi->mods[idx];
+ bi->mods[idx] = (struct boot_module){
+ .start = addr,
+ .size = size,
+ };
+
+ printk(XENLOG_INFO " module[%u]: addr %lx size %lx\n", idx, addr, size);
+
+ return idx;
+}
+
static int __init find_hyperlaunch_node(const void *fdt)
{
int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
index 955aead497..8c98a256eb 100644
--- a/xen/common/domain-builder/fdt.h
+++ b/xen/common/domain-builder/fdt.h
@@ -2,6 +2,8 @@
#ifndef __XEN_DOMAIN_BUILDER_FDT_H__
#define __XEN_DOMAIN_BUILDER_FDT_H__
+#include <xen/libfdt/libfdt-xen.h>
+
struct boot_info;
/* hyperlaunch fdt is required to be module 0 */
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
index ac2b84775d..ace6b6875b 100644
--- a/xen/include/xen/domain-builder.h
+++ b/xen/include/xen/domain-builder.h
@@ -5,5 +5,8 @@
struct boot_info;
void builder_init(struct boot_info *bi);
+int fdt_read_multiboot_module(const void *fdt, int node,
+ int address_cells, int size_cells,
+ struct boot_info *bi)
#endif /* __XEN_DOMAIN_BUILDER_H__ */
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index a5340bc9f4..deafb25d98 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -12,6 +12,17 @@
#define LIBFDT_XEN_H
#include <xen/libfdt/libfdt.h>
+#include <xen/errno.h>
+
+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;
+
+ *val = fdt32_to_cpu(*(const fdt32_t *)prop->data);
+ return 0;
+}
static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
paddr_t *address,
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 05/13] x86/hyperlaunch: Add helpers to locate multiboot modules
2025-04-17 12:48 ` [PATCH v4 05/13] x86/hyperlaunch: Add helpers to locate multiboot modules Alejandro Vallejo
@ 2025-04-18 22:30 ` dmkhn
2025-04-23 12:12 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 22:30 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Bertrand Marquis, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:27PM +0100, Alejandro Vallejo wrote:
> Hyperlaunch mandates either a reg or module-index DT prop on nodes that
> contain `multiboot,module" under their "compatible" prop. This patch
> introduces a helper to generically find such index, appending the module
> to the list of modules if it wasn't already (i.e: because it's given via
> the "reg" prop).
>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> v4:
> * Remove stray reg prop parser in libfdt-xen.h.
> * Remove fdt32_as_uX accessors.
> * Brough fdt_prop_as_u32() accesor from later patches.
> * So it can be used in place of a hardcoded fdt32_as_u32().
> * Limited MAX_NR_BOOTMODS to INT_MAX.
> * Preserved BOOTMOD_XEN on module append logic.
> * Add missing bounds check to module-index parsed in multiboot module helper.
> * Converted idx variable to uint32_t for better bounds checking.
> * Braces from switch statement to conform to coding style.
> * Added missing XENLOG_X.
> * Print address_cells and size_cells on error parsing reg properties.
> * Added (transient) missing declaration for extern helper.
> * becomes static on the next patch.
> ---
> xen/common/domain-builder/fdt.c | 162 ++++++++++++++++++++++++++++
> xen/common/domain-builder/fdt.h | 2 +
> xen/include/xen/domain-builder.h | 3 +
> xen/include/xen/libfdt/libfdt-xen.h | 11 ++
> 4 files changed, 178 insertions(+)
>
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index b5ff8220da..d73536fed6 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -13,6 +13,168 @@
>
> #include "fdt.h"
>
> +/*
> + * Unpacks a "reg" property into its address and size constituents.
> + *
> + * @param prop Pointer to an FDT "reg" property.
> + * @param address_cells Number of 4-octet cells that make up an "address".
> + * @param size_cells Number of 4-octet cells that make up a "size".
> + * @param p_addr[out] Address encoded in the property.
> + * @param p_size[out] Size encoded in the property.
> + * @returns -EINVAL on malformed property, 0 otherwise.
> + */
> +static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
I would do s/read_fdt_prop_as_reg/fdt_prop_as_reg/ similar to fdt_prop_as_u32()
below.
> + int address_cells, int size_cells,
> + uint64_t *p_addr, uint64_t *p_size)
> +{
> + const fdt32_t *cell = (const fdt32_t *)prop->data;
> + uint64_t addr, size;
> +
> + if ( fdt32_to_cpu(prop->len) !=
> + (address_cells + size_cells) * sizeof(*cell) )
> + {
> + printk(XENLOG_ERR " cannot read reg %lu+%lu from prop len %u\n",
> + address_cells * sizeof(*cell), size_cells * sizeof(*cell),
> + fdt32_to_cpu(prop->len));
> + return -EINVAL;
> + }
> +
> + switch ( address_cells )
> + {
> + case 1:
> + addr = fdt32_to_cpu(*cell);
> + break;
> + case 2:
> + addr = fdt64_to_cpu(*(const fdt64_t *)cell);
> + break;
> + default:
> + printk(XENLOG_ERR " unsupported address_cells=%d\n", address_cells);
> + return -EINVAL;
> + }
> +
> + cell += address_cells;
> + switch ( size_cells )
> + {
> + case 1:
> + size = fdt32_to_cpu(*cell);
> + break;
> + case 2:
> + size = fdt64_to_cpu(*(const fdt64_t *)cell);
> + break;
> + default:
> + printk(XENLOG_ERR " unsupported size_cells=%d\n", size_cells);
> + return -EINVAL;
> + }
> +
> + *p_addr = addr;
> + *p_size = size;
> +
> + return 0;
> +}
> +
> +/*
> + * Locate a multiboot module given its node offset in the FDT.
> + *
> + * The module location may be given via either FDT property:
> + * * reg = <address, size>
> + * * Mutates `bi` to append the module.
> + * * module-index = <idx>
> + * * Leaves `bi` unchanged.
> + *
> + * @param fdt Pointer to the full FDT.
> + * @param node Offset for the module node.
> + * @param address_cells Number of 4-octet cells that make up an "address".
> + * @param size_cells Number of 4-octet cells that make up a "size".
> + * @param bi[inout] Xen's representation of the boot parameters.
> + * @return -EINVAL on malformed nodes, otherwise
> + * index inside `bi->mods`
> + */
> +int __init fdt_read_multiboot_module(const void *fdt, int node,
> + int address_cells, int size_cells,
> + struct boot_info *bi)
> +{
> + const struct fdt_property *prop;
> + uint64_t addr, size;
> + int ret;
> + uint32_t idx;
> +
> + if ( fdt_node_check_compatible(fdt, node, "multiboot,module") )
> + {
> + printk(XENLOG_ERR " bad module. multiboot,module not found");
> + return -ENODATA;
> + }
> +
> + /* Location given as a `module-index` property. */
> + if ( (prop = fdt_get_property(fdt, node, "module-index", NULL)) )
> + {
> + if ( fdt_get_property(fdt, node, "reg", NULL) )
> + {
> + printk(XENLOG_ERR " found both reg and module-index for module\n");
> + return -EINVAL;
> + }
> + if ( (ret = fdt_prop_as_u32(prop, &idx)) )
> + {
> + printk(XENLOG_ERR " bad module-index prop\n");
> + return ret;
> + }
> + if ( idx >= MAX_NR_BOOTMODS )
> + {
> + printk(XENLOG_ERR " module-index overflow. %s=%u\n",
> + STR(MAX_NR_BOOTMODS), MAX_NR_BOOTMODS);
> + return -EINVAL;
> + }
> +
> + return idx;
> + }
> +
> + /* Otherwise location given as a `reg` property. */
> + if ( !(prop = fdt_get_property(fdt, node, "reg", NULL)) )
> + {
> + printk(XENLOG_ERR " no location for multiboot,module\n");
> + return -EINVAL;
> + }
> + if ( fdt_get_property(fdt, node, "module-index", NULL) )
> + {
> + printk(XENLOG_ERR " found both reg and module-index for module\n");
> + return -EINVAL;
> + }
> +
> + ret = read_fdt_prop_as_reg(prop, address_cells, size_cells, &addr, &size);
> + if ( ret < 0 )
> + {
> + printk(XENLOG_ERR " failed reading reg for multiboot,module\n");
> + return -EINVAL;
> + }
> +
> + idx = bi->nr_modules;
> + if ( idx > MAX_NR_BOOTMODS )
> + {
> + /*
> + * MAX_NR_BOOTMODS must fit in 31 bits so it's representable in the
> + * positive side of an int; for the return value.
> + */
> + BUILD_BUG_ON(MAX_NR_BOOTMODS > (uint64_t)INT_MAX);
> + printk(XENLOG_ERR " idx=%u exceeds len=%u\n", idx, MAX_NR_BOOTMODS);
> + return -EINVAL;
> + }
> +
> + /*
> + * Append new module to the existing list
> + *
> + * Note that bi->nr_modules points to Xen itself, so we must shift it first
> + */
> + bi->nr_modules++;
> + bi->mods[bi->nr_modules] = bi->mods[idx];
> + bi->mods[idx] = (struct boot_module){
> + .start = addr,
> + .size = size,
> + };
> +
> + printk(XENLOG_INFO " module[%u]: addr %lx size %lx\n", idx, addr, size);
> +
> + return idx;
> +}
> +
> static int __init find_hyperlaunch_node(const void *fdt)
> {
> int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
> index 955aead497..8c98a256eb 100644
> --- a/xen/common/domain-builder/fdt.h
> +++ b/xen/common/domain-builder/fdt.h
> @@ -2,6 +2,8 @@
> #ifndef __XEN_DOMAIN_BUILDER_FDT_H__
> #define __XEN_DOMAIN_BUILDER_FDT_H__
>
> +#include <xen/libfdt/libfdt-xen.h>
> +
> struct boot_info;
>
> /* hyperlaunch fdt is required to be module 0 */
> diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
> index ac2b84775d..ace6b6875b 100644
> --- a/xen/include/xen/domain-builder.h
> +++ b/xen/include/xen/domain-builder.h
> @@ -5,5 +5,8 @@
> struct boot_info;
>
> void builder_init(struct boot_info *bi);
> +int fdt_read_multiboot_module(const void *fdt, int node,
> + int address_cells, int size_cells,
> + struct boot_info *bi)
>
> #endif /* __XEN_DOMAIN_BUILDER_H__ */
> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> index a5340bc9f4..deafb25d98 100644
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -12,6 +12,17 @@
> #define LIBFDT_XEN_H
>
> #include <xen/libfdt/libfdt.h>
> +#include <xen/errno.h>
> +
> +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;
> +
> + *val = fdt32_to_cpu(*(const fdt32_t *)prop->data);
> + return 0;
> +}
My understanding is domain builder establishes its own shims around libfdt so
libfdt is kept unmodified and it is easier to pick up libfdt updates.
So, IMO, this function should reside in xen/common/domain-builder/fdt.c
Thoughts?
>
> static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
> paddr_t *address,
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 05/13] x86/hyperlaunch: Add helpers to locate multiboot modules
2025-04-18 22:30 ` dmkhn
@ 2025-04-23 12:12 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 12:12 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Roger Pau Monné,
Stefano Stabellini, Bertrand Marquis, Jason Andryuk
On Fri Apr 18, 2025 at 11:30 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:27PM +0100, Alejandro Vallejo wrote:
>> Hyperlaunch mandates either a reg or module-index DT prop on nodes that
>> contain `multiboot,module" under their "compatible" prop. This patch
>> introduces a helper to generically find such index, appending the module
>> to the list of modules if it wasn't already (i.e: because it's given via
>> the "reg" prop).
>>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> v4:
>> * Remove stray reg prop parser in libfdt-xen.h.
>> * Remove fdt32_as_uX accessors.
>> * Brough fdt_prop_as_u32() accesor from later patches.
>> * So it can be used in place of a hardcoded fdt32_as_u32().
>> * Limited MAX_NR_BOOTMODS to INT_MAX.
>> * Preserved BOOTMOD_XEN on module append logic.
>> * Add missing bounds check to module-index parsed in multiboot module helper.
>> * Converted idx variable to uint32_t for better bounds checking.
>> * Braces from switch statement to conform to coding style.
>> * Added missing XENLOG_X.
>> * Print address_cells and size_cells on error parsing reg properties.
>> * Added (transient) missing declaration for extern helper.
>> * becomes static on the next patch.
>> ---
>> xen/common/domain-builder/fdt.c | 162 ++++++++++++++++++++++++++++
>> xen/common/domain-builder/fdt.h | 2 +
>> xen/include/xen/domain-builder.h | 3 +
>> xen/include/xen/libfdt/libfdt-xen.h | 11 ++
>> 4 files changed, 178 insertions(+)
>>
>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
>> index b5ff8220da..d73536fed6 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -13,6 +13,168 @@
>>
>> #include "fdt.h"
>>
>> +/*
>> + * Unpacks a "reg" property into its address and size constituents.
>> + *
>> + * @param prop Pointer to an FDT "reg" property.
>> + * @param address_cells Number of 4-octet cells that make up an "address".
>> + * @param size_cells Number of 4-octet cells that make up a "size".
>> + * @param p_addr[out] Address encoded in the property.
>> + * @param p_size[out] Size encoded in the property.
>> + * @returns -EINVAL on malformed property, 0 otherwise.
>> + */
>> +static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
>
> I would do s/read_fdt_prop_as_reg/fdt_prop_as_reg/ similar to fdt_prop_as_u32()
> below.
Yes, that sounds better.
>
>> + int address_cells, int size_cells,
>> + uint64_t *p_addr, uint64_t *p_size)
>> +{
>> + const fdt32_t *cell = (const fdt32_t *)prop->data;
>> + uint64_t addr, size;
>> +
>> + if ( fdt32_to_cpu(prop->len) !=
>> + (address_cells + size_cells) * sizeof(*cell) )
>> + {
>> + printk(XENLOG_ERR " cannot read reg %lu+%lu from prop len %u\n",
>> + address_cells * sizeof(*cell), size_cells * sizeof(*cell),
>> + fdt32_to_cpu(prop->len));
>> + return -EINVAL;
>> + }
>> +
>> + switch ( address_cells )
>> + {
>> + case 1:
>> + addr = fdt32_to_cpu(*cell);
>> + break;
>> + case 2:
>> + addr = fdt64_to_cpu(*(const fdt64_t *)cell);
>> + break;
>> + default:
>> + printk(XENLOG_ERR " unsupported address_cells=%d\n", address_cells);
>> + return -EINVAL;
>> + }
>> +
>> + cell += address_cells;
>> + switch ( size_cells )
>> + {
>> + case 1:
>> + size = fdt32_to_cpu(*cell);
>> + break;
>> + case 2:
>> + size = fdt64_to_cpu(*(const fdt64_t *)cell);
>> + break;
>> + default:
>> + printk(XENLOG_ERR " unsupported size_cells=%d\n", size_cells);
>> + return -EINVAL;
>> + }
>> +
>> + *p_addr = addr;
>> + *p_size = size;
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Locate a multiboot module given its node offset in the FDT.
>> + *
>> + * The module location may be given via either FDT property:
>> + * * reg = <address, size>
>> + * * Mutates `bi` to append the module.
>> + * * module-index = <idx>
>> + * * Leaves `bi` unchanged.
>> + *
>> + * @param fdt Pointer to the full FDT.
>> + * @param node Offset for the module node.
>> + * @param address_cells Number of 4-octet cells that make up an "address".
>> + * @param size_cells Number of 4-octet cells that make up a "size".
>> + * @param bi[inout] Xen's representation of the boot parameters.
>> + * @return -EINVAL on malformed nodes, otherwise
>> + * index inside `bi->mods`
>> + */
>> +int __init fdt_read_multiboot_module(const void *fdt, int node,
>> + int address_cells, int size_cells,
>> + struct boot_info *bi)
>> +{
>> + const struct fdt_property *prop;
>> + uint64_t addr, size;
>> + int ret;
>> + uint32_t idx;
>> +
>> + if ( fdt_node_check_compatible(fdt, node, "multiboot,module") )
>> + {
>> + printk(XENLOG_ERR " bad module. multiboot,module not found");
>> + return -ENODATA;
>> + }
>> +
>> + /* Location given as a `module-index` property. */
>> + if ( (prop = fdt_get_property(fdt, node, "module-index", NULL)) )
>> + {
>> + if ( fdt_get_property(fdt, node, "reg", NULL) )
>> + {
>> + printk(XENLOG_ERR " found both reg and module-index for module\n");
>> + return -EINVAL;
>> + }
>> + if ( (ret = fdt_prop_as_u32(prop, &idx)) )
>> + {
>> + printk(XENLOG_ERR " bad module-index prop\n");
>> + return ret;
>> + }
>> + if ( idx >= MAX_NR_BOOTMODS )
>> + {
>> + printk(XENLOG_ERR " module-index overflow. %s=%u\n",
>> + STR(MAX_NR_BOOTMODS), MAX_NR_BOOTMODS);
>> + return -EINVAL;
>> + }
>> +
>> + return idx;
>> + }
>> +
>> + /* Otherwise location given as a `reg` property. */
>> + if ( !(prop = fdt_get_property(fdt, node, "reg", NULL)) )
>> + {
>> + printk(XENLOG_ERR " no location for multiboot,module\n");
>> + return -EINVAL;
>> + }
>> + if ( fdt_get_property(fdt, node, "module-index", NULL) )
>> + {
>> + printk(XENLOG_ERR " found both reg and module-index for module\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = read_fdt_prop_as_reg(prop, address_cells, size_cells, &addr, &size);
>> + if ( ret < 0 )
>> + {
>> + printk(XENLOG_ERR " failed reading reg for multiboot,module\n");
>> + return -EINVAL;
>> + }
>> +
>> + idx = bi->nr_modules;
>> + if ( idx > MAX_NR_BOOTMODS )
>> + {
>> + /*
>> + * MAX_NR_BOOTMODS must fit in 31 bits so it's representable in the
>> + * positive side of an int; for the return value.
>> + */
>> + BUILD_BUG_ON(MAX_NR_BOOTMODS > (uint64_t)INT_MAX);
>> + printk(XENLOG_ERR " idx=%u exceeds len=%u\n", idx, MAX_NR_BOOTMODS);
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Append new module to the existing list
>> + *
>> + * Note that bi->nr_modules points to Xen itself, so we must shift it first
>> + */
>> + bi->nr_modules++;
>> + bi->mods[bi->nr_modules] = bi->mods[idx];
>> + bi->mods[idx] = (struct boot_module){
>> + .start = addr,
>> + .size = size,
>> + };
>> +
>> + printk(XENLOG_INFO " module[%u]: addr %lx size %lx\n", idx, addr, size);
>> +
>> + return idx;
>> +}
>> +
>> static int __init find_hyperlaunch_node(const void *fdt)
>> {
>> int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
>> diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
>> index 955aead497..8c98a256eb 100644
>> --- a/xen/common/domain-builder/fdt.h
>> +++ b/xen/common/domain-builder/fdt.h
>> @@ -2,6 +2,8 @@
>> #ifndef __XEN_DOMAIN_BUILDER_FDT_H__
>> #define __XEN_DOMAIN_BUILDER_FDT_H__
>>
>> +#include <xen/libfdt/libfdt-xen.h>
>> +
>> struct boot_info;
>>
>> /* hyperlaunch fdt is required to be module 0 */
>> diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
>> index ac2b84775d..ace6b6875b 100644
>> --- a/xen/include/xen/domain-builder.h
>> +++ b/xen/include/xen/domain-builder.h
>> @@ -5,5 +5,8 @@
>> struct boot_info;
>>
>> void builder_init(struct boot_info *bi);
>> +int fdt_read_multiboot_module(const void *fdt, int node,
>> + int address_cells, int size_cells,
>> + struct boot_info *bi)
>>
>> #endif /* __XEN_DOMAIN_BUILDER_H__ */
>> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
>> index a5340bc9f4..deafb25d98 100644
>> --- a/xen/include/xen/libfdt/libfdt-xen.h
>> +++ b/xen/include/xen/libfdt/libfdt-xen.h
>> @@ -12,6 +12,17 @@
>> #define LIBFDT_XEN_H
>>
>> #include <xen/libfdt/libfdt.h>
>> +#include <xen/errno.h>
>> +
>> +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;
>> +
>> + *val = fdt32_to_cpu(*(const fdt32_t *)prop->data);
>> + return 0;
>> +}
>
> My understanding is domain builder establishes its own shims around libfdt so
> libfdt is kept unmodified and it is easier to pick up libfdt updates.
>
> So, IMO, this function should reside in xen/common/domain-builder/fdt.c
>
> Thoughts?
Ugh. Too much code motion is not good for your head. I did mean to move
them all (I mentioned it in an earlier response to Jan, I think). Will
do for good this time.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 06/13] x86/hyperlaunch: locate dom0 kernel with hyperlaunch
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (4 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 05/13] x86/hyperlaunch: Add helpers to locate multiboot modules Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 22:39 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree Alejandro Vallejo
` (7 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Look for a subnode of type `multiboot,kernel` within a domain node. If
found, locate it using the multiboot module helper to generically ensure
it lives in the module list. 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>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Stop printing on the fallback path of builder_init().
It's in fact the most common path and just adds noise.
* Add missing XENLOG_X.
* Simplified check to log error on nr_domains != 1.
* s/XENLOG_ERR/XENLOG_WARNING/ on duplicate kernel.
* Turned foo == 0 into !foo in the "multiboot,kernel" check
---
xen/arch/x86/setup.c | 5 ---
xen/common/domain-builder/core.c | 9 +++++
xen/common/domain-builder/fdt.c | 64 ++++++++++++++++++++++++++++++--
xen/include/xen/domain-builder.h | 3 --
4 files changed, 70 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ccc57cc70a..4f669f3c60 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1285,11 +1285,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/common/domain-builder/core.c b/xen/common/domain-builder/core.c
index 3b062e85ec..924cb495a3 100644
--- a/xen/common/domain-builder/core.c
+++ b/xen/common/domain-builder/core.c
@@ -54,6 +54,15 @@ void __init builder_init(struct boot_info *bi)
printk(XENLOG_INFO " number of domains: %u\n", bi->nr_domains);
}
+ else
+ {
+ /* Find first unknown boot module to use as dom0 kernel */
+ unsigned int 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/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index d73536fed6..1fae6add3b 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -89,9 +89,9 @@ static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
* @return -EINVAL on malformed nodes, otherwise
* index inside `bi->mods`
*/
-int __init fdt_read_multiboot_module(const void *fdt, int node,
- int address_cells, int size_cells,
- struct boot_info *bi)
+static int __init fdt_read_multiboot_module(const void *fdt, int node,
+ int address_cells, int size_cells,
+ struct boot_info *bi)
{
const struct fdt_property *prop;
uint64_t addr, size;
@@ -175,6 +175,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node,
return idx;
}
+static int __init process_domain_node(
+ struct boot_info *bi, const 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";
+ int address_cells = fdt_address_cells(fdt, dom_node);
+ int size_cells = fdt_size_cells(fdt, dom_node);
+
+ fdt_for_each_subnode(node, fdt, dom_node)
+ {
+ if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
+ {
+ int idx;
+
+ if ( bd->kernel )
+ {
+ printk(XENLOG_WARNING
+ " duplicate kernel for domain %s\n", name);
+ continue;
+ }
+
+ idx = fdt_read_multiboot_module(fdt, node, address_cells,
+ size_cells, bi);
+ if ( idx < 0 )
+ {
+ printk(XENLOG_ERR
+ " failed processing kernel for domain %s\n", name);
+ return idx;
+ }
+
+ printk(XENLOG_INFO " kernel: multiboot-index=%d\n", idx);
+ bi->mods[idx].type = BOOTMOD_KERNEL;
+ bd->kernel = &bi->mods[idx];
+ }
+ }
+
+ if ( !bd->kernel )
+ {
+ printk(XENLOG_ERR "error: no kernel assigned to domain\n");
+ return -ENODATA;
+ }
+
+ return 0;
+}
+
static int __init find_hyperlaunch_node(const void *fdt)
{
int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
@@ -237,8 +283,20 @@ 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 "warning: only creating first %u domains\n",
+ MAX_NR_BOOTDOMS);
+ break;
+ }
+
if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
+ {
+ 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/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
index ace6b6875b..ac2b84775d 100644
--- a/xen/include/xen/domain-builder.h
+++ b/xen/include/xen/domain-builder.h
@@ -5,8 +5,5 @@
struct boot_info;
void builder_init(struct boot_info *bi);
-int fdt_read_multiboot_module(const void *fdt, int node,
- int address_cells, int size_cells,
- struct boot_info *bi)
#endif /* __XEN_DOMAIN_BUILDER_H__ */
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 06/13] x86/hyperlaunch: locate dom0 kernel with hyperlaunch
2025-04-17 12:48 ` [PATCH v4 06/13] x86/hyperlaunch: locate dom0 kernel with hyperlaunch Alejandro Vallejo
@ 2025-04-18 22:39 ` dmkhn
2025-04-23 12:16 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 22:39 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:28PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> Look for a subnode of type `multiboot,kernel` within a domain node. If
> found, locate it using the multiboot module helper to generically ensure
> it lives in the module list. 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>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> v4:
> * Stop printing on the fallback path of builder_init().
> It's in fact the most common path and just adds noise.
> * Add missing XENLOG_X.
> * Simplified check to log error on nr_domains != 1.
> * s/XENLOG_ERR/XENLOG_WARNING/ on duplicate kernel.
> * Turned foo == 0 into !foo in the "multiboot,kernel" check
> ---
> xen/arch/x86/setup.c | 5 ---
> xen/common/domain-builder/core.c | 9 +++++
> xen/common/domain-builder/fdt.c | 64 ++++++++++++++++++++++++++++++--
> xen/include/xen/domain-builder.h | 3 --
> 4 files changed, 70 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index ccc57cc70a..4f669f3c60 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1285,11 +1285,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/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> index 3b062e85ec..924cb495a3 100644
> --- a/xen/common/domain-builder/core.c
> +++ b/xen/common/domain-builder/core.c
> @@ -54,6 +54,15 @@ void __init builder_init(struct boot_info *bi)
>
> printk(XENLOG_INFO " number of domains: %u\n", bi->nr_domains);
> }
> + else
> + {
> + /* Find first unknown boot module to use as dom0 kernel */
> + unsigned int 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/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index d73536fed6..1fae6add3b 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -89,9 +89,9 @@ static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
> * @return -EINVAL on malformed nodes, otherwise
> * index inside `bi->mods`
> */
> -int __init fdt_read_multiboot_module(const void *fdt, int node,
> - int address_cells, int size_cells,
> - struct boot_info *bi)
> +static int __init fdt_read_multiboot_module(const void *fdt, int node,
> + int address_cells, int size_cells,
> + struct boot_info *bi)
> {
> const struct fdt_property *prop;
> uint64_t addr, size;
> @@ -175,6 +175,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node,
> return idx;
> }
>
> +static int __init process_domain_node(
> + struct boot_info *bi, const 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";
> + int address_cells = fdt_address_cells(fdt, dom_node);
> + int size_cells = fdt_size_cells(fdt, dom_node);
> +
> + fdt_for_each_subnode(node, fdt, dom_node)
> + {
> + if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
Suggest to restructure the code to reduce levels of indentation, e.g.:
int idx;
if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
continue;
if ( bd->kernel )
...
> + {
> + int idx;
> +
> + if ( bd->kernel )
> + {
> + printk(XENLOG_WARNING
> + " duplicate kernel for domain %s\n", name);
> + continue;
> + }
> +
> + idx = fdt_read_multiboot_module(fdt, node, address_cells,
> + size_cells, bi);
> + if ( idx < 0 )
> + {
> + printk(XENLOG_ERR
> + " failed processing kernel for domain %s\n", name);
> + return idx;
> + }
> +
> + printk(XENLOG_INFO " kernel: multiboot-index=%d\n", idx);
> + bi->mods[idx].type = BOOTMOD_KERNEL;
> + bd->kernel = &bi->mods[idx];
> + }
> + }
> +
> + if ( !bd->kernel )
> + {
> + printk(XENLOG_ERR "error: no kernel assigned to domain\n");
Add domain name printout similar to above logging?
> + return -ENODATA;
> + }
> +
> + return 0;
> +}
> +
> static int __init find_hyperlaunch_node(const void *fdt)
> {
> int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
> @@ -237,8 +283,20 @@ 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 "warning: only creating first %u domains\n",
> + MAX_NR_BOOTDOMS);
> + break;
> + }
> +
> if ( !fdt_node_check_compatible(fdt, node, "xen,domain") )
> + {
> + 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/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
> index ace6b6875b..ac2b84775d 100644
> --- a/xen/include/xen/domain-builder.h
> +++ b/xen/include/xen/domain-builder.h
> @@ -5,8 +5,5 @@
> struct boot_info;
>
> void builder_init(struct boot_info *bi);
> -int fdt_read_multiboot_module(const void *fdt, int node,
> - int address_cells, int size_cells,
> - struct boot_info *bi)
>
> #endif /* __XEN_DOMAIN_BUILDER_H__ */
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 06/13] x86/hyperlaunch: locate dom0 kernel with hyperlaunch
2025-04-18 22:39 ` dmkhn
@ 2025-04-23 12:16 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 12:16 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Fri Apr 18, 2025 at 11:39 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:28PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> Look for a subnode of type `multiboot,kernel` within a domain node. If
>> found, locate it using the multiboot module helper to generically ensure
>> it lives in the module list. 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>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> v4:
>> * Stop printing on the fallback path of builder_init().
>> It's in fact the most common path and just adds noise.
>> * Add missing XENLOG_X.
>> * Simplified check to log error on nr_domains != 1.
>> * s/XENLOG_ERR/XENLOG_WARNING/ on duplicate kernel.
>> * Turned foo == 0 into !foo in the "multiboot,kernel" check
>> ---
>> xen/arch/x86/setup.c | 5 ---
>> xen/common/domain-builder/core.c | 9 +++++
>> xen/common/domain-builder/fdt.c | 64 ++++++++++++++++++++++++++++++--
>> xen/include/xen/domain-builder.h | 3 --
>> 4 files changed, 70 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index ccc57cc70a..4f669f3c60 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1285,11 +1285,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/common/domain-builder/core.c b/xen/common/domain-builder/core.c
>> index 3b062e85ec..924cb495a3 100644
>> --- a/xen/common/domain-builder/core.c
>> +++ b/xen/common/domain-builder/core.c
>> @@ -54,6 +54,15 @@ void __init builder_init(struct boot_info *bi)
>>
>> printk(XENLOG_INFO " number of domains: %u\n", bi->nr_domains);
>> }
>> + else
>> + {
>> + /* Find first unknown boot module to use as dom0 kernel */
>> + unsigned int 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/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
>> index d73536fed6..1fae6add3b 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -89,9 +89,9 @@ static int __init read_fdt_prop_as_reg(const struct fdt_property *prop,
>> * @return -EINVAL on malformed nodes, otherwise
>> * index inside `bi->mods`
>> */
>> -int __init fdt_read_multiboot_module(const void *fdt, int node,
>> - int address_cells, int size_cells,
>> - struct boot_info *bi)
>> +static int __init fdt_read_multiboot_module(const void *fdt, int node,
>> + int address_cells, int size_cells,
>> + struct boot_info *bi)
>> {
>> const struct fdt_property *prop;
>> uint64_t addr, size;
>> @@ -175,6 +175,52 @@ int __init fdt_read_multiboot_module(const void *fdt, int node,
>> return idx;
>> }
>>
>> +static int __init process_domain_node(
>> + struct boot_info *bi, const 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";
>> + int address_cells = fdt_address_cells(fdt, dom_node);
>> + int size_cells = fdt_size_cells(fdt, dom_node);
>> +
>> + fdt_for_each_subnode(node, fdt, dom_node)
>> + {
>> + if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
>
> Suggest to restructure the code to reduce levels of indentation, e.g.:
>
> int idx;
>
> if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
> continue;
>
> if ( bd->kernel )
> ...
>
This hunk checking for "multiboot,kernel" is part of an if-elseif in a
later patch that also checks for "multiboot,ramdisk", so we can't just
do an early continue here without forcing a bigger diff later on.
>
>> + {
>> + int idx;
>> +
>> + if ( bd->kernel )
>> + {
>> + printk(XENLOG_WARNING
>> + " duplicate kernel for domain %s\n", name);
>> + continue;
>> + }
>> +
>> + idx = fdt_read_multiboot_module(fdt, node, address_cells,
>> + size_cells, bi);
>> + if ( idx < 0 )
>> + {
>> + printk(XENLOG_ERR
>> + " failed processing kernel for domain %s\n", name);
>> + return idx;
>> + }
>> +
>> + printk(XENLOG_INFO " kernel: multiboot-index=%d\n", idx);
>> + bi->mods[idx].type = BOOTMOD_KERNEL;
>> + bd->kernel = &bi->mods[idx];
>> + }
>> + }
>> +
>> + if ( !bd->kernel )
>> + {
>> + printk(XENLOG_ERR "error: no kernel assigned to domain\n");
>
> Add domain name printout similar to above logging?
Good point.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (5 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 06/13] x86/hyperlaunch: locate dom0 kernel with hyperlaunch Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 22:53 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 08/13] x86/hyperlaunch: locate dom0 initrd with hyperlaunch Alejandro Vallejo
` (6 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Bertrand Marquis, Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Add support to read the command line from the hyperlauunch device tree.
The device tree command line is located in the "bootargs" property of the
"multiboot,kernel" node.
A boot loader command line, e.g. a grub module string field, takes
precendence ove the device tree one since it is easier to modify.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Removed __init from header declarations.
* Removed ifdef guards, as DCE ensures the missing definitions don't
matter.
* Removed ifdef guards from new helpers in domain-builder/fdt.c
* Rely on DCE instead.
* Restored checks for cmdline_pa being zero.
* swapped offset and poffset varnames in libfdt-xen.h helper.
* swapped name and pname varnames in libfdt-xen.h helper.
* Turned foo == 0 into !foo in libfdt-xen.h helper.
---
xen/arch/x86/include/asm/bootinfo.h | 6 ++++--
xen/arch/x86/setup.c | 10 +++++++--
xen/common/domain-builder/core.c | 32 +++++++++++++++++++++++++++++
xen/common/domain-builder/fdt.c | 6 ++++++
xen/common/domain-builder/fdt.h | 24 ++++++++++++++++++++++
xen/include/xen/domain-builder.h | 4 ++++
xen/include/xen/libfdt/libfdt-xen.h | 23 +++++++++++++++++++++
7 files changed, 101 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index 1e3d582e45..5b2c93b1ef 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/setup.c b/xen/arch/x86/setup.c
index 4f669f3c60..4cae13163b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -984,7 +984,10 @@ static size_t __init domain_cmdline_size(const struct boot_info *bi,
{
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 if ( bd->kernel->cmdline_pa )
+ s += strlen(__va(bd->kernel->cmdline_pa));
if ( s == 0 )
return s;
@@ -1047,7 +1050,10 @@ 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 if ( bd->kernel->cmdline_pa )
strlcpy(cmdline,
cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
cmdline_size);
diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
index 924cb495a3..4b4230f2ff 100644
--- a/xen/common/domain-builder/core.c
+++ b/xen/common/domain-builder/core.c
@@ -8,9 +8,41 @@
#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)
+{
+ const void *fdt;
+ int size;
+
+ if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
+ return 0;
+
+ fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+ size = fdt_cmdline_prop_size(fdt, offset);
+ bootstrap_unmap();
+
+ return max(0, size);
+}
+
+int __init builder_get_cmdline(
+ struct boot_info *bi, int offset, char *cmdline, size_t size)
+{
+ const void *fdt;
+ int ret;
+
+ if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
+ return 0;
+
+ fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
+ ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
+ bootstrap_unmap();
+
+ return ret;
+}
+
void __init builder_init(struct boot_info *bi)
{
bi->hyperlaunch_enabled = false;
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 1fae6add3b..50fde2d007 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -209,6 +209,12 @@ static int __init process_domain_node(
printk(XENLOG_INFO " kernel: multiboot-index=%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] )
+ bd->kernel->fdt_cmdline = fdt_get_prop_offset(
+ fdt, node, "bootargs", &bd->kernel->cmdline_pa);
}
}
diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
index 8c98a256eb..d2ae7d5c36 100644
--- a/xen/common/domain-builder/fdt.h
+++ b/xen/common/domain-builder/fdt.h
@@ -9,6 +9,30 @@ struct boot_info;
/* hyperlaunch fdt is required to be module 0 */
#define HYPERLAUNCH_MODULE_IDX 0
+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(const struct boot_info *bi);
int walk_hyperlaunch_fdt(struct boot_info *bi);
diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
index ac2b84775d..ac0fd8f60b 100644
--- a/xen/include/xen/domain-builder.h
+++ b/xen/include/xen/domain-builder.h
@@ -4,6 +4,10 @@
struct boot_info;
+size_t builder_get_cmdline_size(const struct boot_info *bi, int offset);
+int builder_get_cmdline(const struct boot_info *bi, int offset,
+ char *cmdline, size_t size);
+
void builder_init(struct boot_info *bi);
#endif /* __XEN_DOMAIN_BUILDER_H__ */
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index deafb25d98..afc76e7750 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -24,6 +24,29 @@ static inline int __init fdt_prop_as_u32(
return 0;
}
+static inline bool __init fdt_get_prop_offset(
+ const void *fdt, int node, const char *pname, unsigned long *poffset)
+{
+ int ret, offset;
+ const char *name;
+
+ fdt_for_each_property_offset(offset, fdt, node)
+ {
+ fdt_getprop_by_offset(fdt, offset, &name, &ret);
+
+ if ( ret < 0 )
+ continue;
+
+ if ( !strcmp(name, pname) )
+ {
+ *poffset = offset;
+ return true;
+ }
+ }
+
+ return false;
+}
+
static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
paddr_t *address,
paddr_t *size)
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree
2025-04-17 12:48 ` [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree Alejandro Vallejo
@ 2025-04-18 22:53 ` dmkhn
2025-04-23 13:01 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 22:53 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Bertrand Marquis, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:29PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> Add support to read the command line from the hyperlauunch device tree.
> The device tree command line is located in the "bootargs" property of the
> "multiboot,kernel" node.
>
> A boot loader command line, e.g. a grub module string field, takes
> precendence ove the device tree one since it is easier to modify.
^^ over
Also, I would explain the command line parsing rules in the code commentary for
domain_cmdline_size() below.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> v4:
> * Removed __init from header declarations.
> * Removed ifdef guards, as DCE ensures the missing definitions don't
> matter.
> * Removed ifdef guards from new helpers in domain-builder/fdt.c
> * Rely on DCE instead.
> * Restored checks for cmdline_pa being zero.
> * swapped offset and poffset varnames in libfdt-xen.h helper.
> * swapped name and pname varnames in libfdt-xen.h helper.
> * Turned foo == 0 into !foo in libfdt-xen.h helper.
> ---
> xen/arch/x86/include/asm/bootinfo.h | 6 ++++--
> xen/arch/x86/setup.c | 10 +++++++--
> xen/common/domain-builder/core.c | 32 +++++++++++++++++++++++++++++
> xen/common/domain-builder/fdt.c | 6 ++++++
> xen/common/domain-builder/fdt.h | 24 ++++++++++++++++++++++
> xen/include/xen/domain-builder.h | 4 ++++
> xen/include/xen/libfdt/libfdt-xen.h | 23 +++++++++++++++++++++
> 7 files changed, 101 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index 1e3d582e45..5b2c93b1ef 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/setup.c b/xen/arch/x86/setup.c
> index 4f669f3c60..4cae13163b 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -984,7 +984,10 @@ static size_t __init domain_cmdline_size(const struct boot_info *bi,
> {
> 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 if ( bd->kernel->cmdline_pa )
> + s += strlen(__va(bd->kernel->cmdline_pa));
>
> if ( s == 0 )
> return s;
> @@ -1047,7 +1050,10 @@ 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 if ( bd->kernel->cmdline_pa )
> strlcpy(cmdline,
> cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
> cmdline_size);
> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> index 924cb495a3..4b4230f2ff 100644
> --- a/xen/common/domain-builder/core.c
> +++ b/xen/common/domain-builder/core.c
> @@ -8,9 +8,41 @@
> #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)
> +{
> + const void *fdt;
> + int size;
> +
> + if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> + return 0;
> +
> + fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> + size = fdt_cmdline_prop_size(fdt, offset);
> + bootstrap_unmap();
> +
> + return max(0, size);
> +}
> +
> +int __init builder_get_cmdline(
> + struct boot_info *bi, int offset, char *cmdline, size_t size)
> +{
> + const void *fdt;
> + int ret;
> +
> + if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
> + return 0;
> +
> + fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
> + ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
> + bootstrap_unmap();
> +
> + return ret;
> +}
> +
> void __init builder_init(struct boot_info *bi)
> {
> bi->hyperlaunch_enabled = false;
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index 1fae6add3b..50fde2d007 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -209,6 +209,12 @@ static int __init process_domain_node(
> printk(XENLOG_INFO " kernel: multiboot-index=%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] )
> + bd->kernel->fdt_cmdline = fdt_get_prop_offset(
> + fdt, node, "bootargs", &bd->kernel->cmdline_pa);
> }
> }
>
> diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
> index 8c98a256eb..d2ae7d5c36 100644
> --- a/xen/common/domain-builder/fdt.h
> +++ b/xen/common/domain-builder/fdt.h
> @@ -9,6 +9,30 @@ struct boot_info;
> /* hyperlaunch fdt is required to be module 0 */
> #define HYPERLAUNCH_MODULE_IDX 0
>
> +static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
> +{
> + int ret;
> +
> + fdt_get_property_by_offset(fdt, offset, &ret);
Perhaps something like
(void)fdt_get_property_by_offset...
since there's no need to check the return value?
> +
> + 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(const struct boot_info *bi);
> int walk_hyperlaunch_fdt(struct boot_info *bi);
>
> diff --git a/xen/include/xen/domain-builder.h b/xen/include/xen/domain-builder.h
> index ac2b84775d..ac0fd8f60b 100644
> --- a/xen/include/xen/domain-builder.h
> +++ b/xen/include/xen/domain-builder.h
> @@ -4,6 +4,10 @@
>
> struct boot_info;
>
> +size_t builder_get_cmdline_size(const struct boot_info *bi, int offset);
> +int builder_get_cmdline(const struct boot_info *bi, int offset,
> + char *cmdline, size_t size);
> +
> void builder_init(struct boot_info *bi);
>
> #endif /* __XEN_DOMAIN_BUILDER_H__ */
> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> index deafb25d98..afc76e7750 100644
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -24,6 +24,29 @@ static inline int __init fdt_prop_as_u32(
> return 0;
> }
>
> +static inline bool __init fdt_get_prop_offset(
> + const void *fdt, int node, const char *pname, unsigned long *poffset)
> +{
> + int ret, offset;
> + const char *name;
> +
> + fdt_for_each_property_offset(offset, fdt, node)
> + {
> + fdt_getprop_by_offset(fdt, offset, &name, &ret);
Add
(void)fdt_get_property_by_offset...
here as well?
> +
> + if ( ret < 0 )
> + continue;
> +
> + if ( !strcmp(name, pname) )
> + {
> + *poffset = offset;
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> static inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
> paddr_t *address,
> paddr_t *size)
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree
2025-04-18 22:53 ` dmkhn
@ 2025-04-23 13:01 ` Alejandro Vallejo
2025-04-23 13:08 ` Jan Beulich
0 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 13:01 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Bertrand Marquis, Jason Andryuk
On Fri Apr 18, 2025 at 11:53 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:29PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> Add support to read the command line from the hyperlauunch device tree.
>> The device tree command line is located in the "bootargs" property of the
>> "multiboot,kernel" node.
>>
>> A boot loader command line, e.g. a grub module string field, takes
>> precendence ove the device tree one since it is easier to modify.
>
> ^^ over
Oops. yes.
>
> Also, I would explain the command line parsing rules in the code commentary for
> domain_cmdline_size() below.
As in, also state the contents of the commit message in the function
header?
>
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> v4:
>> * Removed __init from header declarations.
>> * Removed ifdef guards, as DCE ensures the missing definitions don't
>> matter.
>> * Removed ifdef guards from new helpers in domain-builder/fdt.c
>> * Rely on DCE instead.
>> * Restored checks for cmdline_pa being zero.
>> * swapped offset and poffset varnames in libfdt-xen.h helper.
>> * swapped name and pname varnames in libfdt-xen.h helper.
>> * Turned foo == 0 into !foo in libfdt-xen.h helper.
>> ---
>> xen/arch/x86/include/asm/bootinfo.h | 6 ++++--
>> xen/arch/x86/setup.c | 10 +++++++--
>> xen/common/domain-builder/core.c | 32 +++++++++++++++++++++++++++++
>> xen/common/domain-builder/fdt.c | 6 ++++++
>> xen/common/domain-builder/fdt.h | 24 ++++++++++++++++++++++
>> xen/include/xen/domain-builder.h | 4 ++++
>> xen/include/xen/libfdt/libfdt-xen.h | 23 +++++++++++++++++++++
>> 7 files changed, 101 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
>> index 1e3d582e45..5b2c93b1ef 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/setup.c b/xen/arch/x86/setup.c
>> index 4f669f3c60..4cae13163b 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -984,7 +984,10 @@ static size_t __init domain_cmdline_size(const struct boot_info *bi,
>> {
>> 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 if ( bd->kernel->cmdline_pa )
>> + s += strlen(__va(bd->kernel->cmdline_pa));
>>
>> if ( s == 0 )
>> return s;
>> @@ -1047,7 +1050,10 @@ 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 if ( bd->kernel->cmdline_pa )
>> strlcpy(cmdline,
>> cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader),
>> cmdline_size);
>> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
>> index 924cb495a3..4b4230f2ff 100644
>> --- a/xen/common/domain-builder/core.c
>> +++ b/xen/common/domain-builder/core.c
>> @@ -8,9 +8,41 @@
>> #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)
>> +{
>> + const void *fdt;
>> + int size;
>> +
>> + if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>> + return 0;
>> +
>> + fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>> + size = fdt_cmdline_prop_size(fdt, offset);
>> + bootstrap_unmap();
>> +
>> + return max(0, size);
>> +}
>> +
>> +int __init builder_get_cmdline(
>> + struct boot_info *bi, int offset, char *cmdline, size_t size)
>> +{
>> + const void *fdt;
>> + int ret;
>> +
>> + if ( !IS_ENABLED(CONFIG_DOMAIN_BUILDER) )
>> + return 0;
>> +
>> + fdt = bootstrap_map_bm(&bi->mods[HYPERLAUNCH_MODULE_IDX]);
>> + ret = fdt_cmdline_prop_copy(fdt, offset, cmdline, size);
>> + bootstrap_unmap();
>> +
>> + return ret;
>> +}
>> +
>> void __init builder_init(struct boot_info *bi)
>> {
>> bi->hyperlaunch_enabled = false;
>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
>> index 1fae6add3b..50fde2d007 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -209,6 +209,12 @@ static int __init process_domain_node(
>> printk(XENLOG_INFO " kernel: multiboot-index=%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] )
>> + bd->kernel->fdt_cmdline = fdt_get_prop_offset(
>> + fdt, node, "bootargs", &bd->kernel->cmdline_pa);
>> }
>> }
>>
>> diff --git a/xen/common/domain-builder/fdt.h b/xen/common/domain-builder/fdt.h
>> index 8c98a256eb..d2ae7d5c36 100644
>> --- a/xen/common/domain-builder/fdt.h
>> +++ b/xen/common/domain-builder/fdt.h
>> @@ -9,6 +9,30 @@ struct boot_info;
>> /* hyperlaunch fdt is required to be module 0 */
>> #define HYPERLAUNCH_MODULE_IDX 0
>>
>> +static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
>> +{
>> + int ret;
>> +
>> + fdt_get_property_by_offset(fdt, offset, &ret);
>
> Perhaps something like
>
> (void)fdt_get_property_by_offset...
>
> since there's no need to check the return value?
I vaguely seem to remember doing something like that a few years ago
(because it does show intent and many linters require it) and being told
not to. But maybe I misremember. I'm definitely happy to use that
convention here and later unless someone pushes back for some reason.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree
2025-04-23 13:01 ` Alejandro Vallejo
@ 2025-04-23 13:08 ` Jan Beulich
2025-04-24 5:11 ` dmkhn
0 siblings, 1 reply; 46+ messages in thread
From: Jan Beulich @ 2025-04-23 13:08 UTC (permalink / raw)
To: Alejandro Vallejo, dmkhn
Cc: xen-devel, Daniel P. Smith, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Bertrand Marquis, Jason Andryuk
On 23.04.2025 15:01, Alejandro Vallejo wrote:
> On Fri Apr 18, 2025 at 11:53 PM BST, dmkhn wrote:
>> On Thu, Apr 17, 2025 at 01:48:29PM +0100, Alejandro Vallejo wrote:
>>> --- a/xen/common/domain-builder/fdt.h
>>> +++ b/xen/common/domain-builder/fdt.h
>>> @@ -9,6 +9,30 @@ struct boot_info;
>>> /* hyperlaunch fdt is required to be module 0 */
>>> #define HYPERLAUNCH_MODULE_IDX 0
>>>
>>> +static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
>>> +{
>>> + int ret;
>>> +
>>> + fdt_get_property_by_offset(fdt, offset, &ret);
>>
>> Perhaps something like
>>
>> (void)fdt_get_property_by_offset...
>>
>> since there's no need to check the return value?
>
> I vaguely seem to remember doing something like that a few years ago
> (because it does show intent and many linters require it) and being told
> not to. But maybe I misremember. I'm definitely happy to use that
> convention here and later unless someone pushes back for some reason.
Unless we settle on the need for such for Misra's sake, I'd like to ask
to avoid them. We generally try to avoid casts as much as possible. We
then also shouldn't add ones like suggested here.
Jan
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree
2025-04-23 13:08 ` Jan Beulich
@ 2025-04-24 5:11 ` dmkhn
0 siblings, 0 replies; 46+ messages in thread
From: dmkhn @ 2025-04-24 5:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Alejandro Vallejo, xen-devel, Daniel P. Smith, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Bertrand Marquis, Jason Andryuk
On Wed, Apr 23, 2025 at 03:08:18PM +0200, Jan Beulich wrote:
> On 23.04.2025 15:01, Alejandro Vallejo wrote:
> > On Fri Apr 18, 2025 at 11:53 PM BST, dmkhn wrote:
> >> On Thu, Apr 17, 2025 at 01:48:29PM +0100, Alejandro Vallejo wrote:
> >>> --- a/xen/common/domain-builder/fdt.h
> >>> +++ b/xen/common/domain-builder/fdt.h
> >>> @@ -9,6 +9,30 @@ struct boot_info;
> >>> /* hyperlaunch fdt is required to be module 0 */
> >>> #define HYPERLAUNCH_MODULE_IDX 0
> >>>
> >>> +static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
> >>> +{
> >>> + int ret;
> >>> +
> >>> + fdt_get_property_by_offset(fdt, offset, &ret);
> >>
> >> Perhaps something like
> >>
> >> (void)fdt_get_property_by_offset...
> >>
> >> since there's no need to check the return value?
> >
> > I vaguely seem to remember doing something like that a few years ago
> > (because it does show intent and many linters require it) and being told
> > not to. But maybe I misremember. I'm definitely happy to use that
> > convention here and later unless someone pushes back for some reason.
>
> Unless we settle on the need for such for Misra's sake, I'd like to ask
> to avoid them. We generally try to avoid casts as much as possible. We
> then also shouldn't add ones like suggested here.
Ack.
>
> Jan
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 08/13] x86/hyperlaunch: locate dom0 initrd with hyperlaunch
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (6 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 07/13] x86/hyperlaunch: obtain cmdline from device tree Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 22:58 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 09/13] x86/hyperlaunch: add domain id parsing to domain config Alejandro Vallejo
` (5 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Look for a subnode of type `multiboot,ramdisk` within a domain node and
parse via the fdt_read_multiboot_module() helper. After a successful
helper call, the module index is returned and the module is guaranteed
to be in the module list.
Fix unused typo in adjacent comment.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* s/XENLOG_ERR/XENLOG_WARNING/ on duplicate ramdisk.
* Removed stray ")" in printk
* s/else if/if/ (because of continue)
* Removed trailing continue
---
xen/arch/x86/setup.c | 4 ++--
xen/common/domain-builder/fdt.c | 25 +++++++++++++++++++++++++
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4cae13163b..76ceb5221f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -2150,11 +2150,11 @@ void asmlinkage __init noreturn __start_xen(void)
* 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
+ * any remaining unclaimed boot modules, and report them as unused initrd
* candidates.
*/
initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
- if ( initrdidx < MAX_NR_BOOTMODS )
+ if ( !bi->hyperlaunch_enabled && initrdidx < MAX_NR_BOOTMODS )
{
bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
bi->domains[0].module = &bi->mods[initrdidx];
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 50fde2d007..c0f526a4ce 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -216,6 +216,31 @@ static int __init process_domain_node(
bd->kernel->fdt_cmdline = fdt_get_prop_offset(
fdt, node, "bootargs", &bd->kernel->cmdline_pa);
}
+ else if ( !fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") )
+ {
+ int idx;
+
+ if ( bd->module )
+ {
+ printk(XENLOG_WARNING
+ "Duplicate module for domain %s\n", name);
+ continue;
+ }
+
+ idx = fdt_read_multiboot_module(fdt, node, address_cells,
+ size_cells, bi);
+ if ( idx < 0 )
+ {
+ printk(XENLOG_ERR
+ " failed processing module for domain %s\n",
+ name);
+ return -EINVAL;
+ }
+
+ printk(XENLOG_INFO " module: multiboot-index=%d\n", idx);
+ bi->mods[idx].type = BOOTMOD_RAMDISK;
+ bd->module = &bi->mods[idx];
+ }
}
if ( !bd->kernel )
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 08/13] x86/hyperlaunch: locate dom0 initrd with hyperlaunch
2025-04-17 12:48 ` [PATCH v4 08/13] x86/hyperlaunch: locate dom0 initrd with hyperlaunch Alejandro Vallejo
@ 2025-04-18 22:58 ` dmkhn
2025-04-23 13:01 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 22:58 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:30PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> Look for a subnode of type `multiboot,ramdisk` within a domain node and
> parse via the fdt_read_multiboot_module() helper. After a successful
> helper call, the module index is returned and the module is guaranteed
> to be in the module list.
>
> Fix unused typo in adjacent comment.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> v4:
> * s/XENLOG_ERR/XENLOG_WARNING/ on duplicate ramdisk.
> * Removed stray ")" in printk
> * s/else if/if/ (because of continue)
> * Removed trailing continue
> ---
> xen/arch/x86/setup.c | 4 ++--
> xen/common/domain-builder/fdt.c | 25 +++++++++++++++++++++++++
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 4cae13163b..76ceb5221f 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -2150,11 +2150,11 @@ void asmlinkage __init noreturn __start_xen(void)
> * 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
> + * any remaining unclaimed boot modules, and report them as unused initrd
> * candidates.
> */
> initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
> - if ( initrdidx < MAX_NR_BOOTMODS )
> + if ( !bi->hyperlaunch_enabled && initrdidx < MAX_NR_BOOTMODS )
> {
> bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
> bi->domains[0].module = &bi->mods[initrdidx];
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index 50fde2d007..c0f526a4ce 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -216,6 +216,31 @@ static int __init process_domain_node(
> bd->kernel->fdt_cmdline = fdt_get_prop_offset(
> fdt, node, "bootargs", &bd->kernel->cmdline_pa);
> }
> + else if ( !fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") )
> + {
> + int idx;
> +
> + if ( bd->module )
> + {
> + printk(XENLOG_WARNING
> + "Duplicate module for domain %s\n", name);
> + continue;
> + }
> +
> + idx = fdt_read_multiboot_module(fdt, node, address_cells,
> + size_cells, bi);
> + if ( idx < 0 )
> + {
> + printk(XENLOG_ERR
> + " failed processing module for domain %s\n",
> + name);
> + return -EINVAL;
Propagate fdt_read_multiboot_module()'s error to the caller, i.e.:
return idx;
?
> + }
> +
> + printk(XENLOG_INFO " module: multiboot-index=%d\n", idx);
> + bi->mods[idx].type = BOOTMOD_RAMDISK;
> + bd->module = &bi->mods[idx];
> + }
> }
>
> if ( !bd->kernel )
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 08/13] x86/hyperlaunch: locate dom0 initrd with hyperlaunch
2025-04-18 22:58 ` dmkhn
@ 2025-04-23 13:01 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 13:01 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Fri Apr 18, 2025 at 11:58 PM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:30PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> Look for a subnode of type `multiboot,ramdisk` within a domain node and
>> parse via the fdt_read_multiboot_module() helper. After a successful
>> helper call, the module index is returned and the module is guaranteed
>> to be in the module list.
>>
>> Fix unused typo in adjacent comment.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> v4:
>> * s/XENLOG_ERR/XENLOG_WARNING/ on duplicate ramdisk.
>> * Removed stray ")" in printk
>> * s/else if/if/ (because of continue)
>> * Removed trailing continue
>> ---
>> xen/arch/x86/setup.c | 4 ++--
>> xen/common/domain-builder/fdt.c | 25 +++++++++++++++++++++++++
>> 2 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 4cae13163b..76ceb5221f 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -2150,11 +2150,11 @@ void asmlinkage __init noreturn __start_xen(void)
>> * 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
>> + * any remaining unclaimed boot modules, and report them as unused initrd
>> * candidates.
>> */
>> initrdidx = first_boot_module_index(bi, BOOTMOD_UNKNOWN);
>> - if ( initrdidx < MAX_NR_BOOTMODS )
>> + if ( !bi->hyperlaunch_enabled && initrdidx < MAX_NR_BOOTMODS )
>> {
>> bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
>> bi->domains[0].module = &bi->mods[initrdidx];
>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
>> index 50fde2d007..c0f526a4ce 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -216,6 +216,31 @@ static int __init process_domain_node(
>> bd->kernel->fdt_cmdline = fdt_get_prop_offset(
>> fdt, node, "bootargs", &bd->kernel->cmdline_pa);
>> }
>> + else if ( !fdt_node_check_compatible(fdt, node, "multiboot,ramdisk") )
>> + {
>> + int idx;
>> +
>> + if ( bd->module )
>> + {
>> + printk(XENLOG_WARNING
>> + "Duplicate module for domain %s\n", name);
>> + continue;
>> + }
>> +
>> + idx = fdt_read_multiboot_module(fdt, node, address_cells,
>> + size_cells, bi);
>> + if ( idx < 0 )
>> + {
>> + printk(XENLOG_ERR
>> + " failed processing module for domain %s\n",
>> + name);
>> + return -EINVAL;
>
> Propagate fdt_read_multiboot_module()'s error to the caller, i.e.:
>
> return idx;
Sure
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 09/13] x86/hyperlaunch: add domain id parsing to domain config
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (7 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 08/13] x86/hyperlaunch: locate dom0 initrd with hyperlaunch Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 23:08 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 10/13] x86/hyperlaunch: specify dom0 mode with device tree Alejandro Vallejo
` (4 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
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>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Add missing newline
* Adjusted printks
* Checked domid node against list of outstanding domids.
---
xen/arch/x86/setup.c | 5 ++--
xen/common/domain-builder/fdt.c | 51 ++++++++++++++++++++++++++++++++-
2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 76ceb5221f..04ad2d0937 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1033,8 +1033,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/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index c0f526a4ce..0d3c713041 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -2,6 +2,7 @@
/*
* Copyright (C) 2024, Apertus Solutions, LLC
*/
+#include <xen/domain.h>
#include <xen/err.h>
#include <xen/init.h>
#include <xen/lib.h>
@@ -178,12 +179,54 @@ static int __init fdt_read_multiboot_module(const void *fdt, int node,
static int __init process_domain_node(
struct boot_info *bi, const 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";
int address_cells = fdt_address_cells(fdt, dom_node);
int size_cells = fdt_size_cells(fdt, dom_node);
+ fdt_for_each_property_offset(property, fdt, dom_node)
+ {
+ const struct fdt_property *prop;
+ const char *prop_name;
+ int name_len, rc;
+
+ 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) )
+ {
+ uint32_t val = DOMID_INVALID;
+
+ if ( (rc = fdt_prop_as_u32(prop, &val)) )
+ {
+ printk(XENLOG_ERR
+ " failed processing domain id for domain %s\n", name);
+ return rc;
+ }
+ if ( val >= DOMID_FIRST_RESERVED )
+ {
+ printk(XENLOG_ERR " invalid domain id for domain %s\n", name);
+ return -EINVAL;
+ }
+
+ for ( unsigned int i = 0; i < bi->nr_domains; i++ )
+ {
+ if ( bi->domains[i].domid == val )
+ {
+ printk(XENLOG_ERR " duplicate id for domain %s\n", name);
+ return -EINVAL;
+ }
+ }
+
+ bd->domid = val;
+ printk(XENLOG_INFO " domid: %d\n", bd->domid);
+ }
+ }
+
fdt_for_each_subnode(node, fdt, dom_node)
{
if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
@@ -249,6 +292,12 @@ static int __init process_domain_node(
return -ENODATA;
}
+ if ( bd->domid == DOMID_INVALID )
+ bd->domid = get_initial_domain_id();
+ else if ( bd->domid != get_initial_domain_id() )
+ printk(XENLOG_WARNING
+ "Warning: Unsuported boot with missing initial domid\n");
+
return 0;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 09/13] x86/hyperlaunch: add domain id parsing to domain config
2025-04-17 12:48 ` [PATCH v4 09/13] x86/hyperlaunch: add domain id parsing to domain config Alejandro Vallejo
@ 2025-04-18 23:08 ` dmkhn
2025-04-23 13:03 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 23:08 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini
On Thu, Apr 17, 2025 at 01:48:31PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> 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>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> v4:
> * Add missing newline
> * Adjusted printks
> * Checked domid node against list of outstanding domids.
> ---
> xen/arch/x86/setup.c | 5 ++--
> xen/common/domain-builder/fdt.c | 51 ++++++++++++++++++++++++++++++++-
> 2 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 76ceb5221f..04ad2d0937 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1033,8 +1033,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/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index c0f526a4ce..0d3c713041 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -2,6 +2,7 @@
> /*
> * Copyright (C) 2024, Apertus Solutions, LLC
> */
> +#include <xen/domain.h>
> #include <xen/err.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> @@ -178,12 +179,54 @@ static int __init fdt_read_multiboot_module(const void *fdt, int node,
> static int __init process_domain_node(
> struct boot_info *bi, const 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";
> int address_cells = fdt_address_cells(fdt, dom_node);
> int size_cells = fdt_size_cells(fdt, dom_node);
>
> + fdt_for_each_property_offset(property, fdt, dom_node)
> + {
> + const struct fdt_property *prop;
> + const char *prop_name;
> + int name_len, rc;
> +
> + 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) )
> + {
> + uint32_t val = DOMID_INVALID;
> +
> + if ( (rc = fdt_prop_as_u32(prop, &val)) )
> + {
> + printk(XENLOG_ERR
> + " failed processing domain id for domain %s\n", name);
> + return rc;
> + }
> + if ( val >= DOMID_FIRST_RESERVED )
> + {
> + printk(XENLOG_ERR " invalid domain id for domain %s\n", name);
> + return -EINVAL;
> + }
> +
> + for ( unsigned int i = 0; i < bi->nr_domains; i++ )
> + {
> + if ( bi->domains[i].domid == val )
> + {
> + printk(XENLOG_ERR " duplicate id for domain %s\n", name);
> + return -EINVAL;
> + }
> + }
> +
> + bd->domid = val;
> + printk(XENLOG_INFO " domid: %d\n", bd->domid);
> + }
> + }
> +
> fdt_for_each_subnode(node, fdt, dom_node)
> {
> if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
> @@ -249,6 +292,12 @@ static int __init process_domain_node(
> return -ENODATA;
> }
>
> + if ( bd->domid == DOMID_INVALID )
> + bd->domid = get_initial_domain_id();
> + else if ( bd->domid != get_initial_domain_id() )
> + printk(XENLOG_WARNING
> + "Warning: Unsuported boot with missing initial domid\n");
s/Unsuported/Unsupported/
Also, add bd->domid to the printout?
> +
> return 0;
> }
>
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 09/13] x86/hyperlaunch: add domain id parsing to domain config
2025-04-18 23:08 ` dmkhn
@ 2025-04-23 13:03 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 13:03 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini
On Sat Apr 19, 2025 at 12:08 AM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:31PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> 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>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> v4:
>> * Add missing newline
>> * Adjusted printks
>> * Checked domid node against list of outstanding domids.
>> ---
>> xen/arch/x86/setup.c | 5 ++--
>> xen/common/domain-builder/fdt.c | 51 ++++++++++++++++++++++++++++++++-
>> 2 files changed, 53 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 76ceb5221f..04ad2d0937 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1033,8 +1033,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/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
>> index c0f526a4ce..0d3c713041 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -2,6 +2,7 @@
>> /*
>> * Copyright (C) 2024, Apertus Solutions, LLC
>> */
>> +#include <xen/domain.h>
>> #include <xen/err.h>
>> #include <xen/init.h>
>> #include <xen/lib.h>
>> @@ -178,12 +179,54 @@ static int __init fdt_read_multiboot_module(const void *fdt, int node,
>> static int __init process_domain_node(
>> struct boot_info *bi, const 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";
>> int address_cells = fdt_address_cells(fdt, dom_node);
>> int size_cells = fdt_size_cells(fdt, dom_node);
>>
>> + fdt_for_each_property_offset(property, fdt, dom_node)
>> + {
>> + const struct fdt_property *prop;
>> + const char *prop_name;
>> + int name_len, rc;
>> +
>> + 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) )
>> + {
>> + uint32_t val = DOMID_INVALID;
>> +
>> + if ( (rc = fdt_prop_as_u32(prop, &val)) )
>> + {
>> + printk(XENLOG_ERR
>> + " failed processing domain id for domain %s\n", name);
>> + return rc;
>> + }
>> + if ( val >= DOMID_FIRST_RESERVED )
>> + {
>> + printk(XENLOG_ERR " invalid domain id for domain %s\n", name);
>> + return -EINVAL;
>> + }
>> +
>> + for ( unsigned int i = 0; i < bi->nr_domains; i++ )
>> + {
>> + if ( bi->domains[i].domid == val )
>> + {
>> + printk(XENLOG_ERR " duplicate id for domain %s\n", name);
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + bd->domid = val;
>> + printk(XENLOG_INFO " domid: %d\n", bd->domid);
>> + }
>> + }
>> +
>> fdt_for_each_subnode(node, fdt, dom_node)
>> {
>> if ( !fdt_node_check_compatible(fdt, node, "multiboot,kernel") )
>> @@ -249,6 +292,12 @@ static int __init process_domain_node(
>> return -ENODATA;
>> }
>>
>> + if ( bd->domid == DOMID_INVALID )
>> + bd->domid = get_initial_domain_id();
>> + else if ( bd->domid != get_initial_domain_id() )
>> + printk(XENLOG_WARNING
>> + "Warning: Unsuported boot with missing initial domid\n");
>
> s/Unsuported/Unsupported/
Ack
>
> Also, add bd->domid to the printout?
Yes, I'll do a rundown of the error strings and ensure they have
identifying tokens.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 10/13] x86/hyperlaunch: specify dom0 mode with device tree
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (8 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 09/13] x86/hyperlaunch: add domain id parsing to domain config Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 23:10 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 11/13] x86/hyperlaunch: add memory parsing to domain config Alejandro Vallejo
` (3 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
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>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* printk adjustments
* reject nodes with conflicting mode settings
---
xen/arch/x86/include/asm/boot-domain.h | 5 +++++
xen/arch/x86/setup.c | 3 ++-
xen/common/domain-builder/fdt.c | 21 +++++++++++++++++++++
3 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index d7c6042e25..e316d4bcde 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -13,6 +13,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 *module;
const char *cmdline;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 04ad2d0937..05e3d2cf2a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1020,7 +1020,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) ?
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 0d3c713041..6809c7f917 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -225,6 +225,27 @@ static int __init process_domain_node(
bd->domid = val;
printk(XENLOG_INFO " domid: %d\n", bd->domid);
}
+ else if ( !strncmp(prop_name, "mode", name_len) )
+ {
+ if ( (rc = fdt_prop_as_u32(prop, &bd->mode)) )
+ {
+ printk(XENLOG_ERR
+ " failed processing mode for domain %s\n", name);
+ return rc;
+ }
+
+ if ( (bd->mode & BUILD_MODE_PARAVIRT) &&
+ (bd->mode & BUILD_MODE_ENABLE_DM) )
+ {
+ printk(XENLOG_ERR " mode: invalid (pv+hvm)\n");
+ return -EINVAL;
+ }
+
+ printk(XENLOG_INFO " mode: %s\n",
+ (bd->mode & BUILD_MODE_PARAVIRT) ? "pv" :
+ (bd->mode & BUILD_MODE_ENABLE_DM) ? "hvm" :
+ "pvh");
+ }
}
fdt_for_each_subnode(node, fdt, dom_node)
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 10/13] x86/hyperlaunch: specify dom0 mode with device tree
2025-04-17 12:48 ` [PATCH v4 10/13] x86/hyperlaunch: specify dom0 mode with device tree Alejandro Vallejo
@ 2025-04-18 23:10 ` dmkhn
0 siblings, 0 replies; 46+ messages in thread
From: dmkhn @ 2025-04-18 23:10 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:32PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> 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>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> v4:
> * printk adjustments
> * reject nodes with conflicting mode settings
> ---
> xen/arch/x86/include/asm/boot-domain.h | 5 +++++
> xen/arch/x86/setup.c | 3 ++-
> xen/common/domain-builder/fdt.c | 21 +++++++++++++++++++++
> 3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
> index d7c6042e25..e316d4bcde 100644
> --- a/xen/arch/x86/include/asm/boot-domain.h
> +++ b/xen/arch/x86/include/asm/boot-domain.h
> @@ -13,6 +13,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 *module;
> const char *cmdline;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 04ad2d0937..05e3d2cf2a 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1020,7 +1020,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) ?
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index 0d3c713041..6809c7f917 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -225,6 +225,27 @@ static int __init process_domain_node(
> bd->domid = val;
> printk(XENLOG_INFO " domid: %d\n", bd->domid);
> }
> + else if ( !strncmp(prop_name, "mode", name_len) )
> + {
> + if ( (rc = fdt_prop_as_u32(prop, &bd->mode)) )
> + {
> + printk(XENLOG_ERR
> + " failed processing mode for domain %s\n", name);
> + return rc;
> + }
> +
> + if ( (bd->mode & BUILD_MODE_PARAVIRT) &&
> + (bd->mode & BUILD_MODE_ENABLE_DM) )
> + {
> + printk(XENLOG_ERR " mode: invalid (pv+hvm)\n");
> + return -EINVAL;
> + }
> +
> + printk(XENLOG_INFO " mode: %s\n",
> + (bd->mode & BUILD_MODE_PARAVIRT) ? "pv" :
> + (bd->mode & BUILD_MODE_ENABLE_DM) ? "hvm" :
> + "pvh");
> + }
> }
>
> fdt_for_each_subnode(node, fdt, dom_node)
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 11/13] x86/hyperlaunch: add memory parsing to domain config
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (9 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 10/13] x86/hyperlaunch: specify dom0 mode with device tree Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 23:21 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 12/13] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree Alejandro Vallejo
` (2 subsequent siblings)
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Bertrand Marquis, Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
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>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
Can't use KB() rather tha SZ_1K, as that's strictly for literals.
v4:
* Don't override fdt_prop_as_u64() rc on error.
* Add separate printk statements for each memory prop error.
* Fix bug setting up dom0_size, dom0_min_size and dom0_max_size
* Was overriding dom0_size on all 3 paths.
* Pre-initialise max_pages of all boot_domains to be LONG_MAX, just as
dom0_max_size.
* Eventually dom0_max_size drops out in the bigger series, so we
need that logic to be correct.
---
xen/arch/x86/dom0_build.c | 8 ++++++
xen/arch/x86/include/asm/boot-domain.h | 4 +++
xen/arch/x86/setup.c | 5 +++-
xen/common/domain-builder/fdt.c | 36 ++++++++++++++++++++++++++
xen/include/xen/libfdt/libfdt-xen.h | 10 +++++++
5 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 0b467fd4a4..8db24dbc0a 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -627,6 +627,14 @@ int __init construct_dom0(const 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, ULONG_MAX) && bd->mem_pages )
+ dom0_size.nr_pages = bd->mem_pages;
+ if ( !get_memsize(&dom0_min_size, ULONG_MAX) && bd->min_pages )
+ dom0_min_size.nr_pages = bd->min_pages;
+ if ( !get_memsize(&dom0_max_size, ULONG_MAX) && bd->max_pages )
+ dom0_max_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/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index e316d4bcde..fa8ea1cc66 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -18,6 +18,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 *module;
const char *cmdline;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 05e3d2cf2a..455dad454c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -296,7 +296,10 @@ static const char *cmdline_cook(const char *p, const char *loader_name);
struct boot_info __initdata xen_boot_info = {
.loader = "unknown",
.cmdline = "",
- .domains = { [0 ... MAX_NR_BOOTDOMS - 1] = { .domid = DOMID_INVALID } },
+ .domains = { [0 ... MAX_NR_BOOTDOMS - 1] = {
+ .domid = DOMID_INVALID,
+ .max_pages = ULONG_MAX,
+ }},
};
static struct boot_info *__init multiboot_fill_boot_info(
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 6809c7f917..90218d120a 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -7,6 +7,7 @@
#include <xen/init.h>
#include <xen/lib.h>
#include <xen/libfdt/libfdt.h>
+#include <xen/sizes.h>
#include <asm/bootinfo.h>
#include <asm/page.h>
@@ -246,6 +247,41 @@ static int __init process_domain_node(
(bd->mode & BUILD_MODE_ENABLE_DM) ? "hvm" :
"pvh");
}
+ else if ( strncmp(prop_name, "memory", name_len) == 0 )
+ {
+ uint64_t kb;
+ if ( (rc = fdt_prop_as_u64(prop, &kb)) )
+ {
+ printk(XENLOG_ERR " bad \"memory\" prop on domain %s\n", name);
+ return rc;
+ }
+ bd->mem_pages = PFN_DOWN(kb * SZ_1K);
+ printk(XENLOG_ERR " memory: %lu KiB\n", kb);
+ }
+ else if ( !strncmp(prop_name, "mem-min", name_len) )
+ {
+ uint64_t kb;
+ if ( (rc = fdt_prop_as_u64(prop, &kb)) )
+ {
+ printk(XENLOG_ERR
+ " bad \"mem-min\" prop on domain %s\n", name);
+ return rc;
+ }
+ bd->min_pages = PFN_DOWN(kb * SZ_1K);
+ printk(XENLOG_INFO " min memory: %lu kb\n", kb);
+ }
+ else if ( !strncmp(prop_name, "mem-max", name_len) )
+ {
+ uint64_t kb;
+ if ( (rc = fdt_prop_as_u64(prop, &kb)) )
+ {
+ printk(XENLOG_ERR
+ " bad \"mem-max\" prop on domain %s\n", name);
+ return rc;
+ }
+ bd->max_pages = PFN_DOWN(kb * SZ_1K);
+ printk(XENLOG_INFO " max memory: %lu KiB\n", kb);
+ }
}
fdt_for_each_subnode(node, fdt, dom_node)
diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
index afc76e7750..3054b48a83 100644
--- a/xen/include/xen/libfdt/libfdt-xen.h
+++ b/xen/include/xen/libfdt/libfdt-xen.h
@@ -24,6 +24,16 @@ static inline int __init fdt_prop_as_u32(
return 0;
}
+static inline int __init fdt_prop_as_u64(
+ const struct fdt_property *prop, uint64_t *val)
+{
+ if ( !prop || fdt32_to_cpu(prop->len) < sizeof(uint64_t) )
+ return -EINVAL;
+
+ *val = fdt64_to_cpu(*(const fdt64_t *)prop->data);
+ return 0;
+}
+
static inline bool __init fdt_get_prop_offset(
const void *fdt, int node, const char *pname, unsigned long *poffset)
{
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 11/13] x86/hyperlaunch: add memory parsing to domain config
2025-04-17 12:48 ` [PATCH v4 11/13] x86/hyperlaunch: add memory parsing to domain config Alejandro Vallejo
@ 2025-04-18 23:21 ` dmkhn
2025-04-23 13:05 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 23:21 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Bertrand Marquis, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:33PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> 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>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> Can't use KB() rather tha SZ_1K, as that's strictly for literals.
Oh, that's right! Thanks!
>
> v4:
> * Don't override fdt_prop_as_u64() rc on error.
> * Add separate printk statements for each memory prop error.
> * Fix bug setting up dom0_size, dom0_min_size and dom0_max_size
> * Was overriding dom0_size on all 3 paths.
> * Pre-initialise max_pages of all boot_domains to be LONG_MAX, just as
> dom0_max_size.
> * Eventually dom0_max_size drops out in the bigger series, so we
> need that logic to be correct.
> ---
> xen/arch/x86/dom0_build.c | 8 ++++++
> xen/arch/x86/include/asm/boot-domain.h | 4 +++
> xen/arch/x86/setup.c | 5 +++-
> xen/common/domain-builder/fdt.c | 36 ++++++++++++++++++++++++++
> xen/include/xen/libfdt/libfdt-xen.h | 10 +++++++
> 5 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 0b467fd4a4..8db24dbc0a 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -627,6 +627,14 @@ int __init construct_dom0(const 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, ULONG_MAX) && bd->mem_pages )
> + dom0_size.nr_pages = bd->mem_pages;
> + if ( !get_memsize(&dom0_min_size, ULONG_MAX) && bd->min_pages )
> + dom0_min_size.nr_pages = bd->min_pages;
> + if ( !get_memsize(&dom0_max_size, ULONG_MAX) && bd->max_pages )
> + dom0_max_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/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
> index e316d4bcde..fa8ea1cc66 100644
> --- a/xen/arch/x86/include/asm/boot-domain.h
> +++ b/xen/arch/x86/include/asm/boot-domain.h
> @@ -18,6 +18,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 *module;
> const char *cmdline;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 05e3d2cf2a..455dad454c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -296,7 +296,10 @@ static const char *cmdline_cook(const char *p, const char *loader_name);
> struct boot_info __initdata xen_boot_info = {
> .loader = "unknown",
> .cmdline = "",
> - .domains = { [0 ... MAX_NR_BOOTDOMS - 1] = { .domid = DOMID_INVALID } },
> + .domains = { [0 ... MAX_NR_BOOTDOMS - 1] = {
> + .domid = DOMID_INVALID,
> + .max_pages = ULONG_MAX,
> + }},
> };
>
> static struct boot_info *__init multiboot_fill_boot_info(
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index 6809c7f917..90218d120a 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -7,6 +7,7 @@
> #include <xen/init.h>
> #include <xen/lib.h>
> #include <xen/libfdt/libfdt.h>
> +#include <xen/sizes.h>
>
> #include <asm/bootinfo.h>
> #include <asm/page.h>
> @@ -246,6 +247,41 @@ static int __init process_domain_node(
> (bd->mode & BUILD_MODE_ENABLE_DM) ? "hvm" :
> "pvh");
> }
> + else if ( strncmp(prop_name, "memory", name_len) == 0 )
Use !strncmp(...) like in the below code?
> + {
> + uint64_t kb;
> + if ( (rc = fdt_prop_as_u64(prop, &kb)) )
> + {
> + printk(XENLOG_ERR " bad \"memory\" prop on domain %s\n", name);
> + return rc;
> + }
> + bd->mem_pages = PFN_DOWN(kb * SZ_1K);
> + printk(XENLOG_ERR " memory: %lu KiB\n", kb);
> + }
> + else if ( !strncmp(prop_name, "mem-min", name_len) )
> + {
> + uint64_t kb;
> + if ( (rc = fdt_prop_as_u64(prop, &kb)) )
> + {
> + printk(XENLOG_ERR
> + " bad \"mem-min\" prop on domain %s\n", name);
> + return rc;
> + }
> + bd->min_pages = PFN_DOWN(kb * SZ_1K);
> + printk(XENLOG_INFO " min memory: %lu kb\n", kb);
> + }
> + else if ( !strncmp(prop_name, "mem-max", name_len) )
> + {
> + uint64_t kb;
> + if ( (rc = fdt_prop_as_u64(prop, &kb)) )
> + {
> + printk(XENLOG_ERR
> + " bad \"mem-max\" prop on domain %s\n", name);
> + return rc;
> + }
> + bd->max_pages = PFN_DOWN(kb * SZ_1K);
> + printk(XENLOG_INFO " max memory: %lu KiB\n", kb);
> + }
> }
>
> fdt_for_each_subnode(node, fdt, dom_node)
> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
> index afc76e7750..3054b48a83 100644
> --- a/xen/include/xen/libfdt/libfdt-xen.h
> +++ b/xen/include/xen/libfdt/libfdt-xen.h
> @@ -24,6 +24,16 @@ static inline int __init fdt_prop_as_u32(
> return 0;
> }
>
> +static inline int __init fdt_prop_as_u64(
> + const struct fdt_property *prop, uint64_t *val)
> +{
> + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(uint64_t) )
> + return -EINVAL;
> +
> + *val = fdt64_to_cpu(*(const fdt64_t *)prop->data);
> + return 0;
> +}
Perhaps move the call to common/domain-builder/fdt.c?
> +
> static inline bool __init fdt_get_prop_offset(
> const void *fdt, int node, const char *pname, unsigned long *poffset)
> {
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 11/13] x86/hyperlaunch: add memory parsing to domain config
2025-04-18 23:21 ` dmkhn
@ 2025-04-23 13:05 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 13:05 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Bertrand Marquis, Jason Andryuk
On Sat Apr 19, 2025 at 12:21 AM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:33PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> 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>
>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> Can't use KB() rather tha SZ_1K, as that's strictly for literals.
>
> Oh, that's right! Thanks!
>
>>
>> v4:
>> * Don't override fdt_prop_as_u64() rc on error.
>> * Add separate printk statements for each memory prop error.
>> * Fix bug setting up dom0_size, dom0_min_size and dom0_max_size
>> * Was overriding dom0_size on all 3 paths.
>> * Pre-initialise max_pages of all boot_domains to be LONG_MAX, just as
>> dom0_max_size.
>> * Eventually dom0_max_size drops out in the bigger series, so we
>> need that logic to be correct.
>> ---
>> xen/arch/x86/dom0_build.c | 8 ++++++
>> xen/arch/x86/include/asm/boot-domain.h | 4 +++
>> xen/arch/x86/setup.c | 5 +++-
>> xen/common/domain-builder/fdt.c | 36 ++++++++++++++++++++++++++
>> xen/include/xen/libfdt/libfdt-xen.h | 10 +++++++
>> 5 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
>> index 0b467fd4a4..8db24dbc0a 100644
>> --- a/xen/arch/x86/dom0_build.c
>> +++ b/xen/arch/x86/dom0_build.c
>> @@ -627,6 +627,14 @@ int __init construct_dom0(const 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, ULONG_MAX) && bd->mem_pages )
>> + dom0_size.nr_pages = bd->mem_pages;
>> + if ( !get_memsize(&dom0_min_size, ULONG_MAX) && bd->min_pages )
>> + dom0_min_size.nr_pages = bd->min_pages;
>> + if ( !get_memsize(&dom0_max_size, ULONG_MAX) && bd->max_pages )
>> + dom0_max_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/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
>> index e316d4bcde..fa8ea1cc66 100644
>> --- a/xen/arch/x86/include/asm/boot-domain.h
>> +++ b/xen/arch/x86/include/asm/boot-domain.h
>> @@ -18,6 +18,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 *module;
>> const char *cmdline;
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 05e3d2cf2a..455dad454c 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -296,7 +296,10 @@ static const char *cmdline_cook(const char *p, const char *loader_name);
>> struct boot_info __initdata xen_boot_info = {
>> .loader = "unknown",
>> .cmdline = "",
>> - .domains = { [0 ... MAX_NR_BOOTDOMS - 1] = { .domid = DOMID_INVALID } },
>> + .domains = { [0 ... MAX_NR_BOOTDOMS - 1] = {
>> + .domid = DOMID_INVALID,
>> + .max_pages = ULONG_MAX,
>> + }},
>> };
>>
>> static struct boot_info *__init multiboot_fill_boot_info(
>> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
>> index 6809c7f917..90218d120a 100644
>> --- a/xen/common/domain-builder/fdt.c
>> +++ b/xen/common/domain-builder/fdt.c
>> @@ -7,6 +7,7 @@
>> #include <xen/init.h>
>> #include <xen/lib.h>
>> #include <xen/libfdt/libfdt.h>
>> +#include <xen/sizes.h>
>>
>> #include <asm/bootinfo.h>
>> #include <asm/page.h>
>> @@ -246,6 +247,41 @@ static int __init process_domain_node(
>> (bd->mode & BUILD_MODE_ENABLE_DM) ? "hvm" :
>> "pvh");
>> }
>> + else if ( strncmp(prop_name, "memory", name_len) == 0 )
>
> Use !strncmp(...) like in the below code?
Sure
>
>> + {
>> + uint64_t kb;
>> + if ( (rc = fdt_prop_as_u64(prop, &kb)) )
>> + {
>> + printk(XENLOG_ERR " bad \"memory\" prop on domain %s\n", name);
>> + return rc;
>> + }
>> + bd->mem_pages = PFN_DOWN(kb * SZ_1K);
>> + printk(XENLOG_ERR " memory: %lu KiB\n", kb);
>> + }
>> + else if ( !strncmp(prop_name, "mem-min", name_len) )
>> + {
>> + uint64_t kb;
>> + if ( (rc = fdt_prop_as_u64(prop, &kb)) )
>> + {
>> + printk(XENLOG_ERR
>> + " bad \"mem-min\" prop on domain %s\n", name);
>> + return rc;
>> + }
>> + bd->min_pages = PFN_DOWN(kb * SZ_1K);
>> + printk(XENLOG_INFO " min memory: %lu kb\n", kb);
>> + }
>> + else if ( !strncmp(prop_name, "mem-max", name_len) )
>> + {
>> + uint64_t kb;
>> + if ( (rc = fdt_prop_as_u64(prop, &kb)) )
>> + {
>> + printk(XENLOG_ERR
>> + " bad \"mem-max\" prop on domain %s\n", name);
>> + return rc;
>> + }
>> + bd->max_pages = PFN_DOWN(kb * SZ_1K);
>> + printk(XENLOG_INFO " max memory: %lu KiB\n", kb);
>> + }
>> }
>>
>> fdt_for_each_subnode(node, fdt, dom_node)
>> diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h
>> index afc76e7750..3054b48a83 100644
>> --- a/xen/include/xen/libfdt/libfdt-xen.h
>> +++ b/xen/include/xen/libfdt/libfdt-xen.h
>> @@ -24,6 +24,16 @@ static inline int __init fdt_prop_as_u32(
>> return 0;
>> }
>>
>> +static inline int __init fdt_prop_as_u64(
>> + const struct fdt_property *prop, uint64_t *val)
>> +{
>> + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(uint64_t) )
>> + return -EINVAL;
>> +
>> + *val = fdt64_to_cpu(*(const fdt64_t *)prop->data);
>> + return 0;
>> +}
>
> Perhaps move the call to common/domain-builder/fdt.c?
Yes, all new additions to that header ought to have gone to fdt.c
instead. That was my original intent, but then didn't go through with
everything.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 12/13] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (10 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 11/13] x86/hyperlaunch: add memory parsing to domain config Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 23:22 ` dmkhn
2025-04-17 12:48 ` [PATCH v4 13/13] x86/hyperlaunch: add capabilities to boot domain Alejandro Vallejo
2025-04-17 13:00 ` [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
Introduce the `cpus` property, named as such for dom0less compatibility, that
represents the maximum number of vcpus 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>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* s/UINT_MAX/UINT32_MAX/ (cosmetic, but...)
* s/vpcpus/vcpus/ in commit message.
* Reworded printk()
---
xen/arch/x86/dom0_build.c | 3 +++
xen/arch/x86/include/asm/boot-domain.h | 2 ++
xen/common/domain-builder/fdt.c | 11 +++++++++++
3 files changed, 16 insertions(+)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 8db24dbc0a..f734104c74 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -635,6 +635,9 @@ int __init construct_dom0(const struct boot_domain *bd)
if ( !get_memsize(&dom0_max_size, ULONG_MAX) && bd->max_pages )
dom0_max_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/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index fa8ea1cc66..969c02a6ea 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -22,6 +22,8 @@ struct boot_domain {
unsigned long min_pages;
unsigned long max_pages;
+ unsigned int max_vcpus;
+
struct boot_module *kernel;
struct boot_module *module;
const char *cmdline;
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 90218d120a..295ab6e8b3 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -282,6 +282,17 @@ static int __init process_domain_node(
bd->max_pages = PFN_DOWN(kb * SZ_1K);
printk(XENLOG_INFO " max memory: %lu KiB\n", kb);
}
+ else if ( !strncmp(prop_name, "cpus", name_len) )
+ {
+ uint32_t val = UINT32_MAX;
+ if ( (rc = fdt_prop_as_u32(prop, &val)) )
+ {
+ printk(XENLOG_ERR " bad \"cpus\" prop on domain %s\n", name);
+ return rc;
+ }
+ bd->max_vcpus = val;
+ printk(XENLOG_INFO " cpus: %d\n", bd->max_vcpus);
+ }
}
fdt_for_each_subnode(node, fdt, dom_node)
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 12/13] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree
2025-04-17 12:48 ` [PATCH v4 12/13] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree Alejandro Vallejo
@ 2025-04-18 23:22 ` dmkhn
0 siblings, 0 replies; 46+ messages in thread
From: dmkhn @ 2025-04-18 23:22 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:34PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> Introduce the `cpus` property, named as such for dom0less compatibility, that
> represents the maximum number of vcpus 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>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> v4:
> * s/UINT_MAX/UINT32_MAX/ (cosmetic, but...)
> * s/vpcpus/vcpus/ in commit message.
> * Reworded printk()
> ---
> xen/arch/x86/dom0_build.c | 3 +++
> xen/arch/x86/include/asm/boot-domain.h | 2 ++
> xen/common/domain-builder/fdt.c | 11 +++++++++++
> 3 files changed, 16 insertions(+)
>
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 8db24dbc0a..f734104c74 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -635,6 +635,9 @@ int __init construct_dom0(const struct boot_domain *bd)
> if ( !get_memsize(&dom0_max_size, ULONG_MAX) && bd->max_pages )
> dom0_max_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/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
> index fa8ea1cc66..969c02a6ea 100644
> --- a/xen/arch/x86/include/asm/boot-domain.h
> +++ b/xen/arch/x86/include/asm/boot-domain.h
> @@ -22,6 +22,8 @@ struct boot_domain {
> unsigned long min_pages;
> unsigned long max_pages;
>
> + unsigned int max_vcpus;
> +
> struct boot_module *kernel;
> struct boot_module *module;
> const char *cmdline;
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index 90218d120a..295ab6e8b3 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -282,6 +282,17 @@ static int __init process_domain_node(
> bd->max_pages = PFN_DOWN(kb * SZ_1K);
> printk(XENLOG_INFO " max memory: %lu KiB\n", kb);
> }
> + else if ( !strncmp(prop_name, "cpus", name_len) )
> + {
> + uint32_t val = UINT32_MAX;
> + if ( (rc = fdt_prop_as_u32(prop, &val)) )
> + {
> + printk(XENLOG_ERR " bad \"cpus\" prop on domain %s\n", name);
> + return rc;
> + }
> + bd->max_vcpus = val;
> + printk(XENLOG_INFO " cpus: %d\n", bd->max_vcpus);
> + }
> }
>
> fdt_for_each_subnode(node, fdt, dom_node)
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 13/13] x86/hyperlaunch: add capabilities to boot domain
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (11 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 12/13] x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree Alejandro Vallejo
@ 2025-04-17 12:48 ` Alejandro Vallejo
2025-04-18 23:24 ` dmkhn
2025-04-17 13:00 ` [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
13 siblings, 1 reply; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 12:48 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Jason Andryuk, Alejandro Vallejo
From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
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>
Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
v4:
* Dropped Jason's R-by.
* Refactored caps printinng logic
* It just wasn't xenlog-compatible as it was.
* Moved pv_shim check to builder_init, so the capability is just not given.
* And inlined the create_flags variable now that's tractable.
* Validated input capabilities after coming out of the DT.
---
xen/arch/x86/include/asm/boot-domain.h | 5 +++++
xen/arch/x86/setup.c | 3 ++-
xen/common/domain-builder/core.c | 2 ++
xen/common/domain-builder/fdt.c | 20 ++++++++++++++++++++
4 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
index 969c02a6ea..5c143d82af 100644
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ b/xen/arch/x86/include/asm/boot-domain.h
@@ -13,6 +13,11 @@
struct boot_domain {
domid_t domid;
+#define BUILD_CAPS_NONE (0U)
+#define BUILD_CAPS_CONTROL (1U << 0)
+#define BUILD_CAPS__ALL BUILD_CAPS_CONTROL
+ 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 455dad454c..3cdd8bc2f9 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1040,7 +1040,8 @@ 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);
+ d = domain_create(bd->domid, &dom0_cfg,
+ (bd->capabilities & BUILD_CAPS_CONTROL) ? CDF_privileged : 0);
if ( IS_ERR(d) )
panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
index 4b4230f2ff..d1a5d6125e 100644
--- a/xen/common/domain-builder/core.c
+++ b/xen/common/domain-builder/core.c
@@ -8,6 +8,7 @@
#include <xen/lib.h>
#include <asm/bootinfo.h>
+#include <asm/pv/shim.h>
#include <asm/setup.h>
#include "fdt.h"
@@ -93,6 +94,7 @@ void __init builder_init(struct boot_info *bi)
bi->mods[i].type = BOOTMOD_KERNEL;
bi->domains[0].kernel = &bi->mods[i];
+ bi->domains[0].capabilities |= pv_shim ? 0 : BUILD_CAPS_CONTROL;
bi->nr_domains = 1;
}
}
diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
index 295ab6e8b3..3e3a84e2d0 100644
--- a/xen/common/domain-builder/fdt.c
+++ b/xen/common/domain-builder/fdt.c
@@ -293,6 +293,26 @@ static int __init process_domain_node(
bd->max_vcpus = val;
printk(XENLOG_INFO " cpus: %d\n", bd->max_vcpus);
}
+ else if ( !strncmp(prop_name, "capabilities", name_len) )
+ {
+ if ( (rc = fdt_prop_as_u32(prop, &bd->capabilities)) )
+ {
+ printk(XENLOG_ERR
+ " bad \"capabilities\" on domain %s\n", name);
+ return rc;
+ }
+
+ if ( bd->capabilities & ~BUILD_CAPS__ALL )
+ {
+ printk(XENLOG_WARNING " unknown capabilities: %#x\n",
+ bd->capabilities & ~BUILD_CAPS__ALL);
+
+ bd->capabilities &= BUILD_CAPS__ALL;
+ }
+
+ printk(XENLOG_INFO " caps: %s\n",
+ bd->capabilities & BUILD_CAPS_CONTROL ? "c" : "");
+ }
}
fdt_for_each_subnode(node, fdt, dom_node)
--
2.43.0
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [PATCH v4 13/13] x86/hyperlaunch: add capabilities to boot domain
2025-04-17 12:48 ` [PATCH v4 13/13] x86/hyperlaunch: add capabilities to boot domain Alejandro Vallejo
@ 2025-04-18 23:24 ` dmkhn
2025-04-23 13:07 ` Alejandro Vallejo
0 siblings, 1 reply; 46+ messages in thread
From: dmkhn @ 2025-04-18 23:24 UTC (permalink / raw)
To: Alejandro Vallejo
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Thu, Apr 17, 2025 at 01:48:35PM +0100, Alejandro Vallejo wrote:
> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>
> 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>
> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> v4:
> * Dropped Jason's R-by.
> * Refactored caps printinng logic
> * It just wasn't xenlog-compatible as it was.
> * Moved pv_shim check to builder_init, so the capability is just not given.
> * And inlined the create_flags variable now that's tractable.
> * Validated input capabilities after coming out of the DT.
> ---
> xen/arch/x86/include/asm/boot-domain.h | 5 +++++
> xen/arch/x86/setup.c | 3 ++-
> xen/common/domain-builder/core.c | 2 ++
> xen/common/domain-builder/fdt.c | 20 ++++++++++++++++++++
> 4 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
> index 969c02a6ea..5c143d82af 100644
> --- a/xen/arch/x86/include/asm/boot-domain.h
> +++ b/xen/arch/x86/include/asm/boot-domain.h
> @@ -13,6 +13,11 @@
> struct boot_domain {
> domid_t domid;
>
> +#define BUILD_CAPS_NONE (0U)
> +#define BUILD_CAPS_CONTROL (1U << 0)
> +#define BUILD_CAPS__ALL BUILD_CAPS_CONTROL
> + 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 455dad454c..3cdd8bc2f9 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1040,7 +1040,8 @@ 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);
> + d = domain_create(bd->domid, &dom0_cfg,
> + (bd->capabilities & BUILD_CAPS_CONTROL) ? CDF_privileged : 0);
> if ( IS_ERR(d) )
> panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>
> diff --git a/xen/common/domain-builder/core.c b/xen/common/domain-builder/core.c
> index 4b4230f2ff..d1a5d6125e 100644
> --- a/xen/common/domain-builder/core.c
> +++ b/xen/common/domain-builder/core.c
> @@ -8,6 +8,7 @@
> #include <xen/lib.h>
>
> #include <asm/bootinfo.h>
> +#include <asm/pv/shim.h>
> #include <asm/setup.h>
>
> #include "fdt.h"
> @@ -93,6 +94,7 @@ void __init builder_init(struct boot_info *bi)
>
> bi->mods[i].type = BOOTMOD_KERNEL;
> bi->domains[0].kernel = &bi->mods[i];
> + bi->domains[0].capabilities |= pv_shim ? 0 : BUILD_CAPS_CONTROL;
> bi->nr_domains = 1;
> }
> }
> diff --git a/xen/common/domain-builder/fdt.c b/xen/common/domain-builder/fdt.c
> index 295ab6e8b3..3e3a84e2d0 100644
> --- a/xen/common/domain-builder/fdt.c
> +++ b/xen/common/domain-builder/fdt.c
> @@ -293,6 +293,26 @@ static int __init process_domain_node(
> bd->max_vcpus = val;
> printk(XENLOG_INFO " cpus: %d\n", bd->max_vcpus);
> }
> + else if ( !strncmp(prop_name, "capabilities", name_len) )
> + {
> + if ( (rc = fdt_prop_as_u32(prop, &bd->capabilities)) )
> + {
> + printk(XENLOG_ERR
> + " bad \"capabilities\" on domain %s\n", name);
> + return rc;
> + }
> +
> + if ( bd->capabilities & ~BUILD_CAPS__ALL )
> + {
> + printk(XENLOG_WARNING " unknown capabilities: %#x\n",
> + bd->capabilities & ~BUILD_CAPS__ALL);
> +
> + bd->capabilities &= BUILD_CAPS__ALL;
> + }
> +
> + printk(XENLOG_INFO " caps: %s\n",
> + bd->capabilities & BUILD_CAPS_CONTROL ? "c" : "");
> + }
> }
>
> fdt_for_each_subnode(node, fdt, dom_node)
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v4 13/13] x86/hyperlaunch: add capabilities to boot domain
2025-04-18 23:24 ` dmkhn
@ 2025-04-23 13:07 ` Alejandro Vallejo
0 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-23 13:07 UTC (permalink / raw)
To: dmkhn
Cc: xen-devel, Daniel P. Smith, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, Jason Andryuk
On Sat Apr 19, 2025 at 12:24 AM BST, dmkhn wrote:
> On Thu, Apr 17, 2025 at 01:48:35PM +0100, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com>
>>
>> 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>
>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>
> Reviewed-by: Denis Mukhin <dmukhin@ford.com>
Thanks
Though now that CDF_hardware has been introduced to staging, this will have to
change to introduce BUILD_CAPS_HARDWARE as well.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 00/13] Hyperlaunch device tree for dom0
2025-04-17 12:48 [PATCH v4 00/13] Hyperlaunch device tree for dom0 Alejandro Vallejo
` (12 preceding siblings ...)
2025-04-17 12:48 ` [PATCH v4 13/13] x86/hyperlaunch: add capabilities to boot domain Alejandro Vallejo
@ 2025-04-17 13:00 ` Alejandro Vallejo
13 siblings, 0 replies; 46+ messages in thread
From: Alejandro Vallejo @ 2025-04-17 13:00 UTC (permalink / raw)
To: Alejandro Vallejo, xen-devel
Cc: Jan Beulich, Andrew Cooper, Roger Pau Monné, Anthony PERARD,
Michal Orzel, Julien Grall, Stefano Stabellini, Bertrand Marquis
On Thu Apr 17, 2025 at 1:48 PM BST, Alejandro Vallejo wrote:
> Hi,
>
> Here's a new version. Took a while to integrate all the feedback, but
> here it is.
>
> v4 pipeline: https://gitlab.com/xen-project/people/agvallejo/xen/-/pipelines/1772300721
>
> v3: https://lore.kernel.org/xen-devel/20250408160802.49870-1-agarciav@amd.com/
> v2: https://lore.kernel.org/xen-devel/20241226165740.29812-1-dpsmith@apertussolutions.com/
> v1: https://lore.kernel.org/xen-devel/20241123182044.30687-1-dpsmith@apertussolutions.com/
>
> Cheers,
> Alejandro
>
> ========= Original cover letter:
>
> The Hyperlaunch device tree for dom0 series is the second split out for the
> introduction of the Hyperlaunch domain builder logic. These changes focus on
> introducing the ability to express a domain configuration that is then used to
> populate the struct boot_domain structure for dom0. This ability to express a
> domain configuration provides the next step towards a general domain builder.
>
> The splitting of Hyperlaunch into a set of series are twofold, to reduce the
> effort in reviewing a much larger series, and to reduce the effort in handling
> the knock-on effects to the construction logic from requested review changes.
>
> Alejandro Vallejo (1):
> x86/hyperlaunch: Add helpers to locate multiboot modules
>
> Daniel P. Smith (12):
> x86/boot: add cmdline to struct boot_domain
> kconfig: introduce domain builder config options
> common/hyperlaunch: introduce the domain builder
> x86/hyperlaunch: initial support for hyperlaunch device tree
> x86/hyperlaunch: locate dom0 kernel with hyperlaunch
> x86/hyperlaunch: obtain cmdline from device tree
> x86/hyperlaunch: locate dom0 initrd with hyperlaunch
> x86/hyperlaunch: add domain id parsing to domain config
> x86/hyperlaunch: specify dom0 mode with device tree
> x86/hyperlaunch: add memory parsing to domain config
> x86/hyperlaunch: add max vcpu parsing of hyperlaunch device tree
> x86/hyperlaunch: add capabilities to boot domain
>
> xen/arch/x86/Kconfig | 1 +
> xen/arch/x86/dom0_build.c | 11 +
> xen/arch/x86/hvm/dom0_build.c | 31 +-
> xen/arch/x86/include/asm/boot-domain.h | 17 +
> xen/arch/x86/include/asm/bootinfo.h | 10 +-
> xen/arch/x86/pv/dom0_build.c | 4 +-
> xen/arch/x86/setup.c | 91 +++--
> xen/common/Kconfig | 5 +
> xen/common/Makefile | 1 +
> xen/common/domain-builder/Kconfig | 18 +
> xen/common/domain-builder/Makefile | 2 +
> xen/common/domain-builder/core.c | 110 ++++++
> xen/common/domain-builder/fdt.c | 488 +++++++++++++++++++++++++
> xen/common/domain-builder/fdt.h | 39 ++
> xen/include/xen/domain-builder.h | 13 +
> xen/include/xen/libfdt/libfdt-xen.h | 44 +++
> 16 files changed, 839 insertions(+), 46 deletions(-)
> create mode 100644 xen/common/domain-builder/Kconfig
> create mode 100644 xen/common/domain-builder/Makefile
> create mode 100644 xen/common/domain-builder/core.c
> create mode 100644 xen/common/domain-builder/fdt.c
> create mode 100644 xen/common/domain-builder/fdt.h
> create mode 100644 xen/include/xen/domain-builder.h
Bah, I just noticed after sending. The commit messages of the last 10
patches are meant to be common/hyperlaunch. Or just hyperlaunch.
It _is_ effectively still x86-only, but it's living under common.
Cheers,
Alejandro
^ permalink raw reply [flat|nested] 46+ messages in thread