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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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?
> 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.
>       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.
Regards,
Jason

> +        strlcpy((char *)si->cmd_line, bd->cmdline, sizeof(si->cmd_line));
>   
>   #ifdef CONFIG_VIDEO
>       if ( !pv_shim && fill_console_start_info((void *)(si + 1