Re: [PATCH v9 6/6] x86/boot: add cmdline to struct boot_domain

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 11/15/24 13:20, Jason Andryuk wrote:
> On 2024-11-15 08:12, Daniel P. Smith wrote:
> > 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>
>
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 533a1e2bbe05..b9ca9c486fe5 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -963,10 +963,31 @@ 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)
> >   {
> > -    static char __initdata cmdline[MAX_GUEST_CMDLINE];
> > +    size_t s = 0;
> > +
> > +    s += bi->kextra ? strlen(bi->kextra) : 0;
> > +    s += bd->kernel->cmdline_pa ? strlen(__va(bd->kernel- 
> > >cmdline_pa)) : 0;
> > +    /* Should only be called if one of extra or cmdline_pa are valid */
> > +    ASSERT(s > 0);
> > +
> > +    /*
> > +     * Add additional space for the following cases:
> > +     *  - 7 chars for " noapic"
> > +     *  - 13 chars for longest acpi opiton, " acpi=verbose"
> option
>
> > +     *  - 1 char to hold \0
> > +     */
> > +    s += 7 + 13 + 1;
> Seems a little fragile.  Sizing but also depending on code elsewhere. 
> Interesting - "verbose" wouldn't actually get updated into acpi_param. 
> Anyway, using sizeof(acpi_param) seems better.  Maybe:
>          s += strlen(" noapic") + strlen(" acpi=") + sizeof(acpi_param) 
> + 1;
True, the strlen() is more reasonable and self documenting. Yes, I 
overlooked the return in the option parser that would not copy verbose, 
plus sizeof(acpi_param) will ensure adequate spacing.
> > +
> > +    return s;
> > +}
> > +
> > +static struct domain *__init create_dom0(struct boot_info *bi)
> > +{
> > +    char *cmdline = NULL;
> >       struct xen_domctl_createdomain dom0_cfg = {
> >           .flags = IS_ENABLED(CONFIG_TBOOT) ? 
> > XEN_DOMCTL_CDF_s3_integrity : 0,
> >           .max_evtchn_port = -1,
> > @@ -1008,17 +1029,23 @@ static struct domain *__init 
> > create_dom0(struct boot_info *bi)
> >       /* Grab the DOM0 command line. */
> >       if ( bd->kernel->cmdline_pa || bi->kextra )
>  From your other email, since you don't need the length, just non-zero:
>
>      if ( (bd->kernel->cmdline_pa && __va(bd->kernel->cmdline_pa)[0]) ||
>            bi->kextra )
Ack.

> >       {
> > +        size_t cmdline_size = domain_cmdline_size(bi, bd);
> > +
> > +        if ( !(cmdline = xzalloc_array(char, cmdline_size)) )
> Just xmalloc_array since it'll be overwritten immediately?
Yes and no, the concern I was worried about is that the allocation may 
end up being slight bigger than what is copied in to the buffer. If we 
do not zero the buffer, then those trailing bytes will have random data 
in them. In a perfect world, nothing should ever reach those bytes based 
on the current usage of the buffer. But from my perspective, it would be 
safer to zero the buffer than rely on the world being perfect. I am not 
fixed on not switching, just providing my 2 cents,
> > +            panic("Error allocating cmdline buffer for %pd\n", d);
> > +
> >           if ( bd->kernel->cmdline_pa )
> > -            safe_strcpy(cmdline,
> > -                        cmdline_cook(__va(bd->kernel->cmdline_pa), 
> > bi->loader));
> > +            strlcpy(cmdline,
> > +                    cmdline_cook(__va(bd->kernel->cmdline_pa),bi- 
> > >loader),
> > +                    cmdline_size);
> >           if ( bi->kextra )
> >               /* kextra always includes exactly one leading space. */
> > -            safe_strcat(cmdline, bi->kextra);
> > +            strlcat(cmdline, bi->kextra, cmdline_size);
> >           /* Append any extra parameters. */
> >           if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> > -            safe_strcat(cmdline, " noapic");
> > +            strlcat(cmdline, " noapic", cmdline_size);
> >           if ( (strlen(acpi_param) == 0) && acpi_disabled )
> >           {
> > @@ -1028,17 +1055,21 @@ 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->cmdline = cmdline;
> > +        bd->kernel->cmdline_pa = __pa(bd->cmdline);
> Should cmdline_pa go away if we now have a valid cmdline variable?
In the PVH dom0 case, we are still relying on cmdline_pa as the 
reference to get at the command line in the function pvh_load_kernel(). 
With the introduction of cmdline to boot_domain, I could convert the 
interface of pvh_load_kernel() to take the boot_domain instance, 
removing the need to update cmdline_pa. Not sure if you were asking 
this, but as for cmdline_pa going completely away, that is not possible. 
First the sequence of events do not allow it, and there is an one-off 
case for PVH dom0 where the cmdline_pa of the initrd module is copied 
into the domain.
v/r,
dps