Re: [RFC PATCH v2 07/16] hvmloader: Grab the hvmlite info page and parse the cmdline

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/10/15 16:03, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/firmware/hvmloader/hvmloader.c | 69 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 68 insertions(+), 1 deletion(-)

As part of using the DMLite infrastructure, hvmloader should gain
appropriate elfnotes.  This will avoid the need to duplicate the dmlite
codepath in libxc just for hvmloader.

>
> diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
> index 716d03c..9df12ac 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -62,7 +62,7 @@ asm (
>      "    mov  %ax,%ss                \n"

mov %ebx, start_info

and have

static const struct hvm_start_info *hvmlite_start_info;

somewhere other than the main() stack.  However, I would avoid naming it
"hvmlite".

>      /* Initialise all 32-bit GPRs to zero. */
>      "    xor  %eax,%eax              \n"
> -    "    xor  %ebx,%ebx              \n"
> +    /* Keep ebx, for HVMLite start info */
>      "    xor  %ecx,%ecx              \n"
>      "    xor  %edx,%edx              \n"
>      "    xor  %esp,%esp              \n"
> @@ -249,15 +249,82 @@ static void acpi_enable_sci(void)
>      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>  }
>  
> +static const char *module_list_order = NULL;
> +void cmdline_parser(const char *the_cmdline)
> +{
> +    char cmdline[MAX_GUEST_CMDLINE];
> +    char *p, *q;
> +    char *optval;
> +
> +    strncpy(cmdline, the_cmdline, sizeof (cmdline));
> +    cmdline[MAX_GUEST_CMDLINE-1] = '\0';

Why do you need to copy the command line?  It is already in writable
hvmloader memory.

You also need to check for NULL, being the indication that no command
line has been provided.

~Andrew

> +
> +    for ( p = cmdline; p < (cmdline + MAX_GUEST_CMDLINE) && *p; p++ )
> +    {
> +        if ( *p == ' ' )
> +            continue;
> +
> +        /* search for the end of the parameter name */
> +        for ( q = p; *q && *q != '=' && *q != ' '; q++ )
> +            ;
> +
> +        /* search for the end of the optional paremeter value */
> +        if ( *q == '=' )
> +        {
> +            optval = q+1;
> +            if (*optval == '\0' || *optval == ' ') {
> +                optval = NULL;
> +            }
> +        } else
> +            optval = NULL;
> +        *q = '\0';
> +
> +        if ( optval )
> +        {
> +            for ( q = optval; *q && *q != ' '; q++ )
> +                ;
> +            *q = '\0';
> +        }
> +
> +        /* compare known parameters */
> +        if ( !strcmp(p, "modules") )
> +        {
> +            printf("  cmdline: found '%s', with val '%s'\n", p, optval);
> +            if ( optval )
> +            {
> +                unsigned size = strlen(optval) + 1;
> +                char *tmp = scratch_alloc(size, 0);
> +                strncpy(tmp, optval, size);
> +                module_list_order = tmp;
> +            }
> +        } else {
> +            printf("  Unknown cmdline option '%s'", p);
> +            if ( optval )
> +                printf(" with val '%s'\n", optval);
> +            else
> +                printf("\n");
> +        }
> +
> +        p = q;
> +    }
> +}
> +
>  int main(void)
>  {
>      const struct bios_config *bios;
>      int acpi_enabled;
> +    const struct hvm_start_info *hvmlite_start_info;
> +
> +    /* Load hvmlite start info pointer from ebx. */
> +    asm volatile ( "mov %%ebx,%0" : "=r" (hvmlite_start_info) );
>  
>      /* Initialise hypercall stubs with RET, rendering them no-ops. */
>      memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
>  
>      printf("HVM Loader\n");
> +    BUG_ON(hvmlite_start_info->magic != HVM_START_MAGIC_VALUE);
> +    printf("cmdline: %s\n", (char*)hvmlite_start_info->cmdline_paddr);
> +    cmdline_parser((char*)hvmlite_start_info->cmdline_paddr);
>  
>      init_hypercalls();
>