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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Aug 27, 2015 at 06:43:39AM -0600, Jan Beulich wrote:
> >>> 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.

Well, it looks that this sentence is not precise. I should fix this.
Anyway, I am not playing with segment registers in C code because
it is not needed and as you pointed out it is dangerous.

> >  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__))
>
> ???

Please look below.

> > +#define __packed   __attribute__((__packed__))
> > +#define __text             __attribute__((__section__(".text")))
> > +#define __used             __attribute__((__used__))
>
> Likely better to include compiler.h instead.

As I know you do not like to include such headers in early C files
because it makes code fragile and it looks strange. I agree with you
to some extent. So, I decided to define needed constants ourselves.
Whatever we do we should be consistent. Hence, if we include compiler.h
here we should do the same in reloc.c too if it is required.

> > +#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.

Ditto. So, what is your decision? Include or define? If include then
we should think how to generate relevant dependencies automatically.

> > +#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".

I thought that it is true but it is not. Sadly, without this binary is 
bigger... :-(((
However, you are right that the name could be better.

> > +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,

Yep, delimiter or something like that looks better.

> as they should (if at all) only end the line.

Yes, I included them just in case and they should not appear in command line.

> > +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).

Nope, it does not work in that way. However, I discovered that newer GCC
versions generate .rodata for switch/case. So, anyway we must cope with
at least two different sections and link them properly.

> > +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.

Are you sure that without __cdecl compiler will not try to optimize
cmdline_parse_early() call and try to pass arguments using registers
or anything else not conforming to cdecl calling convention? Right
now I am not sure about that. However, I suppose that if we remove
static then there is a chance that function will be always called
according to cdecl (I must check this). Anyway, I think that we
should retain (even add in case of reloc.c:reloc(), others?) __cdecl
before 32-bit functions which are called from assembly. Just in case.

Daniel

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