* [PATCH] xen/arch/x86: Fix early boot command line parsing
@ 2013-12-02 19:15 Daniel Kiper
2013-12-03 2:14 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Kiper @ 2013-12-02 19:15 UTC (permalink / raw)
To: jbeulich, keir, xen-devel; +Cc: Daniel Kiper
There is no reliable way to encode nul character as a character so encode
it as a number. Read: http://sourceware.org/binutils/docs/as/Characters.html.
Octal and hex encoding does not work on at least my system (GNU assembler
version 2.22 (x86_64-linux-gnu) using BFD version (GNU Binutils for Debian) 2.22).
Without this fix e.g. no-real-mode option at the end of xen.gz command line
is not detected. Additionally, encode other characters accordingly to
the gas documentation.
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
xen/arch/x86/boot/cmdline.S | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
index 05ffb94..4ea56b3 100644
--- a/xen/arch/x86/boot/cmdline.S
+++ b/xen/arch/x86/boot/cmdline.S
@@ -138,11 +138,11 @@
call .Lstrlen
add $4,%esp
xadd %eax,%ebx
- cmpb $'\0',(%ebx)
+ cmpb $0,(%ebx)
je 3f
- cmpb $' ',(%ebx)
+ cmpb $' ,(%ebx)
je 3f
- cmpb $'=',(%ebx)
+ cmpb $'=,(%ebx)
jne 1b
3: pop %ebx
ret
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] xen/arch/x86: Fix early boot command line parsing
2013-12-02 19:15 [PATCH] xen/arch/x86: Fix early boot command line parsing Daniel Kiper
@ 2013-12-03 2:14 ` Andrew Cooper
2013-12-03 10:34 ` Jan Beulich
2013-12-03 10:59 ` Daniel Kiper
0 siblings, 2 replies; 5+ messages in thread
From: Andrew Cooper @ 2013-12-03 2:14 UTC (permalink / raw)
To: Daniel Kiper, jbeulich, keir, xen-devel
On 02/12/2013 19:15, Daniel Kiper wrote:
> There is no reliable way to encode nul character as a character so encode
> it as a number. Read: http://sourceware.org/binutils/docs/as/Characters.html.
> Octal and hex encoding does not work on at least my system (GNU assembler
> version 2.22 (x86_64-linux-gnu) using BFD version (GNU Binutils for Debian) 2.22).
> Without this fix e.g. no-real-mode option at the end of xen.gz command line
> is not detected. Additionally, encode other characters accordingly to
> the gas documentation.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> xen/arch/x86/boot/cmdline.S | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
> index 05ffb94..4ea56b3 100644
> --- a/xen/arch/x86/boot/cmdline.S
> +++ b/xen/arch/x86/boot/cmdline.S
> @@ -138,11 +138,11 @@
> call .Lstrlen
> add $4,%esp
> xadd %eax,%ebx
> - cmpb $'\0',(%ebx)
> + cmpb $0,(%ebx)
> je 3f
> - cmpb $' ',(%ebx)
> + cmpb $' ,(%ebx)
> je 3f
> - cmpb $'=',(%ebx)
> + cmpb $'=,(%ebx)
> jne 1b
> 3: pop %ebx
> ret
My version of binutils 2.22 for Debian Wheezy does perfectly well with
these constants as-are.
What does the disassembly from your toolchain look like? I find it
quite hard to believe that the above syntax has been broken by a recent
version of binutils.
~Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/arch/x86: Fix early boot command line parsing
2013-12-03 2:14 ` Andrew Cooper
@ 2013-12-03 10:34 ` Jan Beulich
2013-12-03 10:47 ` Andrew Cooper
2013-12-03 10:59 ` Daniel Kiper
1 sibling, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2013-12-03 10:34 UTC (permalink / raw)
To: Andrew Cooper, Daniel Kiper; +Cc: xen-devel, keir
>>> On 03.12.13 at 03:14, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 02/12/2013 19:15, Daniel Kiper wrote:
>> There is no reliable way to encode nul character as a character so encode
>> it as a number. Read:
> http://sourceware.org/binutils/docs/as/Characters.html.
>> Octal and hex encoding does not work on at least my system (GNU assembler
>> version 2.22 (x86_64-linux-gnu) using BFD version (GNU Binutils for Debian)
> 2.22).
>> Without this fix e.g. no-real-mode option at the end of xen.gz command line
>> is not detected. Additionally, encode other characters accordingly to
>> the gas documentation.
>>
>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>> xen/arch/x86/boot/cmdline.S | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
>> index 05ffb94..4ea56b3 100644
>> --- a/xen/arch/x86/boot/cmdline.S
>> +++ b/xen/arch/x86/boot/cmdline.S
>> @@ -138,11 +138,11 @@
>> call .Lstrlen
>> add $4,%esp
>> xadd %eax,%ebx
>> - cmpb $'\0',(%ebx)
>> + cmpb $0,(%ebx)
>> je 3f
>> - cmpb $' ',(%ebx)
>> + cmpb $' ,(%ebx)
>> je 3f
>> - cmpb $'=',(%ebx)
>> + cmpb $'=,(%ebx)
>> jne 1b
>> 3: pop %ebx
>> ret
>
> My version of binutils 2.22 for Debian Wheezy does perfectly well with
> these constants as-are.
Does it? The '\0' is consistently being translated as '0' rather than
the intended NIL for every gas from 2.18 to 2.23.2.
I'm not eager to take the other two adjustments though - the
code is better readable/understandable the with a closing quote
in place.
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/arch/x86: Fix early boot command line parsing
2013-12-03 10:34 ` Jan Beulich
@ 2013-12-03 10:47 ` Andrew Cooper
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2013-12-03 10:47 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Daniel Kiper, keir
On 03/12/13 10:34, Jan Beulich wrote:
>>>> On 03.12.13 at 03:14, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 02/12/2013 19:15, Daniel Kiper wrote:
>>> There is no reliable way to encode nul character as a character so encode
>>> it as a number. Read:
>> http://sourceware.org/binutils/docs/as/Characters.html.
>>> Octal and hex encoding does not work on at least my system (GNU assembler
>>> version 2.22 (x86_64-linux-gnu) using BFD version (GNU Binutils for Debian)
>> 2.22).
>>> Without this fix e.g. no-real-mode option at the end of xen.gz command line
>>> is not detected. Additionally, encode other characters accordingly to
>>> the gas documentation.
>>>
>>> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
>>> ---
>>> xen/arch/x86/boot/cmdline.S | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
>>> index 05ffb94..4ea56b3 100644
>>> --- a/xen/arch/x86/boot/cmdline.S
>>> +++ b/xen/arch/x86/boot/cmdline.S
>>> @@ -138,11 +138,11 @@
>>> call .Lstrlen
>>> add $4,%esp
>>> xadd %eax,%ebx
>>> - cmpb $'\0',(%ebx)
>>> + cmpb $0,(%ebx)
>>> je 3f
>>> - cmpb $' ',(%ebx)
>>> + cmpb $' ,(%ebx)
>>> je 3f
>>> - cmpb $'=',(%ebx)
>>> + cmpb $'=,(%ebx)
>>> jne 1b
>>> 3: pop %ebx
>>> ret
>> My version of binutils 2.22 for Debian Wheezy does perfectly well with
>> these constants as-are.
> Does it? The '\0' is consistently being translated as '0' rather than
> the intended NIL for every gas from 2.18 to 2.23.2.
>
> I'm not eager to take the other two adjustments though - the
> code is better readable/understandable the with a closing quote
> in place.
>
> Jan
>
Ah - I missed the qualification of "at the end of the command line" - my
appologies. My "perfectly well" was based on my recent use of
"no-real-mode" working fine, but it wasn't at the end of the command line.
Looking at the current disassembly,
0f c1 c3 xadd %eax,%ebx
80 3b 30 cmpb $0x30,(%ebx)
74 0a je 8028c2b1 <idx+0x8028c0b1>
80 3b 20 cmpb $0x20,(%ebx)
74 05 je 8028c2b1 <idx+0x8028c0b1>
80 3b 3d cmpb $0x3d,(%ebx)
75 c2 jne 8028c273 <idx+0x8028c073>
5b pop %ebx
c3 ret
The NULL check is indeed being mistranslated, but the space and = are fine.
I would suggest switching it to $0 and leaving a line comment indicating
that this is a null check? It is sad that '\0' doesn't work, as it is
far more informative of the intent of the code.
~Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xen/arch/x86: Fix early boot command line parsing
2013-12-03 2:14 ` Andrew Cooper
2013-12-03 10:34 ` Jan Beulich
@ 2013-12-03 10:59 ` Daniel Kiper
1 sibling, 0 replies; 5+ messages in thread
From: Daniel Kiper @ 2013-12-03 10:59 UTC (permalink / raw)
To: Andrew Cooper; +Cc: keir, jbeulich, xen-devel
On Tue, Dec 03, 2013 at 02:14:18AM +0000, Andrew Cooper wrote:
> On 02/12/2013 19:15, Daniel Kiper wrote:
> > There is no reliable way to encode nul character as a character so encode
> > it as a number. Read: http://sourceware.org/binutils/docs/as/Characters.html.
> > Octal and hex encoding does not work on at least my system (GNU assembler
> > version 2.22 (x86_64-linux-gnu) using BFD version (GNU Binutils for Debian) 2.22).
> > Without this fix e.g. no-real-mode option at the end of xen.gz command line
> > is not detected. Additionally, encode other characters accordingly to
> > the gas documentation.
> >
> > Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> > ---
> > xen/arch/x86/boot/cmdline.S | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/cmdline.S b/xen/arch/x86/boot/cmdline.S
> > index 05ffb94..4ea56b3 100644
> > --- a/xen/arch/x86/boot/cmdline.S
> > +++ b/xen/arch/x86/boot/cmdline.S
> > @@ -138,11 +138,11 @@
> > call .Lstrlen
> > add $4,%esp
> > xadd %eax,%ebx
> > - cmpb $'\0',(%ebx)
> > + cmpb $0,(%ebx)
> > je 3f
> > - cmpb $' ',(%ebx)
> > + cmpb $' ,(%ebx)
> > je 3f
> > - cmpb $'=',(%ebx)
> > + cmpb $'=,(%ebx)
> > jne 1b
> > 3: pop %ebx
> > ret
>
> My version of binutils 2.22 for Debian Wheezy does perfectly well with
> these constants as-are.
>
> What does the disassembly from your toolchain look like? I find it
objdump -dm i386 xen/arch/x86/boot/head.o
[...]
29f: 0f c1 c3 xadd %eax,%ebx
2a2: 80 3b 30 cmpb $0x30,(%ebx)
^^^^^ == '0' != NUL
2a5: 74 0a je 2b1 <__start+0x246>
2a7: 80 3b 20 cmpb $0x20,(%ebx)
2aa: 74 05 je 2b1 <__start+0x246>
2ac: 80 3b 3d cmpb $0x3d,(%ebx)
2af: 75 c2 jne 273 <__start+0x208>
> quite hard to believe that the above syntax has been broken by a recent
> version of binutils.
I see the same issue on older machines too. If you show me how to encode
NUL in gas assembly file I am happy to do that. Current notation does
not work. Please test with no-real-mode (or other option checked
at early boot stage) at the end of Xen boot command line. It is
completely ignored. Once I found description of that issue somewhere
but I do not remember where. Later I was hurt by it during EFI work.
Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-12-03 10:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-02 19:15 [PATCH] xen/arch/x86: Fix early boot command line parsing Daniel Kiper
2013-12-03 2:14 ` Andrew Cooper
2013-12-03 10:34 ` Jan Beulich
2013-12-03 10:47 ` Andrew Cooper
2013-12-03 10:59 ` Daniel Kiper
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).