Re: [Xen-devel] [PATCH v2 21/23] x86/boot: implement early command line parser in C

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.07.15 at 16:29, <daniel.kiper@xxxxxxxxxx> wrote:
> Current early command line parser implementation in assembler
> is very difficult to change to relocatable stuff using segment
> registers. This requires a lot of changes in very weird and
> fragile code. So, reimplement this functionality in C. This
> way code will be relocatable out of the box and much easier
> to maintain.

All appreciated and nice, but the goal of making the code
relocatable by playing with segment registers sounds fragile:
This breaks assumptions the compiler may validly make.

>  xen/arch/x86/boot/cmdline.S    |  367 -------------------------------------
>  xen/arch/x86/boot/cmdline.c    |  396 
> ++++++++++++++++++++++++++++++++++++++++

A fundamental expectation I would have had is for the C file to be
noticeably smaller than the assembly file.

> --- /dev/null
> +++ b/xen/arch/x86/boot/cmdline.c
>[...]
> +#define VESA_WIDTH   0
> +#define VESA_HEIGHT  1
> +#define VESA_DEPTH   2
> +
> +#define VESA_SIZE    3

These should go away in favor of using individual (sub)structure fields.

> +#define __cdecl              __attribute__((__cdecl__))

???

> +#define __packed     __attribute__((__packed__))
> +#define __text               __attribute__((__section__(".text")))
> +#define __used               __attribute__((__used__))

Likely better to include compiler.h instead.

> +#define max(x,y) ({ \
> +        const typeof(x) _x = (x);       \
> +        const typeof(y) _y = (y);       \
> +        (void) (&_x == &_y);            \
> +        _x > _y ? _x : _y; })

I also wonder whether -imacros .../xen/kernel.h wouldn't be a better
approach here. Please really think hard on how to avoid duplications
like these.

> +#define strlen_static(s) (sizeof(s) - 1)

What is this good for? A decent compiler should be able to deal with
strlen("..."). Plus your macro is longer that what it tries to "abbreviate".

> +static const char empty_chars[] __text = " \n\r\t";

What is empty about them? DYM blank or (white) space or separator
or delimiter? I also wonder whether \n and \r are actually usefully here,
as they should (if at all) only end the line.

> +/**
> + * strlen - Find the length of a string
> + * @s: The string to be sized
> + */
> +static size_t strlen(const char *s)

Comments are certainly nice, but in this special case I'd rather suggest
against bloating the code by commenting standard library functions.

> +static int strtoi(const char *s, const char *stop, const char **next)
> +{
> +    int base = 10, i, ores = 0, res = 0;
> +
> +    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;
> +
> +        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');
> +
> +        if ( ores > res )
> +        {
> +            res = -1;
> +            goto out;
> +        }
> +
> +        ores = res;
> +    }
> +
> +out:

C labels intended by at least one space please.

> +static const char *find_opt(const char *cmdline, const char *opt, int arg)
> +{
> +    size_t lc, lo;
> +    static const char mm[] __text = "--";

I'd be surprised if there weren't compiler/assembler versions
complaining about a section type conflict here. I can see why you
want everything in one section, but I'd rather suggest achieving
this at the linking step (which I would suppose to already be taking
care of this).

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

return find_opt(cmdline, nrm, 0) || find_opt(cmdline, tboot, 1);

> +static u8 edd_parse(const char *cmdline)
> +{
> +    const char *c;
> +    size_t la;
> +    static const char edd[] __text = "edd=";
> +    static const char edd_off[] __text = "off";
> +    static const char edd_skipmbr[] __text = "skipmbr";
> +
> +    c = find_opt(cmdline, edd, 1);
> +
> +    if ( !c )
> +        return 0;
> +
> +    c += strlen_static(edd);
> +    la = strcspn(c, empty_chars);
> +
> +    if ( !strncmp(c, edd_off, max(la, strlen_static(edd_off))) )
> +        return 2;
> +    else if ( !strncmp(c, edd_skipmbr, max(la, strlen_static(edd_skipmbr))) )

Pointless else.

> +        return 1;
> +
> +    return 0;

And the last two returns can be folded again anyway.

> +static void __cdecl __used cmdline_parse_early(const char *cmdline, 
> early_boot_opts_t *ebo)

I don't see the point of the __cdecl, and (as said before) dislike the
static __used pair.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -429,8 +429,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)

Is there a reason you can't look at efi.flags instead here (and in
the other case you abuse skip_realmode as meaning EFI)?

> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -1,3 +1,5 @@
> +#include "video.h"

Please move this farther down, making invisible all its definitions to
code not supposed to use them.

Jan

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