Add the single arch-specific field in an "arch" subfield defined in
asm/bootfdt.h.
No functional change intended.
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/x86/hvm/dom0_build.c | 2 +-
xen/arch/x86/include/asm/boot-domain.h | 33 --------------------------
xen/arch/x86/include/asm/bootfdt.h | 6 +++++
xen/arch/x86/include/asm/bootinfo.h | 1 -
xen/arch/x86/pv/dom0_build.c | 2 +-
xen/arch/x86/setup.c | 12 ++++++----
xen/include/xen/bootfdt.h | 4 ++++
7 files changed, 19 insertions(+), 41 deletions(-)
delete mode 100644 xen/arch/x86/include/asm/boot-domain.h
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 2bb8ef355c..8d2734f2b5 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -648,7 +648,7 @@ static int __init pvh_load_kernel(
{
struct domain *d = bd->d;
struct boot_module *image = bd->kernel;
- struct boot_module *initrd = bd->module;
+ struct boot_module *initrd = bd->initrd;
void *image_base = bootstrap_map_bm(image);
void *image_start = image_base + image->arch.headroom;
unsigned long image_len = image->size;
diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h
deleted file mode 100644
index d7c6042e25..0000000000
--- a/xen/arch/x86/include/asm/boot-domain.h
+++ /dev/null
@@ -1,33 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Copyright (c) 2024 Apertus Solutions, LLC
- * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
- * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
- */
-
-#ifndef __XEN_X86_BOOTDOMAIN_H__
-#define __XEN_X86_BOOTDOMAIN_H__
-
-#include <public/xen.h>
-
-struct boot_domain {
- domid_t domid;
-
- struct boot_module *kernel;
- struct boot_module *module;
- const char *cmdline;
-
- struct domain *d;
-};
-
-#endif
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/arch/x86/include/asm/bootfdt.h b/xen/arch/x86/include/asm/bootfdt.h
index a4c4bf30b9..c21dbe961b 100644
--- a/xen/arch/x86/include/asm/bootfdt.h
+++ b/xen/arch/x86/include/asm/bootfdt.h
@@ -3,6 +3,12 @@
#define X86_BOOTFDT_H
#include <xen/types.h>
+#include <public/xen.h>
+
+struct arch_boot_domain
+{
+ domid_t domid;
+};
struct arch_boot_module
{
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index d33b100e04..4f2cc5906e 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -12,7 +12,6 @@
#include <xen/init.h>
#include <xen/multiboot.h>
#include <xen/types.h>
-#include <asm/boot-domain.h>
/* Max number of boot modules a bootloader can provide in addition to Xen */
#define MAX_NR_BOOTMODS 63
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index a4b5362357..c37bea9454 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -375,7 +375,7 @@ static int __init dom0_construct(const struct boot_domain *bd)
struct vcpu *v = d->vcpu[0];
struct boot_module *image = bd->kernel;
- struct boot_module *initrd = bd->module;
+ struct boot_module *initrd = bd->initrd;
void *image_base;
unsigned long image_len;
void *image_start;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 7e70b46332..5adb7af930 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -297,7 +297,9 @@ 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] = {
+ .arch.domid = DOMID_INVALID
+ }},
/*
* There's a MAX_NR_BOOTMODS-th entry in the array. It's not off by one.
*
@@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
/* Create initial domain. Not d0 for pvshim. */
- bd->domid = get_initial_domain_id();
- d = domain_create(bd->domid, &dom0_cfg,
+ bd->arch.domid = get_initial_domain_id();
+ d = domain_create(bd->arch.domid, &dom0_cfg,
pv_shim ? 0 : CDF_privileged | CDF_hardware);
if ( IS_ERR(d) )
- panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
+ panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
init_dom0_cpuid_policy(d);
@@ -2171,7 +2173,7 @@ void asmlinkage __init noreturn __start_xen(void)
if ( initrdidx < MAX_NR_BOOTMODS )
{
bi->mods[initrdidx].kind = BOOTMOD_RAMDISK;
- bi->domains[0].module = &bi->mods[initrdidx];
+ bi->domains[0].initrd = &bi->mods[initrdidx];
if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
printk(XENLOG_WARNING
"Multiple initrd candidates, picking module #%u\n",
diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
index be0abe30ef..8ea52290b7 100644
--- a/xen/include/xen/bootfdt.h
+++ b/xen/include/xen/bootfdt.h
@@ -108,6 +108,10 @@ struct boot_domain {
struct boot_module *initrd;
const char* cmdline;
+
+#if __has_include(<asm/bootfdt.h>)
+ struct arch_boot_domain arch;
+#endif
};
/*
--
2.43.0On 01.07.2025 12:56, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/include/asm/bootfdt.h
> +++ b/xen/arch/x86/include/asm/bootfdt.h
> @@ -3,6 +3,12 @@
> #define X86_BOOTFDT_H
>
> #include <xen/types.h>
> +#include <public/xen.h>
> +
> +struct arch_boot_domain
> +{
> + domid_t domid;
> +};
>
> struct arch_boot_module
> {
>[...]
> @@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> /* Create initial domain. Not d0 for pvshim. */
> - bd->domid = get_initial_domain_id();
> - d = domain_create(bd->domid, &dom0_cfg,
> + bd->arch.domid = get_initial_domain_id();
> + d = domain_create(bd->arch.domid, &dom0_cfg,
> pv_shim ? 0 : CDF_privileged | CDF_hardware);
> if ( IS_ERR(d) )
> - panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
> + panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
This being the only place where the (now) arch-specific field is used, why
does it exist? A local variable would do? And if it's needed for
(supposedly arch-agnostic) hyperlaunch, then it probably shouldn't be
arch-specific? Daniel, Jason?
Jan
On Wed Jul 2, 2025 at 3:15 PM CEST, Jan Beulich wrote:
> On 01.07.2025 12:56, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/include/asm/bootfdt.h
>> +++ b/xen/arch/x86/include/asm/bootfdt.h
>> @@ -3,6 +3,12 @@
>> #define X86_BOOTFDT_H
>>
>> #include <xen/types.h>
>> +#include <public/xen.h>
>> +
>> +struct arch_boot_domain
>> +{
>> + domid_t domid;
>> +};
>>
>> struct arch_boot_module
>> {
>>[...]
>> @@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>
>> /* Create initial domain. Not d0 for pvshim. */
>> - bd->domid = get_initial_domain_id();
>> - d = domain_create(bd->domid, &dom0_cfg,
>> + bd->arch.domid = get_initial_domain_id();
>> + d = domain_create(bd->arch.domid, &dom0_cfg,
>> pv_shim ? 0 : CDF_privileged | CDF_hardware);
>> if ( IS_ERR(d) )
>> - panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>> + panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
>
> This being the only place where the (now) arch-specific field is used, why
> does it exist? A local variable would do? And if it's needed for
> (supposedly arch-agnostic) hyperlaunch, then it probably shouldn't be
> arch-specific? Daniel, Jason?
>
> Jan
As for the arch-agnostic side of things, arm needs some extra work to be
able to do it safely. dom0less currently constructs domains immediately after
parsing them, which is problematic for cases where some domains have the prop
and others don't. The domid allocation strategy may preclude further otherwise
good domains from being created just because their domid was stolen by a domain
that didn't actually care about which domid it got.
It'll eventually want to leave the arch-specific area, but I don't want to do
that work now.
Cheers,
Alejandro
On 02.07.2025 17:09, Alejandro Vallejo wrote:
> On Wed Jul 2, 2025 at 3:15 PM CEST, Jan Beulich wrote:
>> On 01.07.2025 12:56, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/include/asm/bootfdt.h
>>> +++ b/xen/arch/x86/include/asm/bootfdt.h
>>> @@ -3,6 +3,12 @@
>>> #define X86_BOOTFDT_H
>>>
>>> #include <xen/types.h>
>>> +#include <public/xen.h>
>>> +
>>> +struct arch_boot_domain
>>> +{
>>> + domid_t domid;
>>> +};
>>>
>>> struct arch_boot_module
>>> {
>>> [...]
>>> @@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>>> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>>
>>> /* Create initial domain. Not d0 for pvshim. */
>>> - bd->domid = get_initial_domain_id();
>>> - d = domain_create(bd->domid, &dom0_cfg,
>>> + bd->arch.domid = get_initial_domain_id();
>>> + d = domain_create(bd->arch.domid, &dom0_cfg,
>>> pv_shim ? 0 : CDF_privileged | CDF_hardware);
>>> if ( IS_ERR(d) )
>>> - panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>>> + panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
>>
>> This being the only place where the (now) arch-specific field is used, why
>> does it exist? A local variable would do? And if it's needed for
>> (supposedly arch-agnostic) hyperlaunch, then it probably shouldn't be
>> arch-specific? Daniel, Jason?
>
> As for the arch-agnostic side of things, arm needs some extra work to be
> able to do it safely. dom0less currently constructs domains immediately after
> parsing them, which is problematic for cases where some domains have the prop
> and others don't. The domid allocation strategy may preclude further otherwise
> good domains from being created just because their domid was stolen by a domain
> that didn't actually care about which domid it got.
>
> It'll eventually want to leave the arch-specific area, but I don't want to do
> that work now.
But if the domU field is fine to live in a common struct despite being unused
on x86, why can't the domid field live in a common struct too, despite being
unused on non-x86? Otherwise it'll be extra churn for no gain to later move it
there.
Jan
On Wed Jul 2, 2025 at 5:15 PM CEST, Jan Beulich wrote:
> On 02.07.2025 17:09, Alejandro Vallejo wrote:
>> On Wed Jul 2, 2025 at 3:15 PM CEST, Jan Beulich wrote:
>>> On 01.07.2025 12:56, Alejandro Vallejo wrote:
>>>> --- a/xen/arch/x86/include/asm/bootfdt.h
>>>> +++ b/xen/arch/x86/include/asm/bootfdt.h
>>>> @@ -3,6 +3,12 @@
>>>> #define X86_BOOTFDT_H
>>>>
>>>> #include <xen/types.h>
>>>> +#include <public/xen.h>
>>>> +
>>>> +struct arch_boot_domain
>>>> +{
>>>> + domid_t domid;
>>>> +};
>>>>
>>>> struct arch_boot_module
>>>> {
>>>> [...]
>>>> @@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>>>> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>>>
>>>> /* Create initial domain. Not d0 for pvshim. */
>>>> - bd->domid = get_initial_domain_id();
>>>> - d = domain_create(bd->domid, &dom0_cfg,
>>>> + bd->arch.domid = get_initial_domain_id();
>>>> + d = domain_create(bd->arch.domid, &dom0_cfg,
>>>> pv_shim ? 0 : CDF_privileged | CDF_hardware);
>>>> if ( IS_ERR(d) )
>>>> - panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>>>> + panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
>>>
>>> This being the only place where the (now) arch-specific field is used, why
>>> does it exist? A local variable would do? And if it's needed for
>>> (supposedly arch-agnostic) hyperlaunch, then it probably shouldn't be
>>> arch-specific? Daniel, Jason?
>>
>> As for the arch-agnostic side of things, arm needs some extra work to be
>> able to do it safely. dom0less currently constructs domains immediately after
>> parsing them, which is problematic for cases where some domains have the prop
>> and others don't. The domid allocation strategy may preclude further otherwise
>> good domains from being created just because their domid was stolen by a domain
>> that didn't actually care about which domid it got.
>>
>> It'll eventually want to leave the arch-specific area, but I don't want to do
>> that work now.
>
> But if the domU field is fine to live in a common struct despite being unused
> on x86, why can't the domid field live in a common struct too, despite being
> unused on non-x86? Otherwise it'll be extra churn for no gain to later move it
> there.
>
> Jan
Mostly out of tidiness. Otherwise it's hard to know which fields serve a purpose
where.
I genuinely forgot about the domU field. I'm more than happy to drop that arch
subfield and have domid in the main body of the struct, but I suspect MISRA
would have something to say about dead data?
Cheers,
Alejandro
On 02.07.2025 17:34, Alejandro Vallejo wrote:
> On Wed Jul 2, 2025 at 5:15 PM CEST, Jan Beulich wrote:
>> On 02.07.2025 17:09, Alejandro Vallejo wrote:
>>> On Wed Jul 2, 2025 at 3:15 PM CEST, Jan Beulich wrote:
>>>> On 01.07.2025 12:56, Alejandro Vallejo wrote:
>>>>> --- a/xen/arch/x86/include/asm/bootfdt.h
>>>>> +++ b/xen/arch/x86/include/asm/bootfdt.h
>>>>> @@ -3,6 +3,12 @@
>>>>> #define X86_BOOTFDT_H
>>>>>
>>>>> #include <xen/types.h>
>>>>> +#include <public/xen.h>
>>>>> +
>>>>> +struct arch_boot_domain
>>>>> +{
>>>>> + domid_t domid;
>>>>> +};
>>>>>
>>>>> struct arch_boot_module
>>>>> {
>>>>> [...]
>>>>> @@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>>>>> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>>>>
>>>>> /* Create initial domain. Not d0 for pvshim. */
>>>>> - bd->domid = get_initial_domain_id();
>>>>> - d = domain_create(bd->domid, &dom0_cfg,
>>>>> + bd->arch.domid = get_initial_domain_id();
>>>>> + d = domain_create(bd->arch.domid, &dom0_cfg,
>>>>> pv_shim ? 0 : CDF_privileged | CDF_hardware);
>>>>> if ( IS_ERR(d) )
>>>>> - panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>>>>> + panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
>>>>
>>>> This being the only place where the (now) arch-specific field is used, why
>>>> does it exist? A local variable would do? And if it's needed for
>>>> (supposedly arch-agnostic) hyperlaunch, then it probably shouldn't be
>>>> arch-specific? Daniel, Jason?
>>>
>>> As for the arch-agnostic side of things, arm needs some extra work to be
>>> able to do it safely. dom0less currently constructs domains immediately after
>>> parsing them, which is problematic for cases where some domains have the prop
>>> and others don't. The domid allocation strategy may preclude further otherwise
>>> good domains from being created just because their domid was stolen by a domain
>>> that didn't actually care about which domid it got.
>>>
>>> It'll eventually want to leave the arch-specific area, but I don't want to do
>>> that work now.
>>
>> But if the domU field is fine to live in a common struct despite being unused
>> on x86, why can't the domid field live in a common struct too, despite being
>> unused on non-x86? Otherwise it'll be extra churn for no gain to later move it
>> there.
>
> Mostly out of tidiness. Otherwise it's hard to know which fields serve a purpose
> where.
>
> I genuinely forgot about the domU field. I'm more than happy to drop that arch
> subfield and have domid in the main body of the struct, but I suspect MISRA
> would have something to say about dead data?
In principle yes (and then also about the domU field), but we rejected the
respective rule altogether (for now? plus for a reason that I must have forgot
and that escapes me).
Jan
On Thu Jul 3, 2025 at 8:04 AM CEST, Jan Beulich wrote:
> On 02.07.2025 17:34, Alejandro Vallejo wrote:
>> On Wed Jul 2, 2025 at 5:15 PM CEST, Jan Beulich wrote:
>>> On 02.07.2025 17:09, Alejandro Vallejo wrote:
>>>> On Wed Jul 2, 2025 at 3:15 PM CEST, Jan Beulich wrote:
>>>>> On 01.07.2025 12:56, Alejandro Vallejo wrote:
>>>>>> --- a/xen/arch/x86/include/asm/bootfdt.h
>>>>>> +++ b/xen/arch/x86/include/asm/bootfdt.h
>>>>>> @@ -3,6 +3,12 @@
>>>>>> #define X86_BOOTFDT_H
>>>>>>
>>>>>> #include <xen/types.h>
>>>>>> +#include <public/xen.h>
>>>>>> +
>>>>>> +struct arch_boot_domain
>>>>>> +{
>>>>>> + domid_t domid;
>>>>>> +};
>>>>>>
>>>>>> struct arch_boot_module
>>>>>> {
>>>>>> [...]
>>>>>> @@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>>>>>> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>>>>>
>>>>>> /* Create initial domain. Not d0 for pvshim. */
>>>>>> - bd->domid = get_initial_domain_id();
>>>>>> - d = domain_create(bd->domid, &dom0_cfg,
>>>>>> + bd->arch.domid = get_initial_domain_id();
>>>>>> + d = domain_create(bd->arch.domid, &dom0_cfg,
>>>>>> pv_shim ? 0 : CDF_privileged | CDF_hardware);
>>>>>> if ( IS_ERR(d) )
>>>>>> - panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>>>>>> + panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
>>>>>
>>>>> This being the only place where the (now) arch-specific field is used, why
>>>>> does it exist? A local variable would do? And if it's needed for
>>>>> (supposedly arch-agnostic) hyperlaunch, then it probably shouldn't be
>>>>> arch-specific? Daniel, Jason?
>>>>
>>>> As for the arch-agnostic side of things, arm needs some extra work to be
>>>> able to do it safely. dom0less currently constructs domains immediately after
>>>> parsing them, which is problematic for cases where some domains have the prop
>>>> and others don't. The domid allocation strategy may preclude further otherwise
>>>> good domains from being created just because their domid was stolen by a domain
>>>> that didn't actually care about which domid it got.
>>>>
>>>> It'll eventually want to leave the arch-specific area, but I don't want to do
>>>> that work now.
>>>
>>> But if the domU field is fine to live in a common struct despite being unused
>>> on x86, why can't the domid field live in a common struct too, despite being
>>> unused on non-x86? Otherwise it'll be extra churn for no gain to later move it
>>> there.
>>
>> Mostly out of tidiness. Otherwise it's hard to know which fields serve a purpose
>> where.
>>
>> I genuinely forgot about the domU field. I'm more than happy to drop that arch
>> subfield and have domid in the main body of the struct, but I suspect MISRA
>> would have something to say about dead data?
>
> In principle yes (and then also about the domU field), but we rejected the
> respective rule altogether (for now? plus for a reason that I must have forgot
> and that escapes me).
>
> Jan
Actually, moving it to an arch-specific field is rather annoying. everyone but
x86 needs the field. I'll just compile it out for x86 specifically with ifdef
guards, even if it is common code.
For the record, I hope to get rid of it on arm/riscv/ppc entirely later on by
deducing domU vs dom0 from the capabilities property.
Cheers,
Alejandro
On Wed Jul 2, 2025 at 3:15 PM CEST, Jan Beulich wrote:
> On 01.07.2025 12:56, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/include/asm/bootfdt.h
>> +++ b/xen/arch/x86/include/asm/bootfdt.h
>> @@ -3,6 +3,12 @@
>> #define X86_BOOTFDT_H
>>
>> #include <xen/types.h>
>> +#include <public/xen.h>
>> +
>> +struct arch_boot_domain
>> +{
>> + domid_t domid;
>> +};
>>
>> struct arch_boot_module
>> {
>>[...]
>> @@ -1048,11 +1050,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>
>> /* Create initial domain. Not d0 for pvshim. */
>> - bd->domid = get_initial_domain_id();
>> - d = domain_create(bd->domid, &dom0_cfg,
>> + bd->arch.domid = get_initial_domain_id();
>> + d = domain_create(bd->arch.domid, &dom0_cfg,
>> pv_shim ? 0 : CDF_privileged | CDF_hardware);
>> if ( IS_ERR(d) )
>> - panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>> + panic("Error creating d%u: %ld\n", bd->arch.domid, PTR_ERR(d));
>
> This being the only place where the (now) arch-specific field is used, why
> does it exist? A local variable would do? And if it's needed for
> (supposedly arch-agnostic) hyperlaunch, then it probably shouldn't be
> arch-specific? Daniel, Jason?
>
> Jan
It eventually becomes a holding spot for the domid property of each domain in
the DTB. It exists so we can describe every domain fully ahead of trying to
construct it.
Cheers,
Alejandro
© 2016 - 2025 Red Hat, Inc.