On 9/3/24 18:35, Andrew Cooper wrote:
On 30/08/2024 10:46 pm, Daniel P. Smith wrote:
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index eee20bb1753c..dd94ee2e736b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1034,9 +1044,10 @@ void asmlinkage __init noreturn __start_xen(unsigned
long mbi_p)
mod = __va(mbi->mods_addr);
}
+ multiboot_to_bootinfo(mbi);
Actually, peeking ahead to the end of the series, we've got this:
void __start_xen(unsigned long mbi_p)
{
...
multiboot_info_t *mbi;
module_t *mod;
...
if ( pvh_boot )
{
ASSERT(mbi_p == 0);
pvh_init(&mbi, &mod);
}
else
{
mbi = __va(mbi_p);
mod = __va(mbi->mods_addr);
}
multiboot_to_bootinfo(mbi, mod);
which are the sum total of the mbi and mod pointers. Worse, pvh_init()
is transforming the PVH into into MB1 info, just to be transformed
immediately to BI.
I expect this is work for the end of the series (I can't think of a nice
way to disentangle it earlier), but could we end up with something more
like:
if ( pvh_boot )
{
ASSERT(mbi_p == 0);
pvh_fill_boot_info();
}
else
{
multiboot_info_t *mbi = __va(mbi_p);
multiboot_fill_boot_info(mbi, __va(mbi->mods_addr));
}
?
Or perhaps even just pass mbi_p in, and have multiboot_fill_boot_info()
do the __va()'s itself.
If so, we probably want to make a naming and possibly prototype
difference in this patch.
Let me combine this with some of Alejandro's comments. What if I were to
reshape it to look like this,
/*
* This level of indirection may not be desired, dropping it is not an
* issue. I am proposing it because there is going to be a need to
* access the instance from distant unit files.
*/
struct boot_info __init *get_boot_info(void)
{
static struct boot_info __initdata info;
return &info;
}
static struct boot_info __init *multiboot_fill_boot_info(
unsigned long mbi_p)
{
struct boot_info *bi = get_boot_info();
multiboot_info_t *mbi = __va(mbi_p);
module_t mods = __va(mbi->mods_addr);
...
return bi;
}
, then in the PVH boot code
struct boot_info __init *pvh_fill_boot_info(void)
{
struct boot_info *bi = get_boot_info();
...
return bi;
}
, and then something similar for efi.
What does everyone think?
v/r,
dps
