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

[Daniel Kiper]

On Thu, Sep 01, 2016 at 01:41:26AM -0600, Jan Beulich wrote:
> >>> On 31.08.16 at 21:31,  wrote:
> > On Wed, Aug 31, 2016 at 07:01:10AM -0600, Jan Beulich wrote:
> >> >>> On 30.08.16 at 21:58,  wrote:
> >> > On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote:
> >> >> >>> On 20.08.16 at 00:43,  wrote:
> >
> > [...]
> >
> >> >> > +static unsigned int strtoui(const char *s, const char *stop, const 
> >> >> > char
> > **next)
> >> >> > +{
> >> >> > +char l;
> >> >> > +unsigned int base = 10, ores = 0, res = 0;
> >> >> > +
> >> >> > +if ( *s == '0' )
> >> >> > +  base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
> >> >> > +
> >> >> > +for ( ; *s != '\0'; ++s )
> >> >> > +{
> >> >> > +if ( stop && strchr(stop, *s) )
> >> >> > +goto out;
> >> >> > +
> >> >> > +if ( *s < '0' || (*s > '7' && base == 8) )
> >> >> > +{
> >> >> > +res = UINT_MAX;
> >> >> > +goto out;
> >> >> > +}
> >> >> > +
> >> >> > +l = tolower(*s);
> >> >> > +
> >> >> > +if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
> >> >> > +{
> >> >> > +res = UINT_MAX;
> >> >> > +goto out;
> >> >> > +}
> >> >> > +
> >> >> > +res *= base;
> >> >> > +res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
> >> >> > +
> >> >> > +if ( ores > res )
> >> >> > +{
> >> >> > +res = UINT_MAX;
> >> >> > +goto out;
> >> >> > +}
> >> >>
> >> >> Without having spent time to try and find an example, it feels like this
> >> >> check won't catch all possible overflow conditions. If you care about
> >> >> overflow, please make sure you catch all cases.
> >> >
> >> > Hmmm How come? Could you give an example?
> >>
> >> Excuse me, but shouldn't you instead demonstrate the logic is
> >> correct? Or - consider what I had said - try to find an example
> >> yourself? It's not that difficult: With 16-bit word size
> >> 0x * 10 = 0x1fffe, which truncates to 0xfffe and is hence
> >> larger than both inputs but still produced an overflow. This
> >> easily extends to 32- and 64-bit word size.
> >
> > Oh, boy. I forgot about multiplication. I think that we can define
> > res as unsigned long and then check that it is < UINT_MAX.
> > If not then return UINT_MAX.
>
> Aren't we in 32-bit code here, i.e. sizeof(int) == sizeof(long)?

Yep, this should be unsigned long long here.

Daniel

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

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

[Jan Beulich]

>>> On 31.08.16 at 21:31,  wrote:
> On Wed, Aug 31, 2016 at 07:01:10AM -0600, Jan Beulich wrote:
>> >>> On 30.08.16 at 21:58,  wrote:
>> > On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote:
>> >> >>> On 20.08.16 at 00:43,  wrote:
> 
> [...]
> 
>> >> > +static unsigned int strtoui(const char *s, const char *stop, const 
>> >> > char 
> **next)
>> >> > +{
>> >> > +char l;
>> >> > +unsigned int base = 10, ores = 0, res = 0;
>> >> > +
>> >> > +if ( *s == '0' )
>> >> > +  base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
>> >> > +
>> >> > +for ( ; *s != '\0'; ++s )
>> >> > +{
>> >> > +if ( stop && strchr(stop, *s) )
>> >> > +goto out;
>> >> > +
>> >> > +if ( *s < '0' || (*s > '7' && base == 8) )
>> >> > +{
>> >> > +res = UINT_MAX;
>> >> > +goto out;
>> >> > +}
>> >> > +
>> >> > +l = tolower(*s);
>> >> > +
>> >> > +if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
>> >> > +{
>> >> > +res = UINT_MAX;
>> >> > +goto out;
>> >> > +}
>> >> > +
>> >> > +res *= base;
>> >> > +res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
>> >> > +
>> >> > +if ( ores > res )
>> >> > +{
>> >> > +res = UINT_MAX;
>> >> > +goto out;
>> >> > +}
>> >>
>> >> Without having spent time to try and find an example, it feels like this
>> >> check won't catch all possible overflow conditions. If you care about
>> >> overflow, please make sure you catch all cases.
>> >
>> > Hmmm How come? Could you give an example?
>>
>> Excuse me, but shouldn't you instead demonstrate the logic is
>> correct? Or - consider what I had said - try to find an example
>> yourself? It's not that difficult: With 16-bit word size
>> 0x * 10 = 0x1fffe, which truncates to 0xfffe and is hence
>> larger than both inputs but still produced an overflow. This
>> easily extends to 32- and 64-bit word size.
> 
> Oh, boy. I forgot about multiplication. I think that we can define
> res as unsigned long and then check that it is < UINT_MAX.
> If not then return UINT_MAX.

Aren't we in 32-bit code here, i.e. sizeof(int) == sizeof(long)?

Jan


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

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

[Daniel Kiper]

On Wed, Aug 31, 2016 at 07:01:10AM -0600, Jan Beulich wrote:
> >>> On 30.08.16 at 21:58,  wrote:
> > On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote:
> >> >>> On 20.08.16 at 00:43,  wrote:

[...]

> >> > +static unsigned int strtoui(const char *s, const char *stop, const char 
> >> > **next)
> >> > +{
> >> > +char l;
> >> > +unsigned int base = 10, ores = 0, res = 0;
> >> > +
> >> > +if ( *s == '0' )
> >> > +  base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
> >> > +
> >> > +for ( ; *s != '\0'; ++s )
> >> > +{
> >> > +if ( stop && strchr(stop, *s) )
> >> > +goto out;
> >> > +
> >> > +if ( *s < '0' || (*s > '7' && base == 8) )
> >> > +{
> >> > +res = UINT_MAX;
> >> > +goto out;
> >> > +}
> >> > +
> >> > +l = tolower(*s);
> >> > +
> >> > +if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
> >> > +{
> >> > +res = UINT_MAX;
> >> > +goto out;
> >> > +}
> >> > +
> >> > +res *= base;
> >> > +res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
> >> > +
> >> > +if ( ores > res )
> >> > +{
> >> > +res = UINT_MAX;
> >> > +goto out;
> >> > +}
> >>
> >> Without having spent time to try and find an example, it feels like this
> >> check won't catch all possible overflow conditions. If you care about
> >> overflow, please make sure you catch all cases.
> >
> > Hmmm How come? Could you give an example?
>
> Excuse me, but shouldn't you instead demonstrate the logic is
> correct? Or - consider what I had said - try to find an example
> yourself? It's not that difficult: With 16-bit word size
> 0x * 10 = 0x1fffe, which truncates to 0xfffe and is hence
> larger than both inputs but still produced an overflow. This
> easily extends to 32- and 64-bit word size.

Oh, boy. I forgot about multiplication. I think that we can define
res as unsigned long and then check that it is < UINT_MAX.
If not then return UINT_MAX.

Daniel

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

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

[Jan Beulich]

>>> On 30.08.16 at 21:58,  wrote:
> On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote:
>> >>> On 20.08.16 at 00:43,  wrote:
>> > +#define NULL  ((void *)0)
>> > +
>> > +#define __packed  __attribute__((__packed__))
>> > +#define __stdcall __attribute__((__stdcall__))
>> > +
>> > +#define max(x,y) ({ \
>> > +const typeof(x) _x = (x);   \
>> > +const typeof(y) _y = (y);   \
>> > +(void) (&_x == &_y);\
>> > +_x > _y ? _x : _y; })
>>
>> Now that you add a second instance of (some of) these, please
>> move them to a new (local to this directory) header, e.g. defs.h.
>>
>> > +#define tolower(c) ((c) | 0x20)
>> > +
>> > +typedef unsigned char bool_t;
>>
>> _Bool and bool please and ...
>>
>> > +typedef unsigned char u8;
>> > +typedef unsigned short u16;
>> > +typedef unsigned int size_t;
>> > +
>> > +#define FALSE 0
>> > +#define TRUE  1
>>
>> ... these replaced by true and false. In fact I see no reason why
>> you couldn't include xen/stdbool.h here now that it can be more
>> generally used.
> 
> Maybe we should include xen/stdbool.h in defs.h and move to it (defs.h)
> all macros, types definitions, etc. from reloc.c and cmdline.c. This way
> we can share more stuff without duplicating it in both files. Does it
> make sense?

Well, that's what I did suggest in the very first comment still visible
in context above.

>> > +/*
>> > + * static const char *delim_chars = _chars_comma[1];
>> > + *
>> > + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 
>> > 4.1.1-21),
>> > + * put _chars_comma[1] directly into *delim_chars. This means that 
>> > the address
>> > + * in *delim_chars is not properly updated during runtime. Newer 
>> > compilers are much
>> > + * smarter and build fully relocatable code even if above shown construct 
>> > is used.
>> > + * However, define delim_chars[] separately to properly build Xen code on
>> > + * older systems.
>> > + */
>>
>> I have to admit that I don't really understand what you want to
>> say with this comment.
> 
> I tried to use following thing suggested by you earlier:
>   static const char *delim_chars = _chars_comma[1];

I've never suggested anything like that. All I suggested is the
#define approach you've now said you'd give a try.

>> > +static unsigned int strtoui(const char *s, const char *stop, const char 
>> > **next)
>> > +{
>> > +char l;
>> > +unsigned int base = 10, ores = 0, res = 0;
>> > +
>> > +if ( *s == '0' )
>> > +  base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
>> > +
>> > +for ( ; *s != '\0'; ++s )
>> > +{
>> > +if ( stop && strchr(stop, *s) )
>> > +goto out;
>> > +
>> > +if ( *s < '0' || (*s > '7' && base == 8) )
>> > +{
>> > +res = UINT_MAX;
>> > +goto out;
>> > +}
>> > +
>> > +l = tolower(*s);
>> > +
>> > +if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
>> > +{
>> > +res = UINT_MAX;
>> > +goto out;
>> > +}
>> > +
>> > +res *= base;
>> > +res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
>> > +
>> > +if ( ores > res )
>> > +{
>> > +res = UINT_MAX;
>> > +goto out;
>> > +}
>>
>> Without having spent time to try and find an example, it feels like this
>> check won't catch all possible overflow conditions. If you care about
>> overflow, please make sure you catch all cases.
> 
> Hmmm How come? Could you give an example?

Excuse me, but shouldn't you instead demonstrate the logic is
correct? Or - consider what I had said - try to find an example
yourself? It's not that difficult: With 16-bit word size
0x * 10 = 0x1fffe, which truncates to 0xfffe and is hence
larger than both inputs but still produced an overflow. This
easily extends to 32- and 64-bit word size.

Jan


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

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

[Daniel Kiper]

On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote:
> >>> On 20.08.16 at 00:43,  wrote:
> > +#define NULL   ((void *)0)
> > +
> > +#define __packed   __attribute__((__packed__))
> > +#define __stdcall  __attribute__((__stdcall__))
> > +
> > +#define max(x,y) ({ \
> > +const typeof(x) _x = (x);   \
> > +const typeof(y) _y = (y);   \
> > +(void) (&_x == &_y);\
> > +_x > _y ? _x : _y; })
>
> Now that you add a second instance of (some of) these, please
> move them to a new (local to this directory) header, e.g. defs.h.
>
> > +#define tolower(c) ((c) | 0x20)
> > +
> > +typedef unsigned char bool_t;
>
> _Bool and bool please and ...
>
> > +typedef unsigned char u8;
> > +typedef unsigned short u16;
> > +typedef unsigned int size_t;
> > +
> > +#define FALSE  0
> > +#define TRUE   1
>
> ... these replaced by true and false. In fact I see no reason why
> you couldn't include xen/stdbool.h here now that it can be more
> generally used.

Maybe we should include xen/stdbool.h in defs.h and move to it (defs.h)
all macros, types definitions, etc. from reloc.c and cmdline.c. This way
we can share more stuff without duplicating it in both files. Does it
make sense?

> > +/*
> > + * Space and TAB are obvious delimiters. However, I am
> > + * adding "\n" and "\r" here too. Just in case when
> > + * crazy bootloader/user puts them somewhere.
> > + */
> > +static const char delim_chars_comma[] = ", \n\r\t";
> > +static const char delim_chars[] = " \n\r\t";
>
> I realize it's minor, but why two arrays instead of
>
> #define delim_chars (delim_chars_comma + 1)
>
> ?

I can try it.

> > +/*
> > + * static const char *delim_chars = _chars_comma[1];
> > + *
> > + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 
> > 4.1.1-21),
> > + * put _chars_comma[1] directly into *delim_chars. This means that 
> > the address
> > + * in *delim_chars is not properly updated during runtime. Newer compilers 
> > are much
> > + * smarter and build fully relocatable code even if above shown construct 
> > is used.
> > + * However, define delim_chars[] separately to properly build Xen code on
> > + * older systems.
> > + */
>
> I have to admit that I don't really understand what you want to
> say with this comment.

I tried to use following thing suggested by you earlier:
  static const char *delim_chars = _chars_comma[1];

Sadly, it does not work because after building/linking by older
GCC/linker delim_chars contains static address and cannot be
properly relocated during runtime. Newer GCC/linkers are much
smarter and all references to delim_chars and finally to
_chars_comma[1] are properly relocated during runtime.
However, there is a chance that your approach with #define
will work with all supported build environments.

> > +static unsigned int strtoui(const char *s, const char *stop, const char 
> > **next)
> > +{
> > +char l;
> > +unsigned int base = 10, ores = 0, res = 0;
> > +
> > +if ( *s == '0' )
> > +  base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
> > +
> > +for ( ; *s != '\0'; ++s )
> > +{
> > +if ( stop && strchr(stop, *s) )
> > +goto out;
> > +
> > +if ( *s < '0' || (*s > '7' && base == 8) )
> > +{
> > +res = UINT_MAX;
> > +goto out;
> > +}
> > +
> > +l = tolower(*s);
> > +
> > +if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
> > +{
> > +res = UINT_MAX;
> > +goto out;
> > +}
> > +
> > +res *= base;
> > +res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
> > +
> > +if ( ores > res )
> > +{
> > +res = UINT_MAX;
> > +goto out;
> > +}
>
> Without having spent time to try and find an example, it feels like this
> check won't catch all possible overflow conditions. If you care about
> overflow, please make sure you catch all cases.

Hmmm How come? Could you give an example?

Daniel

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

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

[Jan Beulich]

>>> On 20.08.16 at 00:43,  wrote:
> +#define NULL ((void *)0)
> +
> +#define __packed __attribute__((__packed__))
> +#define __stdcall__attribute__((__stdcall__))
> +
> +#define max(x,y) ({ \
> +const typeof(x) _x = (x);   \
> +const typeof(y) _y = (y);   \
> +(void) (&_x == &_y);\
> +_x > _y ? _x : _y; })

Now that you add a second instance of (some of) these, please
move them to a new (local to this directory) header, e.g. defs.h.

> +#define tolower(c) ((c) | 0x20)
> +
> +typedef unsigned char bool_t;

_Bool and bool please and ...

> +typedef unsigned char u8;
> +typedef unsigned short u16;
> +typedef unsigned int size_t;
> +
> +#define FALSE0
> +#define TRUE 1

... these replaced by true and false. In fact I see no reason why
you couldn't include xen/stdbool.h here now that it can be more
generally used.

> +/*
> + * Space and TAB are obvious delimiters. However, I am
> + * adding "\n" and "\r" here too. Just in case when
> + * crazy bootloader/user puts them somewhere.
> + */
> +static const char delim_chars_comma[] = ", \n\r\t";
> +static const char delim_chars[] = " \n\r\t";

I realize it's minor, but why two arrays instead of

#define delim_chars (delim_chars_comma + 1)

?

> +/*
> + * static const char *delim_chars = _chars_comma[1];
> + *
> + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 
> 4.1.1-21),
> + * put _chars_comma[1] directly into *delim_chars. This means that the 
> address
> + * in *delim_chars is not properly updated during runtime. Newer compilers 
> are much
> + * smarter and build fully relocatable code even if above shown construct is 
> used.
> + * However, define delim_chars[] separately to properly build Xen code on
> + * older systems.
> + */

I have to admit that I don't really understand what you want to
say with this comment.

> +static unsigned int strtoui(const char *s, const char *stop, const char 
> **next)
> +{
> +char l;
> +unsigned int base = 10, ores = 0, res = 0;
> +
> +if ( *s == '0' )
> +  base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
> +
> +for ( ; *s != '\0'; ++s )
> +{
> +if ( stop && strchr(stop, *s) )
> +goto out;
> +
> +if ( *s < '0' || (*s > '7' && base == 8) )
> +{
> +res = UINT_MAX;
> +goto out;
> +}
> +
> +l = tolower(*s);
> +
> +if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
> +{
> +res = UINT_MAX;
> +goto out;
> +}
> +
> +res *= base;
> +res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
> +
> +if ( ores > res )
> +{
> +res = UINT_MAX;
> +goto out;
> +}

Without having spent time to try and find an example, it feels like this
check won't catch all possible overflow conditions. If you care about
overflow, please make sure you catch all cases.

> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -220,8 +220,20 @@ trampoline_boot_cpu_entry:
>  /* Jump to the common bootstrap entry point. */
>  jmp trampoline_protmode_entry
>  
> +#include "video.h"
> +
> +/* Keep in sync with cmdline.c:early_boot_opts_t type! */
> +early_boot_opts:
>  skip_realmode:
>  .byte   0
> +opt_edd:
> +.byte   0   /* edd=on/off/skipmbr */
> +opt_edid:
> +.byte   0   /* EDID parsing option 
> (force/no/default). */
> +GLOBAL(boot_vid_mode)
> +.word   VIDEO_80x25 /* If we don't run at all, 
> assume basic video mode 3 at 80x25. */
> +vesa_size:
> +.word   0,0,0   /* width x depth x height */

While I don't mind you using the packed attribute on the C variant,
please insert a padding byte here and there to make the four words
aligned, and add an alignment directive to make the whole structure
at least word aligned.

Jan

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

[Xen-devel] [PATCH v5 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 
---
v4 - suggestions/fixes:
   - move to stdcall calling convention
 (suggested by Jan Beulich),
   - define bool_t and use it properly
 (suggested by Jan Beulich),
   - put list of delimiter chars into
 static const char[]
 (suggested by Jan Beulich),
   - use strlen() instead of strlen_opt()
 (suggested by Jan Beulich),
   - change strtoi() to strtoui() and
 optimize it a bit
 (suggested by Jan Beulich),
   - define strchr() and use it in strtoui()
 (suggested by Jan Beulich),
   - optimize vga_parse()
 (suggested by Jan Beulich),
   - move !cmdline check from assembly to C
 (suggested by Jan Beulich),
   - remove my name from copyright (Oracle requirement)
 (suggested by Konrad Rzeszutek Wilk).

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.mk   |2 +
 xen/arch/x86/boot/cmdline.S|  367 ---
 xen/arch/x86/boot/cmdline.c|  376 
 xen/arch/x86/boot/edd.S|3 -
 xen/arch/x86/boot/head.S   |8 +
 xen/arch/x86/boot/trampoline.S |   12 ++
 xen/arch/x86/boot/video.S  |7 -
 10 files changed, 408 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 d193820..23637d8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -248,9 +248,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 71ec34e..9464b7b 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -212,6 +212,6 @@ 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/*.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
rm -f note.o
$(MAKE) -f $(BASEDIR)/Rules.mk -C test clean
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.mk b/xen/arch/x86/boot/build32.mk
index 39e6453..3d01698 100644
--- a/xen/arch/x86/boot/build32.mk
+++ b/xen/arch/x86/boot/build32.mk
@@ -32,6 +32,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 past parsed digits.
-.Latoi:
-push%ebx
-push%ecx