* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C @ 2015-08-16 13:48 George Diamantopoulos 2015-08-16 17:22 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 13+ messages in thread From: George Diamantopoulos @ 2015-08-16 13:48 UTC (permalink / raw) To: xen-devel [-- Attachment #1.1: Type: text/plain, Size: 664 bytes --] Hello, I've been trying to compile xen with multiboot2 support, but building has been failing. Tried with both gcc-4.8.4 and gcc-5.2 on ivybridge and amdfam10h, same results. The error I'm getting is: Error: non-empty .rodata: 0x090 build32.mk:22: recipe for target 'cmdline.o' failed Cummulative multiboot2 patch applied on xen-unstable: http://pastebin.com/LCzT8CbR Other patch: http://pastebin.com/y7LzZ6SR Build log: http://pastebin.com/aEUfJkrf xen compiles cleanly without the multiboot2 patches, so I thought I'd post here. I hope this is a real issue and not an error on my part. If it is, I apologise in advance for tainting this thread. BR, George [-- Attachment #1.2: Type: text/html, Size: 991 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-08-16 13:48 [PATCH v2 21/23] x86/boot: implement early command line parser in C George Diamantopoulos @ 2015-08-16 17:22 ` Konrad Rzeszutek Wilk 2015-08-17 20:37 ` Daniel Kiper 0 siblings, 1 reply; 13+ messages in thread From: Konrad Rzeszutek Wilk @ 2015-08-16 17:22 UTC (permalink / raw) To: George Diamantopoulos, xen-devel, Daniel Kiper On August 16, 2015 9:48:05 AM EDT, George Diamantopoulos <georgediam@gmail.com> wrote: >Hello, > >I've been trying to compile xen with multiboot2 support, but building >has >been failing. Tried with both gcc-4.8.4 and gcc-5.2 on ivybridge and >amdfam10h, same results. > CCing Daniel who has been troubleshooting that. It is compiler tools related and with older compilers it works. >The error I'm getting is: > >Error: non-empty .rodata: 0x090 >build32.mk:22: recipe for target 'cmdline.o' failed > >Cummulative multiboot2 patch applied on xen-unstable: >http://pastebin.com/LCzT8CbR >Other patch: http://pastebin.com/y7LzZ6SR >Build log: http://pastebin.com/aEUfJkrf > >xen compiles cleanly without the multiboot2 patches, so I thought I'd >post >here. I hope this is a real issue and not an error on my part. If it >is, I >apologise in advance for tainting this thread. > >BR, >George > > >------------------------------------------------------------------------ > >_______________________________________________ >Xen-devel mailing list >Xen-devel@lists.xen.org >http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-08-16 17:22 ` Konrad Rzeszutek Wilk @ 2015-08-17 20:37 ` Daniel Kiper 0 siblings, 0 replies; 13+ messages in thread From: Daniel Kiper @ 2015-08-17 20:37 UTC (permalink / raw) To: Konrad Rzeszutek Wilk; +Cc: xen-devel, George Diamantopoulos On Sun, Aug 16, 2015 at 01:22:05PM -0400, Konrad Rzeszutek Wilk wrote: > On August 16, 2015 9:48:05 AM EDT, George Diamantopoulos <georgediam@gmail.com> wrote: > >Hello, > > > >I've been trying to compile xen with multiboot2 support, but building > >has > >been failing. Tried with both gcc-4.8.4 and gcc-5.2 on ivybridge and > >amdfam10h, same results. > > > > CCing Daniel who has been troubleshooting that. It is compiler tools related and with older compilers it works. New GCC calculates jump address for C switch/case using .rodata section. The simplest solution/workaround seems change to series of ifs but not nice. I must think about better fix for this issue. Daniel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 00/23] x86: multiboot2 protocol support @ 2015-07-20 14:28 Daniel Kiper 2015-07-20 14:29 ` [PATCH v2 21/23] x86/boot: implement early command line parser in C Daniel Kiper 2015-07-20 14:29 ` Daniel Kiper 0 siblings, 2 replies; 13+ messages in thread From: Daniel Kiper @ 2015-07-20 14:28 UTC (permalink / raw) To: xen-devel, grub-devel Cc: jgross, keir, ian.campbell, andrew.cooper3, stefano.stabellini, roy.franz, ning.sun, david.vrabel, jbeulich, phcoder, wei.liu2, qiaowei.ren, richard.l.maliszewski, gang.wei, fu.wei Hi, I am sending, long awaited, second version of multiboot2 protocol support for legacy BIOS and EFI platforms. It fixes all major issues discovered until now. There are still some minor problems which should be fixed in one way or another. I will address them in next releases. This series, in general, is not targeted to Xen 4.6. However, there are some fixes at the beginning of it which are worth considering, I think. The final goal is xen.efi binary file which could be loaded by EFI loader, multiboot (v1) protocol (only on legacy BIOS platforms) and multiboot2 protocol. This way we will have: - smaller Xen code base, - one code base for xen.gz and xen.efi, - one build method for xen.gz and xen.efi; xen.efi will be extracted from xen file using objcopy; PE header will be contained in ELF file and will precede Xen code, - xen.efi build will not so strongly depend on a given GCC and binutils version. ARM guys should check at least patches #9 - #18 and #20. In general earlier mentioned patches touches common EFI code but they are not change functionality significantly. GRUB2 patch series will follow this patch series. GRUB2 guys should check patches #20 and #23 but I am sending to you all Xen related patches just in case. If you are not interested in this patch series at all please drop me a line and I will remove you from distribution list. Daniel .gitignore | 5 +- xen/arch/x86/Makefile | 21 ++-- xen/arch/x86/Rules.mk | 4 + xen/arch/x86/boot/Makefile | 10 +- xen/arch/x86/boot/build32.mk | 4 +- xen/arch/x86/boot/cmdline.S | 367 --------------------------------------------------------------- xen/arch/x86/boot/cmdline.c | 396 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/boot/edd.S | 3 - xen/arch/x86/boot/head.S | 474 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------- xen/arch/x86/boot/reloc.c | 242 +++++++++++++++++++++++++++++++++++------- xen/arch/x86/boot/trampoline.S | 25 ++++- xen/arch/x86/boot/video.S | 6 -- xen/arch/x86/boot/wakeup.S | 6 +- xen/arch/x86/boot/x86_64.S | 34 +++--- xen/arch/x86/dmi_scan.c | 4 +- xen/arch/x86/domain_page.c | 2 +- xen/arch/x86/efi/Makefile | 16 +-- xen/arch/x86/efi/efi-boot.h | 68 ++++++++++-- xen/arch/x86/efi/stub.c | 16 ++- xen/arch/x86/mm.c | 3 +- xen/arch/x86/mpparse.c | 4 +- xen/arch/x86/setup.c | 50 ++++----- xen/arch/x86/shutdown.c | 2 +- xen/arch/x86/time.c | 2 +- xen/arch/x86/x86_64/asm-offsets.c | 10 ++ xen/arch/x86/x86_64/mm.c | 2 +- xen/arch/x86/xen.lds.S | 6 +- xen/common/efi/boot.c | 461 ++++++++++++++++++++++++++++++++++++++++++++++--------------------------------- xen/common/efi/runtime.c | 23 ++-- xen/drivers/acpi/osl.c | 2 +- xen/include/asm-x86/config.h | 3 + xen/include/asm-x86/page.h | 2 +- xen/include/xen/efi.h | 17 ++- xen/include/xen/multiboot2.h | 182 ++++++++++++++++++++++++++++++++ 34 files changed, 1709 insertions(+), 763 deletions(-) Daniel Kiper (23): x86/boot: remove unneeded instruction x86/boot: copy only text section from *.lnk file to *.bin file x86: zero BSS using stosl instead of stosb x86/boot: call reloc() using cdecl calling convention x86/boot/reloc: create generic alloc and copy functions x86/boot: use %ecx instead of %eax x86/boot/reloc: Rename some variables and rearrange code a bit x86: add multiboot2 protocol support efi: create efi_enabled() efi: build xen.gz with EFI code efi: split out efi_init() efi: split out efi_console_set_mode() efi: split out efi_get_gop() efi: split out efi_find_gop_mode() efi: split out efi_tables() efi: split out efi_variables() efi: split out efi_set_gop_mode() efi: split out efi_exit_boot() x86/efi: create new early memory allocator x86: add multiboot2 protocol support for EFI platforms x86/boot: implement early command line parser in C x86: make Xen early boot code relocatable x86: add multiboot2 protocol support for relocatable images ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-07-20 14:28 [PATCH v2 00/23] x86: multiboot2 protocol support Daniel Kiper @ 2015-07-20 14:29 ` Daniel Kiper 2015-07-20 14:29 ` Daniel Kiper 1 sibling, 0 replies; 13+ messages in thread From: Daniel Kiper @ 2015-07-20 14:29 UTC (permalink / raw) To: xen-devel, grub-devel Cc: jgross, keir, ian.campbell, andrew.cooper3, stefano.stabellini, roy.franz, ning.sun, david.vrabel, jbeulich, phcoder, wei.liu2, qiaowei.ren, richard.l.maliszewski, gang.wei, fu.wei 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. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- .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 | 396 ++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/boot/edd.S | 3 - xen/arch/x86/boot/head.S | 17 ++ xen/arch/x86/boot/trampoline.S | 14 ++ xen/arch/x86/boot/video.S | 6 - 10 files changed, 439 insertions(+), 380 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 f6ddb00..e0618b9 100644 --- a/.gitignore +++ b/.gitignore @@ -223,9 +223,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 0335445..82c5a93 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -170,4 +170,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.mk b/xen/arch/x86/boot/build32.mk index c83effe..d681643 100644 --- a/xen/arch/x86/boot/build32.mk +++ b/xen/arch/x86/boot/build32.mk @@ -30,6 +30,8 @@ CFLAGS := $(filter-out -flto,$(CFLAGS)) esac; \ done +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..5ea50a4 --- /dev/null +++ b/xen/arch/x86/boot/cmdline.c @@ -0,0 +1,396 @@ +/* + * Copyright (c) 2015 Oracle Co. + * Daniel Kiper <daniel.kiper@oracle.com> + * + * 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(), 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 VESA_WIDTH 0 +#define VESA_HEIGHT 1 +#define VESA_DEPTH 2 + +#define VESA_SIZE 3 + +#define NULL ((void *)0) + +#define __cdecl __attribute__((__cdecl__)) +#define __packed __attribute__((__packed__)) +#define __text __attribute__((__section__(".text"))) +#define __used __attribute__((__used__)) + +#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) + +#define strlen_static(s) (sizeof(s) - 1) + +typedef unsigned char u8; +typedef unsigned short u16; +typedef unsigned int size_t; + +#define U16_MAX ((u16)(~0U)) + +/* + * 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_size[VESA_SIZE]; +} early_boot_opts_t; + +static const char empty_chars[] __text = " \n\r\t"; + +/** + * strlen - Find the length of a string + * @s: The string to be sized + */ +static size_t strlen(const char *s) +{ + const char *sc; + + for ( sc = s; *sc != '\0'; ++sc ) + /* nothing */; + return sc - s; +} + +/** + * strncmp - Compare two length-limited strings + * @cs: One string + * @ct: Another string + * @count: The maximum number of bytes to compare + */ +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; +} + +/** + * strspn - Calculate the length of the initial substring of @s which only contain letters in @accept + * @s: The string to be searched + * @accept: The string to search for + */ +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; +} + +/** + * strcspn - Calculate the length of the initial substring of @s which does not contain letters in @reject + * @s: The string to be searched + * @reject: The string to avoid + */ +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 int strtoi(const char *s, const char *stop, const char **next) +{ + int base = 10, i, ores = 0, res = 0; + + if ( *s == '0' ) + base = (tolower(*++s) == 'x') ? (++s, 16) : 8; + + for ( ; *s != '\0'; ++s ) + { + for ( i = 0; stop && stop[i] != '\0'; ++i ) + if ( *s == stop[i] ) + goto out; + + if ( *s < '0' || (*s > '7' && base == 8) ) + { + res = -1; + goto out; + } + + if ( *s > '9' && (base != 16 || tolower(*s) < 'a' || tolower(*s) > 'f') ) + { + res = -1; + goto out; + } + + res *= base; + res += (tolower(*s) >= 'a') ? (tolower(*s) - 'a' + 10) : (*s - '0'); + + if ( ores > res ) + { + res = -1; + goto out; + } + + ores = res; + } + +out: + if ( next ) + *next = s; + + return res; +} + +static const char *find_opt(const char *cmdline, const char *opt, int arg) +{ + size_t lc, lo; + static const char mm[] __text = "--"; + + lo = strlen(opt); + + for ( ; ; ) + { + cmdline += strspn(cmdline, empty_chars); + + if ( *cmdline == '\0' ) + return NULL; + + lc = strcspn(cmdline, empty_chars); + + if ( !strncmp(cmdline, mm, max(lc, strlen_static(mm))) ) + return NULL; + + if ( !strncmp(cmdline, opt, arg ? lo : max(lc, lo)) ) + return cmdline; + + cmdline += lc; + } +} + +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; +} + +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))) ) + return 1; + + return 0; +} + +static u8 edid_parse(const char *cmdline) +{ + const char *c; + size_t la; + static const char edid[] __text = "edid="; + static const char edid_force[] __text = "force"; + static const char edid_no[] __text = "no"; + + c = find_opt(cmdline, edid, 1); + + if ( !c ) + return 0; + + c += strlen_static(edid); + la = strcspn(c, empty_chars); + + if ( !strncmp(c, edid_no, max(la, strlen_static(edid_no))) ) + return 1; + else if ( !strncmp(c, edid_force, max(la, strlen_static(edid_force))) ) + return 2; + + return 0; +} + +static u16 rows2vmode(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; + int tmp; + size_t la; + static const char empty_chars_comma[] __text = " \n\r\t,"; + static const char x[] __text = "x"; + static const char vga[] __text = "vga="; + static const char vga_current[] __text = "current"; + static const char vga_gfx[] __text = "gfx-"; + static const char vga_mode[] __text = "mode-"; + static const char vga_text_80x[] __text = "text-80x"; + + c = find_opt(cmdline, vga, 1); + + if ( !c ) + return; + + ebo->boot_vid_mode = ASK_VGA; + + c += strlen_static(vga); + la = strcspn(c, empty_chars_comma); + + if ( !strncmp(c, vga_current, max(la, strlen_static(vga_current))) ) + ebo->boot_vid_mode = VIDEO_CURRENT_MODE; + else if ( !strncmp(c, vga_text_80x, strlen_static(vga_text_80x)) ) + { + c += strlen_static(vga_text_80x); + ebo->boot_vid_mode = rows2vmode(strtoi(c, empty_chars_comma, NULL)); + } + else if ( !strncmp(c, vga_gfx, strlen_static(vga_gfx)) ) + { + tmp = strtoi(c + strlen_static(vga_gfx), x, &c); + + if ( tmp < 0 || tmp > U16_MAX ) + return; + + ebo->vesa_size[VESA_WIDTH] = tmp; + + tmp = strtoi(++c, x, &c); + + if ( tmp < 0 || tmp > U16_MAX ) + return; + + ebo->vesa_size[VESA_HEIGHT] = tmp; + + tmp = strtoi(++c, empty_chars_comma, NULL); + + if ( tmp < 0 || tmp > U16_MAX ) + return; + + ebo->vesa_size[VESA_DEPTH] = tmp; + + ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE; + } + else if ( !strncmp(c, vga_mode, strlen_static(vga_mode)) ) + { + tmp = strtoi(c + strlen_static(vga_mode), empty_chars_comma, NULL); + + if ( tmp < 0 || tmp > U16_MAX ) + return; + + ebo->boot_vid_mode = tmp; + } +} + +static void __cdecl __used cmdline_parse_early(const char *cmdline, early_boot_opts_t *ebo) +{ + 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 056047f..3f1054d 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -429,8 +429,24 @@ trampoline_setup: cmp $sym_phys(__trampoline_seg_stop),%edi jb 1b + /* Do not parse command line on EFI platform here. */ + cmpb $1,sym_phys(skip_realmode) + je 1f + + /* Bail if there is no command line to parse. */ + mov sym_phys(multiboot_ptr),%ebx + testl $MBI_CMDLINE,MB_flags(%ebx) + jz 1f + + cmpl $0,MB_cmdline(%ebx) + jz 1f + + pushl $sym_phys(early_boot_opts) + pushl MB_cmdline(%ebx) call cmdline_parse_early + add $8,%esp /* Remove cmdline_parse_early() args from stack. */ +1: /* Switch to low-memory stack. */ mov sym_phys(trampoline_phys),%edi lea 0x10000(%edi),%esp @@ -446,6 +462,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 ccb40fb..3c2714d 100644 --- a/xen/arch/x86/boot/trampoline.S +++ b/xen/arch/x86/boot/trampoline.S @@ -1,3 +1,5 @@ +#include "video.h" + .code16 /* NB. bootsym() is only usable in real mode, or via BOOT_PSEUDORM_DS. */ @@ -201,8 +203,20 @@ trampoline_boot_cpu_entry: /* Jump to the common bootstrap entry point. */ jmp trampoline_protmode_entry +/* + * 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 b238bf3..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,11 +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. */ -GLOBAL(boot_vid_mode) - .word VIDEO_80x25 GLOBAL(boot_vid_info) .byte 0, 0 /* orig_x, orig_y */ .byte 3 /* text mode 3 */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-07-20 14:28 [PATCH v2 00/23] x86: multiboot2 protocol support Daniel Kiper 2015-07-20 14:29 ` [PATCH v2 21/23] x86/boot: implement early command line parser in C Daniel Kiper @ 2015-07-20 14:29 ` Daniel Kiper 2015-08-10 20:31 ` Konrad Rzeszutek Wilk ` (3 more replies) 1 sibling, 4 replies; 13+ messages in thread From: Daniel Kiper @ 2015-07-20 14:29 UTC (permalink / raw) To: xen-devel, grub-devel Cc: jgross, keir, ian.campbell, andrew.cooper3, stefano.stabellini, roy.franz, ning.sun, david.vrabel, jbeulich, phcoder, wei.liu2, qiaowei.ren, richard.l.maliszewski, gang.wei, fu.wei 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. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- .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 | 396 ++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/boot/edd.S | 3 - xen/arch/x86/boot/head.S | 17 ++ xen/arch/x86/boot/trampoline.S | 14 ++ xen/arch/x86/boot/video.S | 6 - 10 files changed, 439 insertions(+), 380 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 f6ddb00..e0618b9 100644 --- a/.gitignore +++ b/.gitignore @@ -223,9 +223,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 0335445..82c5a93 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -170,4 +170,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.mk b/xen/arch/x86/boot/build32.mk index c83effe..d681643 100644 --- a/xen/arch/x86/boot/build32.mk +++ b/xen/arch/x86/boot/build32.mk @@ -30,6 +30,8 @@ CFLAGS := $(filter-out -flto,$(CFLAGS)) esac; \ done +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..5ea50a4 --- /dev/null +++ b/xen/arch/x86/boot/cmdline.c @@ -0,0 +1,396 @@ +/* + * Copyright (c) 2015 Oracle Co. + * Daniel Kiper <daniel.kiper@oracle.com> + * + * 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(), 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 VESA_WIDTH 0 +#define VESA_HEIGHT 1 +#define VESA_DEPTH 2 + +#define VESA_SIZE 3 + +#define NULL ((void *)0) + +#define __cdecl __attribute__((__cdecl__)) +#define __packed __attribute__((__packed__)) +#define __text __attribute__((__section__(".text"))) +#define __used __attribute__((__used__)) + +#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) + +#define strlen_static(s) (sizeof(s) - 1) + +typedef unsigned char u8; +typedef unsigned short u16; +typedef unsigned int size_t; + +#define U16_MAX ((u16)(~0U)) + +/* + * 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_size[VESA_SIZE]; +} early_boot_opts_t; + +static const char empty_chars[] __text = " \n\r\t"; + +/** + * strlen - Find the length of a string + * @s: The string to be sized + */ +static size_t strlen(const char *s) +{ + const char *sc; + + for ( sc = s; *sc != '\0'; ++sc ) + /* nothing */; + return sc - s; +} + +/** + * strncmp - Compare two length-limited strings + * @cs: One string + * @ct: Another string + * @count: The maximum number of bytes to compare + */ +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; +} + +/** + * strspn - Calculate the length of the initial substring of @s which only contain letters in @accept + * @s: The string to be searched + * @accept: The string to search for + */ +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; +} + +/** + * strcspn - Calculate the length of the initial substring of @s which does not contain letters in @reject + * @s: The string to be searched + * @reject: The string to avoid + */ +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 int strtoi(const char *s, const char *stop, const char **next) +{ + int base = 10, i, ores = 0, res = 0; + + if ( *s == '0' ) + base = (tolower(*++s) == 'x') ? (++s, 16) : 8; + + for ( ; *s != '\0'; ++s ) + { + for ( i = 0; stop && stop[i] != '\0'; ++i ) + if ( *s == stop[i] ) + goto out; + + if ( *s < '0' || (*s > '7' && base == 8) ) + { + res = -1; + goto out; + } + + if ( *s > '9' && (base != 16 || tolower(*s) < 'a' || tolower(*s) > 'f') ) + { + res = -1; + goto out; + } + + res *= base; + res += (tolower(*s) >= 'a') ? (tolower(*s) - 'a' + 10) : (*s - '0'); + + if ( ores > res ) + { + res = -1; + goto out; + } + + ores = res; + } + +out: + if ( next ) + *next = s; + + return res; +} + +static const char *find_opt(const char *cmdline, const char *opt, int arg) +{ + size_t lc, lo; + static const char mm[] __text = "--"; + + lo = strlen(opt); + + for ( ; ; ) + { + cmdline += strspn(cmdline, empty_chars); + + if ( *cmdline == '\0' ) + return NULL; + + lc = strcspn(cmdline, empty_chars); + + if ( !strncmp(cmdline, mm, max(lc, strlen_static(mm))) ) + return NULL; + + if ( !strncmp(cmdline, opt, arg ? lo : max(lc, lo)) ) + return cmdline; + + cmdline += lc; + } +} + +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; +} + +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))) ) + return 1; + + return 0; +} + +static u8 edid_parse(const char *cmdline) +{ + const char *c; + size_t la; + static const char edid[] __text = "edid="; + static const char edid_force[] __text = "force"; + static const char edid_no[] __text = "no"; + + c = find_opt(cmdline, edid, 1); + + if ( !c ) + return 0; + + c += strlen_static(edid); + la = strcspn(c, empty_chars); + + if ( !strncmp(c, edid_no, max(la, strlen_static(edid_no))) ) + return 1; + else if ( !strncmp(c, edid_force, max(la, strlen_static(edid_force))) ) + return 2; + + return 0; +} + +static u16 rows2vmode(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; + int tmp; + size_t la; + static const char empty_chars_comma[] __text = " \n\r\t,"; + static const char x[] __text = "x"; + static const char vga[] __text = "vga="; + static const char vga_current[] __text = "current"; + static const char vga_gfx[] __text = "gfx-"; + static const char vga_mode[] __text = "mode-"; + static const char vga_text_80x[] __text = "text-80x"; + + c = find_opt(cmdline, vga, 1); + + if ( !c ) + return; + + ebo->boot_vid_mode = ASK_VGA; + + c += strlen_static(vga); + la = strcspn(c, empty_chars_comma); + + if ( !strncmp(c, vga_current, max(la, strlen_static(vga_current))) ) + ebo->boot_vid_mode = VIDEO_CURRENT_MODE; + else if ( !strncmp(c, vga_text_80x, strlen_static(vga_text_80x)) ) + { + c += strlen_static(vga_text_80x); + ebo->boot_vid_mode = rows2vmode(strtoi(c, empty_chars_comma, NULL)); + } + else if ( !strncmp(c, vga_gfx, strlen_static(vga_gfx)) ) + { + tmp = strtoi(c + strlen_static(vga_gfx), x, &c); + + if ( tmp < 0 || tmp > U16_MAX ) + return; + + ebo->vesa_size[VESA_WIDTH] = tmp; + + tmp = strtoi(++c, x, &c); + + if ( tmp < 0 || tmp > U16_MAX ) + return; + + ebo->vesa_size[VESA_HEIGHT] = tmp; + + tmp = strtoi(++c, empty_chars_comma, NULL); + + if ( tmp < 0 || tmp > U16_MAX ) + return; + + ebo->vesa_size[VESA_DEPTH] = tmp; + + ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE; + } + else if ( !strncmp(c, vga_mode, strlen_static(vga_mode)) ) + { + tmp = strtoi(c + strlen_static(vga_mode), empty_chars_comma, NULL); + + if ( tmp < 0 || tmp > U16_MAX ) + return; + + ebo->boot_vid_mode = tmp; + } +} + +static void __cdecl __used cmdline_parse_early(const char *cmdline, early_boot_opts_t *ebo) +{ + 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 056047f..3f1054d 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -429,8 +429,24 @@ trampoline_setup: cmp $sym_phys(__trampoline_seg_stop),%edi jb 1b + /* Do not parse command line on EFI platform here. */ + cmpb $1,sym_phys(skip_realmode) + je 1f + + /* Bail if there is no command line to parse. */ + mov sym_phys(multiboot_ptr),%ebx + testl $MBI_CMDLINE,MB_flags(%ebx) + jz 1f + + cmpl $0,MB_cmdline(%ebx) + jz 1f + + pushl $sym_phys(early_boot_opts) + pushl MB_cmdline(%ebx) call cmdline_parse_early + add $8,%esp /* Remove cmdline_parse_early() args from stack. */ +1: /* Switch to low-memory stack. */ mov sym_phys(trampoline_phys),%edi lea 0x10000(%edi),%esp @@ -446,6 +462,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 ccb40fb..3c2714d 100644 --- a/xen/arch/x86/boot/trampoline.S +++ b/xen/arch/x86/boot/trampoline.S @@ -1,3 +1,5 @@ +#include "video.h" + .code16 /* NB. bootsym() is only usable in real mode, or via BOOT_PSEUDORM_DS. */ @@ -201,8 +203,20 @@ trampoline_boot_cpu_entry: /* Jump to the common bootstrap entry point. */ jmp trampoline_protmode_entry +/* + * 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 b238bf3..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,11 +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. */ -GLOBAL(boot_vid_mode) - .word VIDEO_80x25 GLOBAL(boot_vid_info) .byte 0, 0 /* orig_x, orig_y */ .byte 3 /* text mode 3 */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-07-20 14:29 ` Daniel Kiper @ 2015-08-10 20:31 ` Konrad Rzeszutek Wilk 2015-08-11 14:43 ` Konrad Rzeszutek Wilk ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: Konrad Rzeszutek Wilk @ 2015-08-10 20:31 UTC (permalink / raw) To: Daniel Kiper Cc: jgross, grub-devel, keir, ian.campbell, stefano.stabellini, andrew.cooper3, gang.wei, roy.franz, ning.sun, david.vrabel, jbeulich, phcoder, xen-devel, wei.liu2, richard.l.maliszewski, qiaowei.ren, fu.wei On Mon, Jul 20, 2015 at 04:29:16PM +0200, Daniel Kiper 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. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> I did not look at the str* functions that were added in but just at how the parameters parsing was done. Also at the assembler code and with that Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> .. snip.. > diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c > new file mode 100644 > index 0000000..5ea50a4 > --- /dev/null > +++ b/xen/arch/x86/boot/cmdline.c > @@ -0,0 +1,396 @@ > +/* > + * Copyright (c) 2015 Oracle Co. Oracle Corporation. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-07-20 14:29 ` Daniel Kiper 2015-08-10 20:31 ` Konrad Rzeszutek Wilk @ 2015-08-11 14:43 ` Konrad Rzeszutek Wilk 2015-08-27 12:43 ` Jan Beulich 2015-08-27 12:43 ` Jan Beulich 3 siblings, 0 replies; 13+ messages in thread From: Konrad Rzeszutek Wilk @ 2015-08-11 14:43 UTC (permalink / raw) To: Daniel Kiper Cc: jgross, grub-devel, keir, ian.campbell, stefano.stabellini, andrew.cooper3, gang.wei, roy.franz, ning.sun, david.vrabel, jbeulich, phcoder, xen-devel, wei.liu2, richard.l.maliszewski, qiaowei.ren, fu.wei > +static void vga_parse(const char *cmdline, early_boot_opts_t *ebo) > +{ > + const char *c; > + int tmp; > + size_t la; > + static const char empty_chars_comma[] __text = " \n\r\t,"; > + static const char x[] __text = "x"; > + static const char vga[] __text = "vga="; > + static const char vga_current[] __text = "current"; > + static const char vga_gfx[] __text = "gfx-"; > + static const char vga_mode[] __text = "mode-"; > + static const char vga_text_80x[] __text = "text-80x"; > + > + c = find_opt(cmdline, vga, 1); > + > + if ( !c ) > + return; > + > + ebo->boot_vid_mode = ASK_VGA; > + > + c += strlen_static(vga); > + la = strcspn(c, empty_chars_comma); > + > + if ( !strncmp(c, vga_current, max(la, strlen_static(vga_current))) ) > + ebo->boot_vid_mode = VIDEO_CURRENT_MODE; > + else if ( !strncmp(c, vga_text_80x, strlen_static(vga_text_80x)) ) > + { > + c += strlen_static(vga_text_80x); > + ebo->boot_vid_mode = rows2vmode(strtoi(c, empty_chars_comma, NULL)); > + } > + else if ( !strncmp(c, vga_gfx, strlen_static(vga_gfx)) ) > + { > + tmp = strtoi(c + strlen_static(vga_gfx), x, &c); > + > + if ( tmp < 0 || tmp > U16_MAX ) > + return; > + > + ebo->vesa_size[VESA_WIDTH] = tmp; > + > + tmp = strtoi(++c, x, &c); My ancient OL5 installation: Choked on that with: gcc -O1 -fno-omit-frame-pointer -m32 -march=i686 -g -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -fno-stack-protector -fno-exceptions -Werror -fno-builtin -msoft-float -c -fpic cmdline.c -o cmdline.o cc1: warnings being treated as errors cmdline.c: In function ‘vga_parse’: cmdline.c:363: warning: operation on ‘c’ may be undefined Moving the ++c before the tmp assigment make the compiler happy. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-07-20 14:29 ` Daniel Kiper 2015-08-10 20:31 ` Konrad Rzeszutek Wilk 2015-08-11 14:43 ` Konrad Rzeszutek Wilk @ 2015-08-27 12:43 ` Jan Beulich 2015-08-27 12:43 ` Jan Beulich 3 siblings, 0 replies; 13+ messages in thread From: Jan Beulich @ 2015-08-27 12:43 UTC (permalink / raw) To: Daniel Kiper Cc: Juergen Gross, grub-devel, wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3, roy.franz, ning.sun, david.vrabel, phcoder, xen-devel, qiaowei.ren, keir, richard.l.maliszewski, gang.wei, fu.wei >>> On 20.07.15 at 16:29, <daniel.kiper@oracle.com> wrote: > Current early command line parser implementation in assembler > is very difficult to change to relocatable stuff using segment > registers. This requires a lot of changes in very weird and > fragile code. So, reimplement this functionality in C. This > way code will be relocatable out of the box and much easier > to maintain. All appreciated and nice, but the goal of making the code relocatable by playing with segment registers sounds fragile: This breaks assumptions the compiler may validly make. > xen/arch/x86/boot/cmdline.S | 367 ------------------------------------- > xen/arch/x86/boot/cmdline.c | 396 ++++++++++++++++++++++++++++++++++++++++ A fundamental expectation I would have had is for the C file to be noticeably smaller than the assembly file. > --- /dev/null > +++ b/xen/arch/x86/boot/cmdline.c >[...] > +#define VESA_WIDTH 0 > +#define VESA_HEIGHT 1 > +#define VESA_DEPTH 2 > + > +#define VESA_SIZE 3 These should go away in favor of using individual (sub)structure fields. > +#define __cdecl __attribute__((__cdecl__)) ??? > +#define __packed __attribute__((__packed__)) > +#define __text __attribute__((__section__(".text"))) > +#define __used __attribute__((__used__)) Likely better to include compiler.h instead. > +#define max(x,y) ({ \ > + const typeof(x) _x = (x); \ > + const typeof(y) _y = (y); \ > + (void) (&_x == &_y); \ > + _x > _y ? _x : _y; }) I also wonder whether -imacros .../xen/kernel.h wouldn't be a better approach here. Please really think hard on how to avoid duplications like these. > +#define strlen_static(s) (sizeof(s) - 1) What is this good for? A decent compiler should be able to deal with strlen("..."). Plus your macro is longer that what it tries to "abbreviate". > +static const char empty_chars[] __text = " \n\r\t"; What is empty about them? DYM blank or (white) space or separator or delimiter? I also wonder whether \n and \r are actually usefully here, as they should (if at all) only end the line. > +/** > + * strlen - Find the length of a string > + * @s: The string to be sized > + */ > +static size_t strlen(const char *s) Comments are certainly nice, but in this special case I'd rather suggest against bloating the code by commenting standard library functions. > +static int strtoi(const char *s, const char *stop, const char **next) > +{ > + int base = 10, i, ores = 0, res = 0; > + > + if ( *s == '0' ) > + base = (tolower(*++s) == 'x') ? (++s, 16) : 8; > + > + for ( ; *s != '\0'; ++s ) > + { > + for ( i = 0; stop && stop[i] != '\0'; ++i ) > + if ( *s == stop[i] ) > + goto out; > + > + if ( *s < '0' || (*s > '7' && base == 8) ) > + { > + res = -1; > + goto out; > + } > + > + if ( *s > '9' && (base != 16 || tolower(*s) < 'a' || tolower(*s) > 'f') ) > + { > + res = -1; > + goto out; > + } > + > + res *= base; > + res += (tolower(*s) >= 'a') ? (tolower(*s) - 'a' + 10) : (*s - '0'); > + > + if ( ores > res ) > + { > + res = -1; > + goto out; > + } > + > + ores = res; > + } > + > +out: C labels intended by at least one space please. > +static const char *find_opt(const char *cmdline, const char *opt, int arg) > +{ > + size_t lc, lo; > + static const char mm[] __text = "--"; I'd be surprised if there weren't compiler/assembler versions complaining about a section type conflict here. I can see why you want everything in one section, but I'd rather suggest achieving this at the linking step (which I would suppose to already be taking care of this). > +static u8 skip_realmode(const char *cmdline) > +{ > + static const char nrm[] __text = "no-real-mode"; > + static const char tboot[] __text = "tboot="; > + > + if ( find_opt(cmdline, nrm, 0) || find_opt(cmdline, tboot, 1) ) > + return 1; > + > + return 0; return find_opt(cmdline, nrm, 0) || find_opt(cmdline, tboot, 1); > +static u8 edd_parse(const char *cmdline) > +{ > + const char *c; > + size_t la; > + static const char edd[] __text = "edd="; > + static const char edd_off[] __text = "off"; > + static const char edd_skipmbr[] __text = "skipmbr"; > + > + c = find_opt(cmdline, edd, 1); > + > + if ( !c ) > + return 0; > + > + c += strlen_static(edd); > + la = strcspn(c, empty_chars); > + > + if ( !strncmp(c, edd_off, max(la, strlen_static(edd_off))) ) > + return 2; > + else if ( !strncmp(c, edd_skipmbr, max(la, strlen_static(edd_skipmbr))) ) Pointless else. > + return 1; > + > + return 0; And the last two returns can be folded again anyway. > +static void __cdecl __used cmdline_parse_early(const char *cmdline, early_boot_opts_t *ebo) I don't see the point of the __cdecl, and (as said before) dislike the static __used pair. > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -429,8 +429,24 @@ trampoline_setup: > cmp $sym_phys(__trampoline_seg_stop),%edi > jb 1b > > + /* Do not parse command line on EFI platform here. */ > + cmpb $1,sym_phys(skip_realmode) Is there a reason you can't look at efi.flags instead here (and in the other case you abuse skip_realmode as meaning EFI)? > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -1,3 +1,5 @@ > +#include "video.h" Please move this farther down, making invisible all its definitions to code not supposed to use them. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-07-20 14:29 ` Daniel Kiper ` (2 preceding siblings ...) 2015-08-27 12:43 ` Jan Beulich @ 2015-08-27 12:43 ` Jan Beulich 2015-09-22 17:03 ` Daniel Kiper 2015-09-22 17:03 ` Daniel Kiper 3 siblings, 2 replies; 13+ messages in thread From: Jan Beulich @ 2015-08-27 12:43 UTC (permalink / raw) To: Daniel Kiper Cc: Juergen Gross, grub-devel, wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3, roy.franz, ning.sun, david.vrabel, phcoder, xen-devel, qiaowei.ren, keir, richard.l.maliszewski, gang.wei, fu.wei >>> On 20.07.15 at 16:29, <daniel.kiper@oracle.com> wrote: > Current early command line parser implementation in assembler > is very difficult to change to relocatable stuff using segment > registers. This requires a lot of changes in very weird and > fragile code. So, reimplement this functionality in C. This > way code will be relocatable out of the box and much easier > to maintain. All appreciated and nice, but the goal of making the code relocatable by playing with segment registers sounds fragile: This breaks assumptions the compiler may validly make. > xen/arch/x86/boot/cmdline.S | 367 ------------------------------------- > xen/arch/x86/boot/cmdline.c | 396 ++++++++++++++++++++++++++++++++++++++++ A fundamental expectation I would have had is for the C file to be noticeably smaller than the assembly file. > --- /dev/null > +++ b/xen/arch/x86/boot/cmdline.c >[...] > +#define VESA_WIDTH 0 > +#define VESA_HEIGHT 1 > +#define VESA_DEPTH 2 > + > +#define VESA_SIZE 3 These should go away in favor of using individual (sub)structure fields. > +#define __cdecl __attribute__((__cdecl__)) ??? > +#define __packed __attribute__((__packed__)) > +#define __text __attribute__((__section__(".text"))) > +#define __used __attribute__((__used__)) Likely better to include compiler.h instead. > +#define max(x,y) ({ \ > + const typeof(x) _x = (x); \ > + const typeof(y) _y = (y); \ > + (void) (&_x == &_y); \ > + _x > _y ? _x : _y; }) I also wonder whether -imacros .../xen/kernel.h wouldn't be a better approach here. Please really think hard on how to avoid duplications like these. > +#define strlen_static(s) (sizeof(s) - 1) What is this good for? A decent compiler should be able to deal with strlen("..."). Plus your macro is longer that what it tries to "abbreviate". > +static const char empty_chars[] __text = " \n\r\t"; What is empty about them? DYM blank or (white) space or separator or delimiter? I also wonder whether \n and \r are actually usefully here, as they should (if at all) only end the line. > +/** > + * strlen - Find the length of a string > + * @s: The string to be sized > + */ > +static size_t strlen(const char *s) Comments are certainly nice, but in this special case I'd rather suggest against bloating the code by commenting standard library functions. > +static int strtoi(const char *s, const char *stop, const char **next) > +{ > + int base = 10, i, ores = 0, res = 0; > + > + if ( *s == '0' ) > + base = (tolower(*++s) == 'x') ? (++s, 16) : 8; > + > + for ( ; *s != '\0'; ++s ) > + { > + for ( i = 0; stop && stop[i] != '\0'; ++i ) > + if ( *s == stop[i] ) > + goto out; > + > + if ( *s < '0' || (*s > '7' && base == 8) ) > + { > + res = -1; > + goto out; > + } > + > + if ( *s > '9' && (base != 16 || tolower(*s) < 'a' || tolower(*s) > 'f') ) > + { > + res = -1; > + goto out; > + } > + > + res *= base; > + res += (tolower(*s) >= 'a') ? (tolower(*s) - 'a' + 10) : (*s - '0'); > + > + if ( ores > res ) > + { > + res = -1; > + goto out; > + } > + > + ores = res; > + } > + > +out: C labels intended by at least one space please. > +static const char *find_opt(const char *cmdline, const char *opt, int arg) > +{ > + size_t lc, lo; > + static const char mm[] __text = "--"; I'd be surprised if there weren't compiler/assembler versions complaining about a section type conflict here. I can see why you want everything in one section, but I'd rather suggest achieving this at the linking step (which I would suppose to already be taking care of this). > +static u8 skip_realmode(const char *cmdline) > +{ > + static const char nrm[] __text = "no-real-mode"; > + static const char tboot[] __text = "tboot="; > + > + if ( find_opt(cmdline, nrm, 0) || find_opt(cmdline, tboot, 1) ) > + return 1; > + > + return 0; return find_opt(cmdline, nrm, 0) || find_opt(cmdline, tboot, 1); > +static u8 edd_parse(const char *cmdline) > +{ > + const char *c; > + size_t la; > + static const char edd[] __text = "edd="; > + static const char edd_off[] __text = "off"; > + static const char edd_skipmbr[] __text = "skipmbr"; > + > + c = find_opt(cmdline, edd, 1); > + > + if ( !c ) > + return 0; > + > + c += strlen_static(edd); > + la = strcspn(c, empty_chars); > + > + if ( !strncmp(c, edd_off, max(la, strlen_static(edd_off))) ) > + return 2; > + else if ( !strncmp(c, edd_skipmbr, max(la, strlen_static(edd_skipmbr))) ) Pointless else. > + return 1; > + > + return 0; And the last two returns can be folded again anyway. > +static void __cdecl __used cmdline_parse_early(const char *cmdline, early_boot_opts_t *ebo) I don't see the point of the __cdecl, and (as said before) dislike the static __used pair. > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -429,8 +429,24 @@ trampoline_setup: > cmp $sym_phys(__trampoline_seg_stop),%edi > jb 1b > > + /* Do not parse command line on EFI platform here. */ > + cmpb $1,sym_phys(skip_realmode) Is there a reason you can't look at efi.flags instead here (and in the other case you abuse skip_realmode as meaning EFI)? > --- a/xen/arch/x86/boot/trampoline.S > +++ b/xen/arch/x86/boot/trampoline.S > @@ -1,3 +1,5 @@ > +#include "video.h" Please move this farther down, making invisible all its definitions to code not supposed to use them. Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-08-27 12:43 ` Jan Beulich @ 2015-09-22 17:03 ` Daniel Kiper 2015-09-22 17:03 ` Daniel Kiper 1 sibling, 0 replies; 13+ messages in thread From: Daniel Kiper @ 2015-09-22 17:03 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, grub-devel, wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3, roy.franz, ning.sun, david.vrabel, phcoder, xen-devel, qiaowei.ren, keir, richard.l.maliszewski, gang.wei, fu.wei On Thu, Aug 27, 2015 at 06:43:39AM -0600, Jan Beulich wrote: > >>> On 20.07.15 at 16:29, <daniel.kiper@oracle.com> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-08-27 12:43 ` Jan Beulich 2015-09-22 17:03 ` Daniel Kiper @ 2015-09-22 17:03 ` Daniel Kiper 2015-09-23 7:25 ` Jan Beulich 2015-09-23 7:25 ` Jan Beulich 1 sibling, 2 replies; 13+ messages in thread From: Daniel Kiper @ 2015-09-22 17:03 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, grub-devel, wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3, roy.franz, ning.sun, david.vrabel, phcoder, xen-devel, qiaowei.ren, keir, richard.l.maliszewski, gang.wei, fu.wei On Thu, Aug 27, 2015 at 06:43:39AM -0600, Jan Beulich wrote: > >>> On 20.07.15 at 16:29, <daniel.kiper@oracle.com> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-09-22 17:03 ` Daniel Kiper @ 2015-09-23 7:25 ` Jan Beulich 2015-09-23 7:25 ` Jan Beulich 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2015-09-23 7:25 UTC (permalink / raw) To: Daniel Kiper Cc: Juergen Gross, grub-devel, wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3, roy.franz, ning.sun, david.vrabel, phcoder, xen-devel, qiaowei.ren, keir, richard.l.maliszewski, gang.wei, fu.wei >>> On 22.09.15 at 19:03, <daniel.kiper@oracle.com> wrote: > On Thu, Aug 27, 2015 at 06:43:39AM -0600, Jan Beulich wrote: >> >>> On 20.07.15 at 16:29, <daniel.kiper@oracle.com> wrote: >> > +#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. I disagree - reloc.c serves a completely different purpose, and hence may have different considerations applied when it comes to what to (not) include. >> > +#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. I think the question should rather be whether we can't make cmdline.c build the normal way, not the reloc.c one. >> > +#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... :-((( Perhaps as a result of some (missing) option(s)? > However, you are right that the name could be better. Or just drop the macro. >> > +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. If you mean to retain characters in that array that aren't obviously needed, please explain such in a comment so people won't have to guess whether to delete them, or will know it is (relatively) safe to delete them in case their presence causes problems. But even better would be to just have the obvious blank characters (space and tab) there. >> > +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? Yes, I am - the moment you drop the "static". At that point the compiler has no choice, unless it is being told on the command line to use a different calling convention Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 21/23] x86/boot: implement early command line parser in C 2015-09-22 17:03 ` Daniel Kiper 2015-09-23 7:25 ` Jan Beulich @ 2015-09-23 7:25 ` Jan Beulich 1 sibling, 0 replies; 13+ messages in thread From: Jan Beulich @ 2015-09-23 7:25 UTC (permalink / raw) To: Daniel Kiper Cc: Juergen Gross, grub-devel, wei.liu2, ian.campbell, stefano.stabellini, andrew.cooper3, roy.franz, ning.sun, david.vrabel, phcoder, xen-devel, qiaowei.ren, keir, richard.l.maliszewski, gang.wei, fu.wei >>> On 22.09.15 at 19:03, <daniel.kiper@oracle.com> wrote: > On Thu, Aug 27, 2015 at 06:43:39AM -0600, Jan Beulich wrote: >> >>> On 20.07.15 at 16:29, <daniel.kiper@oracle.com> wrote: >> > +#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. I disagree - reloc.c serves a completely different purpose, and hence may have different considerations applied when it comes to what to (not) include. >> > +#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. I think the question should rather be whether we can't make cmdline.c build the normal way, not the reloc.c one. >> > +#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... :-((( Perhaps as a result of some (missing) option(s)? > However, you are right that the name could be better. Or just drop the macro. >> > +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. If you mean to retain characters in that array that aren't obviously needed, please explain such in a comment so people won't have to guess whether to delete them, or will know it is (relatively) safe to delete them in case their presence causes problems. But even better would be to just have the obvious blank characters (space and tab) there. >> > +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? Yes, I am - the moment you drop the "static". At that point the compiler has no choice, unless it is being told on the command line to use a different calling convention Jan ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-09-23 7:25 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-16 13:48 [PATCH v2 21/23] x86/boot: implement early command line parser in C George Diamantopoulos 2015-08-16 17:22 ` Konrad Rzeszutek Wilk 2015-08-17 20:37 ` Daniel Kiper -- strict thread matches above, loose matches on Subject: below -- 2015-07-20 14:28 [PATCH v2 00/23] x86: multiboot2 protocol support Daniel Kiper 2015-07-20 14:29 ` [PATCH v2 21/23] x86/boot: implement early command line parser in C Daniel Kiper 2015-07-20 14:29 ` Daniel Kiper 2015-08-10 20:31 ` Konrad Rzeszutek Wilk 2015-08-11 14:43 ` Konrad Rzeszutek Wilk 2015-08-27 12:43 ` Jan Beulich 2015-08-27 12:43 ` Jan Beulich 2015-09-22 17:03 ` Daniel Kiper 2015-09-22 17:03 ` Daniel Kiper 2015-09-23 7:25 ` Jan Beulich 2015-09-23 7:25 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.