[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C
From: |
Jan Beulich |
Subject: |
Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C |
Date: |
Thu, 27 Aug 2015 06:43:39 -0600 |
>>> On 20.07.15 at 16:29, <address@hidden> 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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C,
Jan Beulich <=