* [PATCH v9 0/6] Boot modules for Hyperlaunch
@ 2024-11-15 13:11 Daniel P. Smith
2024-11-15 13:11 ` [PATCH v9 1/6] x86/boot: convert domain construction to use boot info Daniel P. Smith
` (5 more replies)
0 siblings, 6 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 13:11 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini
The Boot Modules for Hyperlaunch series is an effort to split out preliminary
changes necessary for the introduction of the Hyperlaunch domain builder
logic. These preliminary changes revolve around introducing the struct
boot_module and struct boot_domain structures. This includes converting the
dom0 construction path to use these structures. These abstractions lay the
groundwork to transform and extend the dom0 construction logic into a limited,
but 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.
Much thanks to AMD for supporting this work.
Lastly it should be noted that this series builds/relies upon Andy Cooper's
"x86/ucode: Fix module-handling use-after-free's" series.
Documentation on Hyperlaunch:
https://wiki.xenproject.org/wiki/Hyperlaunch
Original Hyperlaunch v1 patch series:
https://lists.xenproject.org/archives/html/xen-devel/2022-07/msg00345.html
V/r,
Daniel P. Smith
Changes since v8:
- moved commit 'convert domain construction to use boot info' forward
- reworked the remove module commit
- addressed a few code style comments
Changes since v7:
- patches re-organized with some collapsing into others
Changes since v6:
- Dropped patches that were merged from v5
- patch 8 title and commit message rewritten
- boot module interator patch merged with xsm patch, its first use
- incorporated review requests
- made additional style optimizations as a result of review requests
Changes since v5:
- switched to per patch change logs
- incorporated review requests
Changes since v4:
- added requested inline code comments
- moved instance of struct boot_info to unit level and extern'ed
- array of struct boot_module moved into struct boot_info
- renamed function to multiboot_fill_bootinfo, now returns *struct boot_info
- multiboot_fill_bootinfo changed to take multiboot_info_t addr as param
- added missing guard that checked there were multiboot1/2 modules passed
- renmaed struct elements per the review
- fixed errant commit messages per the review
- corrected coding style per review
- attempted to repalce all open codings of page/addr translations touched
- unified use of `bi` as var name for pointer ref to struct boot_info
- when appropriate, ensure variables where typed, eg size_t, paddr_t, etc.
- dropped all uses of "a = b = c"
Changes since v3:
- reduced scope to x86 only
- broke changes into a smaller chunks with a linear progression
- concerns about deconflicting with Arm deferred
- conversion from mb1 to boot modules no longer attempted at entry points
- the temporary conversion function is now the permenant means to convert
- incorporated suggestion from Andy Cooper for handling bootstrap_map
Changes since v2:
- combined v2 patches 7 and 8 for common review
- rebased the v2 series onto the current tip of staging (sorry)
- fixed the placement of the patch changelogs
- provided the changes description in the cover letter
Changes since v1:
- the v2 and v3 series implement functionality from v1 patches 2-4
- v2 series objective is to enable efficient patch review in support
of merging the functionality into the hypervisor. It implements a
subset of the v1 series, incorporating changes from community
feedback.
- the bootstrap map is made accessible early in the v2 series via both
multiboot and boot module arguments until later in the series where
multiboot use is retired. This allows for incremental conversion across
several patches from multiboot to boot modules.
- the 32-bit x86 boot environment header is removed, and changes are
made to allow the new common bootinfo headers to be used instead.
- Arm and RISC-V architecture bootinfo headers are added to ensure that
builds on those architectures can complete correctly.
- The KConfig patch to set the maximum number of boot modules allowed
is not included in this series, replaced with a static maximum define.
Daniel P. Smith (6):
x86/boot: convert domain construction to use boot info
x86/boot: introduce module release
x86/boot: add start and size fields to struct boot_module
x86/boot: introduce boot domain
x86/boot: introduce domid field to struct boot_domain
x86/boot: add cmdline to struct boot_domain
xen/arch/x86/cpu/microcode/core.c | 8 +-
xen/arch/x86/dom0_build.c | 10 +-
xen/arch/x86/hvm/dom0_build.c | 41 +++---
xen/arch/x86/include/asm/bootdomain.h | 35 +++++
xen/arch/x86/include/asm/bootinfo.h | 13 +-
xen/arch/x86/include/asm/dom0_build.h | 13 +-
xen/arch/x86/include/asm/setup.h | 13 +-
xen/arch/x86/pv/dom0_build.c | 80 +++++------
xen/arch/x86/setup.c | 188 +++++++++++++++-----------
xen/xsm/xsm_policy.c | 2 +-
10 files changed, 239 insertions(+), 164 deletions(-)
create mode 100644 xen/arch/x86/include/asm/bootdomain.h
--
2.30.2
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v9 1/6] x86/boot: convert domain construction to use boot info
2024-11-15 13:11 [PATCH v9 0/6] Boot modules for Hyperlaunch Daniel P. Smith
@ 2024-11-15 13:11 ` Daniel P. Smith
2024-11-15 14:25 ` Andrew Cooper
2024-11-15 16:33 ` Jason Andryuk
2024-11-15 13:12 ` [PATCH v9 2/6] x86/boot: introduce module release Daniel P. Smith
` (4 subsequent siblings)
5 siblings, 2 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 13:11 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
With all the components used to construct dom0 encapsulated in struct boot_info
and struct boot_module, it is no longer necessary to pass all them as
parameters down the domain construction call chain. Change the parameter list
to pass the struct boot_info instance and the struct domain reference.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v8:
- moved forward in the series
Changes since v7:
- renamed from "x86/boot: convert create_dom0 to use boot info"
Changes since v5:
- change headroom back to unsigned long
- make mod_idx unsigned int
---
xen/arch/x86/dom0_build.c | 8 ++--
xen/arch/x86/hvm/dom0_build.c | 46 ++++++++++++--------
xen/arch/x86/include/asm/dom0_build.h | 12 ++----
xen/arch/x86/include/asm/setup.h | 8 ++--
xen/arch/x86/pv/dom0_build.c | 62 +++++++++++++++++----------
xen/arch/x86/setup.c | 33 ++++++++------
6 files changed, 95 insertions(+), 74 deletions(-)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 72747b92475a..e8f5bf5447bc 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -596,9 +596,7 @@ int __init dom0_setup_permissions(struct domain *d)
return rc;
}
-int __init construct_dom0(struct domain *d, const module_t *image,
- unsigned long image_headroom, module_t *initrd,
- const char *cmdline)
+int __init construct_dom0(struct boot_info *bi, struct domain *d)
{
int rc;
@@ -610,9 +608,9 @@ int __init construct_dom0(struct domain *d, const module_t *image,
process_pending_softirqs();
if ( is_hvm_domain(d) )
- rc = dom0_construct_pvh(d, image, image_headroom, initrd, cmdline);
+ rc = dom0_construct_pvh(bi, d);
else if ( is_pv_domain(d) )
- rc = dom0_construct_pv(d, image, image_headroom, initrd, cmdline);
+ rc = dom0_construct_pv(bi, d);
else
panic("Cannot construct Dom0. No guest interface available\n");
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 3dd913bdb029..d1bdf1b14601 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -16,6 +16,7 @@
#include <acpi/actables.h>
+#include <asm/bootinfo.h>
#include <asm/bzimage.h>
#include <asm/dom0_build.h>
#include <asm/hvm/support.h>
@@ -642,15 +643,15 @@ static bool __init check_and_adjust_load_address(
return true;
}
-static int __init pvh_load_kernel(struct domain *d, const module_t *image,
- unsigned long image_headroom,
- module_t *initrd, void *image_base,
- const char *cmdline, paddr_t *entry,
- paddr_t *start_info_addr)
+static int __init pvh_load_kernel(
+ struct domain *d, struct boot_module *image, struct boot_module *initrd,
+ paddr_t *entry, paddr_t *start_info_addr)
{
- void *image_start = image_base + image_headroom;
- unsigned long image_len = image->mod_end;
- unsigned long initrd_len = initrd ? initrd->mod_end : 0;
+ void *image_base = bootstrap_map_bm(image);
+ void *image_start = image_base + image->headroom;
+ unsigned long image_len = image->mod->mod_end;
+ unsigned long initrd_len = initrd ? initrd->mod->mod_end : 0;
+ const char *cmdline = __va(image->cmdline_pa);
struct elf_binary elf;
struct elf_dom_parms parms;
paddr_t last_addr;
@@ -725,8 +726,8 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
if ( initrd != NULL )
{
- rc = hvm_copy_to_guest_phys(last_addr, mfn_to_virt(initrd->mod_start),
- initrd_len, v);
+ rc = hvm_copy_to_guest_phys(
+ last_addr, mfn_to_virt(initrd->mod->mod_start), initrd_len, v);
if ( rc )
{
printk("Unable to copy initrd to guest\n");
@@ -736,9 +737,9 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
mod.paddr = last_addr;
mod.size = initrd_len;
last_addr += ROUNDUP(initrd_len, elf_64bit(&elf) ? 8 : 4);
- if ( initrd->string )
+ if ( initrd->cmdline_pa )
{
- char *str = __va(initrd->string);
+ char *str = __va(initrd->cmdline_pa);
size_t len = strlen(str) + 1;
rc = hvm_copy_to_guest_phys(last_addr, str, len, v);
@@ -1300,16 +1301,26 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
}
}
-int __init dom0_construct_pvh(struct domain *d, const module_t *image,
- unsigned long image_headroom,
- module_t *initrd,
- const char *cmdline)
+int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
{
paddr_t entry, start_info;
+ struct boot_module *image;
+ struct boot_module *initrd = NULL;
+ unsigned int idx;
int rc;
printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
+ idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
+ if ( idx >= bi->nr_modules )
+ panic("Missing kernel boot module for %pd construction\n", d);
+
+ image = &bi->mods[idx];
+
+ idx = first_boot_module_index(bi, BOOTMOD_RAMDISK);
+ if ( idx < bi->nr_modules )
+ initrd = &bi->mods[idx];
+
if ( is_hardware_domain(d) )
{
/*
@@ -1347,8 +1358,7 @@ int __init dom0_construct_pvh(struct domain *d, const module_t *image,
return rc;
}
- rc = pvh_load_kernel(d, image, image_headroom, initrd, bootstrap_map(image),
- cmdline, &entry, &start_info);
+ rc = pvh_load_kernel(d, image, initrd, &entry, &start_info);
if ( rc )
{
printk("Failed to load Dom0 kernel\n");
diff --git a/xen/arch/x86/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h
index 107c1ff98367..2d67b17213dc 100644
--- a/xen/arch/x86/include/asm/dom0_build.h
+++ b/xen/arch/x86/include/asm/dom0_build.h
@@ -13,15 +13,9 @@ unsigned long dom0_compute_nr_pages(struct domain *d,
unsigned long initrd_len);
int dom0_setup_permissions(struct domain *d);
-int dom0_construct_pv(struct domain *d, const module_t *image,
- unsigned long image_headroom,
- module_t *initrd,
- const char *cmdline);
-
-int dom0_construct_pvh(struct domain *d, const module_t *image,
- unsigned long image_headroom,
- module_t *initrd,
- const char *cmdline);
+struct boot_info;
+int dom0_construct_pv(struct boot_info *bi, struct domain *d);
+int dom0_construct_pvh(struct boot_info *bi, struct domain *d);
unsigned long dom0_paging_pages(const struct domain *d,
unsigned long nr_pages);
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 25c15ef9140d..8a415087e9a4 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -26,11 +26,9 @@ void subarch_init_memory(void);
void init_IRQ(void);
-int construct_dom0(
- struct domain *d,
- const module_t *image, unsigned long image_headroom,
- module_t *initrd,
- const char *cmdline);
+struct boot_info;
+int construct_dom0(struct boot_info *bi, struct domain *d);
+
void setup_io_bitmap(struct domain *d);
extern struct boot_info xen_boot_info;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index cc882bee61c3..6be3d7745fab 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -14,6 +14,7 @@
#include <xen/softirq.h>
#include <xen/vga.h>
+#include <asm/bootinfo.h>
#include <asm/bzimage.h>
#include <asm/dom0_build.h>
#include <asm/guest.h>
@@ -354,13 +355,10 @@ static struct page_info * __init alloc_chunk(struct domain *d,
return page;
}
-static int __init dom0_construct(struct domain *d,
- const module_t *image,
- unsigned long image_headroom,
- module_t *initrd,
- const char *cmdline)
+static int __init dom0_construct(struct boot_info *bi, struct domain *d)
{
- int i, rc, order, machine;
+ unsigned int i;
+ int rc, order, machine;
bool compatible, compat;
struct cpu_user_regs *regs;
unsigned long pfn, mfn;
@@ -374,10 +372,13 @@ static int __init dom0_construct(struct domain *d,
unsigned int flush_flags = 0;
start_info_t *si;
struct vcpu *v = d->vcpu[0];
- void *image_base = bootstrap_map(image);
- unsigned long image_len = image->mod_end;
- void *image_start = image_base + image_headroom;
- unsigned long initrd_len = initrd ? initrd->mod_end : 0;
+ struct boot_module *image;
+ struct boot_module *initrd = NULL;
+ void *image_base;
+ unsigned long image_len;
+ void *image_start;
+ unsigned long initrd_len = 0;
+ const char *cmdline;
l4_pgentry_t *l4tab = NULL, *l4start = NULL;
l3_pgentry_t *l3tab = NULL, *l3start = NULL;
l2_pgentry_t *l2tab = NULL, *l2start = NULL;
@@ -414,6 +415,23 @@ static int __init dom0_construct(struct domain *d,
printk(XENLOG_INFO "*** Building a PV Dom%d ***\n", d->domain_id);
+ i = first_boot_module_index(bi, BOOTMOD_KERNEL);
+ if ( i >= bi->nr_modules )
+ panic("Missing kernel boot module for %pd construction\n", d);
+
+ image = &bi->mods[i];
+ image_base = bootstrap_map_bm(image);
+ image_len = image->mod->mod_end;
+ image_start = image_base + image->headroom;
+ cmdline = __va(image->cmdline_pa);
+
+ i = first_boot_module_index(bi, BOOTMOD_RAMDISK);
+ if ( i < bi->nr_modules )
+ {
+ initrd = &bi->mods[i];
+ initrd_len = initrd->mod->mod_end;
+ }
+
d->max_pages = ~0U;
if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
@@ -613,7 +631,8 @@ static int __init dom0_construct(struct domain *d,
initrd_pfn = vinitrd_start ?
(vinitrd_start - v_start) >> PAGE_SHIFT :
domain_tot_pages(d);
- initrd_mfn = mfn = initrd->mod_start;
+ initrd_mfn = initrd->mod->mod_start;
+ mfn = initrd_mfn;
count = PFN_UP(initrd_len);
if ( d->arch.physaddr_bitsize &&
((mfn + count - 1) >> (d->arch.physaddr_bitsize - PAGE_SHIFT)) )
@@ -628,12 +647,13 @@ static int __init dom0_construct(struct domain *d,
free_domheap_pages(page, order);
page += 1UL << order;
}
- memcpy(page_to_virt(page), mfn_to_virt(initrd->mod_start),
+ memcpy(page_to_virt(page), mfn_to_virt(initrd->mod->mod_start),
initrd_len);
- mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT;
+ mpt_alloc = pfn_to_paddr(initrd->mod->mod_start);
init_domheap_pages(mpt_alloc,
mpt_alloc + PAGE_ALIGN(initrd_len));
- initrd->mod_start = initrd_mfn = mfn_x(page_to_mfn(page));
+ initrd_mfn = mfn_x(page_to_mfn(page));
+ initrd->mod->mod_start = initrd_mfn;
}
else
{
@@ -650,7 +670,7 @@ static int __init dom0_construct(struct domain *d,
* Either way, tell discard_initial_images() to not free it a second
* time.
*/
- initrd->mod_end = 0;
+ initrd->mod->mod_end = 0;
iommu_memory_setup(d, "initrd", mfn_to_page(_mfn(initrd_mfn)),
PFN_UP(initrd_len), &flush_flags);
@@ -664,7 +684,7 @@ static int __init dom0_construct(struct domain *d,
nr_pages - domain_tot_pages(d));
if ( initrd )
{
- mpt_alloc = (paddr_t)initrd->mod_start << PAGE_SHIFT;
+ mpt_alloc = pfn_to_paddr(initrd->mod->mod_start);
printk("\n Init. ramdisk: %"PRIpaddr"->%"PRIpaddr,
mpt_alloc, mpt_alloc + initrd_len);
}
@@ -892,7 +912,7 @@ static int __init dom0_construct(struct domain *d,
if ( pfn >= initrd_pfn )
{
if ( pfn < initrd_pfn + PFN_UP(initrd_len) )
- mfn = initrd->mod_start + (pfn - initrd_pfn);
+ mfn = initrd->mod->mod_start + (pfn - initrd_pfn);
else
mfn -= PFN_UP(initrd_len);
}
@@ -1060,11 +1080,7 @@ out:
return rc;
}
-int __init dom0_construct_pv(struct domain *d,
- const module_t *image,
- unsigned long image_headroom,
- module_t *initrd,
- const char *cmdline)
+int __init dom0_construct_pv(struct boot_info *bi, struct domain *d)
{
unsigned long cr4 = read_cr4();
int rc;
@@ -1082,7 +1098,7 @@ int __init dom0_construct_pv(struct domain *d,
write_cr4(cr4 & ~X86_CR4_SMAP);
}
- rc = dom0_construct(d, image, image_headroom, initrd, cmdline);
+ rc = dom0_construct(bi, d);
if ( cr4 & X86_CR4_SMAP )
{
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4feef9f2e05a..495e90a7e132 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -950,10 +950,7 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
return n;
}
-static struct domain *__init create_dom0(const module_t *image,
- unsigned long headroom,
- module_t *initrd, const char *kextra,
- const char *loader)
+static struct domain *__init create_dom0(struct boot_info *bi)
{
static char __initdata cmdline[MAX_GUEST_CMDLINE];
@@ -970,6 +967,14 @@ static struct domain *__init create_dom0(const module_t *image,
};
struct domain *d;
domid_t domid;
+ struct boot_module *image;
+ unsigned int idx;
+
+ idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
+ if ( idx >= bi->nr_modules )
+ panic("Missing kernel boot module for building domain\n");
+
+ image = &bi->mods[idx];
if ( opt_dom0_pvh )
{
@@ -996,14 +1001,15 @@ static struct domain *__init create_dom0(const module_t *image,
panic("Error creating d%uv0\n", domid);
/* Grab the DOM0 command line. */
- if ( image->string || kextra )
+ if ( image->cmdline_pa || bi->kextra )
{
- if ( image->string )
- safe_strcpy(cmdline, cmdline_cook(__va(image->string), loader));
+ if ( image->cmdline_pa )
+ safe_strcpy(
+ cmdline, cmdline_cook(__va(image->cmdline_pa), bi->loader));
- if ( kextra )
+ if ( bi->kextra )
/* kextra always includes exactly one leading space. */
- safe_strcat(cmdline, kextra);
+ safe_strcat(cmdline, bi->kextra);
/* Append any extra parameters. */
if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
@@ -1020,9 +1026,11 @@ static struct domain *__init create_dom0(const module_t *image,
safe_strcat(cmdline, " acpi=");
safe_strcat(cmdline, acpi_param);
}
+
+ image->cmdline_pa = __pa(cmdline);
}
- if ( construct_dom0(d, image, headroom, initrd, cmdline) != 0 )
+ if ( construct_dom0(bi, d) != 0 )
panic("Could not construct domain 0\n");
return d;
@@ -2114,10 +2122,7 @@ void asmlinkage __init noreturn __start_xen(void)
* We're going to setup domain0 using the module(s) that we stashed safely
* above our heap. The second module, if present, is an initrd ramdisk.
*/
- dom0 = create_dom0(bi->mods[0].mod, bi->mods[0].headroom,
- initrdidx < bi->nr_modules ? bi->mods[initrdidx].mod
- : NULL,
- bi->kextra, bi->loader);
+ dom0 = create_dom0(bi);
if ( !dom0 )
panic("Could not set up DOM0 guest OS\n");
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v9 2/6] x86/boot: introduce module release
2024-11-15 13:11 [PATCH v9 0/6] Boot modules for Hyperlaunch Daniel P. Smith
2024-11-15 13:11 ` [PATCH v9 1/6] x86/boot: convert domain construction to use boot info Daniel P. Smith
@ 2024-11-15 13:12 ` Daniel P. Smith
2024-11-15 16:50 ` Jason Andryuk
2024-11-15 13:12 ` [PATCH v9 3/6] x86/boot: add start and size fields to struct boot_module Daniel P. Smith
` (3 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 13:12 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
A precarious approach was used to release the pages used to hold a boot module.
The precariousness stemmed from the fact that in the case of PV dom0, the
initrd module pages may be either mapped or copied into the dom0 address space.
In the former case, the PV dom0 construction code will set the size of the
module to zero, relying on discard_initial_images() to skip any modules with a
size of zero. In the latter case, the pages are freed by the PV dom0
construction code. This freeing of pages is done so that in either case, the
initrd variable can be reused for tracking the initrd location in dom0 memory
through the remaining dom0 construction code.
To encapsulate the logical action of releasing a boot module, the function
release_boot_module() is introduced along with the `released` flag added to
boot module. The boot module flag `released` allows the tracking of when a boot
module has been released by release_boot_module().
As part of adopting release_boot_module() the function discard_initial_images()
is renamed to free_boot_modules(), a name that better reflects the functions
actions.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v8:
- completely reworked the commit
- switch backed to a releasing all but pv initrd approach
- renamed discard_initial_images to free_boot_modules
---
xen/arch/x86/hvm/dom0_build.c | 2 +-
xen/arch/x86/include/asm/bootinfo.h | 2 ++
xen/arch/x86/include/asm/setup.h | 4 +++-
xen/arch/x86/pv/dom0_build.c | 27 +++++++++++++--------------
xen/arch/x86/setup.c | 27 +++++++++++++++------------
5 files changed, 34 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index d1bdf1b14601..d1410e1a02b0 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
}
/* Free temporary buffers. */
- discard_initial_images();
+ free_boot_modules();
if ( cmdline != NULL )
{
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index b9c94b370d57..f76876386763 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -34,8 +34,10 @@ struct boot_module {
/*
* Module State Flags:
* relocated: indicates module has been relocated in memory.
+ * released: indicates module's pages have been freed.
*/
bool relocated:1;
+ bool released:1;
/*
* A boot module may need decompressing by Xen. Headroom is an estimate of
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 8a415087e9a4..4ad493637892 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -34,13 +34,15 @@ void setup_io_bitmap(struct domain *d);
extern struct boot_info xen_boot_info;
unsigned long initial_images_nrpages(nodeid_t node);
-void discard_initial_images(void);
+void free_boot_modules(void);
struct boot_module;
void *bootstrap_map_bm(const struct boot_module *bm);
void *bootstrap_map(const module_t *mod);
void bootstrap_unmap(void);
+void release_boot_module(struct boot_module *bm);
+
struct rangeset;
int remove_xen_ranges(struct rangeset *r);
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 6be3d7745fab..2580162f3df4 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -649,9 +649,12 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
}
memcpy(page_to_virt(page), mfn_to_virt(initrd->mod->mod_start),
initrd_len);
- mpt_alloc = pfn_to_paddr(initrd->mod->mod_start);
- init_domheap_pages(mpt_alloc,
- mpt_alloc + PAGE_ALIGN(initrd_len));
+ /*
+ * The initrd was copied but the initrd variable is reused in the
+ * calculations below. As to not leak the memory used for the
+ * module free at this time.
+ */
+ release_boot_module(initrd);
initrd_mfn = mfn_x(page_to_mfn(page));
initrd->mod->mod_start = initrd_mfn;
}
@@ -660,18 +663,14 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
while ( count-- )
if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
BUG();
+ /*
+ * We have mapped the initrd directly into dom0, and assigned the
+ * pages. Tell the boot_module handling that we've freed it, so the
+ * memory is left alone.
+ */
+ initrd->released = true;
}
- /*
- * We have either:
- * - Mapped the initrd directly into dom0, or
- * - Copied it and freed the module.
- *
- * Either way, tell discard_initial_images() to not free it a second
- * time.
- */
- initrd->mod->mod_end = 0;
-
iommu_memory_setup(d, "initrd", mfn_to_page(_mfn(initrd_mfn)),
PFN_UP(initrd_len), &flush_flags);
}
@@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
}
/* Free temporary buffers. */
- discard_initial_images();
+ free_boot_modules();
/* Set up start info area. */
si = (start_info_t *)vstartinfo_start;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 495e90a7e132..0bda1326a485 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -346,27 +346,30 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
return nr;
}
-void __init discard_initial_images(void) /* a.k.a. Free boot modules */
+void __init release_boot_module(struct boot_module *bm)
+{
+ uint64_t start = pfn_to_paddr(bm->mod->mod_start);
+ uint64_t size = bm->mod->mod_end;
+
+ ASSERT(!bm->released);
+
+ init_domheap_pages(start, start + PAGE_ALIGN(size));
+
+ bm->released = true;
+}
+
+void __init free_boot_modules(void)
{
struct boot_info *bi = &xen_boot_info;
unsigned int i;
for ( i = 0; i < bi->nr_modules; ++i )
{
- uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
- uint64_t size = bi->mods[i].mod->mod_end;
-
- /*
- * Sometimes the initrd is mapped, rather than copied, into dom0.
- * Size being 0 is how we're instructed to leave the module alone.
- */
- if ( size == 0 )
+ if ( bi->mods[i].released )
continue;
- init_domheap_pages(start, start + PAGE_ALIGN(size));
+ release_boot_module(&bi->mods[i]);
}
-
- bi->nr_modules = 0;
}
static void __init init_idle_domain(void)
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v9 3/6] x86/boot: add start and size fields to struct boot_module
2024-11-15 13:11 [PATCH v9 0/6] Boot modules for Hyperlaunch Daniel P. Smith
2024-11-15 13:11 ` [PATCH v9 1/6] x86/boot: convert domain construction to use boot info Daniel P. Smith
2024-11-15 13:12 ` [PATCH v9 2/6] x86/boot: introduce module release Daniel P. Smith
@ 2024-11-15 13:12 ` Daniel P. Smith
2024-11-15 17:31 ` Jason Andryuk
2024-11-15 13:12 ` [PATCH v9 4/6] x86/boot: introduce boot domain Daniel P. Smith
` (2 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 13:12 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Introduce the start and size fields to struct boot_module and
assigns their value during boot_info construction. All uses of module_t to get
the address and size of a module are replaced with start and size.
The EFI entry point is a special case, as the EFI file loading boot service may
load a file beyond the 4G barrier. As a result, to make the address fit in the
32bit integer used by the MB1 module_t structure, the frame number is stored in
mod_start and size in mod_end. Until the EFI entry point is enlightened to work
with boot_info and boot_module, multiboot_fill_boot_info will handle the
alternate values in mod_start and mod_end when EFI is detected.
A result of the switch to start/size removes all uses of the mod field in
struct boot_modules, along with the uses of bootstra_map() and release_module()
functions. With all usage gone, they all are dropped here.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
--
Changes since v8:
- reversed logic for efi case in multiboot_fill_bootinfo()
- corrected missed paddr_to_pfn()
Changes since v7:
- add the start/size change to bootstrap_map_bm()
- convert all BM start/size when introduced, consolidates:
x86/boot: populate boot module for xen entry
x86/boot: transition relocation calculations to struct boot_module
- consolidates all the removal commits
Changes since v6:
- put the efi conversion for mod_start and mod_end back along with check
- dropped unnecessary cast
- updated the population of start and size fields to take into account efi
Changes since v5:
- switched EFI population of mod_start/mod_end to addresses
---
xen/arch/x86/cpu/microcode/core.c | 8 +--
xen/arch/x86/hvm/dom0_build.c | 6 +-
xen/arch/x86/include/asm/bootinfo.h | 6 +-
xen/arch/x86/include/asm/setup.h | 1 -
xen/arch/x86/pv/dom0_build.c | 15 +++--
xen/arch/x86/setup.c | 89 ++++++++++++++---------------
xen/xsm/xsm_policy.c | 2 +-
7 files changed, 60 insertions(+), 67 deletions(-)
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index fd4b08b45388..1176d5fbd502 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -709,8 +709,8 @@ static int __init cf_check microcode_init_cache(void)
/* early_microcode_load() didn't leave us any work to do. */
return 0;
- size = bi->mods[early_mod_idx].mod->mod_end;
- data = __mfn_to_virt(bi->mods[early_mod_idx].mod->mod_start);
+ size = bi->mods[early_mod_idx].size;
+ data = __va(bi->mods[early_mod_idx].start);
/*
* If opt_scan is set, we're looking for a CPIO archive rather than a raw
@@ -786,7 +786,7 @@ static int __init early_microcode_load(struct boot_info *bi)
bm->type != BOOTMOD_RAMDISK )
continue;
- size = bm->mod->mod_end;
+ size = bm->size;
data = bootstrap_map_bm(bm);
if ( !data )
{
@@ -840,7 +840,7 @@ static int __init early_microcode_load(struct boot_info *bi)
}
bi->mods[idx].type = BOOTMOD_MICROCODE;
- size = bi->mods[idx].mod->mod_end;
+ size = bi->mods[idx].size;
data = bootstrap_map_bm(&bi->mods[idx]);
if ( !data )
{
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index d1410e1a02b0..0bd1e4277bff 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -649,8 +649,8 @@ static int __init pvh_load_kernel(
{
void *image_base = bootstrap_map_bm(image);
void *image_start = image_base + image->headroom;
- unsigned long image_len = image->mod->mod_end;
- unsigned long initrd_len = initrd ? initrd->mod->mod_end : 0;
+ unsigned long image_len = image->size;
+ unsigned long initrd_len = initrd ? initrd->size : 0;
const char *cmdline = __va(image->cmdline_pa);
struct elf_binary elf;
struct elf_dom_parms parms;
@@ -727,7 +727,7 @@ static int __init pvh_load_kernel(
if ( initrd != NULL )
{
rc = hvm_copy_to_guest_phys(
- last_addr, mfn_to_virt(initrd->mod->mod_start), initrd_len, v);
+ last_addr, __va(initrd->start), initrd_len, v);
if ( rc )
{
printk("Unable to copy initrd to guest\n");
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index f76876386763..f8b422913063 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -26,9 +26,6 @@ enum bootmod_type {
};
struct boot_module {
- /* Transitionary only */
- module_t *mod;
-
enum bootmod_type type;
/*
@@ -62,6 +59,9 @@ struct boot_module {
unsigned long headroom;
paddr_t cmdline_pa;
+
+ paddr_t start;
+ size_t size;
};
/*
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 4ad493637892..5c2391a8684b 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -38,7 +38,6 @@ void free_boot_modules(void);
struct boot_module;
void *bootstrap_map_bm(const struct boot_module *bm);
-void *bootstrap_map(const module_t *mod);
void bootstrap_unmap(void);
void release_boot_module(struct boot_module *bm);
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 2580162f3df4..09df12dce694 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -421,7 +421,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
image = &bi->mods[i];
image_base = bootstrap_map_bm(image);
- image_len = image->mod->mod_end;
+ image_len = image->size;
image_start = image_base + image->headroom;
cmdline = __va(image->cmdline_pa);
@@ -429,7 +429,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
if ( i < bi->nr_modules )
{
initrd = &bi->mods[i];
- initrd_len = initrd->mod->mod_end;
+ initrd_len = initrd->size;
}
d->max_pages = ~0U;
@@ -631,7 +631,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
initrd_pfn = vinitrd_start ?
(vinitrd_start - v_start) >> PAGE_SHIFT :
domain_tot_pages(d);
- initrd_mfn = initrd->mod->mod_start;
+ initrd_mfn = paddr_to_pfn(initrd->start);
mfn = initrd_mfn;
count = PFN_UP(initrd_len);
if ( d->arch.physaddr_bitsize &&
@@ -647,8 +647,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
free_domheap_pages(page, order);
page += 1UL << order;
}
- memcpy(page_to_virt(page), mfn_to_virt(initrd->mod->mod_start),
- initrd_len);
+ memcpy(page_to_virt(page), __va(initrd->start), initrd_len);
/*
* The initrd was copied but the initrd variable is reused in the
* calculations below. As to not leak the memory used for the
@@ -656,7 +655,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
*/
release_boot_module(initrd);
initrd_mfn = mfn_x(page_to_mfn(page));
- initrd->mod->mod_start = initrd_mfn;
+ initrd->start = pfn_to_paddr(initrd_mfn);
}
else
{
@@ -683,7 +682,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
nr_pages - domain_tot_pages(d));
if ( initrd )
{
- mpt_alloc = pfn_to_paddr(initrd->mod->mod_start);
+ mpt_alloc = initrd->start;
printk("\n Init. ramdisk: %"PRIpaddr"->%"PRIpaddr,
mpt_alloc, mpt_alloc + initrd_len);
}
@@ -911,7 +910,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
if ( pfn >= initrd_pfn )
{
if ( pfn < initrd_pfn + PFN_UP(initrd_len) )
- mfn = initrd->mod->mod_start + (pfn - initrd_pfn);
+ mfn = paddr_to_pfn(initrd->start) + (pfn - initrd_pfn);
else
mfn -= PFN_UP(initrd_len);
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 0bda1326a485..b4eba122d8a5 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -314,13 +314,29 @@ static struct boot_info *__init multiboot_fill_boot_info(
*/
for ( i = 0; i < MAX_NR_BOOTMODS && i < bi->nr_modules; i++ )
{
- bi->mods[i].mod = &mods[i];
-
bi->mods[i].cmdline_pa = mods[i].string;
+
+ if ( efi_enabled(EFI_LOADER) )
+ {
+ /*
+ * The EFI loader gives us modules which are in frame/size. Switch
+ * to address/size.
+ */
+ bi->mods[i].start = pfn_to_paddr(mods[i].mod_start);
+ bi->mods[i].size = mods[i].mod_end;
+ }
+ else
+ {
+ /*
+ * PVH and BIOS loaders give us modules which are start/end.
+ * Switch to address/size.
+ */
+ bi->mods[i].start = mods[i].mod_start;
+ bi->mods[i].size = mods[i].mod_end - mods[i].mod_start;
+ }
}
/* Variable 'i' should be one entry past the last module. */
- bi->mods[i].mod = &mods[bi->nr_modules];
bi->mods[i].type = BOOTMOD_XEN;
return bi;
@@ -336,8 +352,8 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
for ( nr = i = 0; i < bi->nr_modules; ++i )
{
- unsigned long start = bi->mods[i].mod->mod_start;
- unsigned long end = start + PFN_UP(bi->mods[i].mod->mod_end);
+ unsigned long start = paddr_to_pfn(bi->mods[i].start);
+ unsigned long end = start + PFN_UP(bi->mods[i].size);
if ( end > node_start && node_end > start )
nr += min(node_end, end) - max(node_start, start);
@@ -348,12 +364,9 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
void __init release_boot_module(struct boot_module *bm)
{
- uint64_t start = pfn_to_paddr(bm->mod->mod_start);
- uint64_t size = bm->mod->mod_end;
-
ASSERT(!bm->released);
- init_domheap_pages(start, start + PAGE_ALIGN(size));
+ init_domheap_pages(bm->start, bm->start + PAGE_ALIGN(bm->size));
bm->released = true;
}
@@ -485,15 +498,9 @@ static void *__init bootstrap_map_addr(paddr_t start, paddr_t end)
return ret;
}
-void *__init bootstrap_map(const module_t *mod)
-{
- return bootstrap_map_addr(pfn_to_paddr(mod->mod_start),
- pfn_to_paddr(mod->mod_start) + mod->mod_end);
-}
-
void *__init bootstrap_map_bm(const struct boot_module *bm)
{
- return bootstrap_map(bm->mod);
+ return bootstrap_map_addr(bm->start, bm->start + bm->size);
}
void __init bootstrap_unmap(void)
@@ -671,8 +678,8 @@ static uint64_t __init consider_modules(
for ( i = 0; i < nr_mods ; ++i )
{
- uint64_t start = pfn_to_paddr(mods[i].mod->mod_start);
- uint64_t end = start + PAGE_ALIGN(mods[i].mod->mod_end);
+ uint64_t start = mods[i].start;
+ uint64_t end = start + PAGE_ALIGN(mods[i].size);
if ( i == this_mod )
continue;
@@ -1403,13 +1410,9 @@ void asmlinkage __init noreturn __start_xen(void)
set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
kexec_reserve_area();
- for ( i = 0; !efi_enabled(EFI_LOADER) && i < bi->nr_modules; i++ )
- {
- if ( bi->mods[i].mod->mod_start & (PAGE_SIZE - 1) )
+ for ( i = 0; i < bi->nr_modules; i++ )
+ if ( bi->mods[i].start & (PAGE_SIZE - 1) )
panic("Bootloader didn't honor module alignment request\n");
- bi->mods[i].mod->mod_end -= bi->mods[i].mod->mod_start;
- bi->mods[i].mod->mod_start >>= PAGE_SHIFT;
- }
/*
* TODO: load ucode earlier once multiboot modules become accessible
@@ -1428,13 +1431,12 @@ void asmlinkage __init noreturn __start_xen(void)
* respective reserve_e820_ram() invocation below. No need to
* query efi_boot_mem_unused() here, though.
*/
- xen->mod->mod_start = virt_to_mfn(_stext);
- xen->mod->mod_end = __2M_rwdata_end - _stext;
+ xen->start = virt_to_maddr(_stext);
+ xen->size = __2M_rwdata_end - _stext;
}
bi->mods[0].headroom =
- bzimage_headroom(bootstrap_map_bm(&bi->mods[0]),
- bi->mods[0].mod->mod_end);
+ bzimage_headroom(bootstrap_map_bm(&bi->mods[0]), bi->mods[0].size);
bootstrap_unmap();
#ifndef highmem_start
@@ -1515,7 +1517,7 @@ void asmlinkage __init noreturn __start_xen(void)
for ( j = bi->nr_modules - 1; j >= 0; j-- )
{
struct boot_module *bm = &bi->mods[j];
- unsigned long size = PAGE_ALIGN(bm->headroom + bm->mod->mod_end);
+ unsigned long size = PAGE_ALIGN(bm->headroom + bm->size);
if ( bm->relocated )
continue;
@@ -1527,14 +1529,11 @@ void asmlinkage __init noreturn __start_xen(void)
if ( highmem_start && end > highmem_start )
continue;
- if ( s < end &&
- (bm->headroom ||
- ((end - size) >> PAGE_SHIFT) > bm->mod->mod_start) )
+ if ( s < end && (bm->headroom || (end - size) > bm->start) )
{
- move_memory(end - size + bm->headroom,
- pfn_to_paddr(bm->mod->mod_start), bm->mod->mod_end);
- bm->mod->mod_start = (end - size) >> PAGE_SHIFT;
- bm->mod->mod_end += bm->headroom;
+ move_memory(end - size + bm->headroom, bm->start, bm->size);
+ bm->start = (end - size);
+ bm->size += bm->headroom;
bm->relocated = true;
}
}
@@ -1565,10 +1564,9 @@ void asmlinkage __init noreturn __start_xen(void)
panic("Not enough memory to relocate the dom0 kernel image\n");
for ( i = 0; i < bi->nr_modules; ++i )
{
- const struct boot_module *bm = &bi->mods[i];
- uint64_t s = pfn_to_paddr(bm->mod->mod_start);
+ uint64_t s = bi->mods[i].start, l = bi->mods[i].size;
- reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(bm->mod->mod_end));
+ reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l));
}
if ( !xen_phys_start )
@@ -1646,8 +1644,7 @@ void asmlinkage __init noreturn __start_xen(void)
map_e = boot_e820.map[j].addr + boot_e820.map[j].size;
for ( j = 0; j < bi->nr_modules; ++j )
{
- uint64_t end = pfn_to_paddr(bi->mods[j].mod->mod_start) +
- bi->mods[j].mod->mod_end;
+ uint64_t end = bi->mods[j].start + bi->mods[j].size;
if ( map_e < end )
map_e = end;
@@ -1721,13 +1718,11 @@ void asmlinkage __init noreturn __start_xen(void)
for ( i = 0; i < bi->nr_modules; ++i )
{
- const struct boot_module *bm = &bi->mods[i];
+ unsigned long s = bi->mods[i].start, l = bi->mods[i].size;
- set_pdx_range(bm->mod->mod_start,
- bm->mod->mod_start + PFN_UP(bm->mod->mod_end));
- map_pages_to_xen((unsigned long)mfn_to_virt(bm->mod->mod_start),
- _mfn(bm->mod->mod_start),
- PFN_UP(bm->mod->mod_end), PAGE_HYPERVISOR);
+ set_pdx_range(paddr_to_pfn(s), paddr_to_pfn(s + l) + 1);
+ map_pages_to_xen((unsigned long)maddr_to_virt(s), maddr_to_mfn(s),
+ PFN_UP(l), PAGE_HYPERVISOR);
}
#ifdef CONFIG_KEXEC
diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
index 76280903d5be..7f70d860bd65 100644
--- a/xen/xsm/xsm_policy.c
+++ b/xen/xsm/xsm_policy.c
@@ -43,7 +43,7 @@ int __init xsm_multiboot_policy_init(
struct boot_module *bm = &bi->mods[i];
_policy_start = bootstrap_map_bm(bm);
- _policy_len = bm->mod->mod_end;
+ _policy_len = bm->size;
if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC )
{
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v9 4/6] x86/boot: introduce boot domain
2024-11-15 13:11 [PATCH v9 0/6] Boot modules for Hyperlaunch Daniel P. Smith
` (2 preceding siblings ...)
2024-11-15 13:12 ` [PATCH v9 3/6] x86/boot: add start and size fields to struct boot_module Daniel P. Smith
@ 2024-11-15 13:12 ` Daniel P. Smith
2024-11-27 10:22 ` Jan Beulich
2024-11-15 13:12 ` [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain Daniel P. Smith
2024-11-15 13:12 ` [PATCH v9 6/6] x86/boot: add cmdline " Daniel P. Smith
5 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 13:12 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
To begin moving toward allowing the hypervisor to construct more than one
domain at boot, a container is needed for a domain's build information.
Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial
struct boot_domain that encapsulate the build information for a domain.
Add a kernel and ramdisk boot module reference along with a struct domain
reference to the new struct boot_domain. This allows a struct boot_domain
reference to be the only parameter necessary to pass down through the domain
construction call chain.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
Changes since v8:
- code style correction
---
xen/arch/x86/dom0_build.c | 8 ++++---
xen/arch/x86/hvm/dom0_build.c | 17 +++++----------
xen/arch/x86/include/asm/bootdomain.h | 31 +++++++++++++++++++++++++++
xen/arch/x86/include/asm/bootinfo.h | 5 +++++
xen/arch/x86/include/asm/dom0_build.h | 6 +++---
xen/arch/x86/include/asm/setup.h | 4 ++--
xen/arch/x86/pv/dom0_build.c | 24 +++++++--------------
xen/arch/x86/setup.c | 24 +++++++++------------
8 files changed, 69 insertions(+), 50 deletions(-)
create mode 100644 xen/arch/x86/include/asm/bootdomain.h
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index e8f5bf5447bc..c231191faec7 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -13,6 +13,7 @@
#include <xen/softirq.h>
#include <asm/amd.h>
+#include <asm/bootinfo.h>
#include <asm/dom0_build.h>
#include <asm/guest.h>
#include <asm/hpet.h>
@@ -596,9 +597,10 @@ int __init dom0_setup_permissions(struct domain *d)
return rc;
}
-int __init construct_dom0(struct boot_info *bi, struct domain *d)
+int __init construct_dom0(struct boot_domain *bd)
{
int rc;
+ const struct domain *d = bd->d;
/* Sanity! */
BUG_ON(!pv_shim && d->domain_id != 0);
@@ -608,9 +610,9 @@ int __init construct_dom0(struct boot_info *bi, struct domain *d)
process_pending_softirqs();
if ( is_hvm_domain(d) )
- rc = dom0_construct_pvh(bi, d);
+ rc = dom0_construct_pvh(bd);
else if ( is_pv_domain(d) )
- rc = dom0_construct_pv(bi, d);
+ rc = dom0_construct_pv(bd);
else
panic("Cannot construct Dom0. No guest interface available\n");
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 0bd1e4277bff..69881599e77e 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -1301,26 +1301,19 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
}
}
-int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
+int __init dom0_construct_pvh(struct boot_domain *bd)
{
paddr_t entry, start_info;
- struct boot_module *image;
- struct boot_module *initrd = NULL;
- unsigned int idx;
+ struct boot_module *image = bd->kernel;
+ struct boot_module *initrd = bd->ramdisk;
+ struct domain *d = bd->d;
int rc;
printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
- idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
- if ( idx >= bi->nr_modules )
+ if ( image == NULL )
panic("Missing kernel boot module for %pd construction\n", d);
- image = &bi->mods[idx];
-
- idx = first_boot_module_index(bi, BOOTMOD_RAMDISK);
- if ( idx < bi->nr_modules )
- initrd = &bi->mods[idx];
-
if ( is_hardware_domain(d) )
{
/*
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
new file mode 100644
index 000000000000..12c19ab37bd8
--- /dev/null
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (c) 2024 Apertus Solutions, LLC
+ * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
+ * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
+ */
+
+#ifndef __XEN_X86_BOOTDOMAIN_H__
+#define __XEN_X86_BOOTDOMAIN_H__
+
+struct boot_module;
+struct domain;
+
+struct boot_domain {
+ struct boot_module *kernel;
+ struct boot_module *ramdisk;
+
+ struct domain *d;
+};
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index f8b422913063..9f65e2c8f62d 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -11,10 +11,14 @@
#include <xen/init.h>
#include <xen/multiboot.h>
#include <xen/types.h>
+#include <asm/bootdomain.h>
/* Max number of boot modules a bootloader can provide in addition to Xen */
#define MAX_NR_BOOTMODS 63
+/* Max number of boot domains that Xen can construct */
+#define MAX_NR_BOOTDOMS 1
+
/* Boot module binary type / purpose */
enum bootmod_type {
BOOTMOD_UNKNOWN,
@@ -78,6 +82,7 @@ struct boot_info {
unsigned int nr_modules;
struct boot_module mods[MAX_NR_BOOTMODS + 1];
+ struct boot_domain domains[MAX_NR_BOOTDOMS];
};
/*
diff --git a/xen/arch/x86/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h
index 2d67b17213dc..8c94e87dc576 100644
--- a/xen/arch/x86/include/asm/dom0_build.h
+++ b/xen/arch/x86/include/asm/dom0_build.h
@@ -13,9 +13,9 @@ unsigned long dom0_compute_nr_pages(struct domain *d,
unsigned long initrd_len);
int dom0_setup_permissions(struct domain *d);
-struct boot_info;
-int dom0_construct_pv(struct boot_info *bi, struct domain *d);
-int dom0_construct_pvh(struct boot_info *bi, struct domain *d);
+struct boot_domain;
+int dom0_construct_pv(struct boot_domain *bd);
+int dom0_construct_pvh(struct boot_domain *bd);
unsigned long dom0_paging_pages(const struct domain *d,
unsigned long nr_pages);
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 5c2391a8684b..b517da6144de 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -26,8 +26,8 @@ void subarch_init_memory(void);
void init_IRQ(void);
-struct boot_info;
-int construct_dom0(struct boot_info *bi, struct domain *d);
+struct boot_domain;
+int construct_dom0(struct boot_domain *bd);
void setup_io_bitmap(struct domain *d);
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 09df12dce694..f42aeb031694 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -355,7 +355,7 @@ static struct page_info * __init alloc_chunk(struct domain *d,
return page;
}
-static int __init dom0_construct(struct boot_info *bi, struct domain *d)
+static int __init dom0_construct(struct boot_domain *bd)
{
unsigned int i;
int rc, order, machine;
@@ -371,13 +371,14 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
struct page_info *page = NULL;
unsigned int flush_flags = 0;
start_info_t *si;
+ struct domain *d = bd->d;
struct vcpu *v = d->vcpu[0];
- struct boot_module *image;
- struct boot_module *initrd = NULL;
+ struct boot_module *image = bd->kernel;
+ struct boot_module *initrd = bd->ramdisk;
void *image_base;
unsigned long image_len;
void *image_start;
- unsigned long initrd_len = 0;
+ unsigned long initrd_len = initrd ? initrd->size : 0;
const char *cmdline;
l4_pgentry_t *l4tab = NULL, *l4start = NULL;
l3_pgentry_t *l3tab = NULL, *l3start = NULL;
@@ -415,23 +416,14 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
printk(XENLOG_INFO "*** Building a PV Dom%d ***\n", d->domain_id);
- i = first_boot_module_index(bi, BOOTMOD_KERNEL);
- if ( i >= bi->nr_modules )
+ if ( unlikely(image == NULL) )
panic("Missing kernel boot module for %pd construction\n", d);
- image = &bi->mods[i];
image_base = bootstrap_map_bm(image);
image_len = image->size;
image_start = image_base + image->headroom;
cmdline = __va(image->cmdline_pa);
- i = first_boot_module_index(bi, BOOTMOD_RAMDISK);
- if ( i < bi->nr_modules )
- {
- initrd = &bi->mods[i];
- initrd_len = initrd->size;
- }
-
d->max_pages = ~0U;
if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
@@ -1078,7 +1070,7 @@ out:
return rc;
}
-int __init dom0_construct_pv(struct boot_info *bi, struct domain *d)
+int __init dom0_construct_pv(struct boot_domain *bd)
{
unsigned long cr4 = read_cr4();
int rc;
@@ -1096,7 +1088,7 @@ int __init dom0_construct_pv(struct boot_info *bi, struct domain *d)
write_cr4(cr4 & ~X86_CR4_SMAP);
}
- rc = dom0_construct(bi, d);
+ rc = dom0_construct(bd);
if ( cr4 & X86_CR4_SMAP )
{
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b4eba122d8a5..2ccaa7dc965b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -975,16 +975,9 @@ static struct domain *__init create_dom0(struct boot_info *bi)
.misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
},
};
+ struct boot_domain *bd = &bi->domains[0];
struct domain *d;
domid_t domid;
- struct boot_module *image;
- unsigned int idx;
-
- idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
- if ( idx >= bi->nr_modules )
- panic("Missing kernel boot module for building domain\n");
-
- image = &bi->mods[idx];
if ( opt_dom0_pvh )
{
@@ -1011,11 +1004,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
panic("Error creating d%uv0\n", domid);
/* Grab the DOM0 command line. */
- if ( image->cmdline_pa || bi->kextra )
+ if ( bd->kernel->cmdline_pa || bi->kextra )
{
- if ( image->cmdline_pa )
- safe_strcpy(
- cmdline, cmdline_cook(__va(image->cmdline_pa), bi->loader));
+ if ( bd->kernel->cmdline_pa )
+ safe_strcpy(cmdline,
+ cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader));
if ( bi->kextra )
/* kextra always includes exactly one leading space. */
@@ -1037,10 +1030,11 @@ static struct domain *__init create_dom0(struct boot_info *bi)
safe_strcat(cmdline, acpi_param);
}
- image->cmdline_pa = __pa(cmdline);
+ bd->kernel->cmdline_pa = __pa(cmdline);
}
- if ( construct_dom0(bi, d) != 0 )
+ bd->d = d;
+ if ( construct_dom0(bd) != 0 )
panic("Could not construct domain 0\n");
return d;
@@ -1240,6 +1234,7 @@ void asmlinkage __init noreturn __start_xen(void)
/* Dom0 kernel is always first */
bi->mods[0].type = BOOTMOD_KERNEL;
+ bi->domains[0].kernel = &bi->mods[0];
if ( pvh_boot )
{
@@ -2110,6 +2105,7 @@ void asmlinkage __init noreturn __start_xen(void)
if ( initrdidx < MAX_NR_BOOTMODS )
{
bi->mods[initrdidx].type = BOOTMOD_RAMDISK;
+ bi->domains[0].ramdisk = &bi->mods[initrdidx];
if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS )
printk(XENLOG_WARNING
"Multiple initrd candidates, picking module #%u\n",
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain
2024-11-15 13:11 [PATCH v9 0/6] Boot modules for Hyperlaunch Daniel P. Smith
` (3 preceding siblings ...)
2024-11-15 13:12 ` [PATCH v9 4/6] x86/boot: introduce boot domain Daniel P. Smith
@ 2024-11-15 13:12 ` Daniel P. Smith
2024-11-15 15:31 ` Daniel P. Smith
2024-11-27 10:32 ` Jan Beulich
2024-11-15 13:12 ` [PATCH v9 6/6] x86/boot: add cmdline " Daniel P. Smith
5 siblings, 2 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 13:12 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Add a domid field to struct boot_domain to hold the assigned domain id for the
domain. During initialization, ensure all instances of struct boot_domain have
the invalid domid to ensure that the domid must be set either by convention or
configuration.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
xen/arch/x86/include/asm/bootdomain.h | 2 ++
xen/arch/x86/setup.c | 12 +++++++-----
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 12c19ab37bd8..3873f916f854 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -12,6 +12,8 @@ struct boot_module;
struct domain;
struct boot_domain {
+ domid_t domid;
+
struct boot_module *kernel;
struct boot_module *ramdisk;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2ccaa7dc965b..533a1e2bbe05 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -339,6 +339,9 @@ static struct boot_info *__init multiboot_fill_boot_info(
/* Variable 'i' should be one entry past the last module. */
bi->mods[i].type = BOOTMOD_XEN;
+ for ( i = 0; i < MAX_NR_BOOTDOMS; i++ )
+ bi->domains[i].domid = DOMID_INVALID;
+
return bi;
}
@@ -977,7 +980,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
};
struct boot_domain *bd = &bi->domains[0];
struct domain *d;
- domid_t domid;
if ( opt_dom0_pvh )
{
@@ -993,15 +995,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
/* Create initial domain. Not d0 for pvshim. */
- domid = get_initial_domain_id();
- d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
+ bd->domid = get_initial_domain_id();
+ d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
if ( IS_ERR(d) )
- panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
+ panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
init_dom0_cpuid_policy(d);
if ( alloc_dom0_vcpu0(d) == NULL )
- panic("Error creating d%uv0\n", domid);
+ panic("Error creating d%uv0\n", bd->domid);
/* Grab the DOM0 command line. */
if ( bd->kernel->cmdline_pa || bi->kextra )
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain
2024-11-15 13:11 [PATCH v9 0/6] Boot modules for Hyperlaunch Daniel P. Smith
` (4 preceding siblings ...)
2024-11-15 13:12 ` [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain Daniel P. Smith
@ 2024-11-15 13:12 ` Daniel P. Smith
2024-11-15 15:12 ` Daniel P. Smith
` (2 more replies)
5 siblings, 3 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 13:12 UTC (permalink / raw)
To: xen-devel
Cc: Daniel P. Smith, jason.andryuk, christopher.w.clark,
stefano.stabellini, Jan Beulich, Andrew Cooper,
Roger Pau Monné
Add a container for the "cooked" command line for a domain. This provides for
the backing memory to be directly associated with the domain being constructed.
This is done in anticipation that the domain construction path may need to be
invoked multiple times, thus ensuring each instance had a distinct memory
allocation.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v8:
- switch to a dynamically allocated buffer
- dropped local cmdline var in pv dom0_construct()
Changes since v7:
- updated commit message to expand on intent and purpose
---
xen/arch/x86/include/asm/bootdomain.h | 2 ++
xen/arch/x86/include/asm/dom0_build.h | 1 +
xen/arch/x86/pv/dom0_build.c | 6 ++--
xen/arch/x86/setup.c | 49 ++++++++++++++++++++++-----
4 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
index 3873f916f854..75e7c706d86e 100644
--- a/xen/arch/x86/include/asm/bootdomain.h
+++ b/xen/arch/x86/include/asm/bootdomain.h
@@ -12,6 +12,8 @@ struct boot_module;
struct domain;
struct boot_domain {
+ const char *cmdline;
+
domid_t domid;
struct boot_module *kernel;
diff --git a/xen/arch/x86/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h
index 8c94e87dc576..6ca3ca7c8a43 100644
--- a/xen/arch/x86/include/asm/dom0_build.h
+++ b/xen/arch/x86/include/asm/dom0_build.h
@@ -4,6 +4,7 @@
#include <xen/libelf.h>
#include <xen/sched.h>
+#include <asm/bootinfo.h>
#include <asm/setup.h>
extern unsigned int dom0_memflags;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index f42aeb031694..91bcce1542bc 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -379,7 +379,6 @@ static int __init dom0_construct(struct boot_domain *bd)
unsigned long image_len;
void *image_start;
unsigned long initrd_len = initrd ? initrd->size : 0;
- const char *cmdline;
l4_pgentry_t *l4tab = NULL, *l4start = NULL;
l3_pgentry_t *l3tab = NULL, *l3start = NULL;
l2_pgentry_t *l2tab = NULL, *l2start = NULL;
@@ -422,7 +421,6 @@ static int __init dom0_construct(struct boot_domain *bd)
image_base = bootstrap_map_bm(image);
image_len = image->size;
image_start = image_base + image->headroom;
- cmdline = __va(image->cmdline_pa);
d->max_pages = ~0U;
@@ -972,8 +970,8 @@ static int __init dom0_construct(struct boot_domain *bd)
}
memset(si->cmd_line, 0, sizeof(si->cmd_line));
- if ( cmdline != NULL )
- strlcpy((char *)si->cmd_line, cmdline, 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 533a1e2bbe05..b9ca9c486fe5 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -963,10 +963,31 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
return n;
}
-static struct domain *__init create_dom0(struct boot_info *bi)
+static size_t __init domain_cmdline_size(
+ struct boot_info *bi, struct boot_domain *bd)
{
- static char __initdata cmdline[MAX_GUEST_CMDLINE];
+ size_t s = 0;
+
+ s += bi->kextra ? strlen(bi->kextra) : 0;
+ s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
+ /* Should only be called if one of extra or cmdline_pa are valid */
+ ASSERT(s > 0);
+
+ /*
+ * Add additional space for the following cases:
+ * - 7 chars for " noapic"
+ * - 13 chars for longest acpi opiton, " acpi=verbose"
+ * - 1 char to hold \0
+ */
+ s += 7 + 13 + 1;
+
+ return s;
+}
+
+static struct domain *__init create_dom0(struct boot_info *bi)
+{
+ char *cmdline = NULL;
struct xen_domctl_createdomain dom0_cfg = {
.flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
.max_evtchn_port = -1,
@@ -1008,17 +1029,23 @@ static struct domain *__init create_dom0(struct boot_info *bi)
/* Grab the DOM0 command line. */
if ( bd->kernel->cmdline_pa || bi->kextra )
{
+ size_t cmdline_size = domain_cmdline_size(bi, bd);
+
+ 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 )
{
@@ -1028,17 +1055,21 @@ 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->cmdline = cmdline;
+ bd->kernel->cmdline_pa = __pa(bd->cmdline);
}
bd->d = d;
if ( construct_dom0(bd) != 0 )
panic("Could not construct domain 0\n");
+ if ( cmdline )
+ xfree(cmdline);
+
return d;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v9 1/6] x86/boot: convert domain construction to use boot info
2024-11-15 13:11 ` [PATCH v9 1/6] x86/boot: convert domain construction to use boot info Daniel P. Smith
@ 2024-11-15 14:25 ` Andrew Cooper
2024-11-15 14:32 ` Daniel P. Smith
2024-11-15 16:33 ` Jason Andryuk
1 sibling, 1 reply; 30+ messages in thread
From: Andrew Cooper @ 2024-11-15 14:25 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Jan Beulich, Roger Pau Monné
On 15/11/2024 1:11 pm, Daniel P. Smith wrote:
> With all the components used to construct dom0 encapsulated in struct boot_info
> and struct boot_module, it is no longer necessary to pass all them as
> parameters down the domain construction call chain. Change the parameter list
> to pass the struct boot_info instance and the struct domain reference.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
There are two minor things needing noting in the commit message.
1) dom0_construct() turns i from being signed to unsigned. This is
necessary for it's new use, and compatible with all pre-existing uses.
2) dom0_construct() also splits some 3-way assignments to placate MISRA,
on lines which are modified.
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 3dd913bdb029..d1bdf1b14601 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -642,15 +643,15 @@ static bool __init check_and_adjust_load_address(
> return true;
> }
>
> -static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> - unsigned long image_headroom,
> - module_t *initrd, void *image_base,
> - const char *cmdline, paddr_t *entry,
> - paddr_t *start_info_addr)
> +static int __init pvh_load_kernel(
> + struct domain *d, struct boot_module *image, struct boot_module *initrd,
> + paddr_t *entry, paddr_t *start_info_addr)
> {
> - void *image_start = image_base + image_headroom;
> - unsigned long image_len = image->mod_end;
> - unsigned long initrd_len = initrd ? initrd->mod_end : 0;
> + void *image_base = bootstrap_map_bm(image);
> + void *image_start = image_base + image->headroom;
> + unsigned long image_len = image->mod->mod_end;
> + unsigned long initrd_len = initrd ? initrd->mod->mod_end : 0;
> + const char *cmdline = __va(image->cmdline_pa);
This isn't safe. __va(0) != NULL, so later between ...
> struct elf_binary elf;
> struct elf_dom_parms parms;
> paddr_t last_addr;
> @@ -725,8 +726,8 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
... these two hunks in the calculation for last_addr, we have:
... cmdline ? ROUNDUP(strlen(cmdline) + 1, ...
which does the wrong thing. (And includes the 16bit IVT onto the
guest's cmdline.)
I'd suggest doing the same as we do with initrd_len/etc, and having:
const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) :
NULL;
to maintain the prior semantics.
>
> if ( initrd != NULL )
> {
> - rc = hvm_copy_to_guest_phys(last_addr, mfn_to_virt(initrd->mod_start),
> - initrd_len, v);
> + rc = hvm_copy_to_guest_phys(
> + last_addr, mfn_to_virt(initrd->mod->mod_start), initrd_len, v);
This is a temporary adjustment, ending up shorter than it starts by
patch 3. I've tweaked it to reduce the churn overall. I can live with
83 chars width for a commit or two...
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index cc882bee61c3..6be3d7745fab 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -354,13 +355,10 @@ static struct page_info * __init alloc_chunk(struct domain *d,
> return page;
> }
>
> -static int __init dom0_construct(struct domain *d,
> - const module_t *image,
> - unsigned long image_headroom,
> - module_t *initrd,
> - const char *cmdline)
> +static int __init dom0_construct(struct boot_info *bi, struct domain *d)
> {
> - int i, rc, order, machine;
> + unsigned int i;
> + int rc, order, machine;
> bool compatible, compat;
> struct cpu_user_regs *regs;
> unsigned long pfn, mfn;
> @@ -374,10 +372,13 @@ static int __init dom0_construct(struct domain *d,
> unsigned int flush_flags = 0;
> start_info_t *si;
> struct vcpu *v = d->vcpu[0];
> - void *image_base = bootstrap_map(image);
> - unsigned long image_len = image->mod_end;
> - void *image_start = image_base + image_headroom;
> - unsigned long initrd_len = initrd ? initrd->mod_end : 0;
> + struct boot_module *image;
> + struct boot_module *initrd = NULL;
> + void *image_base;
> + unsigned long image_len;
> + void *image_start;
> + unsigned long initrd_len = 0;
> + const char *cmdline;
I'm tempted to put in some newlines here, just to break up the giant
block of variables.
This use of cmdline in principle needs a similar adjustment to the pvh
case, but it's only used once, so I suggest this instead:
@@ -984,8 +982,8 @@ static int __init dom0_construct(struct boot_info
*bi, struct domain *d)
}
memset(si->cmd_line, 0, sizeof(si->cmd_line));
- if ( cmdline != NULL )
- strlcpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line));
+ if ( image->cmdline_pa )
+ strlcpy((char *)si->cmd_line, __va(image->cmdline_pa),
sizeof(si->cmd_line));
#ifdef CONFIG_VIDEO
if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
[edit] Turns out you do this in patch 6 anyway, so this way around will
reduce churn.
Happy to fix on commit.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 1/6] x86/boot: convert domain construction to use boot info
2024-11-15 14:25 ` Andrew Cooper
@ 2024-11-15 14:32 ` Daniel P. Smith
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 14:32 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Jan Beulich, Roger Pau Monné
On 11/15/24 09:25, Andrew Cooper wrote:
> On 15/11/2024 1:11 pm, Daniel P. Smith wrote:
>> With all the components used to construct dom0 encapsulated in struct boot_info
>> and struct boot_module, it is no longer necessary to pass all them as
>> parameters down the domain construction call chain. Change the parameter list
>> to pass the struct boot_info instance and the struct domain reference.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>
> There are two minor things needing noting in the commit message.
>
> 1) dom0_construct() turns i from being signed to unsigned. This is
> necessary for it's new use, and compatible with all pre-existing uses.
>
> 2) dom0_construct() also splits some 3-way assignments to placate MISRA,
> on lines which are modified.
Ack.
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
>> index 3dd913bdb029..d1bdf1b14601 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -642,15 +643,15 @@ static bool __init check_and_adjust_load_address(
>> return true;
>> }
>>
>> -static int __init pvh_load_kernel(struct domain *d, const module_t *image,
>> - unsigned long image_headroom,
>> - module_t *initrd, void *image_base,
>> - const char *cmdline, paddr_t *entry,
>> - paddr_t *start_info_addr)
>> +static int __init pvh_load_kernel(
>> + struct domain *d, struct boot_module *image, struct boot_module *initrd,
>> + paddr_t *entry, paddr_t *start_info_addr)
>> {
>> - void *image_start = image_base + image_headroom;
>> - unsigned long image_len = image->mod_end;
>> - unsigned long initrd_len = initrd ? initrd->mod_end : 0;
>> + void *image_base = bootstrap_map_bm(image);
>> + void *image_start = image_base + image->headroom;
>> + unsigned long image_len = image->mod->mod_end;
>> + unsigned long initrd_len = initrd ? initrd->mod->mod_end : 0;
>> + const char *cmdline = __va(image->cmdline_pa);
>
> This isn't safe. __va(0) != NULL, so later between ...
Yah, that was careless to assume.
>> struct elf_binary elf;
>> struct elf_dom_parms parms;
>> paddr_t last_addr;
>> @@ -725,8 +726,8 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
>
> ... these two hunks in the calculation for last_addr, we have:
>
> ... cmdline ? ROUNDUP(strlen(cmdline) + 1, ...
>
> which does the wrong thing. (And includes the 16bit IVT onto the
> guest's cmdline.)
>
>
> I'd suggest doing the same as we do with initrd_len/etc, and having:
>
> const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) :
> NULL;
>
> to maintain the prior semantics.
Agreed.
>>
>> if ( initrd != NULL )
>> {
>> - rc = hvm_copy_to_guest_phys(last_addr, mfn_to_virt(initrd->mod_start),
>> - initrd_len, v);
>> + rc = hvm_copy_to_guest_phys(
>> + last_addr, mfn_to_virt(initrd->mod->mod_start), initrd_len, v);
>
> This is a temporary adjustment, ending up shorter than it starts by
> patch 3. I've tweaked it to reduce the churn overall. I can live with
> 83 chars width for a commit or two...
Just trying to ensure I don't get dinged, so no objection on my part.
>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>> index cc882bee61c3..6be3d7745fab 100644
>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>> @@ -354,13 +355,10 @@ static struct page_info * __init alloc_chunk(struct domain *d,
>> return page;
>> }
>>
>> -static int __init dom0_construct(struct domain *d,
>> - const module_t *image,
>> - unsigned long image_headroom,
>> - module_t *initrd,
>> - const char *cmdline)
>> +static int __init dom0_construct(struct boot_info *bi, struct domain *d)
>> {
>> - int i, rc, order, machine;
>> + unsigned int i;
>> + int rc, order, machine;
>> bool compatible, compat;
>> struct cpu_user_regs *regs;
>> unsigned long pfn, mfn;
>> @@ -374,10 +372,13 @@ static int __init dom0_construct(struct domain *d,
>> unsigned int flush_flags = 0;
>> start_info_t *si;
>> struct vcpu *v = d->vcpu[0];
>> - void *image_base = bootstrap_map(image);
>> - unsigned long image_len = image->mod_end;
>> - void *image_start = image_base + image_headroom;
>> - unsigned long initrd_len = initrd ? initrd->mod_end : 0;
>> + struct boot_module *image;
>> + struct boot_module *initrd = NULL;
>> + void *image_base;
>> + unsigned long image_len;
>> + void *image_start;
>> + unsigned long initrd_len = 0;
>> + const char *cmdline;
>
> I'm tempted to put in some newlines here, just to break up the giant
> block of variables.
Yes, this is a very long block of declarations.
> This use of cmdline in principle needs a similar adjustment to the pvh
> case, but it's only used once, so I suggest this instead:
>
> @@ -984,8 +982,8 @@ static int __init dom0_construct(struct boot_info
> *bi, struct domain *d)
> }
>
> memset(si->cmd_line, 0, sizeof(si->cmd_line));
> - if ( cmdline != NULL )
> - strlcpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line));
> + if ( image->cmdline_pa )
> + strlcpy((char *)si->cmd_line, __va(image->cmdline_pa),
> sizeof(si->cmd_line));
>
> #ifdef CONFIG_VIDEO
> if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
>
>
> [edit] Turns out you do this in patch 6 anyway, so this way around will
> reduce churn.
Ack.
> Happy to fix on commit.
No objection.
v/r,
dps
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain
2024-11-15 13:12 ` [PATCH v9 6/6] x86/boot: add cmdline " Daniel P. Smith
@ 2024-11-15 15:12 ` Daniel P. Smith
2024-11-15 16:34 ` Daniel P. Smith
2024-11-15 18:20 ` Jason Andryuk
2 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 15:12 UTC (permalink / raw)
To: xen-devel
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Jan Beulich, Andrew Cooper, Roger Pau Monné
On 11/15/24 08:12, Daniel P. Smith wrote:
> Add a container for the "cooked" command line for a domain. This provides for
> the backing memory to be directly associated with the domain being constructed.
> This is done in anticipation that the domain construction path may need to be
> invoked multiple times, thus ensuring each instance had a distinct memory
> allocation.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> Changes since v8:
> - switch to a dynamically allocated buffer
> - dropped local cmdline var in pv dom0_construct()
>
> Changes since v7:
> - updated commit message to expand on intent and purpose
> ---
> xen/arch/x86/include/asm/bootdomain.h | 2 ++
> xen/arch/x86/include/asm/dom0_build.h | 1 +
> xen/arch/x86/pv/dom0_build.c | 6 ++--
> xen/arch/x86/setup.c | 49 ++++++++++++++++++++++-----
> 4 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
> index 3873f916f854..75e7c706d86e 100644
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -12,6 +12,8 @@ struct boot_module;
> struct domain;
>
> struct boot_domain {
> + const char *cmdline;
> +
> domid_t domid;
>
> struct boot_module *kernel;
> diff --git a/xen/arch/x86/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h
> index 8c94e87dc576..6ca3ca7c8a43 100644
> --- a/xen/arch/x86/include/asm/dom0_build.h
> +++ b/xen/arch/x86/include/asm/dom0_build.h
> @@ -4,6 +4,7 @@
> #include <xen/libelf.h>
> #include <xen/sched.h>
>
> +#include <asm/bootinfo.h>
> #include <asm/setup.h>
>
> extern unsigned int dom0_memflags;
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index f42aeb031694..91bcce1542bc 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -379,7 +379,6 @@ static int __init dom0_construct(struct boot_domain *bd)
> unsigned long image_len;
> void *image_start;
> unsigned long initrd_len = initrd ? initrd->size : 0;
> - const char *cmdline;
> l4_pgentry_t *l4tab = NULL, *l4start = NULL;
> l3_pgentry_t *l3tab = NULL, *l3start = NULL;
> l2_pgentry_t *l2tab = NULL, *l2start = NULL;
> @@ -422,7 +421,6 @@ static int __init dom0_construct(struct boot_domain *bd)
> image_base = bootstrap_map_bm(image);
> image_len = image->size;
> image_start = image_base + image->headroom;
> - cmdline = __va(image->cmdline_pa);
>
> d->max_pages = ~0U;
>
> @@ -972,8 +970,8 @@ static int __init dom0_construct(struct boot_domain *bd)
> }
>
> memset(si->cmd_line, 0, sizeof(si->cmd_line));
> - if ( cmdline != NULL )
> - strlcpy((char *)si->cmd_line, cmdline, 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 533a1e2bbe05..b9ca9c486fe5 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -963,10 +963,31 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
> return n;
> }
>
> -static struct domain *__init create_dom0(struct boot_info *bi)
> +static size_t __init domain_cmdline_size(
> + struct boot_info *bi, struct boot_domain *bd)
> {
> - static char __initdata cmdline[MAX_GUEST_CMDLINE];
> + size_t s = 0;
> +
> + s += bi->kextra ? strlen(bi->kextra) : 0;
Working on the subsequent series and realized this line could/should be
merged with the declaration line;
v/r,
dps
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain
2024-11-15 13:12 ` [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain Daniel P. Smith
@ 2024-11-15 15:31 ` Daniel P. Smith
2024-11-27 10:32 ` Jan Beulich
1 sibling, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 15:31 UTC (permalink / raw)
To: xen-devel
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Jan Beulich, Andrew Cooper, Roger Pau Monné
On 11/15/24 08:12, Daniel P. Smith wrote:
> Add a domid field to struct boot_domain to hold the assigned domain id for the
> domain. During initialization, ensure all instances of struct boot_domain have
> the invalid domid to ensure that the domid must be set either by convention or
> configuration.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> ---
> xen/arch/x86/include/asm/bootdomain.h | 2 ++
> xen/arch/x86/setup.c | 12 +++++++-----
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
> index 12c19ab37bd8..3873f916f854 100644
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -12,6 +12,8 @@ struct boot_module;
> struct domain;
>
> struct boot_domain {
> + domid_t domid;
There is no definition for domid_t in this file, the only reason it has
yet to fail, is that everywhere it is included has xen.h included before it.
v/r,
dps
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 1/6] x86/boot: convert domain construction to use boot info
2024-11-15 13:11 ` [PATCH v9 1/6] x86/boot: convert domain construction to use boot info Daniel P. Smith
2024-11-15 14:25 ` Andrew Cooper
@ 2024-11-15 16:33 ` Jason Andryuk
2024-11-15 17:01 ` Andrew Cooper
1 sibling, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2024-11-15 16:33 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-11-15 08:11, Daniel P. Smith wrote:
> With all the components used to construct dom0 encapsulated in struct boot_info
> and struct boot_module, it is no longer necessary to pass all them as
> parameters down the domain construction call chain. Change the parameter list
> to pass the struct boot_info instance and the struct domain reference.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 3dd913bdb029..d1bdf1b14601 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -1300,16 +1301,26 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
> }
> }
>
> -int __init dom0_construct_pvh(struct domain *d, const module_t *image,
> - unsigned long image_headroom,
> - module_t *initrd,
> - const char *cmdline)
> +int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
> {
> paddr_t entry, start_info;
> + struct boot_module *image;
> + struct boot_module *initrd = NULL;
> + unsigned int idx;
> int rc;
>
> printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
>
> + idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
> + if ( idx >= bi->nr_modules )
What do you think about introducing a new define:
#define BOOTMOD_NOT_FOUND (MAX_NR_BOOTMODS + 1)
For first_boot_module_index() to return. And then:
if ( idx == BOOTMOD_NOT_FOUND )
?
Otherwise it looks good to me, and Andrew's suggestions are good as well.
Regards,
Jason
> + panic("Missing kernel boot module for %pd construction\n", d);
> +
> + image = &bi->mods[idx];
> +
> + idx = first_boot_module_index(bi, BOOTMOD_RAMDISK);
> + if ( idx < bi->nr_modules )
> + initrd = &bi->mods[idx];
> +
> if ( is_hardware_domain(d) )
> {
> /*
> @@ -1347,8 +1358,7 @@ int __init dom0_construct_pvh(struct domain *d, const module_t *image,
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain
2024-11-15 13:12 ` [PATCH v9 6/6] x86/boot: add cmdline " Daniel P. Smith
2024-11-15 15:12 ` Daniel P. Smith
@ 2024-11-15 16:34 ` Daniel P. Smith
2024-11-15 18:20 ` Jason Andryuk
2 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 16:34 UTC (permalink / raw)
To: xen-devel
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Jan Beulich, Andrew Cooper, Roger Pau Monné
On 11/15/24 08:12, Daniel P. Smith wrote:
> Add a container for the "cooked" command line for a domain. This provides for
> the backing memory to be directly associated with the domain being constructed.
> This is done in anticipation that the domain construction path may need to be
> invoked multiple times, thus ensuring each instance had a distinct memory
> allocation.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> Changes since v8:
> - switch to a dynamically allocated buffer
> - dropped local cmdline var in pv dom0_construct()
>
> Changes since v7:
> - updated commit message to expand on intent and purpose
> ---
> xen/arch/x86/include/asm/bootdomain.h | 2 ++
> xen/arch/x86/include/asm/dom0_build.h | 1 +
> xen/arch/x86/pv/dom0_build.c | 6 ++--
> xen/arch/x86/setup.c | 49 ++++++++++++++++++++++-----
> 4 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/include/asm/bootdomain.h
> index 3873f916f854..75e7c706d86e 100644
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -12,6 +12,8 @@ struct boot_module;
> struct domain;
>
> struct boot_domain {
> + const char *cmdline;
> +
> domid_t domid;
>
> struct boot_module *kernel;
> diff --git a/xen/arch/x86/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h
> index 8c94e87dc576..6ca3ca7c8a43 100644
> --- a/xen/arch/x86/include/asm/dom0_build.h
> +++ b/xen/arch/x86/include/asm/dom0_build.h
> @@ -4,6 +4,7 @@
> #include <xen/libelf.h>
> #include <xen/sched.h>
>
> +#include <asm/bootinfo.h>
> #include <asm/setup.h>
>
> extern unsigned int dom0_memflags;
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index f42aeb031694..91bcce1542bc 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -379,7 +379,6 @@ static int __init dom0_construct(struct boot_domain *bd)
> unsigned long image_len;
> void *image_start;
> unsigned long initrd_len = initrd ? initrd->size : 0;
> - const char *cmdline;
> l4_pgentry_t *l4tab = NULL, *l4start = NULL;
> l3_pgentry_t *l3tab = NULL, *l3start = NULL;
> l2_pgentry_t *l2tab = NULL, *l2start = NULL;
> @@ -422,7 +421,6 @@ static int __init dom0_construct(struct boot_domain *bd)
> image_base = bootstrap_map_bm(image);
> image_len = image->size;
> image_start = image_base + image->headroom;
> - cmdline = __va(image->cmdline_pa);
>
> d->max_pages = ~0U;
>
> @@ -972,8 +970,8 @@ static int __init dom0_construct(struct boot_domain *bd)
> }
>
> memset(si->cmd_line, 0, sizeof(si->cmd_line));
> - if ( cmdline != NULL )
> - strlcpy((char *)si->cmd_line, cmdline, 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 533a1e2bbe05..b9ca9c486fe5 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -963,10 +963,31 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
> return n;
> }
>
> -static struct domain *__init create_dom0(struct boot_info *bi)
> +static size_t __init domain_cmdline_size(
> + struct boot_info *bi, struct boot_domain *bd)
> {
> - static char __initdata cmdline[MAX_GUEST_CMDLINE];
> + size_t s = 0;
> +
> + s += bi->kextra ? strlen(bi->kextra) : 0;
> + s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
>
> + /* Should only be called if one of extra or cmdline_pa are valid */
> + ASSERT(s > 0);
> +
> + /*
> + * Add additional space for the following cases:
> + * - 7 chars for " noapic"
> + * - 13 chars for longest acpi opiton, " acpi=verbose"
> + * - 1 char to hold \0
> + */
> + s += 7 + 13 + 1;
> +
> + return s;
> +}
> +
> +static struct domain *__init create_dom0(struct boot_info *bi)
> +{
> + char *cmdline = NULL;
> struct xen_domctl_createdomain dom0_cfg = {
> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> .max_evtchn_port = -1,
> @@ -1008,17 +1029,23 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> /* Grab the DOM0 command line. */
> if ( bd->kernel->cmdline_pa || bi->kextra )
The logic from which this originally derives mistakenly gives the sense,
at least for me, that `string` field from module_t would only be a valid
address if there was a string. I have now discovered this is not the
case but is in fact the address of a zero length string. It just so
happens all the previous logic worked out even for a zero length string.
It also means this block was always being executed, since the check for
a cmdline_pa will always be true. I am open to other suggestions, but my
thinking right now is that the check of cmdline_pa should be a twofold
check, that it is not zero and that it has a string length, e.g.:
if ( (bd->kernel->cmdline_pa &&
strlen(__va(bd->kernel->cmdline_pa))) || bi->kextra )
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 2/6] x86/boot: introduce module release
2024-11-15 13:12 ` [PATCH v9 2/6] x86/boot: introduce module release Daniel P. Smith
@ 2024-11-15 16:50 ` Jason Andryuk
2024-11-15 17:09 ` Andrew Cooper
2024-11-15 17:16 ` Daniel P. Smith
0 siblings, 2 replies; 30+ messages in thread
From: Jason Andryuk @ 2024-11-15 16:50 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-11-15 08:12, Daniel P. Smith wrote:
> A precarious approach was used to release the pages used to hold a boot module.
> The precariousness stemmed from the fact that in the case of PV dom0, the
> initrd module pages may be either mapped or copied into the dom0 address space.
> In the former case, the PV dom0 construction code will set the size of the
> module to zero, relying on discard_initial_images() to skip any modules with a
> size of zero. In the latter case, the pages are freed by the PV dom0
> construction code. This freeing of pages is done so that in either case, the
> initrd variable can be reused for tracking the initrd location in dom0 memory
> through the remaining dom0 construction code.
>
> To encapsulate the logical action of releasing a boot module, the function
> release_boot_module() is introduced along with the `released` flag added to
> boot module. The boot module flag `released` allows the tracking of when a boot
> module has been released by release_boot_module().
>
> As part of adopting release_boot_module() the function discard_initial_images()
> is renamed to free_boot_modules(), a name that better reflects the functions
> actions.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> Changes since v8:
> - completely reworked the commit
> - switch backed to a releasing all but pv initrd approach
> - renamed discard_initial_images to free_boot_modules
> ---
> xen/arch/x86/hvm/dom0_build.c | 2 +-
> xen/arch/x86/include/asm/bootinfo.h | 2 ++
> xen/arch/x86/include/asm/setup.h | 4 +++-
> xen/arch/x86/pv/dom0_build.c | 27 +++++++++++++--------------
> xen/arch/x86/setup.c | 27 +++++++++++++++------------
> 5 files changed, 34 insertions(+), 28 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index d1bdf1b14601..d1410e1a02b0 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
> }
>
> /* Free temporary buffers. */
> - discard_initial_images();
> + free_boot_modules();
This...
> if ( cmdline != NULL )
> {
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index 6be3d7745fab..2580162f3df4 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
> }
>
> /* Free temporary buffers. */
> - discard_initial_images();
> + free_boot_modules();
...and this. I think Andrew requested/suggested moving to a single
free_boot_modules call:
They're both right at the end of construction, so it would
make far more sense for __start_xen() to do this after
create_dom0(). That also avoids needing to export the function.
>
> /* Set up start info area. */
> si = (start_info_t *)vstartinfo_start;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 495e90a7e132..0bda1326a485 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> +void __init free_boot_modules(void)
> {
> struct boot_info *bi = &xen_boot_info;
> unsigned int i;
>
> for ( i = 0; i < bi->nr_modules; ++i )
> {
> - uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
> - uint64_t size = bi->mods[i].mod->mod_end;
> -
> - /*
> - * Sometimes the initrd is mapped, rather than copied, into dom0.
> - * Size being 0 is how we're instructed to leave the module alone.
> - */
> - if ( size == 0 )
> + if ( bi->mods[i].released )
> continue;
>
> - init_domheap_pages(start, start + PAGE_ALIGN(size));
> + release_boot_module(&bi->mods[i]);
> }
> -
> - bi->nr_modules = 0;
IIUC, zero-ing here was a safety feature to ensure boot modules could
not be used after this point. Should it be retained?
Regards,
Jason
> }
>
> static void __init init_idle_domain(void)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 1/6] x86/boot: convert domain construction to use boot info
2024-11-15 16:33 ` Jason Andryuk
@ 2024-11-15 17:01 ` Andrew Cooper
2024-11-15 17:02 ` Jason Andryuk
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Cooper @ 2024-11-15 17:01 UTC (permalink / raw)
To: Jason Andryuk, Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Roger Pau Monné
On 15/11/2024 4:33 pm, Jason Andryuk wrote:
> On 2024-11-15 08:11, Daniel P. Smith wrote:
>> diff --git a/xen/arch/x86/hvm/dom0_build.c
>> b/xen/arch/x86/hvm/dom0_build.c
>> index 3dd913bdb029..d1bdf1b14601 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -1300,16 +1301,26 @@ static void __hwdom_init
>> pvh_setup_mmcfg(struct domain *d)
>> }
>> }
>> -int __init dom0_construct_pvh(struct domain *d, const module_t
>> *image,
>> - unsigned long image_headroom,
>> - module_t *initrd,
>> - const char *cmdline)
>> +int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
>> {
>> paddr_t entry, start_info;
>> + struct boot_module *image;
>> + struct boot_module *initrd = NULL;
>> + unsigned int idx;
>> int rc;
>> printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n",
>> d->domain_id);
>> + idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
>> + if ( idx >= bi->nr_modules )
>
> What do you think about introducing a new define:
>
> #define BOOTMOD_NOT_FOUND (MAX_NR_BOOTMODS + 1)
>
> For first_boot_module_index() to return. And then:
>
> if ( idx == BOOTMOD_NOT_FOUND )
>
> ?
Care would need to be taken vs BOOTMOD_XEN, which could have the same
numeric value in a big HL configuration.
From a "reading the code" point of view, a range check against any
invalid value is better seeing as the next thing we do is index an
array, so I'm marginally on the side of "keep it as it is".
This particular logic can't trip because of earlier checks in
__start_xen(), and gets rewritten in patch 4 in the conversion to
boot_domains, so I'm also not overly fussed at extra polish on this
specific piece of logic.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 1/6] x86/boot: convert domain construction to use boot info
2024-11-15 17:01 ` Andrew Cooper
@ 2024-11-15 17:02 ` Jason Andryuk
0 siblings, 0 replies; 30+ messages in thread
From: Jason Andryuk @ 2024-11-15 17:02 UTC (permalink / raw)
To: Andrew Cooper, Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Roger Pau Monné
On 2024-11-15 12:01, Andrew Cooper wrote:
> On 15/11/2024 4:33 pm, Jason Andryuk wrote:
>> On 2024-11-15 08:11, Daniel P. Smith wrote:
>>> diff --git a/xen/arch/x86/hvm/dom0_build.c
>>> b/xen/arch/x86/hvm/dom0_build.c
>>> index 3dd913bdb029..d1bdf1b14601 100644
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>> @@ -1300,16 +1301,26 @@ static void __hwdom_init
>>> pvh_setup_mmcfg(struct domain *d)
>>> }
>>> }
>>> -int __init dom0_construct_pvh(struct domain *d, const module_t
>>> *image,
>>> - unsigned long image_headroom,
>>> - module_t *initrd,
>>> - const char *cmdline)
>>> +int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d)
>>> {
>>> paddr_t entry, start_info;
>>> + struct boot_module *image;
>>> + struct boot_module *initrd = NULL;
>>> + unsigned int idx;
>>> int rc;
>>> printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n",
>>> d->domain_id);
>>> + idx = first_boot_module_index(bi, BOOTMOD_KERNEL);
>>> + if ( idx >= bi->nr_modules )
>>
>> What do you think about introducing a new define:
>>
>> #define BOOTMOD_NOT_FOUND (MAX_NR_BOOTMODS + 1)
>>
>> For first_boot_module_index() to return. And then:
>>
>> if ( idx == BOOTMOD_NOT_FOUND )
>>
>> ?
>
> Care would need to be taken vs BOOTMOD_XEN, which could have the same
> numeric value in a big HL configuration.
It's a little subtle that BOOTMOD_XEN could be at MAX_NR_BOOTMODS + 1,
and first_boot_module_index() will return that for "not found". Which I
overlooked when making the suggestion.
> From a "reading the code" point of view, a range check against any
> invalid value is better seeing as the next thing we do is index an
> array, so I'm marginally on the side of "keep it as it is".
first_boot_module_index() is looking for a specific BOOTMOD_*, so I
thought it would be a little safer to just return either a valid index
or BOOTMOD_NOT_FOUND (which might have to become ~0).
> This particular logic can't trip because of earlier checks in
> __start_xen(), and gets rewritten in patch 4 in the conversion to
> boot_domains, so I'm also not overly fussed at extra polish on this
> specific piece of logic.
If maintainers are okay with it as-is, then I have no issue with the
code as-is and
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Regards,
Jason
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 2/6] x86/boot: introduce module release
2024-11-15 16:50 ` Jason Andryuk
@ 2024-11-15 17:09 ` Andrew Cooper
2024-11-15 17:16 ` Daniel P. Smith
1 sibling, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2024-11-15 17:09 UTC (permalink / raw)
To: Jason Andryuk, Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Roger Pau Monné
On 15/11/2024 4:50 pm, Jason Andryuk wrote:
> On 2024-11-15 08:12, Daniel P. Smith wrote:
>> A precarious approach was used to release the pages used to hold a
>> boot module.
>> The precariousness stemmed from the fact that in the case of PV dom0,
>> the
>> initrd module pages may be either mapped or copied into the dom0
>> address space.
>> In the former case, the PV dom0 construction code will set the size
>> of the
>> module to zero, relying on discard_initial_images() to skip any
>> modules with a
>> size of zero. In the latter case, the pages are freed by the PV dom0
>> construction code. This freeing of pages is done so that in either
>> case, the
>> initrd variable can be reused for tracking the initrd location in
>> dom0 memory
>> through the remaining dom0 construction code.
>>
>> To encapsulate the logical action of releasing a boot module, the
>> function
>> release_boot_module() is introduced along with the `released` flag
>> added to
>> boot module. The boot module flag `released` allows the tracking of
>> when a boot
>> module has been released by release_boot_module().
>>
>> As part of adopting release_boot_module() the function
>> discard_initial_images()
>> is renamed to free_boot_modules(), a name that better reflects the
>> functions
>> actions.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> Changes since v8:
>> - completely reworked the commit
>> - switch backed to a releasing all but pv initrd approach
>> - renamed discard_initial_images to free_boot_modules
>> ---
>> xen/arch/x86/hvm/dom0_build.c | 2 +-
>> xen/arch/x86/include/asm/bootinfo.h | 2 ++
>> xen/arch/x86/include/asm/setup.h | 4 +++-
>> xen/arch/x86/pv/dom0_build.c | 27 +++++++++++++--------------
>> xen/arch/x86/setup.c | 27 +++++++++++++++------------
>> 5 files changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dom0_build.c
>> b/xen/arch/x86/hvm/dom0_build.c
>> index d1bdf1b14601..d1410e1a02b0 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
>> }
>> /* Free temporary buffers. */
>> - discard_initial_images();
>> + free_boot_modules();
>
> This...
>
>> if ( cmdline != NULL )
>> {
>
>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>> index 6be3d7745fab..2580162f3df4 100644
>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>
>> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info
>> *bi, struct domain *d)
>> }
>> /* Free temporary buffers. */
>> - discard_initial_images();
>> + free_boot_modules();
>
> ...and this. I think Andrew requested/suggested moving to a single
> free_boot_modules call:
> They're both right at the end of construction, so it would
> make far more sense for __start_xen() to do this after
> create_dom0(). That also avoids needing to export the function.
Yeah... It turns out that also breaks PVH Boot in Gitlab, for reasons
we still don't understand.
I'd still like to clean it up, but it wants to be detached from the
mechanics of changing the data-structures.
>
>> /* Set up start info area. */
>> si = (start_info_t *)vstartinfo_start;
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 495e90a7e132..0bda1326a485 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>
>> +void __init free_boot_modules(void)
>> {
>> struct boot_info *bi = &xen_boot_info;
>> unsigned int i;
>> for ( i = 0; i < bi->nr_modules; ++i )
>> {
>> - uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
>> - uint64_t size = bi->mods[i].mod->mod_end;
>> -
>> - /*
>> - * Sometimes the initrd is mapped, rather than copied, into
>> dom0.
>> - * Size being 0 is how we're instructed to leave the module
>> alone.
>> - */
>> - if ( size == 0 )
>> + if ( bi->mods[i].released )
>> continue;
>> - init_domheap_pages(start, start + PAGE_ALIGN(size));
>> + release_boot_module(&bi->mods[i]);
>> }
>> -
>> - bi->nr_modules = 0;
>
> IIUC, zero-ing here was a safety feature to ensure boot modules could
> not be used after this point. Should it be retained?
Clobbering this prevents the loop constructs from working.
Safety is now based on the .released field, which is better IMO.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 2/6] x86/boot: introduce module release
2024-11-15 16:50 ` Jason Andryuk
2024-11-15 17:09 ` Andrew Cooper
@ 2024-11-15 17:16 ` Daniel P. Smith
2024-11-15 17:18 ` Jason Andryuk
1 sibling, 1 reply; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-15 17:16 UTC (permalink / raw)
To: Jason Andryuk, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 11/15/24 11:50, Jason Andryuk wrote:
> On 2024-11-15 08:12, Daniel P. Smith wrote:
>> A precarious approach was used to release the pages used to hold a
>> boot module.
>> The precariousness stemmed from the fact that in the case of PV dom0, the
>> initrd module pages may be either mapped or copied into the dom0
>> address space.
>> In the former case, the PV dom0 construction code will set the size of
>> the
>> module to zero, relying on discard_initial_images() to skip any
>> modules with a
>> size of zero. In the latter case, the pages are freed by the PV dom0
>> construction code. This freeing of pages is done so that in either
>> case, the
>> initrd variable can be reused for tracking the initrd location in dom0
>> memory
>> through the remaining dom0 construction code.
>>
>> To encapsulate the logical action of releasing a boot module, the
>> function
>> release_boot_module() is introduced along with the `released` flag
>> added to
>> boot module. The boot module flag `released` allows the tracking of
>> when a boot
>> module has been released by release_boot_module().
>>
>> As part of adopting release_boot_module() the function
>> discard_initial_images()
>> is renamed to free_boot_modules(), a name that better reflects the
>> functions
>> actions.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> Changes since v8:
>> - completely reworked the commit
>> - switch backed to a releasing all but pv initrd approach
>> - renamed discard_initial_images to free_boot_modules
>> ---
>> xen/arch/x86/hvm/dom0_build.c | 2 +-
>> xen/arch/x86/include/asm/bootinfo.h | 2 ++
>> xen/arch/x86/include/asm/setup.h | 4 +++-
>> xen/arch/x86/pv/dom0_build.c | 27 +++++++++++++--------------
>> xen/arch/x86/setup.c | 27 +++++++++++++++------------
>> 5 files changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/
>> dom0_build.c
>> index d1bdf1b14601..d1410e1a02b0 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
>> }
>> /* Free temporary buffers. */
>> - discard_initial_images();
>> + free_boot_modules();
>
> This...
>
>> if ( cmdline != NULL )
>> {
>
>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>> index 6be3d7745fab..2580162f3df4 100644
>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>
>> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info
>> *bi, struct domain *d)
>> }
>> /* Free temporary buffers. */
>> - discard_initial_images();
>> + free_boot_modules();
>
> ...and this. I think Andrew requested/suggested moving to a single
> free_boot_modules call:
> They're both right at the end of construction, so it would
> make far more sense for __start_xen() to do this after
> create_dom0(). That also avoids needing to export the function.
I wanted to do this and had it written this way. Then I started testing
it and the pvhshim test failed due to not enough ram to build the domU
inside pvshim. I started splitting this commit to see where it broke the
test case, and for an unknown reason, replacing these two calls with a
single call in __start_xen() just after create_dom0() is the cause.
Instead of trying to tear apart the construction logic to determine why,
I backed this part of the change out for the time being.
>> /* Set up start info area. */
>> si = (start_info_t *)vstartinfo_start;
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 495e90a7e132..0bda1326a485 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>
>> +void __init free_boot_modules(void)
>> {
>> struct boot_info *bi = &xen_boot_info;
>> unsigned int i;
>> for ( i = 0; i < bi->nr_modules; ++i )
>> {
>> - uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
>> - uint64_t size = bi->mods[i].mod->mod_end;
>> -
>> - /*
>> - * Sometimes the initrd is mapped, rather than copied, into
>> dom0.
>> - * Size being 0 is how we're instructed to leave the module
>> alone.
>> - */
>> - if ( size == 0 )
>> + if ( bi->mods[i].released )
>> continue;
>> - init_domheap_pages(start, start + PAGE_ALIGN(size));
>> + release_boot_module(&bi->mods[i]);
>> }
>> -
>> - bi->nr_modules = 0;
>
> IIUC, zero-ing here was a safety feature to ensure boot modules could
> not be used after this point. Should it be retained?
The released flag displaced the need for this, but I realized it would
make it stronger if in bootstrap_map_bm() we add a check that the
released flag is not set before mapping. I think this is a stronger
approach without loosing information like the number of boot modules
were passed.
v/r,
dps
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 2/6] x86/boot: introduce module release
2024-11-15 17:16 ` Daniel P. Smith
@ 2024-11-15 17:18 ` Jason Andryuk
2024-11-18 16:13 ` Andrew Cooper
0 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2024-11-15 17:18 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-11-15 12:16, Daniel P. Smith wrote:
> On 11/15/24 11:50, Jason Andryuk wrote:
>> On 2024-11-15 08:12, Daniel P. Smith wrote:
>>> A precarious approach was used to release the pages used to hold a
>>> boot module.
>>> The precariousness stemmed from the fact that in the case of PV dom0,
>>> the
>>> initrd module pages may be either mapped or copied into the dom0
>>> address space.
>>> In the former case, the PV dom0 construction code will set the size
>>> of the
>>> module to zero, relying on discard_initial_images() to skip any
>>> modules with a
>>> size of zero. In the latter case, the pages are freed by the PV dom0
>>> construction code. This freeing of pages is done so that in either
>>> case, the
>>> initrd variable can be reused for tracking the initrd location in
>>> dom0 memory
>>> through the remaining dom0 construction code.
>>>
>>> To encapsulate the logical action of releasing a boot module, the
>>> function
>>> release_boot_module() is introduced along with the `released` flag
>>> added to
>>> boot module. The boot module flag `released` allows the tracking of
>>> when a boot
>>> module has been released by release_boot_module().
>>>
>>> As part of adopting release_boot_module() the function
>>> discard_initial_images()
>>> is renamed to free_boot_modules(), a name that better reflects the
>>> functions
>>> actions.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> ---
>>> Changes since v8:
>>> - completely reworked the commit
>>> - switch backed to a releasing all but pv initrd approach
>>> - renamed discard_initial_images to free_boot_modules
>>> ---
>>> xen/arch/x86/hvm/dom0_build.c | 2 +-
>>> xen/arch/x86/include/asm/bootinfo.h | 2 ++
>>> xen/arch/x86/include/asm/setup.h | 4 +++-
>>> xen/arch/x86/pv/dom0_build.c | 27 +++++++++++++--------------
>>> xen/arch/x86/setup.c | 27 +++++++++++++++------------
>>> 5 files changed, 34 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/
>>> dom0_build.c
>>> index d1bdf1b14601..d1410e1a02b0 100644
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
>>> }
>>> /* Free temporary buffers. */
>>> - discard_initial_images();
>>> + free_boot_modules();
>>
>> This...
>>
>>> if ( cmdline != NULL )
>>> {
>>
>>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>>> index 6be3d7745fab..2580162f3df4 100644
>>> --- a/xen/arch/x86/pv/dom0_build.c
>>> +++ b/xen/arch/x86/pv/dom0_build.c
>>
>>> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info
>>> *bi, struct domain *d)
>>> }
>>> /* Free temporary buffers. */
>>> - discard_initial_images();
>>> + free_boot_modules();
>>
>> ...and this. I think Andrew requested/suggested moving to a single
>> free_boot_modules call:
>> They're both right at the end of construction, so it would
>> make far more sense for __start_xen() to do this after
>> create_dom0(). That also avoids needing to export the function.
>
> I wanted to do this and had it written this way. Then I started testing
> it and the pvhshim test failed due to not enough ram to build the domU
> inside pvshim. I started splitting this commit to see where it broke the
> test case, and for an unknown reason, replacing these two calls with a
> single call in __start_xen() just after create_dom0() is the cause.
> Instead of trying to tear apart the construction logic to determine why,
> I backed this part of the change out for the time being.
Ah, ok. Thanks for the info.
>>> /* Set up start info area. */
>>> si = (start_info_t *)vstartinfo_start;
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index 495e90a7e132..0bda1326a485 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>
>>> +void __init free_boot_modules(void)
>>> {
>>> struct boot_info *bi = &xen_boot_info;
>>> unsigned int i;
>>> for ( i = 0; i < bi->nr_modules; ++i )
>>> {
>>> - uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
>>> - uint64_t size = bi->mods[i].mod->mod_end;
>>> -
>>> - /*
>>> - * Sometimes the initrd is mapped, rather than copied, into
>>> dom0.
>>> - * Size being 0 is how we're instructed to leave the module
>>> alone.
>>> - */
>>> - if ( size == 0 )
>>> + if ( bi->mods[i].released )
>>> continue;
>>> - init_domheap_pages(start, start + PAGE_ALIGN(size));
>>> + release_boot_module(&bi->mods[i]);
>>> }
>>> -
>>> - bi->nr_modules = 0;
>>
>> IIUC, zero-ing here was a safety feature to ensure boot modules could
>> not be used after this point. Should it be retained?
>
> The released flag displaced the need for this, but I realized it would
> make it stronger if in bootstrap_map_bm() we add a check that the
> released flag is not set before mapping. I think this is a stronger
> approach without loosing information like the number of boot modules
> were passed.
Andrew> Clobbering this prevents the loop constructs from working.
I thought the boot modules are unusable after free_boot_modules() is
called, so I'm not clear on the utility of keeping the boot modules
around and/or keeping the loop constructs working. I wondered about,
but didn't write, clearing the boot_module info in release_boot_module()
to eliminate stale data hanging around.
Yes, a bootstrap_map_bm() check is a good idea. Having said that, there
is a lack of checking the return value of bootstrap_map_bm(), so would
you panic?
Regards,
Jason
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 3/6] x86/boot: add start and size fields to struct boot_module
2024-11-15 13:12 ` [PATCH v9 3/6] x86/boot: add start and size fields to struct boot_module Daniel P. Smith
@ 2024-11-15 17:31 ` Jason Andryuk
0 siblings, 0 replies; 30+ messages in thread
From: Jason Andryuk @ 2024-11-15 17:31 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-11-15 08:12, Daniel P. Smith wrote:
> Introduce the start and size fields to struct boot_module and
> assigns their value during boot_info construction. All uses of module_t to get
> the address and size of a module are replaced with start and size.
>
> The EFI entry point is a special case, as the EFI file loading boot service may
> load a file beyond the 4G barrier. As a result, to make the address fit in the
> 32bit integer used by the MB1 module_t structure, the frame number is stored in
> mod_start and size in mod_end. Until the EFI entry point is enlightened to work
> with boot_info and boot_module, multiboot_fill_boot_info will handle the
> alternate values in mod_start and mod_end when EFI is detected.
>
> A result of the switch to start/size removes all uses of the mod field in
> struct boot_modules, along with the uses of bootstra_map() and release_module()
bootstrap_map()
> functions. With all usage gone, they all are dropped here.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
with that:
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain
2024-11-15 13:12 ` [PATCH v9 6/6] x86/boot: add cmdline " Daniel P. Smith
2024-11-15 15:12 ` Daniel P. Smith
2024-11-15 16:34 ` Daniel P. Smith
@ 2024-11-15 18:20 ` Jason Andryuk
2024-11-20 17:57 ` Daniel P. Smith
2 siblings, 1 reply; 30+ messages in thread
From: Jason Andryuk @ 2024-11-15 18:20 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-11-15 08:12, Daniel P. Smith wrote:
> Add a container for the "cooked" command line for a domain. This provides for
> the backing memory to be directly associated with the domain being constructed.
> This is done in anticipation that the domain construction path may need to be
> invoked multiple times, thus ensuring each instance had a distinct memory
> allocation.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 533a1e2bbe05..b9ca9c486fe5 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -963,10 +963,31 @@ static unsigned int __init copy_bios_e820(struct e820entry *map, unsigned int li
> return n;
> }
>
> -static struct domain *__init create_dom0(struct boot_info *bi)
> +static size_t __init domain_cmdline_size(
> + struct boot_info *bi, struct boot_domain *bd)
> {
> - static char __initdata cmdline[MAX_GUEST_CMDLINE];
> + size_t s = 0;
> +
> + s += bi->kextra ? strlen(bi->kextra) : 0;
> + s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
>
> + /* Should only be called if one of extra or cmdline_pa are valid */
> + ASSERT(s > 0);
> +
> + /*
> + * Add additional space for the following cases:
> + * - 7 chars for " noapic"
> + * - 13 chars for longest acpi opiton, " acpi=verbose"
option
> + * - 1 char to hold \0
> + */
> + s += 7 + 13 + 1;
Seems a little fragile. Sizing but also depending on code elsewhere.
Interesting - "verbose" wouldn't actually get updated into acpi_param.
Anyway, using sizeof(acpi_param) seems better. Maybe:
s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) + 1;
> +
> + return s;
> +}
> +
> +static struct domain *__init create_dom0(struct boot_info *bi)
> +{
> + char *cmdline = NULL;
> struct xen_domctl_createdomain dom0_cfg = {
> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> .max_evtchn_port = -1,
> @@ -1008,17 +1029,23 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> /* Grab the DOM0 command line. */
> if ( bd->kernel->cmdline_pa || bi->kextra )
From your other email, since you don't need the length, just non-zero:
if ( (bd->kernel->cmdline_pa && __va(bd->kernel->cmdline_pa)[0]) ||
bi->kextra )
> {
> + size_t cmdline_size = domain_cmdline_size(bi, bd);
> +
> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
Just xmalloc_array since it'll be overwritten immediately?
> + 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 )
> {
> @@ -1028,17 +1055,21 @@ 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->cmdline = cmdline;
> + bd->kernel->cmdline_pa = __pa(bd->cmdline);
Should cmdline_pa go away if we now have a valid cmdline variable?
Regards,
Jason
> }
>
> bd->d = d;
> if ( construct_dom0(bd) != 0 )
> panic("Could not construct domain 0\n");
>
> + if ( cmdline )
> + xfree(cmdline);
> +
> return d;
> }
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 2/6] x86/boot: introduce module release
2024-11-15 17:18 ` Jason Andryuk
@ 2024-11-18 16:13 ` Andrew Cooper
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Cooper @ 2024-11-18 16:13 UTC (permalink / raw)
To: Jason Andryuk, Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Roger Pau Monné
On 15/11/2024 5:18 pm, Jason Andryuk wrote:
> On 2024-11-15 12:16, Daniel P. Smith wrote:
>> On 11/15/24 11:50, Jason Andryuk wrote:
>>> On 2024-11-15 08:12, Daniel P. Smith wrote:
>>>> /* Set up start info area. */
>>>> si = (start_info_t *)vstartinfo_start;
>>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>> index 495e90a7e132..0bda1326a485 100644
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>
>>>> +void __init free_boot_modules(void)
>>>> {
>>>> struct boot_info *bi = &xen_boot_info;
>>>> unsigned int i;
>>>> for ( i = 0; i < bi->nr_modules; ++i )
>>>> {
>>>> - uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
>>>> - uint64_t size = bi->mods[i].mod->mod_end;
>>>> -
>>>> - /*
>>>> - * Sometimes the initrd is mapped, rather than copied,
>>>> into dom0.
>>>> - * Size being 0 is how we're instructed to leave the
>>>> module alone.
>>>> - */
>>>> - if ( size == 0 )
>>>> + if ( bi->mods[i].released )
>>>> continue;
>>>> - init_domheap_pages(start, start + PAGE_ALIGN(size));
>>>> + release_boot_module(&bi->mods[i]);
>>>> }
>>>> -
>>>> - bi->nr_modules = 0;
>>>
>>> IIUC, zero-ing here was a safety feature to ensure boot modules
>>> could not be used after this point. Should it be retained?
>>
>> The released flag displaced the need for this, but I realized it
>> would make it stronger if in bootstrap_map_bm() we add a check that
>> the released flag is not set before mapping. I think this is a
>> stronger approach without loosing information like the number of boot
>> modules were passed.
>
> Andrew> Clobbering this prevents the loop constructs from working.
>
> I thought the boot modules are unusable after free_boot_modules() is
> called, so I'm not clear on the utility of keeping the boot modules
> around and/or keeping the loop constructs working. I wondered about,
> but didn't write, clearing the boot_module info in
> release_boot_module() to eliminate stale data hanging around.
Metadata about which module is which is potentially still interesting.
Either way, clobbering nr_modules was to make discard_initial_images()
idempotent, and the released flag is a better way of doing this now.
>
> Yes, a bootstrap_map_bm() check is a good idea. Having said that,
> there is a lack of checking the return value of bootstrap_map_bm(), so
> would you panic?
This is a long-standing issue.
The only way to fail is prior to the directmap being set up and the sum
of bootstrap_map_*()'s since the last unmap() exceeding ~1G.
Some callers check, others don't. Really there wants to be uniform
handling (probably a backtrace from the innermost layer), and callers
able to handle NULL.
But, e.g. with microcode handling, only being able to map the first 2M
or 4M of a 2G initrd would still be useful. Then again, if we can't map
the initrd, then later parts of boot are still going to go wrong.
Either way - this patch is an improvement. Other improvements can come
later.
~Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain
2024-11-15 18:20 ` Jason Andryuk
@ 2024-11-20 17:57 ` Daniel P. Smith
2024-11-20 22:32 ` Jason Andryuk
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Smith @ 2024-11-20 17:57 UTC (permalink / raw)
To: Jason Andryuk, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 11/15/24 13:20, Jason Andryuk wrote:
> On 2024-11-15 08:12, Daniel P. Smith wrote:
>> Add a container for the "cooked" command line for a domain. This
>> provides for
>> the backing memory to be directly associated with the domain being
>> constructed.
>> This is done in anticipation that the domain construction path may
>> need to be
>> invoked multiple times, thus ensuring each instance had a distinct memory
>> allocation.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>
>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 533a1e2bbe05..b9ca9c486fe5 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -963,10 +963,31 @@ static unsigned int __init copy_bios_e820(struct
>> e820entry *map, unsigned int li
>> return n;
>> }
>> -static struct domain *__init create_dom0(struct boot_info *bi)
>> +static size_t __init domain_cmdline_size(
>> + struct boot_info *bi, struct boot_domain *bd)
>> {
>> - static char __initdata cmdline[MAX_GUEST_CMDLINE];
>> + size_t s = 0;
>> +
>> + s += bi->kextra ? strlen(bi->kextra) : 0;
>> + s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel-
>> >cmdline_pa)) : 0;
>> + /* Should only be called if one of extra or cmdline_pa are valid */
>> + ASSERT(s > 0);
>> +
>> + /*
>> + * Add additional space for the following cases:
>> + * - 7 chars for " noapic"
>> + * - 13 chars for longest acpi opiton, " acpi=verbose"
>
> option
>
>> + * - 1 char to hold \0
>> + */
>> + s += 7 + 13 + 1;
>
> Seems a little fragile. Sizing but also depending on code elsewhere.
> Interesting - "verbose" wouldn't actually get updated into acpi_param.
> Anyway, using sizeof(acpi_param) seems better. Maybe:
>
> s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param)
> + 1;
True, the strlen() is more reasonable and self documenting. Yes, I
overlooked the return in the option parser that would not copy verbose,
plus sizeof(acpi_param) will ensure adequate spacing.
>> +
>> + return s;
>> +}
>> +
>> +static struct domain *__init create_dom0(struct boot_info *bi)
>> +{
>> + char *cmdline = NULL;
>> struct xen_domctl_createdomain dom0_cfg = {
>> .flags = IS_ENABLED(CONFIG_TBOOT) ?
>> XEN_DOMCTL_CDF_s3_integrity : 0,
>> .max_evtchn_port = -1,
>> @@ -1008,17 +1029,23 @@ static struct domain *__init
>> create_dom0(struct boot_info *bi)
>> /* Grab the DOM0 command line. */
>> if ( bd->kernel->cmdline_pa || bi->kextra )
>
> From your other email, since you don't need the length, just non-zero:
>
> if ( (bd->kernel->cmdline_pa && __va(bd->kernel->cmdline_pa)[0]) ||
> bi->kextra )
Ack.
>> {
>> + size_t cmdline_size = domain_cmdline_size(bi, bd);
>> +
>> + if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
>
> Just xmalloc_array since it'll be overwritten immediately?
Yes and no, the concern I was worried about is that the allocation may
end up being slight bigger than what is copied in to the buffer. If we
do not zero the buffer, then those trailing bytes will have random data
in them. In a perfect world, nothing should ever reach those bytes based
on the current usage of the buffer. But from my perspective, it would be
safer to zero the buffer than rely on the world being perfect. I am not
fixed on not switching, just providing my 2 cents,
>> + 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 )
>> {
>> @@ -1028,17 +1055,21 @@ 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->cmdline = cmdline;
>> + bd->kernel->cmdline_pa = __pa(bd->cmdline);
>
> Should cmdline_pa go away if we now have a valid cmdline variable?
In the PVH dom0 case, we are still relying on cmdline_pa as the
reference to get at the command line in the function pvh_load_kernel().
With the introduction of cmdline to boot_domain, I could convert the
interface of pvh_load_kernel() to take the boot_domain instance,
removing the need to update cmdline_pa. Not sure if you were asking
this, but as for cmdline_pa going completely away, that is not possible.
First the sequence of events do not allow it, and there is an one-off
case for PVH dom0 where the cmdline_pa of the initrd module is copied
into the domain.
v/r,
dps
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain
2024-11-20 17:57 ` Daniel P. Smith
@ 2024-11-20 22:32 ` Jason Andryuk
0 siblings, 0 replies; 30+ messages in thread
From: Jason Andryuk @ 2024-11-20 22:32 UTC (permalink / raw)
To: Daniel P. Smith, xen-devel
Cc: christopher.w.clark, stefano.stabellini, Jan Beulich,
Andrew Cooper, Roger Pau Monné
On 2024-11-20 12:57, Daniel P. Smith wrote:
> On 11/15/24 13:20, Jason Andryuk wrote:
>> On 2024-11-15 08:12, Daniel P. Smith wrote:>>
>> Just xmalloc_array since it'll be overwritten immediately?
>
> Yes and no, the concern I was worried about is that the allocation may
> end up being slight bigger than what is copied in to the buffer. If we
> do not zero the buffer, then those trailing bytes will have random data
> in them. In a perfect world, nothing should ever reach those bytes based
> on the current usage of the buffer. But from my perspective, it would be
> safer to zero the buffer than rely on the world being perfect. I am not
> fixed on not switching, just providing my 2 cents,
I only looked at the PVH case, but strlen() is used there to obtain the
copy length. I think it's unnecessary and the code doesn't require a
zeroed buffer. But I also realize it's safer to start from zeroed.
>>> + 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),
Also I just noticed a missing space: "cmdline_pa),bi"
>>> + 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 )
>>> {
>>> @@ -1028,17 +1055,21 @@ 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->cmdline = cmdline;
>>> + bd->kernel->cmdline_pa = __pa(bd->cmdline);
>>
>> Should cmdline_pa go away if we now have a valid cmdline variable?
>
> In the PVH dom0 case, we are still relying on cmdline_pa as the
> reference to get at the command line in the function pvh_load_kernel().
> With the introduction of cmdline to boot_domain, I could convert the
> interface of pvh_load_kernel() to take the boot_domain instance,
> removing the need to update cmdline_pa. Not sure if you were asking
> this, but as for cmdline_pa going completely away, that is not possible.
> First the sequence of events do not allow it, and there is an one-off
> case for PVH dom0 where the cmdline_pa of the initrd module is copied
> into the domain.
I was thinking from this point forward only boot_domain->cmdline is
necessary. Maybe even zero-ing cmdline_pa? With a valid pointer in
cmdline, cmdline_pa shouldn't be necessary anymore.
Regards,
Jason
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 4/6] x86/boot: introduce boot domain
2024-11-15 13:12 ` [PATCH v9 4/6] x86/boot: introduce boot domain Daniel P. Smith
@ 2024-11-27 10:22 ` Jan Beulich
2024-12-04 16:24 ` Daniel P. Smith
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2024-11-27 10:22 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 15.11.2024 14:12, Daniel P. Smith wrote:
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2024 Apertus Solutions, LLC
> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
> + */
> +
> +#ifndef __XEN_X86_BOOTDOMAIN_H__
> +#define __XEN_X86_BOOTDOMAIN_H__
> +
> +struct boot_module;
> +struct domain;
You don't really need these, do you? The uses ...
> +struct boot_domain {
> + struct boot_module *kernel;
> + struct boot_module *ramdisk;
> +
> + struct domain *d;
> +};
... here still introduce the struct tags into global scope, unlike for C++
and unlike ...
> --- a/xen/arch/x86/include/asm/dom0_build.h
> +++ b/xen/arch/x86/include/asm/dom0_build.h
> @@ -13,9 +13,9 @@ unsigned long dom0_compute_nr_pages(struct domain *d,
> unsigned long initrd_len);
> int dom0_setup_permissions(struct domain *d);
>
> -struct boot_info;
> -int dom0_construct_pv(struct boot_info *bi, struct domain *d);
> -int dom0_construct_pvh(struct boot_info *bi, struct domain *d);
> +struct boot_domain;
> +int dom0_construct_pv(struct boot_domain *bd);
> +int dom0_construct_pvh(struct boot_domain *bd);
... when used in prototypes.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain
2024-11-15 13:12 ` [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain Daniel P. Smith
2024-11-15 15:31 ` Daniel P. Smith
@ 2024-11-27 10:32 ` Jan Beulich
2024-12-04 16:45 ` Daniel P. Smith
1 sibling, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2024-11-27 10:32 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 15.11.2024 14:12, Daniel P. Smith wrote:
> Add a domid field to struct boot_domain to hold the assigned domain id for the
> domain. During initialization, ensure all instances of struct boot_domain have
> the invalid domid to ensure that the domid must be set either by convention or
> configuration.
I'm missing the "why" part here - after all ...
> --- a/xen/arch/x86/include/asm/bootdomain.h
> +++ b/xen/arch/x86/include/asm/bootdomain.h
> @@ -12,6 +12,8 @@ struct boot_module;
> struct domain;
>
> struct boot_domain {
> + domid_t domid;
> +
> struct boot_module *kernel;
> struct boot_module *ramdisk;
>
... just out of context here there is struct domain *. I can only guess that
the domain ID is needed for the time until the domain pointer was actually
filled.
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -339,6 +339,9 @@ static struct boot_info *__init multiboot_fill_boot_info(
> /* Variable 'i' should be one entry past the last module. */
> bi->mods[i].type = BOOTMOD_XEN;
>
> + for ( i = 0; i < MAX_NR_BOOTDOMS; i++ )
> + bi->domains[i].domid = DOMID_INVALID;
Generally I think ARRAY_SIZE() is better to use for loop boundaries. Yet
then - why don't you statically initialize the array in xen_boot_info?
> @@ -977,7 +980,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> };
> struct boot_domain *bd = &bi->domains[0];
> struct domain *d;
> - domid_t domid;
>
> if ( opt_dom0_pvh )
> {
> @@ -993,15 +995,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> /* Create initial domain. Not d0 for pvshim. */
> - domid = get_initial_domain_id();
> - d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
> + bd->domid = get_initial_domain_id();
> + d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
> if ( IS_ERR(d) )
> - panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
> + panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
As to the comment at the top - this change alone certainly doesn't clarify
the "why".
> init_dom0_cpuid_policy(d);
>
> if ( alloc_dom0_vcpu0(d) == NULL )
> - panic("Error creating d%uv0\n", domid);
> + panic("Error creating d%uv0\n", bd->domid);
Imo this would better use d->domain_id. And while touching it, %u would also
want swapping for %d.
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 4/6] x86/boot: introduce boot domain
2024-11-27 10:22 ` Jan Beulich
@ 2024-12-04 16:24 ` Daniel P. Smith
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-12-04 16:24 UTC (permalink / raw)
To: Jan Beulich
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 11/27/24 05:22, Jan Beulich wrote:
> On 15.11.2024 14:12, Daniel P. Smith wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (c) 2024 Apertus Solutions, LLC
>> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com>
>> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com>
>> + */
>> +
>> +#ifndef __XEN_X86_BOOTDOMAIN_H__
>> +#define __XEN_X86_BOOTDOMAIN_H__
>> +
>> +struct boot_module;
>> +struct domain;
>
> You don't really need these, do you? The uses ...
Your are correct, they are not necessary for C to compile. I can drop them.
v/r,
dps
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain
2024-11-27 10:32 ` Jan Beulich
@ 2024-12-04 16:45 ` Daniel P. Smith
2024-12-09 8:55 ` Jan Beulich
0 siblings, 1 reply; 30+ messages in thread
From: Daniel P. Smith @ 2024-12-04 16:45 UTC (permalink / raw)
To: Jan Beulich
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 11/27/24 05:32, Jan Beulich wrote:
> On 15.11.2024 14:12, Daniel P. Smith wrote:
>> Add a domid field to struct boot_domain to hold the assigned domain id for the
>> domain. During initialization, ensure all instances of struct boot_domain have
>> the invalid domid to ensure that the domid must be set either by convention or
>> configuration.
>
> I'm missing the "why" part here - after all ...
Which is part of why I rolled these over into the dom0 device tree
series, as it will provide more context to its purpose. This field is
used to store the value parsed in from the device tree. In dom0 device
tree series, this commit could be merged with the commit that introduces
the device tree parsing for domid to provide better context for the
introduction of the structure element.
>> --- a/xen/arch/x86/include/asm/bootdomain.h
>> +++ b/xen/arch/x86/include/asm/bootdomain.h
>> @@ -12,6 +12,8 @@ struct boot_module;
>> struct domain;
>>
>> struct boot_domain {
>> + domid_t domid;
>> +
>> struct boot_module *kernel;
>> struct boot_module *ramdisk;
>>
>
> ... just out of context here there is struct domain *. I can only guess that
> the domain ID is needed for the time until the domain pointer was actually
> filled.
Correct, thus why it makes more sense to merge with the domid device
tree parsing.
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -339,6 +339,9 @@ static struct boot_info *__init multiboot_fill_boot_info(
>> /* Variable 'i' should be one entry past the last module. */
>> bi->mods[i].type = BOOTMOD_XEN;
>>
>> + for ( i = 0; i < MAX_NR_BOOTDOMS; i++ )
>> + bi->domains[i].domid = DOMID_INVALID;
>
> Generally I think ARRAY_SIZE() is better to use for loop boundaries. Yet
> then - why don't you statically initialize the array in xen_boot_info?
I indifferent with ARRAY_SIZE(), I will try and keep that in mind for
future parts of the series. As for static init'ing, good point since we
are already doing it with other fields, I can add this to it.
>> @@ -977,7 +980,6 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>> };
>> struct boot_domain *bd = &bi->domains[0];
>> struct domain *d;
>> - domid_t domid;
>>
>> if ( opt_dom0_pvh )
>> {
>> @@ -993,15 +995,15 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>>
>> /* Create initial domain. Not d0 for pvshim. */
>> - domid = get_initial_domain_id();
>> - d = domain_create(domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>> + bd->domid = get_initial_domain_id();
>> + d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged);
>> if ( IS_ERR(d) )
>> - panic("Error creating d%u: %ld\n", domid, PTR_ERR(d));
>> + panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d));
>
> As to the comment at the top - this change alone certainly doesn't clarify
> the "why".
Agreed.
>> init_dom0_cpuid_policy(d);
>>
>> if ( alloc_dom0_vcpu0(d) == NULL )
>> - panic("Error creating d%uv0\n", domid);
>> + panic("Error creating d%uv0\n", bd->domid);
>
> Imo this would better use d->domain_id. And while touching it, %u would also
> want swapping for %d.
hmm, I was actually considering s/d%u/%pd/ and just pass in d, but was
certain if there was an explicit reason it wasn't used before. If I am
going to change it, would %pd not be more desired here?
v/r,
dps
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain
2024-12-04 16:45 ` Daniel P. Smith
@ 2024-12-09 8:55 ` Jan Beulich
2024-12-11 0:57 ` Daniel P. Smith
0 siblings, 1 reply; 30+ messages in thread
From: Jan Beulich @ 2024-12-09 8:55 UTC (permalink / raw)
To: Daniel P. Smith
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 04.12.2024 17:45, Daniel P. Smith wrote:
> On 11/27/24 05:32, Jan Beulich wrote:
>> On 15.11.2024 14:12, Daniel P. Smith wrote:
>>> init_dom0_cpuid_policy(d);
>>>
>>> if ( alloc_dom0_vcpu0(d) == NULL )
>>> - panic("Error creating d%uv0\n", domid);
>>> + panic("Error creating d%uv0\n", bd->domid);
>>
>> Imo this would better use d->domain_id. And while touching it, %u would also
>> want swapping for %d.
>
> hmm, I was actually considering s/d%u/%pd/ and just pass in d, but was
> certain if there was an explicit reason it wasn't used before. If I am
> going to change it, would %pd not be more desired here?
When writing my original reply, I certainly considered this. The anomaly
here is that you really mean to log a vCPU ID, which would require a
struct vcpu * and use of %pv. Yet you don't have that here, precisely
because the creation of the vCPU failed. That said, since
vsprintf.c:print_vcpu() calls print_domain(), using %pd is certainly an
option here (inconsistencies would arise if %pv and %pd presented domain
IDs in [perhaps just slightly] different ways).
Jan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain
2024-12-09 8:55 ` Jan Beulich
@ 2024-12-11 0:57 ` Daniel P. Smith
0 siblings, 0 replies; 30+ messages in thread
From: Daniel P. Smith @ 2024-12-11 0:57 UTC (permalink / raw)
To: Jan Beulich
Cc: jason.andryuk, christopher.w.clark, stefano.stabellini,
Andrew Cooper, Roger Pau Monné, xen-devel
On 12/9/24 03:55, Jan Beulich wrote:
> On 04.12.2024 17:45, Daniel P. Smith wrote:
>> On 11/27/24 05:32, Jan Beulich wrote:
>>> On 15.11.2024 14:12, Daniel P. Smith wrote:
>>>> init_dom0_cpuid_policy(d);
>>>>
>>>> if ( alloc_dom0_vcpu0(d) == NULL )
>>>> - panic("Error creating d%uv0\n", domid);
>>>> + panic("Error creating d%uv0\n", bd->domid);
>>>
>>> Imo this would better use d->domain_id. And while touching it, %u would also
>>> want swapping for %d.
>>
>> hmm, I was actually considering s/d%u/%pd/ and just pass in d, but was
>> certain if there was an explicit reason it wasn't used before. If I am
>> going to change it, would %pd not be more desired here?
>
> When writing my original reply, I certainly considered this. The anomaly
> here is that you really mean to log a vCPU ID, which would require a
> struct vcpu * and use of %pv. Yet you don't have that here, precisely
> because the creation of the vCPU failed. That said, since
> vsprintf.c:print_vcpu() calls print_domain(), using %pd is certainly an
> option here (inconsistencies would arise if %pv and %pd presented domain
> IDs in [perhaps just slightly] different ways).
Will do, thanks!
v/r,
dps
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2024-12-11 0:58 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-15 13:11 [PATCH v9 0/6] Boot modules for Hyperlaunch Daniel P. Smith
2024-11-15 13:11 ` [PATCH v9 1/6] x86/boot: convert domain construction to use boot info Daniel P. Smith
2024-11-15 14:25 ` Andrew Cooper
2024-11-15 14:32 ` Daniel P. Smith
2024-11-15 16:33 ` Jason Andryuk
2024-11-15 17:01 ` Andrew Cooper
2024-11-15 17:02 ` Jason Andryuk
2024-11-15 13:12 ` [PATCH v9 2/6] x86/boot: introduce module release Daniel P. Smith
2024-11-15 16:50 ` Jason Andryuk
2024-11-15 17:09 ` Andrew Cooper
2024-11-15 17:16 ` Daniel P. Smith
2024-11-15 17:18 ` Jason Andryuk
2024-11-18 16:13 ` Andrew Cooper
2024-11-15 13:12 ` [PATCH v9 3/6] x86/boot: add start and size fields to struct boot_module Daniel P. Smith
2024-11-15 17:31 ` Jason Andryuk
2024-11-15 13:12 ` [PATCH v9 4/6] x86/boot: introduce boot domain Daniel P. Smith
2024-11-27 10:22 ` Jan Beulich
2024-12-04 16:24 ` Daniel P. Smith
2024-11-15 13:12 ` [PATCH v9 5/6] x86/boot: introduce domid field to struct boot_domain Daniel P. Smith
2024-11-15 15:31 ` Daniel P. Smith
2024-11-27 10:32 ` Jan Beulich
2024-12-04 16:45 ` Daniel P. Smith
2024-12-09 8:55 ` Jan Beulich
2024-12-11 0:57 ` Daniel P. Smith
2024-11-15 13:12 ` [PATCH v9 6/6] x86/boot: add cmdline " Daniel P. Smith
2024-11-15 15:12 ` Daniel P. Smith
2024-11-15 16:34 ` Daniel P. Smith
2024-11-15 18:20 ` Jason Andryuk
2024-11-20 17:57 ` Daniel P. Smith
2024-11-20 22:32 ` Jason Andryuk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.