Hi Daniel,
On 19/07/2022 17:36, Daniel P. Smith wrote:
On 7/15/22 15:16, Julien Grall wrote:
Hi Daniel,
On 06/07/2022 22:04, Daniel P. Smith wrote:
For x86 the number of allowable multiboot modules varies between the
different
entry points, non-efi boot, pvh boot, and efi boot. In the case of
both Arm and
x86 this value is fixed to values based on generalized assumptions. With
hyperlaunch for x86 and dom0less on Arm, use of static sizes results
in large
allocations compiled into the hypervisor that will go unused by many
use cases.
This commit introduces a Kconfig variable that is set with sane
defaults based
on configuration selection. This variable is in turned used as the
array size
for the cases where a static allocated array of boot modules is declared.
Signed-off-by: Daniel P. Smith <dpsm...@apertussolutions.com>
Reviewed-by: Christopher Clark <christopher.cl...@starlab.io>
I am not entirely sure where this reviewed-by is coming from. Is this
from internal review?
Yes.
If yes, my recommendation would be to provide the reviewed-by on the
mailing list. Ideally, the review should also be done in the open, but I
understand some company wish to do a fully internal review first.
Since this capability is being jointly developed by Christopher and I,
with myself being the author of code, Christopher reviewed the code as
the co-developer. He did so as a second pair of eyes for any obvious
mistakes and to concur that the implementation was in line with the
approach the two of us architected. Perhaps a SoB line might be more
appropriate than an R-b line.
At least from a committer perspective, this helps me to know whether the
reviewed-by still apply. An example would be if you send a v2, I would
not be able to know whether Christoffer still agreed on the change.
If an SoB line is more appropriate, then on the next version I can
switch it
Thanks for the explanation. To me "signed-off-by" means the person wrote
some code (or sent the patches) code. So from above, it sounds more like
Christoffer did a review.
So I think it is more suitable for him to provide a reviewed-by. For
follow-up, my preference would be Christoffer to provide the reviewed-by
on the ML.
If it is too much overhead, I would suggest to log the latest version
Christoffer reviewed-by in the changelog. I usually do:
Changes in vX:
- Add Christoffer's reviewed-by
Or if he will reviewing every version, just mention it in the cover letter.
Please explain in the commit message why the number of modules was
bumped from 5 to 9.
The number of modules were inconsistent between the different entry
points into __start_xen(). By switching to a Kconfig variable, whose
default was set to the largest value used across the entry points,
results in change for the locations using another value.
Ok. Can you add something like: "For x86, the number of modules is not
consistent across the code base. Use the maximum"?
See below for +1 explanation.
static void __init edd_put_string(u8 *dst, size_t n, const char *src)
{
diff --git a/xen/arch/x86/guest/xen/pvh-boot.c
b/xen/arch/x86/guest/xen/pvh-boot.c
index 498625eae0..834b1ad16b 100644
--- a/xen/arch/x86/guest/xen/pvh-boot.c
+++ b/xen/arch/x86/guest/xen/pvh-boot.c
@@ -32,7 +32,7 @@ bool __initdata pvh_boot;
uint32_t __initdata pvh_start_info_pa;
static multiboot_info_t __initdata pvh_mbi;
-static module_t __initdata pvh_mbi_mods[8];
+static module_t __initdata pvh_mbi_mods[CONFIG_NR_BOOTMOD + 1];
What's the +1 for?
I should clarify in the commit message, but the value set in
CONFIG_NR_BOOTMOD is the max modules that Xen would accept from a
bootloader. Xen startup code expects to be able to append Xen itself as
the array. The +1 allocates an additional entry to store Xen in the
array should a bootloader actually pass CONFIG_NR_BOOTMOD modules to
Xen. There is an existing comment floating in one of these locations
that explained it.
This makes sense. So every use of CONFIG_NR_BOOTMOD would end up to
require +1. Is that correct?
If yes, then I think it would be better to require CONFIG_NR_BOOTMOD to
be at minimum 1. This would reduce the risk to have different array size
again. That said, this is x86 code, so the call is for the x86 maintainers.
static const char *__initdata pvh_loader = "PVH Directboot";
static void __init convert_pvh_info(multiboot_info_t **mbi,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f08b07b8de..2aa1e28c8f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1020,9 +1020,9 @@ void __init noreturn __start_xen(unsigned long
mbi_p)
panic("dom0 kernel not specified. Check bootloader
configuration\n");
/* Check that we don't have a silly number of modules. */
- if ( mbi->mods_count > sizeof(module_map) * 8 )
+ if ( mbi->mods_count > CONFIG_NR_BOOTMODS )
{
- mbi->mods_count = sizeof(module_map) * 8;
+ mbi->mods_count = CONFIG_NR_BOOTMODS;
printk("Excessive multiboot modules - using the first %u
only\n",
mbi->mods_count);
}
AFAIU, this check is to make sure that we will not overrun module_map in
the next line:
bitmap_fill(module_map, mbi->mods_count);
The current definition of module_map will allow 64 modules. But you are
allowing 32768. So I think you either want to keep the check or define
module_map as:
DECLARE_BITMAP(module_map, CONFIG_NR_BOOTMODS);
Yes, in the RFC I had it capped to 64 and lost track of this related
changed when it was bumped to 32768 per the review discussion. Later in
the series, module_map goes away. To ensure stability at this point I
would be inclined to restore the 64 module clamp down check. Thoughts?
I don't know what would a sensible value for x86. I will leave this
question to the x86 maintainers.
Cheers,
--
Julien Grall
