Re: [Xen-devel] [PATCH v3 14/16] x86/boot: implement early command line parser in C

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds
> @@ -25,6 +25,7 @@ SECTIONS
>          *(.text)
>          *(.text.*)
>          *(.rodata)
> +        *(.rodata.*)
>    }

Interesting - didn't you say you don't want this for now?

> --- /dev/null
> +++ b/xen/arch/x86/boot/cmdline.c
> @@ -0,0 +1,357 @@
> +/*
> + * Copyright (c) 2015, 2016 Oracle and/or its affiliates. All rights 
> reserved.
> + *      Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * strlen(), strncmp(), strspn() and strcspn() were copied from
> + * Linux kernel source (linux/lib/string.c).

Any reason you can't just #include ".../common/string.c" here?

> +/*
> + * Space and TAB are obvious delimiters. However, I am
> + * adding "\n" and "\r" here too. Just in case when
> + * crazy bootloader/user put them somewhere.
> + */
> +#define DELIM_CHARS          " \n\r\t"
> +#define DELIM_CHARS_COMMA    DELIM_CHARS ","

static const char[] variables (or really just one, with the comma put
first and the non-comma variant indexing into that variable by 1)?

> +#define __packed     __attribute__((__packed__))

No way to include compiler.h here?

> +/*
> + * Compiler is not able to optimize regular strlen()
> + * if argument is well known string during build.
> + * Hence, introduce optimized strlen_opt().
> + */
> +#define strlen_opt(s) (sizeof(s) - 1)

Do we really care in this code?

> +/* Keep in sync with trampoline.S:early_boot_opts label! */
> +typedef struct __packed {
> +    u8 skip_realmode;
> +    u8 opt_edd;
> +    u8 opt_edid;
> +    u16 boot_vid_mode;
> +    u16 vesa_width;
> +    u16 vesa_height;
> +    u16 vesa_depth;
> +} early_boot_opts_t;

This "keeping in sync" should be automated in some way, e.g. via
a new header and suitable macroization.

> +static int strtoi(const char *s, const char *stop, const char **next)
> +{
> +    int base = 10, i, ores = 0, res = 0;

You don't even handle a '-' on the numbers here, so all the variables
and the function return type should be unsigned int afaict. And the
function name perhaps be strtoui().

> +    if ( *s == '0' )
> +      base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
> +
> +    for ( ; *s != '\0'; ++s )
> +    {
> +        for ( i = 0; stop && stop[i] != '\0'; ++i )
> +            if ( *s == stop[i] )
> +                goto out;

strchr()?

> +        if ( *s < '0' || (*s > '7' && base == 8) )
> +        {
> +            res = -1;
> +            goto out;
> +        }
> +
> +        if ( *s > '9' && (base != 16 || tolower(*s) < 'a' || tolower(*s) > 
> 'f') )
> +        {
> +            res = -1;
> +            goto out;
> +        }
> +
> +        res *= base;
> +        res += (tolower(*s) >= 'a') ? (tolower(*s) - 'a' + 10) : (*s - '0');

With the four instances, how about latching tolower(*s) into a local
variable?

> +static u8 skip_realmode(const char *cmdline)
> +{
> +    return !!find_opt(cmdline, "no-real-mode", 0) || !!find_opt(cmdline, 
> "tboot=", 1);

The || makes the two !! pointless.

Also please settle on which type you want to use for boolean
(find_opt()'s last parameter is "int", yet here you use "u8"), and
perhaps make yourself a bool_t.

> +static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
> +{
> +    const char *c;
> +    int tmp;
> +
> +    c = find_opt(cmdline, "vga=", 1);
> +
> +    if ( !c )
> +        return;
> +
> +    ebo->boot_vid_mode = ASK_VGA;
> +
> +    if ( !strmaxcmp(c, "current", DELIM_CHARS_COMMA) )
> +        ebo->boot_vid_mode = VIDEO_CURRENT_MODE;
> +    else if ( !strsubcmp(c, "text-80x") )
> +    {
> +        c += strlen_opt("text-80x");
> +        ebo->boot_vid_mode = rows2vmode(strtoi(c, DELIM_CHARS_COMMA, NULL));
> +    }
> +    else if ( !strsubcmp(c, "gfx-") )
> +    {
> +        tmp = strtoi(c + strlen_opt("gfx-"), "x", &c);
> +
> +        if ( tmp < 0 || tmp > U16_MAX )
> +            return;
> +
> +        ebo->vesa_width = tmp;
> +
> +        /*
> +         * Increment c outside of strtoi() because otherwise some
> +         * compiler may complain with following message:
> +         * warning: operation on ‘c’ may be undefined.
> +         */
> +        ++c;
> +        tmp = strtoi(c, "x", &c);

The comment is pointless - the operation is firmly undefined if you
put it in the strtoi() invocation.

> +        if ( tmp < 0 || tmp > U16_MAX )
> +            return;
> +
> +        ebo->vesa_height = tmp;
> +
> +        tmp = strtoi(++c, DELIM_CHARS_COMMA, NULL);
> +
> +        if ( tmp < 0 || tmp > U16_MAX )
> +            return;
> +
> +        ebo->vesa_depth = tmp;
> +
> +        ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE;

I realize this is reflecting original behavior, but now that you do it in
C, please leverage that fact and defer storing width and height until
you know the entire option was good.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -436,8 +436,24 @@ trampoline_setup:
>          cmp     $sym_phys(__trampoline_seg_stop),%edi
>          jb      1b
>  
> +        /* Do not parse command line on EFI platform here. */
> +        cmpb    $1,sym_phys(skip_realmode)
> +        je      1f

Doesn't this belong in an earlier patch? It certainly doesn't get
moved here from cmdline.S.

> +        /* Bail if there is no command line to parse. */
> +        mov     sym_phys(multiboot_ptr),%ebx
> +        testl   $MBI_CMDLINE,MB_flags(%ebx)
> +        jz      1f
> +
> +        cmpl    $0,MB_cmdline(%ebx)
> +        jz      1f

Any reason not to leave at least this check to the C code?

> +        pushl   $sym_phys(early_boot_opts)
> +        pushl   MB_cmdline(%ebx)
>          call    cmdline_parse_early
> +        add     $8,%esp             /* Remove cmdline_parse_early() args 
> from stack. */

I don't think such a comment is really useful (seems like I overlooked
a similar one in an earlier patch, on the reloc() invocation).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel