From: Daniel Kiper <daniel.kiper@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Juergen Gross <JGross@suse.com>,
stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com,
cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
qiaowei.ren@intel.com, richard.l.maliszewski@intel.com,
gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v3 14/16] x86/boot: implement early command line parser in C
Date: Wed, 25 May 2016 23:36:42 +0200 [thread overview]
Message-ID: <20160525213642.GQ5490@olila.local.net-space.pl> (raw)
In-Reply-To: <57459BB202000078000EE9E7@prv-mh.provo.novell.com>
On Wed, May 25, 2016 at 04:33:54AM -0600, Jan Beulich wrote:
> >>> On 15.04.16 at 14:33, <daniel.kiper@oracle.com> wrote:
> > --- a/xen/arch/x86/boot/build32.lds
> > +++ b/xen/arch/x86/boot/build32.lds
> > @@ -25,6 +25,7 @@ SECTIONS
> > *(.text)
> > *(.text.*)
> > *(.rodata)
> > + *(.rodata.*)
> > }
>
> Interesting - didn't you say you don't want this for now?
Yep, however, I also said that we should add "*(.rodata.*)"
if it is needed. And it is needed here.
> > --- /dev/null
> > +++ b/xen/arch/x86/boot/cmdline.c
> > @@ -0,0 +1,357 @@
> > +/*
> > + * Copyright (c) 2015, 2016 Oracle and/or its affiliates. All rights reserved.
> > + * Daniel Kiper <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).
>
> Any reason you can't just #include ".../common/string.c" here?
I am confused. Sometimes you request to reduce number of such strange
includes and sometimes you ask me to do something contrary? So, what
is the rule behind these requests?
Additionally, there is a chance that final xen ELF file will be
unnecessary larger because it will contain unused functions. Wait,
maybe there is a linker option which allows us to remove unreferenced
objects from final output.
> > +/*
> > + * Space and TAB are obvious delimiters. However, I am
> > + * adding "\n" and "\r" here too. Just in case when
> > + * crazy bootloader/user put them somewhere.
> > + */
> > +#define DELIM_CHARS " \n\r\t"
> > +#define DELIM_CHARS_COMMA DELIM_CHARS ","
>
> static const char[] variables (or really just one, with the comma put
> first and the non-comma variant indexing into that variable by 1)?
OK.
> > +#define __packed __attribute__((__packed__))
>
> No way to include compiler.h here?
Ditto.
> > +/*
> > + * Compiler is not able to optimize regular strlen()
> > + * if argument is well known string during build.
> > + * Hence, introduce optimized strlen_opt().
> > + */
> > +#define strlen_opt(s) (sizeof(s) - 1)
>
> Do we really care in this code?
Not to strongly but why not?
> > +/* Keep in sync with trampoline.S:early_boot_opts label! */
> > +typedef struct __packed {
> > + u8 skip_realmode;
> > + u8 opt_edd;
> > + u8 opt_edid;
> > + u16 boot_vid_mode;
> > + u16 vesa_width;
> > + u16 vesa_height;
> > + u16 vesa_depth;
> > +} early_boot_opts_t;
>
> This "keeping in sync" should be automated in some way, e.g. via
> a new header and suitable macroization.
OK.
> > +static int strtoi(const char *s, const char *stop, const char **next)
> > +{
> > + int base = 10, i, ores = 0, res = 0;
>
> You don't even handle a '-' on the numbers here, so all the variables
Yep, however, ...
> and the function return type should be unsigned int afaict. And the
> function name perhaps be strtoui().
... we return -1 in case of error.
> > + if ( *s == '0' )
> > + base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
> > +
> > + for ( ; *s != '\0'; ++s )
> > + {
> > + for ( i = 0; stop && stop[i] != '\0'; ++i )
> > + if ( *s == stop[i] )
> > + goto out;
>
> strchr()?
Could be.
> > + if ( *s < '0' || (*s > '7' && base == 8) )
> > + {
> > + res = -1;
> > + goto out;
> > + }
> > +
> > + if ( *s > '9' && (base != 16 || tolower(*s) < 'a' || tolower(*s) > 'f') )
> > + {
> > + res = -1;
> > + goto out;
> > + }
> > +
> > + res *= base;
> > + res += (tolower(*s) >= 'a') ? (tolower(*s) - 'a' + 10) : (*s - '0');
>
> With the four instances, how about latching tolower(*s) into a local
> variable?
Make sense.
> > +static u8 skip_realmode(const char *cmdline)
> > +{
> > + return !!find_opt(cmdline, "no-real-mode", 0) || !!find_opt(cmdline, "tboot=", 1);
>
> The || makes the two !! pointless.
>
> Also please settle on which type you want to use for boolean
> (find_opt()'s last parameter is "int", yet here you use "u8"), and
Could be u8.
> perhaps make yourself a bool_t.
I do not think it make sense here.
> > +static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
> > +{
> > + const char *c;
> > + int tmp;
> > +
> > + c = find_opt(cmdline, "vga=", 1);
> > +
> > + if ( !c )
> > + return;
> > +
> > + ebo->boot_vid_mode = ASK_VGA;
> > +
> > + if ( !strmaxcmp(c, "current", DELIM_CHARS_COMMA) )
> > + ebo->boot_vid_mode = VIDEO_CURRENT_MODE;
> > + else if ( !strsubcmp(c, "text-80x") )
> > + {
> > + c += strlen_opt("text-80x");
> > + ebo->boot_vid_mode = rows2vmode(strtoi(c, DELIM_CHARS_COMMA, NULL));
> > + }
> > + else if ( !strsubcmp(c, "gfx-") )
> > + {
> > + tmp = strtoi(c + strlen_opt("gfx-"), "x", &c);
> > +
> > + if ( tmp < 0 || tmp > U16_MAX )
> > + return;
> > +
> > + ebo->vesa_width = tmp;
> > +
> > + /*
> > + * Increment c outside of strtoi() because otherwise some
> > + * compiler may complain with following message:
> > + * warning: operation on ‘c’ may be undefined.
> > + */
> > + ++c;
> > + tmp = strtoi(c, "x", &c);
>
> The comment is pointless - the operation is firmly undefined if you
> put it in the strtoi() invocation.
In terms of C spec you are right. However, it is quite surprising that older
GCC complains and newer ones do not. Should not we investigate this?
> > + if ( tmp < 0 || tmp > U16_MAX )
> > + return;
> > +
> > + ebo->vesa_height = tmp;
> > +
> > + tmp = strtoi(++c, DELIM_CHARS_COMMA, NULL);
> > +
> > + if ( tmp < 0 || tmp > U16_MAX )
> > + return;
> > +
> > + ebo->vesa_depth = tmp;
> > +
> > + ebo->boot_vid_mode = VIDEO_VESA_BY_SIZE;
>
> I realize this is reflecting original behavior, but now that you do it in
> C, please leverage that fact and defer storing width and height until
> you know the entire option was good.
OK.
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -436,8 +436,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
>
> Doesn't this belong in an earlier patch? It certainly doesn't get
> moved here from cmdline.S.
Potentially.
> > + /* 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
>
> Any reason not to leave at least this check to the C code?
OK.
> > + pushl $sym_phys(early_boot_opts)
> > + pushl MB_cmdline(%ebx)
> > call cmdline_parse_early
> > + add $8,%esp /* Remove cmdline_parse_early() args from stack. */
>
> I don't think such a comment is really useful (seems like I overlooked
> a similar one in an earlier patch, on the reloc() invocation).
This thing is quite obvious but I do not think that this comment hurts.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-05-25 21:37 UTC|newest]
Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-15 12:33 [PATCH v3 00/16] x86: multiboot2 protocol support Daniel Kiper
2016-04-15 12:33 ` [PATCH v3 01/16] x86/boot: do not create unwind tables Daniel Kiper
2016-04-15 15:45 ` Andrew Cooper
2016-04-15 12:33 ` [PATCH v3 02/16] x86: zero BSS using stosl instead of stosb Daniel Kiper
2016-04-15 13:57 ` Konrad Rzeszutek Wilk
2016-04-15 15:48 ` Andrew Cooper
2016-04-15 12:33 ` [PATCH v3 03/16] x86/boot: call reloc() using cdecl calling convention Daniel Kiper
2016-04-15 15:56 ` Andrew Cooper
2016-06-17 8:41 ` Daniel Kiper
2016-06-17 9:30 ` Jan Beulich
2016-05-24 8:42 ` Jan Beulich
2016-04-15 12:33 ` [PATCH v3 04/16] x86/boot/reloc: create generic alloc and copy functions Daniel Kiper
2016-04-15 12:33 ` [PATCH v3 05/16] x86/boot: use %ecx instead of %eax Daniel Kiper
2016-04-15 12:33 ` [PATCH v3 06/16] x86/boot/reloc: Rename some variables and rearrange code a bit Daniel Kiper
2016-04-15 12:33 ` [PATCH v3 07/16] x86/boot: create *.lnk files with linker script Daniel Kiper
2016-04-15 14:04 ` Konrad Rzeszutek Wilk
2016-05-24 9:05 ` Jan Beulich
2016-05-24 12:28 ` Daniel Kiper
2016-05-24 12:52 ` Jan Beulich
2016-06-17 9:06 ` Daniel Kiper
2016-06-17 10:04 ` Jan Beulich
2016-06-17 10:34 ` Daniel Kiper
2016-04-15 12:33 ` [PATCH v3 08/16] x86: add multiboot2 protocol support Daniel Kiper
2016-05-24 15:46 ` Jan Beulich
2016-05-25 16:34 ` Daniel Kiper
2016-05-26 10:28 ` Andrew Cooper
2016-05-27 8:08 ` Jan Beulich
2016-05-27 8:13 ` Andrew Cooper
2016-05-27 8:24 ` Jan Beulich
2016-05-27 8:11 ` Jan Beulich
2016-04-15 12:33 ` [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c Daniel Kiper
2016-05-25 7:03 ` Jan Beulich
2016-05-25 16:45 ` Daniel Kiper
2016-05-27 8:16 ` Jan Beulich
2016-06-01 15:07 ` Daniel Kiper
2016-07-05 18:33 ` Daniel Kiper
2016-07-06 6:55 ` Jan Beulich
2016-07-06 10:27 ` Daniel Kiper
2016-07-06 12:00 ` Jan Beulich
2016-07-06 12:55 ` Daniel Kiper
2016-04-15 12:33 ` [PATCH v3 10/16] efi: create efi_enabled() Daniel Kiper
2016-05-25 7:20 ` Jan Beulich
2016-05-25 17:15 ` Daniel Kiper
2016-05-26 10:31 ` Andrew Cooper
2016-05-27 8:22 ` Jan Beulich
2016-06-01 15:23 ` Daniel Kiper
2016-06-01 15:41 ` Jan Beulich
2016-06-01 19:28 ` Daniel Kiper
2016-06-02 8:06 ` Jan Beulich
2016-04-15 12:33 ` [PATCH v3 11/16] efi: build xen.gz with EFI code Daniel Kiper
2016-05-25 7:53 ` Jan Beulich
2016-05-25 19:07 ` Daniel Kiper
2016-05-27 8:31 ` Jan Beulich
2016-06-01 15:48 ` Daniel Kiper
2016-06-01 15:58 ` Jan Beulich
2016-06-01 19:39 ` Daniel Kiper
2016-04-15 12:33 ` [PATCH v3 12/16 - RFC] x86/efi: create new early memory allocator Daniel Kiper
2016-05-25 8:39 ` Jan Beulich
2016-05-25 19:48 ` Daniel Kiper
2016-05-27 8:37 ` Jan Beulich
2016-06-01 15:58 ` Daniel Kiper
2016-06-01 16:02 ` Jan Beulich
2016-06-01 19:53 ` Daniel Kiper
2016-06-02 8:11 ` Jan Beulich
2016-06-02 10:43 ` Daniel Kiper
2016-06-02 11:10 ` Jan Beulich
2016-06-01 16:01 ` Daniel Kiper
2016-07-05 18:26 ` Daniel Kiper
2016-07-06 7:22 ` Jan Beulich
2016-07-06 11:15 ` Daniel Kiper
2016-04-15 12:33 ` [PATCH v3 13/16 - RFC] x86: add multiboot2 protocol support for EFI platforms Daniel Kiper
2016-05-25 9:32 ` Jan Beulich
2016-05-25 10:29 ` Jan Beulich
2016-05-25 21:02 ` Daniel Kiper
2016-05-27 9:02 ` Jan Beulich
2016-06-01 19:03 ` Daniel Kiper
2016-06-02 8:34 ` Jan Beulich
2016-06-02 16:12 ` Daniel Kiper
2016-06-03 9:26 ` Jan Beulich
2016-06-03 17:06 ` Konrad Rzeszutek Wilk
2016-04-15 12:33 ` [PATCH v3 14/16] x86/boot: implement early command line parser in C Daniel Kiper
2016-05-25 10:33 ` Jan Beulich
2016-05-25 21:36 ` Daniel Kiper [this message]
2016-05-27 9:33 ` Jan Beulich
2016-06-02 8:15 ` Daniel Kiper
2016-06-02 8:39 ` Jan Beulich
2016-04-15 12:33 ` [PATCH v3 15/16 - RFC] x86: make Xen early boot code relocatable Daniel Kiper
2016-05-25 10:48 ` Jan Beulich
2016-04-15 12:33 ` [PATCH v3 16/16] x86: add multiboot2 protocol support for relocatable images Daniel Kiper
2016-05-25 11:03 ` Jan Beulich
2016-06-01 13:35 ` Daniel Kiper
2016-06-01 14:44 ` Jan Beulich
2016-06-01 19:16 ` Daniel Kiper
2016-06-02 8:41 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160525213642.GQ5490@olila.local.net-space.pl \
--to=daniel.kiper@oracle.com \
--cc=JBeulich@suse.com \
--cc=JGross@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=cardoe@cardoe.com \
--cc=david.vrabel@citrix.com \
--cc=fu.wei@linaro.org \
--cc=gang.wei@intel.com \
--cc=ning.sun@intel.com \
--cc=pgnet.dev@gmail.com \
--cc=qiaowei.ren@intel.com \
--cc=richard.l.maliszewski@intel.com \
--cc=stefano.stabellini@eu.citrix.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).