Re: [PATCH v3 03/16] x86/boot: add cmdline to struct boot_domain

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed Apr 9, 2025 at 10:05 PM BST, Denis Mukhin wrote:
> On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@xxxxxxx> 
> wrote:
>
>> 
>> 
>> From: "Daniel P. Smith" dpsmith@xxxxxxxxxxxxxxxxxxxx
>> 
>> 
>> 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@xxxxxxxxxxxxxxxxxxxx
>> 
>> Signed-off-by: Jason Andryuk jason.andryuk@xxxxxxx
>> 
>> Signed-off-by: Alejandro Vallejo agarciav@xxxxxxx
>> 
>> ---
>> No changes on ACPI cmdline handling on PVH, as it's orthogonal to the
>> purpose of this patch.
>> 
>> v3:
>> * s/xfree/XFREE/ on failed construct_dom0() to avoid a dangling
>> cmdline ptr.
>> * Re-flow hvm_copy_to_guest_phys() into a multi-line call.
>> * s/bd->cmdline != NULL/b->cmdline/ (to homogenise with the previous
>> 
>> cmdline pointer check)
>> ---
>> xen/arch/x86/hvm/dom0_build.c | 12 +++----
>> xen/arch/x86/include/asm/boot-domain.h | 1 +
>> xen/arch/x86/pv/dom0_build.c | 4 +--
>> xen/arch/x86/setup.c | 50 +++++++++++++++++++-------
>> 4 files changed, 47 insertions(+), 20 deletions(-)
>> 
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
>> index 2a094b3145..ebad5a49b8 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -653,7 +653,6 @@ static int __init pvh_load_kernel(
>> void *image_start = image_base + image->headroom;
>> 
>> unsigned long image_len = image->size;
>> 
>> unsigned long initrd_len = initrd ? initrd->size : 0;
>> 
>> - const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL;
>> 
>> const char *initrd_cmdline = NULL;
>> struct elf_binary elf;
>> struct elf_dom_parms parms;
>> @@ -736,8 +735,8 @@ static int __init pvh_load_kernel(
>> initrd = NULL;
>> }
>> 
>> - if ( cmdline )
>> - extra_space += elf_round_up(&elf, strlen(cmdline) + 1);
>> + if ( bd->cmdline )
>> 
>> + extra_space += elf_round_up(&elf, strlen(bd->cmdline) + 1);
>
> I think it makes sense to use local variable to cache the results of 
>
>   strlen(bd->cmdline) + 1
>
> since the construct is used multiple times within pvh_load_kernel()
>
> E.g. cmdline_len similarly to {image,initrd}_len.
>

Seeing how you're the second person to propose the same thing, I'll bite
and just do it. I really dislike the wall of locals in
pvh_load_kernel(), but oh well...

Cheers,
Alejandro