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

[Jan Beulich]

>>> On 02.06.16 at 10:15,  wrote:
> On Fri, May 27, 2016 at 03:33:49AM -0600, Jan Beulich wrote:
>> >>> On 25.05.16 at 23:36,  wrote:
>> > On Wed, May 25, 2016 at 04:33:54AM -0600, Jan Beulich wrote:
>> >> >>> On 15.04.16 at 14:33,  wrote:
>> >> > +/*
>> >> > + * 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?
>> >
>> > Not to strongly but why not?
>>
>> Keep things as readable as possible. In fact I wouldn't mind hard
>> coded literal numbers for the string lengths, if they sit right next
>> to the respective string literal.
> 
> As separate variable? Does it pays? I prefer standard strlen() call
> instead of that.

Variable? I said literal numbers. As in

strncmp(str, "xyz", 3);

From such code it is visible at the first glance what the 3 stands for,
and is imo better readable than

strncmp(str, "xyz", strlen("xyz"));

>> >> > +pushl   $sym_phys(early_boot_opts)
>> >> > +pushl   MB_cmdline(%ebx)
>> >> >  callcmdline_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).
>> >
>> > This thing is quite obvious but I do not think that this comment hurts.
>>
>> It may not really hurt, but it draws needless attention to something
>> that is to b expected after any function call getting arguments
>> passed on the stack. You could, btw., make cmdline_parse_early
>> a stdcall function, so you wouldn't have to do that adjustment
>> here.
> 
> If it is acceptable by you then I can do that.

There are two possible issues, which would need checking (which
I only thought of after having written that reply):
- Do all gcc versions we care about support stdcall?
- What's the disposition of those asm() stubs on the callee side?
  (Remember that Andrew had asked for them to get dropped?)

Jan


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

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

[Daniel Kiper]

On Fri, May 27, 2016 at 03:33:49AM -0600, Jan Beulich wrote:
> >>> On 25.05.16 at 23:36,  wrote:
> > On Wed, May 25, 2016 at 04:33:54AM -0600, Jan Beulich wrote:
> >> >>> On 15.04.16 at 14:33,  wrote:

[...]

> >> > +/*
> >> > + * 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?
> >
> > Not to strongly but why not?
>
> Keep things as readable as possible. In fact I wouldn't mind hard
> coded literal numbers for the string lengths, if they sit right next
> to the respective string literal.

As separate variable? Does it pays? I prefer standard strlen() call
instead of that.

> >> > +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
> >
> > Yep, however, ...
> >
> >> and the function return type should be unsigned int afaict. And the
> >> function name perhaps be strtoui().
> >
> > ... we return -1 in case of error.
>
> Which - having looked at some of the callers - could easily be
> UINT_MAX as it seems.

Here it looks safe.

> >> > +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
> >
> > Could be u8.
> >
> >> perhaps make yourself a bool_t.
> >
> > I do not think it make sense here.
>
> I think it makes as much or as little sense as having NULL available.

:-)))

> >> > +/*
> >> > + * 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", );
> >>
> >> The comment is pointless - the operation is firmly undefined if you
> >> put it in the strtoi() invocation.
> >
> > In terms of C spec you are right. However, it is quite surprising that older
> > GCC complains and newer ones do not. Should not we investigate this?
>
> Actually I think I was wrong here. A function call like func(c++, c)

Because argument evaluation order is undefined in C. Am I correct?

> would be undefined, but func(c++, ) isn't. So I guess if there are
> compiler versions getting this wrong, then you should just disregard
> my comment.

By the way, here is quite good description of these problems:
  http://en.cppreference.com/w/c/language/eval_order

> >> > +pushl   $sym_phys(early_boot_opts)
> >> > +pushl   MB_cmdline(%ebx)
> >> >  callcmdline_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).
> >
> > This thing is quite obvious but I do not think that this comment hurts.
>
> It may not really hurt, but it draws needless attention to something
> that is to b expected after any function call getting arguments
> passed on the stack. You could, btw., make cmdline_parse_early
> a stdcall function, so you wouldn't have to do that adjustment
> here.

If it is acceptable by you then I can do that.

Daniel

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

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

[Jan Beulich]

>>> On 25.05.16 at 23:36,  wrote:
> On Wed, May 25, 2016 at 04:33:54AM -0600, Jan Beulich wrote:
>> >>> On 15.04.16 at 14:33,  wrote:
>> > --- /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 
>> > + *
>> > + * 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 .
>> > + *
>> > + * 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?
> 
> I am confused. Sometimes you request to reduce number of such strange
> includes and sometimes you ask me to do something contrary? So, what
> is the rule behind these requests?

The rule is "whatever fits best for the current purpose". Such
"strange" includes should be avoided if their purpose can be
fulfilled by other means. Them being avoided at the price of
quite a bit of code duplication, otoh, doesn't seem well advised
to me.

>> > +/*
>> > + * 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?
> 
> Not to strongly but why not?

Keep things as readable as possible. In fact I wouldn't mind hard
coded literal numbers for the string lengths, if they sit right next
to the respective string literal.

>> > +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
> 
> Yep, however, ...
> 
>> and the function return type should be unsigned int afaict. And the
>> function name perhaps be strtoui().
> 
> ... we return -1 in case of error.

Which - having looked at some of the callers - could easily be
UINT_MAX as it seems.

>> > +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
> 
> Could be u8.
> 
>> perhaps make yourself a bool_t.
> 
> I do not think it make sense here.

I think it makes as much or as little sense as having NULL available.

>> > +/*
>> > + * 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", );
>>
>> The comment is pointless - the operation is firmly undefined if you
>> put it in the strtoi() invocation.
> 
> In terms of C spec you are right. However, it is quite surprising that older
> GCC complains and newer ones do not. Should not we investigate this?

Actually I think I was wrong here. A function call like func(c++, c)
would be undefined, but func(c++, ) isn't. So I guess if there are
compiler versions getting this wrong, then you should just disregard
my comment.

>> > +pushl   $sym_phys(early_boot_opts)
>> > +pushl   MB_cmdline(%ebx)
>> >  callcmdline_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).
> 
> This thing is quite obvious but I do not think that this comment hurts.

It may not really hurt, but it draws needless attention to something
that is to b expected after any function call getting arguments
passed on the stack. You could, btw., make cmdline_parse_early
a stdcall function, so you wouldn't have to do that adjustment
here.

Jan

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

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

[Daniel Kiper]

On Wed, May 25, 2016 at 04:33:54AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33,  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?

Yep, however, I also said that we should add "*(.rodata.*)"
if it is needed. And it is needed here.

> > --- /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 
> > + *
> > + * 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 .
> > + *
> > + * 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?

I am confused. Sometimes you request to reduce number of such strange
includes and sometimes you ask me to do something contrary? So, what
is the rule behind these requests?

Additionally, there is a chance that final xen ELF file will be
unnecessary larger because it will contain unused functions. Wait,
maybe there is a linker option which allows us to remove unreferenced
objects from final output.

> > +/*
> > + * 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)?

OK.

> > +#define __packed   __attribute__((__packed__))
>
> No way to include compiler.h here?

Ditto.

> > +/*
> > + * 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?

Not to strongly but why not?

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

OK.

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

Yep, however, ...

> and the function return type should be unsigned int afaict. And the
> function name perhaps be strtoui().

... we return -1 in case of error.

> > +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()?

Could be.

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

Make sense.

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

Could be u8.

> perhaps make yourself a bool_t.

I do not think it make sense here.

> > +static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
> > +{
> > +const char *c;
> > +int 

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

[Jan Beulich]

>>> On 15.04.16 at 14:33,  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 
> + *
> + * 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 .
> + *
> + * 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_COMMADELIM_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", );
> +
> +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", );

The comment is pointless - the operation is firmly undefined if 

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

[Daniel Kiper]

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 (without playing
with segment registers) and much easier to maintain.

Suggested-by: Andrew Cooper 
Signed-off-by: Daniel Kiper 
---
v3 - suggestions/fixes:
   - optimize some code
 (suggested by Jan Beulich),
   - put VESA data into early_boot_opts_t members
 (suggested by Jan Beulich),
   - rename some functions and variables
 (suggested by Jan Beulich),
   - move around video.h include in xen/arch/x86/boot/trampoline.S
 (suggested by Jan Beulich),
   - fix coding style
 (suggested by Jan Beulich),
   - fix build with older GCC
 (suggested by Konrad Rzeszutek Wilk),
   - remove redundant comments
 (suggested by Jan Beulich),
   - add some comments
   - improve commit message
 (suggested by Jan Beulich).
---
 .gitignore |5 +-
 xen/arch/x86/Makefile  |2 +-
 xen/arch/x86/boot/Makefile |7 +-
 xen/arch/x86/boot/build32.lds  |1 +
 xen/arch/x86/boot/build32.mk   |4 +-
 xen/arch/x86/boot/cmdline.S|  367 
 xen/arch/x86/boot/cmdline.c|  357 ++
 xen/arch/x86/boot/edd.S|3 -
 xen/arch/x86/boot/head.S   |   17 ++
 xen/arch/x86/boot/trampoline.S |   12 ++
 xen/arch/x86/boot/video.S  |6 -
 11 files changed, 400 insertions(+), 381 deletions(-)
 delete mode 100644 xen/arch/x86/boot/cmdline.S
 create mode 100644 xen/arch/x86/boot/cmdline.c

diff --git a/.gitignore b/.gitignore
index 91f690c..6919546 100644
--- a/.gitignore
+++ b/.gitignore
@@ -237,9 +237,10 @@ xen/arch/arm/xen.lds
 xen/arch/x86/asm-offsets.s
 xen/arch/x86/boot/mkelf32
 xen/arch/x86/xen.lds
+xen/arch/x86/boot/cmdline.S
 xen/arch/x86/boot/reloc.S
-xen/arch/x86/boot/reloc.bin
-xen/arch/x86/boot/reloc.lnk
+xen/arch/x86/boot/*.bin
+xen/arch/x86/boot/*.lnk
 xen/arch/x86/efi.lds
 xen/arch/x86/efi/check.efi
 xen/arch/x86/efi/disabled
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 1bcb08b..32d2407 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -176,4 +176,4 @@ clean::
rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32
rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d
rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.o efi/.*.d efi/*.efi 
efi/disabled efi/mkreloc
-   rm -f boot/reloc.S boot/reloc.lnk boot/reloc.bin
+   rm -f boot/cmdline.S boot/reloc.S boot/*.lnk boot/*.bin
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 06893d8..d73cc76 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,9 +1,14 @@
 obj-bin-y += head.o
 
+CMDLINE_DEPS = video.h
+
 RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
$(BASEDIR)/include/xen/multiboot.h \
 $(BASEDIR)/include/xen/multiboot2.h
 
-head.o: reloc.S
+head.o: cmdline.S reloc.S
+
+cmdline.S: cmdline.c $(CMDLINE_DEPS)
+   $(MAKE) -f build32.mk $@ CMDLINE_DEPS="$(CMDLINE_DEPS)"
 
 reloc.S: reloc.c $(RELOC_DEPS)
$(MAKE) -f build32.mk $@ RELOC_DEPS="$(RELOC_DEPS)"
diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds
index 47db9c4..6a234ea 100644
--- a/xen/arch/x86/boot/build32.lds
+++ b/xen/arch/x86/boot/build32.lds
@@ -25,6 +25,7 @@ SECTIONS
 *(.text)
 *(.text.*)
 *(.rodata)
+*(.rodata.*)
   }
 
   /DISCARD/ : {
diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot/build32.mk
index eb02b4b..58e27e1 100644
--- a/xen/arch/x86/boot/build32.mk
+++ b/xen/arch/x86/boot/build32.mk
@@ -23,7 +23,7 @@ CFLAGS := $(filter-out -flto,$(CFLAGS))
$(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' |\
while read idx name sz rest; do \
case "$$name" in \
-   .data|.data.*|.rodata.*|.bss|.bss.*) \
+   .data|.data.*|.bss|.bss.*) \
test $$sz != 0 || continue; \
echo "Error: non-empty $$name: 0x$$sz" >&2; \
exit $$(expr $$idx + 1);; \
@@ -34,6 +34,8 @@ CFLAGS := $(filter-out -flto,$(CFLAGS))
 %.o: %.c
$(CC) $(CFLAGS) -c -fpic $< -o $@
 
+cmdline.o: cmdline.c $(CMDLINE_DEPS)
+
 reloc.o: reloc.c $(RELOC_DEPS)
 
 .PRECIOUS: %.bin %.lnk
diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
deleted file mode 100644
index 00687eb..000
--- a/xen/arch/x86/boot/cmdline.S
+++ /dev/null
@@ -1,367 +0,0 @@
-/**
- * cmdline.S
- *
- * Early command-line parsing.
- */
-
-.code32
-
-#include "video.h"
-
-# NB. String pointer on stack is modified to point