On Wed Apr 9, 2025 at 7:48 AM BST, Jan Beulich wrote:
> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>> From: "Daniel P. Smith" <dpsm...@apertussolutions.com>
>>
>> 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 <dpsm...@apertussolutions.com>
>> Signed-off-by: Jason Andryuk <jason.andr...@amd.com>
>> Signed-off-by: Alejandro Vallejo <agarc...@amd.com>
>> ---
>> 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);
>>
>> last_addr = find_memory(d, &elf, extra_space);
>> if ( last_addr == INVALID_PADDR )
>> @@ -778,9 +777,10 @@ static int __init pvh_load_kernel(
>> /* Free temporary buffers. */
>> free_boot_modules();
>>
>> - if ( cmdline != NULL )
>> + if ( bd->cmdline )
>> {
>> - rc = hvm_copy_to_guest_phys(last_addr, cmdline, strlen(cmdline) +
>> 1, v);
>> + rc = hvm_copy_to_guest_phys(last_addr, bd->cmdline,
>> + strlen(bd->cmdline) + 1, v);
>> if ( rc )
>> {
>> printk("Unable to copy guest command line\n");
>> @@ -791,7 +791,7 @@ static int __init pvh_load_kernel(
>> * Round up to 32/64 bits (depending on the guest kernel bitness) so
>> * the modlist/start_info is aligned.
>> */
>> - last_addr += elf_round_up(&elf, strlen(cmdline) + 1);
>> + last_addr += elf_round_up(&elf, strlen(bd->cmdline) + 1);
>> }
>> if ( initrd != NULL )
>> {
>
> Perhaps better introduce a local variable cmdline_len? That would allow the
> first
> if() to go away (but of course not its body).
I'd agree if the function body was smaller, but it has 16 locals
already. It's already quite hard to know what's going on, so I'd rather
not make the situation worse.
>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -978,10 +978,30 @@ 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)
>
> const for both? And perhaps s/domain/dom0/ in the function name?
>
>> {
>> - static char __initdata cmdline[MAX_GUEST_CMDLINE];
>> + size_t s = bi->kextra ? strlen(bi->kextra) : 0;
>> +
>> + s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel->cmdline_pa)) : 0;
>>
>> + if ( s == 0 )
>> + return s;
>
> While this retains prior behavior, that prior behavior was certainly odd (and
> pretty likely not meant to be like that).
What part of it? How would you propose it to behave? Do you mean that if
no cmdline is passed some ought to be allocated in case we want to
override it?
Either way, such a functional change is better suited for a different
patch that does just that, plus properly handling the acpi adjustments
for PVH dom0.
>
>> @@ -1043,17 +1067,19 @@ 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->kernel->cmdline_pa = 0;
>> + bd->cmdline = cmdline;
>> }
>>
>> bd->d = d;
>> if ( construct_dom0(bd) != 0 )
>> panic("Could not construct domain 0\n");
>>
>> + XFREE(cmdline);
>
> While this tidies the local variable, what about bd->cmdline? As it stands
> this
> gives the impression that you're freeing a pointer here which may still be
> used
> through passing bd elsewhere.
That ought to have been bd->cmdline indeed.
>
> Jan
Cheers,
Alejandro
