Re: [PATCH v2 1/5] x86/setup: Clean up cmdline handling in create_dom0()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/11/2023 4:20 pm, Andrew Cooper wrote:
> On 22/11/2023 9:02 am, Jan Beulich wrote:
>> On 21.11.2023 21:15, Andrew Cooper wrote:
>>> @@ -913,33 +914,30 @@ static struct domain *__init create_dom0(const 
>>> module_t *image,
>>>          panic("Error creating d%uv0\n", domid);
>>>  
>>>      /* Grab the DOM0 command line. */
>>> -    cmdline = image->string ? __va(image->string) : NULL;
>>> -    if ( cmdline || kextra )
>>> +    if ( image->string || kextra )
>>>      {
>>> -        static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
>>> -
>>> -        cmdline = cmdline_cook(cmdline, loader);
>>> -        safe_strcpy(dom0_cmdline, cmdline);
>>> +        if ( image->string )
>>> +            safe_strcpy(cmdline, cmdline_cook(__va(image->string), 
>>> loader));
>>>  
>>>          if ( kextra )
>>>              /* kextra always includes exactly one leading space. */
>>> -            safe_strcat(dom0_cmdline, kextra);
>>> +            safe_strcat(cmdline, kextra);
>>>  
>>>          /* Append any extra parameters. */
>>> -        if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
>>> -            safe_strcat(dom0_cmdline, " noapic");
>>> +        if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
>>> +            safe_strcat(cmdline, " noapic");
>>> +
>>>          if ( (strlen(acpi_param) == 0) && acpi_disabled )
>>>          {
>>>              printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
>>>              safe_strcpy(acpi_param, "off");
>>>          }
>>> -        if ( (strlen(acpi_param) != 0) && !strstr(dom0_cmdline, "acpi=") )
>>> +
>>> +        if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )
>> As you're touching this anyway, how about replacing the strlen() by just
>> *acpi_param? We don't really care exactly how long the string is. (As an
>> aside, strstr() uses like the one here are of course also pretty fragile.
>> But of course that's nothing to care about in this change.)
> There's the same pattern just above it, not touched, and this patch is
> already getting complicated.
>
> I think there's other cleanup to do here, so I'll defer it to later.

It turns out that the optimiser already makes this transformation.

I shan't admit to how long I've just spent thinking I had a problem with
my build...

~Andrew