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

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

On 11/7/24 16:12, Jason Andryuk wrote:
> On 2024-11-02 13:25, 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.
> Is the area only used at domain construction time?  If that runs 
> sequentially, then only a single cmdline buffer would be needed. 
> cmdline_pa can keep pointing to individual cmdlines.  Unless the multi- 
> domain builder needs to keep multiple?  But that could maybe keep 
> cmdline_pa pointing into the device tree?
It turns out that 1024 may not be large enough for all use cases. 
Instead of trying to make an educated guess, this is being switched to 
dynamic allocation for v9.
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > Changes since v7:
> > - updated commit message to expand on intent and purpose
> > ---
> >   xen/arch/x86/include/asm/bootdomain.h |  4 ++++
> >   xen/arch/x86/include/asm/dom0_build.h |  1 +
> >   xen/arch/x86/pv/dom0_build.c          |  4 ++--
> >   xen/arch/x86/setup.c                  | 18 ++++++++----------
> >   4 files changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/xen/arch/x86/include/asm/bootdomain.h b/xen/arch/x86/ 
> > include/asm/bootdomain.h
> > index 3873f916f854..bc51f04a1df6 100644
> > --- a/xen/arch/x86/include/asm/bootdomain.h
> > +++ b/xen/arch/x86/include/asm/bootdomain.h
> > @@ -8,10 +8,14 @@
> >   #ifndef __XEN_X86_BOOTDOMAIN_H__
> >   #define __XEN_X86_BOOTDOMAIN_H__
> > +#include <public/xen.h>
> > +
> >   struct boot_module;
> >   struct domain;
> >   struct boot_domain {
> > +    char cmdline[MAX_GUEST_CMDLINE];
> > +
> You might want to put the 1024 byte chars at the end of the structure. 
> Having the other pointers at the start is probably more useful.
OBE.

> >       domid_t domid;
> >       struct boot_module *kernel;
> > diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> > index 28257bf13127..2c84af52de3e 100644
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -960,8 +960,8 @@ static int __init dom0_construct(struct 
> > boot_domain *bd)
> >       }
> >       memset(si->cmd_line, 0, sizeof(si->cmd_line));
> > -    if ( cmdline != NULL )
> > -        strlcpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line));
> > +    if ( cmdline[0] != '\0' )
> Shouldn't this check bd->cmdline[0] ?  There is a const char *cmdline in 
> this function that should probably be removed.
This was caught in the switch to dynamic allocation and the local var 
was dropped.
v/r,
dps