| Message ID | 1471646606-28519-15-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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. > +/* > + * 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 = &delim_chars_comma[1]; > + * > + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21), > + * put &delim_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
On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote: > >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 = &delim_chars_comma[1]; > > + * > > + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21), > > + * put &delim_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 = &delim_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 &delim_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
>>> On 30.08.16 at 21:58, <daniel.kiper@oracle.com> wrote: > On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote: >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 = &delim_chars_comma[1]; >> > + * >> > + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21), >> > + * put &delim_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 = &delim_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 0x3333 * 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
On Wed, Aug 31, 2016 at 07:01:10AM -0600, Jan Beulich wrote: > >>> On 30.08.16 at 21:58, <daniel.kiper@oracle.com> wrote: > > On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote: > >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 > 0x3333 * 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
>>> On 31.08.16 at 21:31, <daniel.kiper@oracle.com> wrote: > On Wed, Aug 31, 2016 at 07:01:10AM -0600, Jan Beulich wrote: >> >>> On 30.08.16 at 21:58, <daniel.kiper@oracle.com> wrote: >> > On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote: >> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 >> 0x3333 * 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
On Thu, Sep 01, 2016 at 01:41:26AM -0600, Jan Beulich wrote: > >>> On 31.08.16 at 21:31, <daniel.kiper@oracle.com> wrote: > > On Wed, Aug 31, 2016 at 07:01:10AM -0600, Jan Beulich wrote: > >> >>> On 30.08.16 at 21:58, <daniel.kiper@oracle.com> wrote: > >> > On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote: > >> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> 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 > >> 0x3333 * 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
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..0000000 --- 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 - push %edx - push %esi - xor %ebx,%ebx /* %ebx = accumulator */ - mov $10,%ecx /* %ecx = base (default base 10) */ - mov 16+4(%esp),%esi /* %esi = pointer into ascii string. */ - lodsb - cmpb $'0',%al - jne 2f - mov $8,%ecx /* Prefix '0' => octal (base 8) */ - lodsb - cmpb $'x',%al - jne 2f - mov $16,%ecx /* Prefix '0x' => hex (base 16) */ -1: lodsb -2: sub $'0',%al - jb 4f - cmp $9,%al - jbe 3f - sub $'A'-'0'-10,%al - jb 4f - cmp $15,%al - jbe 3f - sub $'a'-'A',%al - jb 4f -3: cmp %cl,%al - jae 4f - movzbl %al,%eax - xchg %eax,%ebx - mul %ecx - xchg %eax,%ebx - add %eax,%ebx - jmp 1b -4: mov %ebx,%eax - dec %esi - mov %esi,16+4(%esp) - pop %esi - pop %edx - pop %ecx - pop %ebx - ret - -.Lstrstr: - push %ecx - push %edx - push %esi - push %edi - xor %eax,%eax - xor %ecx,%ecx - not %ecx - mov 16+4(%esp),%esi - mov 16+8(%esp),%edi - repne scasb - not %ecx - dec %ecx - mov %ecx,%edx -1: mov 16+8(%esp),%edi - mov %esi,%eax - mov %edx,%ecx - repe cmpsb - je 2f - xchg %eax,%esi - inc %esi - cmpb $0,-1(%eax) - jne 1b - xor %eax,%eax -2: pop %edi - pop %esi - pop %edx - pop %ecx - ret - -.Lstr_prefix: - push %esi - push %edi - mov 8+4(%esp),%esi /* 1st arg is prefix string */ - mov 8+8(%esp),%edi /* 2nd arg is main string */ -1: lodsb - test %al,%al - jz 2f - scasb - je 1b - sbb %eax,%eax - or $1,%al - jmp 3f -2: xor %eax,%eax -3: pop %edi - pop %esi - ret - -.Lstrlen: - push %ecx - push %esi - push %edi - xor %eax,%eax - xor %ecx,%ecx - not %ecx - mov 12+4(%esp),%edi - repne scasb - not %ecx - dec %ecx - mov %ecx,%eax - pop %edi - pop %esi - pop %ecx - ret - -.Lfind_option: - mov 4(%esp),%eax - dec %eax - push %ebx -1: pushl 4+8(%esp) - inc %eax - push %eax - call .Lstrstr - add $8,%esp - test %eax,%eax - jz 3f - cmp %eax,4+4(%esp) - je 2f - cmpb $' ',-1(%eax) - jne 1b -2: mov %eax,%ebx - pushl 4+8(%esp) - call .Lstrlen - add $4,%esp - xadd %eax,%ebx - /* NUL check (as $'\0' == 0x30 in GAS) */ - cmpb $0,(%ebx) - je 3f - cmpb $' ',(%ebx) - je 3f - cmpb $'=',(%ebx) - jne 1b -3: pop %ebx - ret - -cmdline_parse_early: - pusha - - /* Bail if there is no command line to parse. */ - mov sym_phys(multiboot_ptr),%ebx - mov MB_flags(%ebx),%eax - test $4,%al - jz .Lcmdline_exit - mov MB_cmdline(%ebx),%eax - test %eax,%eax - jz .Lcmdline_exit - - /* Check for 'no-real-mode' command-line option. */ - pushl $sym_phys(.Lno_rm_opt) - pushl MB_cmdline(%ebx) - call .Lfind_option - test %eax,%eax - setnz %al - or %al,sym_phys(skip_realmode) - - /* Check for 'tboot=' command-line option. */ - movl $sym_phys(.Ltboot_opt),4(%esp) - call .Lfind_option - test %eax,%eax - setnz %al - or %al,sym_phys(skip_realmode) /* tboot= implies no-real-mode */ - -.Lparse_edd: - /* Check for 'edd=' command-line option. */ - movl $sym_phys(.Ledd_opt),4(%esp) - call .Lfind_option - test %eax,%eax - jz .Lparse_edid - cmpb $'=',3(%eax) - jne .Lparse_edid - add $4,%eax - movb $2,sym_phys(opt_edd) /* opt_edd=2: edd=off */ - cmpw $0x666f,(%eax) /* 0x666f == "of" */ - je .Lparse_edid - decb sym_phys(opt_edd) /* opt_edd=1: edd=skipmbr */ - cmpw $0x6b73,(%eax) /* 0x6b73 == "sk" */ - je .Lparse_edid - decb sym_phys(opt_edd) /* opt_edd=0: edd=on (default) */ - -.Lparse_edid: - /* Check for 'edid=' command-line option. */ - movl $sym_phys(.Ledid_opt),4(%esp) - call .Lfind_option - test %eax,%eax - jz .Lparse_vga - cmpb $'=',4(%eax) - jne .Lparse_vga - add $5,%eax - mov %eax,%ebx - push %ebx - pushl $sym_phys(.Ledid_force) - call .Lstr_prefix - add $8,%esp - movb $2,sym_phys(opt_edid) /* opt_edid=2: edid=force */ - test %eax,%eax - jz .Lparse_vga - push %ebx - pushl $sym_phys(.Ledid_no) - call .Lstr_prefix - add $8,%esp - decb sym_phys(opt_edid) /* opt_edid=1: edid=no */ - test %eax,%eax - jz .Lparse_vga - decb sym_phys(opt_edid) /* opt_edid=0: default */ - -.Lparse_vga: - /* Check for 'vga=' command-line option. */ - movl $sym_phys(.Lvga_opt),4(%esp) - call .Lfind_option - add $8,%esp - test %eax,%eax - jz .Lcmdline_exit - cmpb $'=',3(%eax) - jne .Lcmdline_exit - add $4,%eax - - /* Found the 'vga=' option. Default option is to display vga menu. */ - movw $ASK_VGA,sym_phys(boot_vid_mode) - - /* Check for 'vga=text-80x<rows>. */ - mov %eax,%ebx - push %ebx - pushl $sym_phys(.Lvga_text80) - call .Lstr_prefix - add $8,%esp - test %eax,%eax - jnz .Lparse_vga_gfx - - /* We have 'vga=text-80x<rows>'. */ - add $8,%ebx - push %ebx - call .Latoi - add $4,%esp - mov %ax,%bx - lea sym_phys(.Lvga_text_modes),%esi -1: lodsw - test %ax,%ax - jz .Lcmdline_exit - cmp %ax,%bx - lodsw - jne 1b - mov %ax,sym_phys(boot_vid_mode) - jmp .Lcmdline_exit - -.Lparse_vga_gfx: - /* Check for 'vga=gfx-<width>x<height>x<depth>'. */ - push %ebx - pushl $sym_phys(.Lvga_gfx) - call .Lstr_prefix - add $8,%esp - test %eax,%eax - jnz .Lparse_vga_mode - - /* We have 'vga=gfx-<width>x<height>x<depth>'. */ - /* skip 'gfx-' */ - add $4,%ebx - /* parse <width> */ - push %ebx - call .Latoi - pop %esi - mov %ax,sym_phys(vesa_size)+0 - /* skip 'x' */ - lodsb - cmpb $'x',%al - jne .Lcmdline_exit - /* parse <height> */ - push %esi - call .Latoi - pop %esi - mov %ax,sym_phys(vesa_size)+2 - /* skip 'x' */ - lodsb - cmpb $'x',%al - jne .Lcmdline_exit - /* parse <depth> */ - push %esi - call .Latoi - pop %esi - mov %ax,sym_phys(vesa_size)+4 - /* commit to vesa mode */ - movw $VIDEO_VESA_BY_SIZE,sym_phys(boot_vid_mode) - jmp .Lcmdline_exit - -.Lparse_vga_mode: - /* Check for 'vga=mode-<mode>'. */ - push %ebx - pushl $sym_phys(.Lvga_mode) - call .Lstr_prefix - add $8,%esp - test %eax,%eax - jnz .Lparse_vga_current - - /* We have 'vga=mode-<mode>'. */ - add $5,%ebx - push %ebx - call .Latoi - add $4,%esp - mov %ax,sym_phys(boot_vid_mode) - jmp .Lcmdline_exit - -.Lparse_vga_current: - /* Check for 'vga=current'. */ - push %ebx - pushl $sym_phys(.Lvga_current) - call .Lstr_prefix - add $8,%esp - test %eax,%eax - jnz .Lcmdline_exit - - /* We have 'vga=current'. */ - movw $VIDEO_CURRENT_MODE,sym_phys(boot_vid_mode) - -.Lcmdline_exit: - popa - ret - - .pushsection .init.rodata, "a", @progbits - -.Lvga_text_modes: /* rows, mode_number */ - .word 25,VIDEO_80x25 - .word 50,VIDEO_80x50 - .word 43,VIDEO_80x43 - .word 28,VIDEO_80x28 - .word 30,VIDEO_80x30 - .word 34,VIDEO_80x34 - .word 60,VIDEO_80x60 - .word 0 - -.Lvga_opt: - .asciz "vga" -.Lvga_text80: - .asciz "text-80x" -.Lvga_gfx: - .asciz "gfx-" -.Lvga_mode: - .asciz "mode-" -.Lvga_current: - .asciz "current" -.Lno_rm_opt: - .asciz "no-real-mode" -.Ltboot_opt: - .asciz "tboot" -.Ledid_opt: - .asciz "edid" -.Ledid_force: - .asciz "force" -.Ledid_no: - .asciz "no" -.Ledd_opt: - .asciz "edd" - - .popsection diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c new file mode 100644 index 0000000..2da804c --- /dev/null +++ b/xen/arch/x86/boot/cmdline.c @@ -0,0 +1,376 @@ +/* + * Copyright (c) 2015, 2016 Oracle and/or its affiliates. All rights reserved. + * + * 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 <http://www.gnu.org/licenses/>. + * + * strlen(), strncmp(), strchr(), strspn() and strcspn() were copied from + * Linux kernel source (linux/lib/string.c). + * + * max() was copied from xen/xen/include/xen/kernel.h. + */ + +/* + * This entry point is entered from xen/arch/x86/boot/head.S with: + * - 0x4(%esp) = &cmdline, + * - 0x8(%esp) = &early_boot_opts. + */ +asm ( + " .text \n" + " .globl _start \n" + "_start: \n" + " jmp cmdline_parse_early \n" + ); + +#include "video.h" + +#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; }) + +#define tolower(c) ((c) | 0x20) + +typedef unsigned char bool_t; +typedef unsigned char u8; +typedef unsigned short u16; +typedef unsigned int size_t; + +#define FALSE 0 +#define TRUE 1 + +#define U16_MAX ((u16)(~0U)) +#define UINT_MAX (~0U) + +/* Keep in sync with trampoline.S:early_boot_opts label! */ +typedef struct __packed { + bool_t 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; + +/* + * 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"; + +/* + * static const char *delim_chars = &delim_chars_comma[1]; + * + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21), + * put &delim_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. + */ + +static size_t strlen(const char *s) +{ + const char *sc; + + for ( sc = s; *sc != '\0'; ++sc ) + /* nothing */; + return sc - s; +} + +static int strncmp(const char *cs, const char *ct, size_t count) +{ + unsigned char c1, c2; + + while ( count ) + { + c1 = *cs++; + c2 = *ct++; + if ( c1 != c2 ) + return c1 < c2 ? -1 : 1; + if ( !c1 ) + break; + count--; + } + return 0; +} + +static char *strchr(const char *s, int c) +{ + for ( ; *s != (char)c; ++s ) + if ( *s == '\0' ) + return NULL; + return (char *)s; +} + +static size_t strspn(const char *s, const char *accept) +{ + const char *p; + const char *a; + size_t count = 0; + + for ( p = s; *p != '\0'; ++p ) + { + for ( a = accept; *a != '\0'; ++a ) + { + if ( *p == *a ) + break; + } + if ( *a == '\0' ) + return count; + ++count; + } + return count; +} + +static size_t strcspn(const char *s, const char *reject) +{ + const char *p; + const char *r; + size_t count = 0; + + for ( p = s; *p != '\0'; ++p ) + { + for ( r = reject; *r != '\0'; ++r ) + { + if ( *p == *r ) + return count; + } + ++count; + } + return count; +} + +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; + } + + ores = res; + } + + out: + if ( next ) + *next = s; + + return res; +} + +static int strmaxcmp(const char *cs, const char *ct, const char *delim_chars) +{ + return strncmp(cs, ct, max(strcspn(cs, delim_chars), strlen(ct))); +} + +static int strsubcmp(const char *cs, const char *ct) +{ + return strncmp(cs, ct, strlen(ct)); +} + +static const char *find_opt(const char *cmdline, const char *opt, bool_t arg) +{ + size_t lc, lo; + + lo = strlen(opt); + + for ( ; ; ) + { + cmdline += strspn(cmdline, delim_chars); + + if ( *cmdline == '\0' ) + return NULL; + + if ( !strmaxcmp(cmdline, "--", delim_chars) ) + return NULL; + + lc = strcspn(cmdline, delim_chars); + + if ( !strncmp(cmdline, opt, arg ? lo : max(lc, lo)) ) + return cmdline + lo; + + cmdline += lc; + } +} + +static bool_t skip_realmode(const char *cmdline) +{ + return find_opt(cmdline, "no-real-mode", FALSE) || find_opt(cmdline, "tboot=", TRUE); +} + +static u8 edd_parse(const char *cmdline) +{ + const char *c; + + c = find_opt(cmdline, "edd=", TRUE); + + if ( !c ) + return 0; + + if ( !strmaxcmp(c, "off", delim_chars) ) + return 2; + + return !strmaxcmp(c, "skipmbr", delim_chars); +} + +static u8 edid_parse(const char *cmdline) +{ + const char *c; + + c = find_opt(cmdline, "edid=", TRUE); + + if ( !c ) + return 0; + + if ( !strmaxcmp(c, "force", delim_chars) ) + return 2; + + return !strmaxcmp(c, "no", delim_chars); +} + +static u16 rows2vmode(unsigned int rows) +{ + switch ( rows ) + { + case 25: + return VIDEO_80x25; + + case 28: + return VIDEO_80x28; + + case 30: + return VIDEO_80x30; + + case 34: + return VIDEO_80x34; + + case 43: + return VIDEO_80x43; + + case 50: + return VIDEO_80x50; + + case 60: + return VIDEO_80x60; + + default: + return ASK_VGA; + } +} + +static void vga_parse(const char *cmdline, early_boot_opts_t *ebo) +{ + const char *c; + unsigned int tmp, vesa_depth, vesa_height, vesa_width; + + c = find_opt(cmdline, "vga=", TRUE); + + 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("text-80x"); + ebo->boot_vid_mode = rows2vmode(strtoui(c, delim_chars_comma, NULL)); + } + else if ( !strsubcmp(c, "gfx-") ) + { + vesa_width = strtoui(c + strlen("gfx-"), "x", &c); + + if ( vesa_width > U16_MAX ) + return; + + /* + * Increment c outside of strtoui() because otherwise some + * compiler may complain with following message: + * warning: operation on 'c' may be undefined. + */ + ++c; + vesa_height = strtoui(c, "x", &c); + + if ( vesa_height > U16_MAX ) + return; + + vesa_depth = strtoui(++c, delim_chars_comma, NULL); + + if ( vesa_depth > U16_MAX ) + return; + + ebo->vesa_width = vesa_width; + ebo->vesa_height = vesa_height; + ebo->vesa_depth = vesa_depth; + ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE; + } + else if ( !strsubcmp(c, "mode-") ) + { + tmp = strtoui(c + strlen("mode-"), delim_chars_comma, NULL); + + if ( tmp > U16_MAX ) + return; + + ebo->boot_vid_mode = tmp; + } +} + +void __stdcall cmdline_parse_early(const char *cmdline, early_boot_opts_t *ebo) +{ + if ( !cmdline ) + return; + + ebo->skip_realmode = skip_realmode(cmdline); + ebo->opt_edd = edd_parse(cmdline); + ebo->opt_edid = edid_parse(cmdline); + vga_parse(cmdline, ebo); +} diff --git a/xen/arch/x86/boot/edd.S b/xen/arch/x86/boot/edd.S index 5c80da6..73371f9 100644 --- a/xen/arch/x86/boot/edd.S +++ b/xen/arch/x86/boot/edd.S @@ -142,9 +142,6 @@ edd_next: edd_done: ret -opt_edd: - .byte 0 # edd=on/off/skipmbr - GLOBAL(boot_edd_info_nr) .byte 0 GLOBAL(boot_mbr_signature_nr) diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index aca5370..b832b21 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -496,6 +496,13 @@ trampoline_setup: cmpb $0,sym_phys(skip_realmode) jnz 1f + /* Bail if there is no command line to parse. */ + mov sym_phys(multiboot_ptr),%ebx + testl $MBI_CMDLINE,MB_flags(%ebx) + jz 1f + + pushl $sym_phys(early_boot_opts) + pushl MB_cmdline(%ebx) call cmdline_parse_early 1: @@ -514,6 +521,7 @@ trampoline_setup: /* Jump into the relocated trampoline. */ lret +cmdline_parse_early: #include "cmdline.S" reloc: diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S index b013614..8a32728 100644 --- 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 */ GLOBAL(kbd_shift_flags) .byte 0 diff --git a/xen/arch/x86/boot/video.S b/xen/arch/x86/boot/video.S index 2aafbeb..335a51c 100644 --- a/xen/arch/x86/boot/video.S +++ b/xen/arch/x86/boot/video.S @@ -945,7 +945,6 @@ store_edid: #endif ret -opt_edid: .byte 0 # EDID parsing option (force/no/default) mt_end: .word 0 # End of video mode table if built edit_buf: .space 6 # Line editor buffer card_name: .word 0 # Pointer to adapter name @@ -991,12 +990,6 @@ name_bann: .asciz "Video adapter: " force_size: .word 0 # Use this size instead of the one in BIOS vars -vesa_size: .word 0,0,0 # width x depth x height - -/* If we don't run at all, assume basic video mode 3 at 80x25. */ - .align 2 -GLOBAL(boot_vid_mode) - .word VIDEO_80x25 GLOBAL(boot_vid_info) .byte 0, 0 /* orig_x, orig_y */ .byte 3 /* text mode 3 */
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 <andrew.cooper3@citrix.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- 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